-
Notifications
You must be signed in to change notification settings - Fork 795
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
1741 add smb to puppet #1792
1741 add smb to puppet #1792
Conversation
This option is never changed and can be more easily stored as a constant.
self._config.hash_sensitive_data(lm_hash), | ||
self._config.hash_sensitive_data(ntlm_hash), | ||
logger.info( | ||
f'Successfully logged in to {self.host.ip_addr} using user "{user}"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to know which type of credentials were used. use infection_monkey.utils.brute_force.get_credential_string
self._config.hash_sensitive_data(ntlm_hash), | ||
exc, | ||
"Error when trying to copy file using SMB to {self.host.ip_addr} with user " | ||
f'"{user}":{exc}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as previous comment, would be good to know which credential type is used for reproduction of the error
Configuration.hash_sensitive_data(ntlm_hash), | ||
exc, | ||
) | ||
logger.debug(f'Error while logging into {host} using user "{username}": {exc}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be at least info or even error, since we already brute-forced and know the credentials. We should log credential type to ease debugging
@@ -38,7 +38,6 @@ | |||
], | |||
"ping_scan_timeout": 10000, | |||
"smb_download_timeout": 300, | |||
"smb_service_name": "InfectionMonkey", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it's time for this whole file to go?
"title": "SMB service", | ||
"type": "object", | ||
"properties": { | ||
"smb_download_timeout": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not move it to a const in smbexec.py? Other exploiters don't have timeouts in config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered it. For the moment, it's the only thing exercising our ability to send custom options to exploiters. I think it's worth keeping just as a reminder that we can do that for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Superb work, mergeable but left some comments for consideration
@@ -159,7 +159,7 @@ def _exploit_host(self): | |||
except Exception: | |||
status = ScanStatus.SCANNED | |||
pass | |||
T1035Telem(status, UsageEnum.SMB).send() | |||
self.telemetry_messenger.send_telemetry(T1035Telem(status, UsageEnum.SMB)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe now is the time to check on #1571 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not now. I wan't to keep focused on the agent refactor project.
if not self.exploit_result.exploitation_success: | ||
logger.debug("Exploiter SmbExec is giving up...") | ||
return False | ||
return self.exploit_result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets add error_message
to the ExploitResultData.
This file is out of date and an unnecessary maintenance burden.
What does this PR do?
Adds SMBExploiter to puppet
Resolves #1741
PR Checklist
Is the TravisCI build passing?Was the documentation framework updated to reflect the changes?Testing Checklist
Added relevant unit tests?Screenshots