-
Notifications
You must be signed in to change notification settings - Fork 29
feat: NT Hash Authentication #585
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
base: master
Are you sure you want to change the base?
Conversation
TheBestTvarynka
left a comment
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.
Overall, the code is clean and well-documented. I tested the NTLM on my local environment, and it still works well. Thank you very much for the new tests! ❤️
But I have two concerns:
-
The
ffimodule does not compile. How to reproduce:cd ffi/ cargo buildIn
ffi, we handle raw buffers and convert them into Rust buffers. -
I do not like the fact that the feature, which is needed only for NTLM, affects other protocol implementations/credentials handling. The NT hash feature is only supported by the NTLM protocol.
Personally, I prefer the approach suggested by @awakecoding in #472 (comment):If I were to do it again, I think the best approach would be to pass the hash inside the password field with a known prefix unlikely to collide with a real password
Pros of this approach:
- It does not affect credentials handling. In your PR, we have
CredentialsTypeandCredentials, and they have completely different meanings. - It will not affect credentials handling in the
ffilayer and will keep the implementation locally inside thentlmmodule. - If you want to keep separate types for password and hash, then you can define them inside the
ntlmmodule and parse the password at the beginning of theNtlm::initialize_security_context_implmethod.
- It does not affect credentials handling. In your PR, we have
cc @CBenoit
|
Thanks for the review!
@TheBestTvarynka Could you share the build error you got? On my machine it compiled ➜ cd ffi
➜ cargo build
warning: profile package spec `bcrypt-pbkdf` in profile `ffi-production` did not match any packages
warning: profile package spec `num-bigint-dig` in profile `ffi-production` did not match any packages
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.42s
➜ ffi ⚡( feature/pass-the-hash)
▶ It does fail to compile for me with error: tsssp feature should be used only on Windows
--> src/lib.rs:81:1
|
81 | compile_error!("tsssp feature should be used only on Windows");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: could not compile `sspi` (lib) due to 1 previous error
I agree this is a bit intrusive to the existing API. I originally implemented it this way downstream since I wanted there to be no confusion that users were passing in a string containing an NT hash vs a plaintext password. Perhaps it's better to have those safeguards downstream. I think a good middle ground could be to use the existing password field with a known prefix as suggested by @awakecoding, but also keep the separate types for I'm also fine with only doing the prefix approach and documenting usage, my suggested approach is mainly for QoL. |
CBenoit
left a comment
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.
@z-jxy Thank you!
The compilation is failing, but on Windows only from what I see in the CI.
error[E0615]: attempted to take value of method `password` on type `sspi::AuthIdentityBuffers`
--> ffi\src\sspi\sec_winnt_auth_identity.rs:821:27
|
821 | auth_identity_buffers.password = password;
| ^^^^^^^^ method, not a field
|
= help: methods are immutable and cannot be assigned to
error[E0615]: attempted to take value of method `password` on type `sspi::AuthIdentityBuffers`
--> ffi\src\sspi\sec_winnt_auth_identity.rs:1084:27
|
1084 | auth_identity_buffers.password = password;
| ^^^^^^^^ method, not a field
|
= help: methods are immutable and cannot be assigned to
For more information about this error, try `rustc --explain E0615`.
src/auth_identity.rs
Outdated
| impl TryFrom<&[u8]> for NtlmHash { | ||
| type Error = NtlmHashError; | ||
|
|
||
| fn try_from(value: &[u8]) -> Result<Self, Self::Error> { | ||
| if value.len() != 16 { | ||
| return Err(NtlmHashError::ByteLength); | ||
| } | ||
|
|
||
| let mut hash = [0u8; 16]; | ||
| hash.copy_from_slice(value); | ||
|
|
||
| Ok(NtlmHash(hash)) | ||
| } | ||
| } | ||
|
|
||
| impl From<[u8; 16]> for NtlmHash { | ||
| fn from(value: [u8; 16]) -> Self { | ||
| NtlmHash(value) | ||
| } | ||
| } | ||
|
|
||
| impl From<NtlmHash> for [u8; 16] { | ||
| fn from(hash: NtlmHash) -> Self { | ||
| hash.0 | ||
| } | ||
| } | ||
|
|
||
| impl TryFrom<&str> for NtlmHash { | ||
| type Error = NtlmHashError; | ||
|
|
||
| fn try_from(value: &str) -> Result<Self, Self::Error> { | ||
| if value.len() != 32 { | ||
| return Err(NtlmHashError::StringLength); | ||
| } | ||
|
|
||
| let mut hash = [0u8; 16]; | ||
| for i in 0..16 { | ||
| let hex_byte = &value[i * 2..i * 2 + 2]; | ||
| hash[i] = u8::from_str_radix(hex_byte, 16).map_err(|_| NtlmHashError::Hex)?; | ||
| } | ||
|
|
||
| Ok(NtlmHash(hash)) | ||
| } | ||
| } | ||
|
|
||
| // for compatibility with clap | ||
| impl std::str::FromStr for NtlmHash { | ||
| type Err = NtlmHashError; | ||
|
|
||
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| NtlmHash::try_from(s) | ||
| } | ||
| } | ||
|
|
||
| impl AsRef<NtlmHash> for NtlmHash { | ||
| fn as_ref(&self) -> &NtlmHash { | ||
| self | ||
| } | ||
| } |
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.
Nice newtype, but the trait pileup makes it feel heavier than it needs to be. I would avoid some of them, especially for the public API that we can’t change without creating breaking changes.
Good:
- Newtype wrapper around
[u8; 16]is a clean way to avoid mixing arbitrary bytes with "NT hash bytes". TryFrom<&[u8]>is a reasonable convenience, although having an explicit function may be good.
Things that feel unnecessary / non-idiomatic:
- Too many conversion traits for the amount of value they add.
- Boundary decision: should the library accept hex strings at all? Since this crate is primarily protocol / auth logic, I think hex parsing should belong in the CLI/app layer instead.
suggestion: Drop at least
AsRef<NtlmHash> for NtlmHashTryFrom<&str>impl FromStr for NtlmHash
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 agree there's a heavy amount of conversion traits. I'd have to review usage from my fork to see where it would come into play downstream. If I remember correctly there were actual scenarios where each of these were needed.
AsRef<NtlmHash> for NtlmHash I think this was because I couldn't decide whether it should take NtlmHash by reference or value in a function. Since it now implements Copy it makes sense to remove this.
impl FromStr for NtlmHash was needed so it can be used as a value parser for clap. Most CLI apps will probably want to just accept the hash as an argument so this is nice to have
TryFrom<&str> I had originally implemented only this, but later realized that impl FromStr for NtlmHash was needed for it to work with clap, so I just forwarded the implementation for FromStr to TryFrom<&str>. Usually my instinct is to reach for .try_into() before I think about .parse(), but I suppose both aren't really needed.
Boundary decision: should the library accept hex strings at all? Since this crate is primarily protocol / auth logic, I think hex parsing should belong in the CLI/app layer instead.
The idea is to use AuthIdentityBuffers::from_utf8_with_hash which accepts the NtlmHash type and will call to_sspi_password on it so they're not directly passing in the hex value
I think keeping the hex parsing logic makes sense though for QoL. We need to parse the hex string to get the underlying bytes for the hmac key, so this would just make that method public for end users.
Scenario
CLI applications are going to receive the hash as a String, this leaves developers with a few options:
- Accept the CLI argument as a plain String and call
format!with the prefix before passing toAuthIdentityBuffers::from_utf8. If it fails, it fails - They parse the hex string value into
[u8; 16], then construct anNtlmHash, they pass this toAuthIdentityBuffers::from_utf8_with_hashNtlmHashdoesn't do much for them here
- If using clap, they create a new type that wraps
NtlmHash, and implement the same hex parsing logic usingFromStr- This logic will be mostly the same across every application
Instead they could just use the NtlmHash type to parse the string (this can be done with clap since it implements FromStr). Then just pass it directly to AuthIdentityBuffers::from_utf8_with_hash.
I agree with this! |
Sorry for the repeated comments. I’m also sensible to the type safety argument, and I think it does make sense to have a QoL interface like you mention 👍 |
|
Thanks for all the feedback so far. I've made the following changes:
|
NT Hash (Pass-The-Hash) Authentication Support
Adds support for NT hash (pass-the-hash) authentication to
sspi, allowing authentication using pre-computed NT hashes instead of plaintext passwords.This resolves #472
Usage
The following snippet is from my fork of ldap3 (downstream crate that needs this functionality).
Disclosure
Most changes here are a result of copy/pasting my own manual implementation into a folder locally and asking Claude to integrate the functionality to this codebase, so it's possible there's some weird integration choices.
Testing
I've tested this against a local AD environment using my fork of ldap3 and was able to authenticate.