Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Fix Path Traversal vulnerabilities by checking path prefix against in… #1066

Merged
merged 2 commits into from May 3, 2021

Conversation

omriinbar
Copy link
Contributor

Sorry about the large code diff because of the indentations.
Added def isInFileJail in line 551 and called it in smbComNtCreateAndX, findFirst2 and smb2Create functions.

@mpgn
Copy link
Contributor

mpgn commented Apr 25, 2021

d

@martingalloar martingalloar added the in review This issue or pull request is being analyzed label Apr 25, 2021
@martingalloar
Copy link
Contributor

Thanks @omriinbar for providing an PR on this issues! The code changes are quite large due to the indentation and other PEP8-related changes, which make its a little bit harder that it should to evaluate.

While we review the potential impact of the changes and work on adding some tests to check those, I've a question related to the introduced behavior that added as in-line comments. I'm doubting on whether we shouldn't return STATUS_OBJECT_PATH_SYNTAX_BAD instead of STATUS_ACCESS_DENIED or STATUS_NOT_SUPPORTED.

@omriinbar
Copy link
Contributor Author

Changed STATUS_ACCESS_DENIED and STATUS_NOT_SUPPORTED to STATUS_OBJECT_PATH_SYNTAX_BAD

martingalloar added a commit that referenced this pull request Apr 26, 2021
Adding some pseudo-functional tests for the SimpleSMBServer. This spins up a SimpleSMBServer instance and connects to it using our own SMBConnection. Includes checks for #1066.
@martingalloar
Copy link
Contributor

Thanks for the changes. I don't think is fully clear from the documentation (e.g. in MS-CIFS) but at least this is the way that Samba is replying (and I think based on this old thread). Windows (e.g. Win 2019) returns an STATUS_INVALID_PARAMETER.

Coverage was added in #1067.

@martingalloar martingalloar self-requested a review April 26, 2021 19:13
@0xdeaddood 0xdeaddood merged commit 49c643b into fortra:master May 3, 2021
@0xdeaddood 0xdeaddood removed the in review This issue or pull request is being analyzed label May 3, 2021
martingalloar added a commit that referenced this pull request May 3, 2021
Adding some pseudo-functional tests for the `SimpleSMBServer`. This spins up a `SimpleSMBServer` instance and connects to it using our own `SMBConnection`. Includes checks for #1066.

This PR:
- Adds basic unit tests for the path validation function introduced in #1066.
- Adds pseudo-functional tests for `SimpleSMBServer`, checking login, list, get and put calls.
mback2k added a commit to mback2k/curl that referenced this pull request Nov 1, 2021
impacket now performs sanity checks if the requested and to
be served file path actually is inside the real share path.

Ref: fortra/impacket#1066

Fixes curl#7924
mback2k added a commit to mback2k/curl that referenced this pull request Nov 1, 2021
impacket now performs sanity checks if the requested and to
be served file path actually is inside the real share path.

Ref: fortra/impacket#1066

Fixes curl#7924
mback2k added a commit to curl/curl that referenced this pull request Nov 1, 2021
impacket now performs sanity checks if the requested and to
be served file path actually is inside the real share path.

Ref: fortra/impacket#1066

Fixes #7924
Closes #7935
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants