-
Notifications
You must be signed in to change notification settings - Fork 82
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
Enable Kerberos auth for DCERPC #253
Conversation
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.
Thanks @smashery for updating the library! This is a great thing to add support to Kerberos authentication in MSF, awesome work! My only regret is that it is not implemented in RubySMB itself. That would have been even better and many RubySMB users would have benefit from this. But I understand the limitations and all the Kerberos dependencies to MSF.
That being said, I left a few comments for you to review when you get a chance. One main issue I saw during testing was the signature validation. Please, let me know if you have any questions.
Thanks for the review @cdelefuente-r7. Agreed regarding having the Kerberos functionality available here. I wonder whether it would be worth a refactoring job to pull it out into either here, or a separate repo? |
Thanks for updating this @smashery! Everything looks good to me know. I retested and confirmed the signature verification issue is fixed 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.
Sorry for the last minute comment before it lands. I just noticed a few comments from last week were not answered yet. Also, I added one more about the padding length calculation logic in the request.
lib/ruby_smb/dcerpc/request.rb
Outdated
onlyif: -> { has_auth_verifier? }, | ||
length: -> { (16 - (stub.num_bytes % 16)) % 16 } | ||
|
||
string :auth_pad, onlyif: -> { has_auth_verifier? } |
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 I missed this one. Do you think we could use the original implementation here too?
string :auth_pad, onlyif: -> { has_auth_verifier? } | |
string :auth_pad, | |
onlyif: -> { has_auth_verifier? }, | |
length: -> { calculate_padding_size } |
@cdelafuente-r7 I opened a PR implemented the changes you requested here smashery#1. Give it a look and let me know what you think |
Tested this with the Metasploit side of things and it's all looking good to me now! Thanks for your work on this Smashery, this is a great improvement!
|
This PR supports the kerberos-in-DCSync work in the Metasploit repo (see: rapid7/metasploit-framework#18419). Test cases are listed in that PR, and should exercise all of the changes in this PR.