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 desired delete permissions when opening directory #262

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

dwelch-r7
Copy link
Contributor

@dwelch-r7 dwelch-r7 commented Feb 27, 2024

Drafted while we debug some issues with psexec

Pins the bindata version to 2.4.15 as 2.5.0 introduces significant changes that aren't currently compatible with ruby smb

Also aligns the permissions set when openign a directory with those of opening a file

Used by rapid7/metasploit-framework#18895

@dwelch-r7 dwelch-r7 changed the title Add util open directory Correct permissions when opening directory Feb 29, 2024
create_request.create_disposition = disposition

if read
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll defer to @cdelafuente-r7 who might have more insights here on the ramifications of these changes 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

These flags seems to define if other processes are allowed to read, write or delete the file/directory that is opened by this request. The logic to set share access according to the value of read, write and delete arguments does not make a lot of sense to me, but I might be missing something.

That being said, I don't see a context where we want to disallow the access from other processes. Maybe having these 3 flags set by default, regardless of the value of the read, write and delete arguments, would be a good idea?

The problem here is that the user cannot customise the flags easily. He only calls open_directory without the possibility to customise the request. This is something we might want to improve at some point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe having these 3 flags set by default, regardless of the value of the read, write and delete arguments, would be a good idea?

I think that would be a backwards breaking change 😄

I vote we could go for more granular kwargs for the new flags that you want to enable, so we don't break existing metasploit modules 👍

end

if write
create_request.share_access.write_access = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing there's no equivalent create_request.desired_access change needed here?

Copy link
Contributor Author

@dwelch-r7 dwelch-r7 Mar 5, 2024

Choose a reason for hiding this comment

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

yup, it's here in the file_access but not in directory_access

bit1 :write_data, label: 'Write Data'

@adfoster-r7
Copy link
Contributor

This might have broken psexec if we merge these changes, let's hold off on it

@dwelch-r7 dwelch-r7 marked this pull request as draft March 5, 2024 15:00
@dwelch-r7 dwelch-r7 force-pushed the add-util-open-directory branch from c934283 to d556637 Compare March 7, 2024 12:51
Add utility to open a directory

pin bindata to 2.4.15 until we sort out issues with 2.5.0

Use correct permissions for read/write/delete directories

Remove open dir support in util

add kwarg for desired delete access to a directory
@dwelch-r7 dwelch-r7 force-pushed the add-util-open-directory branch from d556637 to 487977c Compare March 7, 2024 16:05
@dwelch-r7
Copy link
Contributor Author

This might have broken psexec if we merge these changes, let's hold off on it

this wasn't the issue with psexec as it turns out, made changes anyway to be more intentional with which ppermissions the user requests

@dwelch-r7 dwelch-r7 marked this pull request as ready for review March 7, 2024 16:07
@adfoster-r7 adfoster-r7 changed the title Correct permissions when opening directory Add desired delete permissions when opening directory Mar 12, 2024
@adfoster-r7 adfoster-r7 merged commit fd13985 into rapid7:master Mar 12, 2024
6 checks passed
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.

3 participants