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

Create get_gpos module, implement get-folder command, and add some module helper functions #320

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

Marshall-Hallenbeck
Copy link
Collaborator

@Marshall-Hallenbeck Marshall-Hallenbeck commented May 21, 2024

I was initially going to have the get_gpos module have the ability to download all of them, hence the helper functions, but LDAP can only query certain GPO information, so it'd have to be another SMB module to download them all; however, the user can just take the GUID output from the module and pass it to --get-folder

get_gpos module:
image

--get-folder feature:
image

@Marshall-Hallenbeck Marshall-Hallenbeck marked this pull request as ready for review May 22, 2024 09:44
@Marshall-Hallenbeck Marshall-Hallenbeck changed the title Create get_gpos module and update how modules perform specific logging & loot saving Create get_gpos module, implement get-folder command, and add some module helper functions May 22, 2024
@NeffIsBack
Copy link
Contributor

Would it be possible to detect a folder on a remote system? Than we could just integrate it into the existing get commands

@Marshall-Hallenbeck
Copy link
Collaborator Author

Would it be possible to detect a folder on a remote system? Than we could just integrate it into the existing get commands

Well the current get-file command references a file, so what would we change it to? I think having them separate makes more sense.

@NeffIsBack
Copy link
Contributor

Would it be possible to detect a folder on a remote system? Than we could just integrate it into the existing get commands

Well the current get-file command references a file, so what would we change it to? I think having them separate makes more sense.

Hmm yeah i was thinking about moving --get-file/--put-file maybe to --get/--put anyway and add some sort of deprecation warning for the old function. But of course that would only make sense if it is possible to do a smart download/upload. Thoughts?

@NeffIsBack NeffIsBack linked an issue Jun 15, 2024 that may be closed by this pull request
@Marshall-Hallenbeck
Copy link
Collaborator Author

just using the verbs "get" and "put" doesn't make it clear what you are getting or putting, so it's sorta confusing, and honestly, changing all that code to just make it a single call sounds like a huge pain in the ass that I'm not interested in doing lol

@NeffIsBack
Copy link
Contributor

just using the verbs "get" and "put" doesn't make it clear what you are getting or putting, so it's sorta confusing, and honestly, changing all that code to just make it a single call sounds like a huge pain in the ass that I'm not interested in doing lol

I mean we are on SMB, imo its pretty clear what you are trying to achieve. But if it cant be changed by a fairly simple check or try&except call its fine i guess. I just try to limit the amount of options the smb protocol has, there are already a lot (too much imo)

@Marshall-Hallenbeck
Copy link
Collaborator Author

I mean we are on SMB, imo its pretty clear what you are trying to achieve

Well I disagree with that... we have numerous options to get things such as users, groups, disks, computers, etc all under the SMB protocol.

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.

Tried to request a non valid folder, imo that should be handled with a proper message:
image

Rest looks good so far:
image

nxc/helpers/modules.py Show resolved Hide resolved
Comment on lines +53 to +59
if self.gpo_name:
if k == "displayname":
context.log.highlight(f"Display Name: {v}")
elif k == "name":
context.log.highlight(f"GUID: {v}")
else:
context.log.highlight(f"{k}: {v}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason you only "pretty print" the key&value pair if self.gpo_name is set and not always?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll double check but I believe because there's random (default?) GPOs?

tests/e2e_commands.txt Outdated Show resolved Hide resolved
self.logger.debug(f"Sharing violation on {remote_path}: {e}")
return False
except Exception as e:
self.logger.debug(f"Other error when attempting to download file {remote_path}: {e}")
Copy link
Contributor

Choose a reason for hiding this comment

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

imo we should include the error message in the fail output not in the debug log

Comment on lines +1324 to +1341
def get_file_single(self, remote_path, download_path, silent=False):
share_name = self.args.share
self.logger.display(f'Copying "{remote_path}" to "{download_path}"')
if not silent:
self.logger.display(f"Copying '{remote_path}' to '{download_path}'")
if self.args.append_host:
download_path = f"{self.hostname}-{remote_path}"
with open(download_path, "wb+") as file:
try:
self.conn.getFile(share_name, remote_path, file.write)
self.logger.success(f'File "{remote_path}" was downloaded to "{download_path}"')
except Exception as e:
self.logger.fail(f'Error writing file "{remote_path}" from share "{share_name}": {e}')
if os.path.getsize(download_path) == 0:
os.remove(download_path)

if self.download_file(share_name, remote_path, file.write):
if not silent:
self.logger.success(f"File '{remote_path}' was downloaded to '{download_path}'")
else:
self.logger.debug("Opening with READ alone failed, trying to open file with READ/WRITE access")
if self.download_file(share_name, remote_path, file.write, FILE_READ_DATA | FILE_WRITE_DATA):
if not silent:
self.logger.success(f"File '{remote_path}' was downloaded to '{download_path}'")
else:
if not silent:
self.logger.fail(f"Error downloading file '{remote_path}' from share '{share_name}'")
Copy link
Contributor

Choose a reason for hiding this comment

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

I am kinda not a huge fan of having 3 different "get file" functions now. Maybe we should merge this function with download_file and make the access_mode and log message conditional? That way we can also get the error message in the fail message.

@NeffIsBack
Copy link
Contributor

NeffIsBack commented Jun 22, 2024

I mean we are on SMB, imo its pretty clear what you are trying to achieve

Well I disagree with that... we have numerous options to get things such as users, groups, disks, computers, etc all under the SMB protocol.

That is true but it is still listed in the Options for put and get remote files. Also the options have descriptions & we have a wiki. Imo that should be enough, but if its not possible it doesnt matter anyway :D

@NeffIsBack NeffIsBack closed this Jun 22, 2024
@NeffIsBack NeffIsBack reopened this Jun 22, 2024
@NeffIsBack
Copy link
Contributor

why the heck did i/it close it lol

@NeffIsBack NeffIsBack removed this from the v1.3.0 milestone Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi-file / Folder upload for --put-file
2 participants