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

Add file write check on smb #404

Merged
merged 2 commits into from
Oct 4, 2024
Merged

Add file write check on smb #404

merged 2 commits into from
Oct 4, 2024

Conversation

tiyeuse
Copy link

@tiyeuse tiyeuse commented Aug 24, 2024

Currently NetExec only checks the "WRITE" permission on a SMB share through writing a folder.
It is also necessary to verify this permission through a file write.
This PR implements this additional check.

@mpgn
Copy link
Collaborator

mpgn commented Aug 24, 2024

I wonder if we still need the check on the folder if we have the file ?

@tiyeuse
Copy link
Author

tiyeuse commented Aug 24, 2024

I think so since write file and create folder are two different permissions.
Testing for each is important. If only one is allowed, nxc will only display "WRITE" without specifying if it's a file or a folder.
In that case, two options (at least) are possible:

  • change the WRITE information in order to be more precise
  • do not change anything but the user has to try by himself in order to understand

mpgn
mpgn previously approved these changes Aug 25, 2024
@mpgn mpgn added this to the v1.3.0 milestone Aug 25, 2024
@NeffIsBack NeffIsBack added the bug-fix This Pull Request fixes a bug label Oct 1, 2024
@NeffIsBack
Copy link
Contributor

I would say we either check only for file creation (because imo that's what most often matters) or we combine the file&folder test and if both pass we add the "write" privilege to the output. Currently that looks a bit ugly and honestly i would be very confused:
image

Thoughts?

@mpgn
Copy link
Collaborator

mpgn commented Oct 3, 2024

File OR Directory = WRITE

@NeffIsBack
Copy link
Contributor

sounds good

Copy link
Contributor

@NeffIsBack NeffIsBack left a comment

Choose a reason for hiding this comment

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

LGTM from my side

@NeffIsBack NeffIsBack merged commit 754ca2d into Pennyw0rth:main Oct 4, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix This Pull Request fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants