Fix encoding issue with non-ASCII characters #238
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- add `safe_encode` to handle specific Metasploit cases when non-ASCII character strings are set to `ASCII-8BIT` - changes exposed `String#encode` calls to `safe_encode` - Monkey patch `rubyntlm`'s `ntlmv2_hash` methods to include a fix for an issue with NTLMv2 hashes when usernames contain non-ASCII characters
|
Hi @cdelafuente-r7 , I was working on some ASCII/UTF-8 issues and hooked up your changes to see if they would fix an issue we were running into on a call. I'll add a before and after comparing BeforeEncoding exception is raised: Note: the username AfterThe exception is now different: Note: the username StacktraceOriginal stacktrace from ruby_smb: and this is from framework logs: Was this PR in isolation able to fix the original metasploit-framework issue, or are there additional changes required in metasploit? |
|
Thanks @cgranleese-r7 for reporting this issue. I tracked down to a |
|
Re-tested your branch and it seems your latest changes fixed the issue on my environment as well.
Thanks for looking into this 👍 |
- Use prepend to monkey patch `Net::NTLM` - Move the patch to a separate file (custom/ntlm.rb) - Change `safe_encode` to a module class to avoid having to include the module everywhere
| @@ -0,0 +1,19 @@ | |||
| require 'net/ntlm' | |||
There was a problem hiding this comment.
To ensure this is loaded whenever ruby_smb is it might be worth adding a require 'custom/ntlm' here.
I am not sold on using Custom as the top level namespace in a gem, but I don't have a better option in mind so just noting this.
There was a problem hiding this comment.
Thanks! Addressed in 10997a3
Comment on lines
+3
to
+4
| module Custom | ||
| module NTLM |
There was a problem hiding this comment.
Filename ruby_smb/ntlm/custom/string_encoder
Suggested change
| module Custom | |
| module NTLM | |
| module RubySMB::NTLM::Custom | |
| module StringEncoder |
| end | ||
| end | ||
|
|
||
| Net::NTLM::EncodeUtil.send(:prepend, Custom::NTLM) |
There was a problem hiding this comment.
Suggested change
| Net::NTLM::EncodeUtil.send(:prepend, Custom::NTLM) | |
| Net::NTLM::EncodeUtil.send(:prepend, RubySMB::NTLM::Custom::StringEncoder) |
| @@ -1,3 +1,5 @@ | |||
| require 'ruby_smb/ntlm/custom/ntlm' | |||
There was a problem hiding this comment.
Suggested change
| require 'ruby_smb/ntlm/custom/ntlm' | |
| require 'ruby_smb/ntlm/custom/string_encoder' |
| @@ -6,11 +6,13 @@ | |||
| require 'openssl/cmac' | |||
| require 'windows_error' | |||
| require 'windows_error/nt_status' | |||
| require 'ruby_smb/ntlm/custom/ntlm' | |||
There was a problem hiding this comment.
Suggested change
| require 'ruby_smb/ntlm/custom/ntlm' | |
| require 'ruby_smb/ntlm/custom/string_encoder' |
|
I was able to reproduce the original issue and validate that the proposed changes fix it. Fixed / After PatchBroken / Before Patch |
smcintyre-r7
added a commit
that referenced
this pull request
Nov 17, 2022
Fix encoding issue with non-ASCII characters
|
I see we still have some outstanding comments here. Should we open another PR to address those? Jeffery's points seem valid here. |
|
Yes that can be a follow up PR, this at least gets the issue addressed as landed. |
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is related to this Metasploit issue.
This PR addresses an authentication error when usernames with non-ASCII characters are used. This PR fixes two things:
ASCII-8BITencoding.Calling
String#encodeon those strings were throwing aEncoding::UndefinedConversionErrorexception. To address this, asafe_encodewrapper method has been added. This method handles this specific Metasploit case and prevents it from breaking. This wrapper simply catchesEncodingexceptions and#force_encodeinstead of#encodethe string. This might not be the best solution, but since it seems to only happen in this specific scenario, I'm confident it is safe to do.Every calls to
String#encodeon a string that could come from a user input directly have been replace by#safe_encode.rubyntlmlibrarySee WinRb/rubyntlm#55 for details. A PR has been submitted, but since we want this to be fixed ASAP, the fix has been added to RubySMB directly. This PR adds the patched
rubyntlm'sntlmv2_hashmethod and forces theRubySMB::Clientto useRubySMB::NTLMinstead ofrubyntlmlibrary.Before the fix
After the fix