Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[NO-ISSUE] Prevent directory escape bypass through repeated URL decoding #4891

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tbonelee
Copy link
Contributor

@tbonelee tbonelee commented Nov 3, 2024

What is this PR for?

This PR addresses an issue in NotebookService where the notebook path validation only performs a single decoding pass.
This allowed a malicious user to bypass validation by double-encoding the ".." token.
By implementing the repeated decoding, we can prevent this bypass.
Additionally, to prevent excessive decoding attempts, a maximum limit on the number of decoding attempts has been added.

What type of PR is it?

Hot Fix

How should this be tested?

  • CI

Questions:

  • Does the license files need to update? No
  • Is there breaking changes for older versions?
    • There may be minor compatibility issues if a user relies on multiple encoded paths, but this is unlikely in realistic scenarios.
  • Does this needs documentation? No

@tbonelee
Copy link
Contributor Author

tbonelee commented Nov 3, 2024

cc @jongyoul

@tbonelee
Copy link
Contributor Author

@pan3793 @Reamer
When you have a moment, could you check this PR? Appreciate it!

Copy link
Contributor

@Reamer Reamer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor comments. The five attempts seem quite random. But I think it's good that an error message is thrown afterwards.

@tbonelee
Copy link
Contributor Author

@Reamer Thank you for the feedback! I've added a commit based on your feedback.

I agree that using five attempts feels somewhat arbitrary. However I couldn't think of a better alternative, so I went with this value.
If you have any other suggestions, I'd love to hear them!

@tbonelee tbonelee requested a review from Reamer November 24, 2024 12:18
Copy link
Contributor

@Reamer Reamer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, let's wait for CI

@pan3793
Copy link
Member

pan3793 commented Nov 25, 2024

I read the change and understand what it does, but I don't understand why should this change.
And please create a JIRA ticket for it, if you think this involves CVE which is not suitable to discuss publicly, you can write a detailed message to private@zeppelin.apache.org

@Reamer
Copy link
Contributor

Reamer commented Nov 28, 2024

I agree with @pan3793 . Please create a Jira ticket and adjust the pull request description accordingly.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants