-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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 Group Policy Preferences (creds) support to db_import #10507
Conversation
And take the Jaden Smith approach, as @busterb quipped to me. :) This one's a little weird, since you normally import scans into Metasploit, but now that creds are first-class in the database, it makes more sense to be able to import them. Currently, your alternatives are post/windows/gather/credentials/gpp, which requires a session, and auxiliary/scanner/smb/smb_enum_gpp, which requires a network scan.
I get that this imports creds that happen to be provided by group policy but the naming here infers it would import so much more. Consider things like password policy, and default softwares installed would also have value from a reconnaissance view. While framework does not really take advantage of theses items yet it could be a future goal of this importer. If we think those are not viable options for expansion here, then I would suggest that something like |
return unless wspace && wspace.respond_to?(:id) | ||
|
||
gpp.each do |p| | ||
create_credential( |
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.
The end result is still the same, but I think it's interesting that you used the Auxiliary::Report
methods here instead of the DbManager
methods directly since you're already in DbManager
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 habit. Misread, sleepy brain. I explicitly did not include
the Msf::Auxiliary::Report
mixin. Wrong context for that, IMHO.
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.
Though I do remember checking this and thought they were coming from DBManager
:
metasploit-framework/lib/msf/core/auxiliary/report.rb
Lines 32 to 38 in 682b086
def create_credential(opts={}) | |
if active_db? | |
framework.db.create_credential(opts) | |
elsif !db_warning_given? | |
vprint_warning('No active DB -- Credential data will not be saved!') | |
end | |
end |
[1] pry(#<Msf::DBManager>)> method(:create_credential)
=> #<Method: Msf::DBManager(Metasploit::Credential::Creation)#create_credential>
[2] pry(#<Msf::DBManager>)>
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.
Oh yeah, I see it now. I guess I should parse all of the search results instead of just the ones that confirm my suspicions.
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 reason I used report_loot
below instead of store_loot
from Msf::Auxiliary::Report
:
[2] pry(#<Msf::DBManager>)> method(:report_loot)
=> #<Method: Msf::DBManager(Msf::DBManager::Loot)#report_loot>
[3] pry(#<Msf::DBManager>)>
Also, the file to be imported still exists on disk, and its data is stored in the database all the same. It would be helpful to be able to retrieve that data and write it to a file. That's been a long-standing complaint I've had with the loot
command.
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.
Appreciate the review! Thank you!
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.
So you actually can retrieve the data and write it to a file with the REST API. If you request the loot from there it returns a data
field with the file contents base64 encoded.
It's actually stored in the database as well, there's just no easy way to get access to it from msfconsole
without dropping to irb
. Definitely something we can add in the future, though.
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.
Yeah, most of us use the console. I've been getting the data via pry
or irb
, but it's less than ideal. The REST API is nice for programmatic interaction, but I don't see us running curl
commands every time we want to retrieve loot data.
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 think it's good we're moving away from the "expert user" experience. I can't reasonably tell someone they should be writing Ruby to retrieve data for a pentest.
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.
Sample file moved to PR description.
Solid point, @jmartin-r7. I'd love to expand this to fill what's expected from a |
workspace_id: wspace.id, | ||
origin_type: :import, | ||
filename: args[:filename], | ||
username: p[: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.
While we're at it, these values should be checked for existence each loop. Done.
And refactor slightly.
A bit of misunderstanding. We're in agreement that loot was enough.
Since the parser is focused on creds.
I've updated the import type to suggest that the focus is on creds. We can flesh out the parser and update the type later. Thanks, @jmartin-r7. |
report_loot( | ||
workspace: wspace, | ||
path: args[:filename], | ||
name: File.basename(args[:filename]), |
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.
Not sure I see the point of this if it's just the basename. But it fills in a column.
Finally ran through all the samples and cross-referenced with MS14-025. https://msdn.microsoft.com/en-us/library/cc232650.aspx https://support.microsoft.com/en-us/help/2962486/ms14-025-vulnerability-in-group-policy-preferences-could-allow-elevati
All right, I think this is ready to go. |
@@ -0,0 +1,41 @@ | |||
require 'rex/parser/group_policy_preferences' | |||
|
|||
module Msf::DBManager::Import::GPP |
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'm leaving the name as GPP
for future expansion... and I'm more liable to screw up the rename now.
Handling this myself, since there are no takers. It's been okayed by a couple folks. |
Release NotesDatabase import (creds and loot) of Group Policy Preferences credentials via the |
And take the Jaden Smith approach, as @busterb quipped to me. :)
This one's a little weird, since you normally import scans into Metasploit, but now that creds are first-class in the database, it makes more sense to be able to import them.
Currently, your alternatives are
post/windows/gather/credentials/gpp
, which requires a session, andauxiliary/scanner/smb/smb_enum_gpp
, which requires a network scan.https://msdn.microsoft.com/en-us/library/cc232652.aspx