Improve credssp credentials handling #173
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
TheBestTvarynka
force-pushed
the
fix/CredsspSubmitBufferBothOld-handling
branch
from
955325d
to
f4cf01e
Compare
awakecoding
approved these changes
Sep 22, 2023
CBenoit
reviewed
Sep 22, 2023
ffi/src/sec_winnt_auth_identity.rs
Outdated
Comment on lines
14
to
16
| use windows_sys::Win32::Security::Authentication::Identity::SspiIsAuthIdentityEncrypted; | ||
| use winapi::um::wincred::{CredUIPromptForWindowsCredentialsW, CREDUI_INFOW}; |
There was a problem hiding this comment.
Note that the winapi crate is abandoned, and we should only use the windows crate from now on
There was a problem hiding this comment.
done
Should we create an issue (task) to replace all winapi usage with the windows crate?
There was a problem hiding this comment.
Because we have a lot of winapi crate usage
There was a problem hiding this comment.
Yes, you are right 👍
Do you think you could open this issue?
There was a problem hiding this comment.
yes, I can do it
There was a problem hiding this comment.
Do you think you could open this issue?
TheBestTvarynka
force-pushed
the
fix/CredsspSubmitBufferBothOld-handling
branch
from
8610f84
to
5d971fa
Compare
CBenoit
approved these changes
Sep 22, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in 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.
Hi, I wrote a fix for the #152 issue.
When we try to connect using the
mstscwe have two options for the source of the credentials: enter them manually ormstscwill use the saved credentials. Themstscpasses theCredsspSubmitBufferBothOld(51) only when we try to log in using the saved credentials. Moreover, it also sets thecredssp_cred.p_spnego_credpointer to NULL. So, in this case, we don't even have any actual credentials to use.Why we can't read the saved creds:
Because they are saved as
CRED_TYPE_DOMAIN_PASSWORD. If we try to read them using theCredReadfunction then we'll get an empty password blob (even when the function succeeded)."The credentials exposed here do not have to be manipulated in user applications, but by the Windows authentication manager ( LSASS), so there is no reason for them to be accessible in the user area." (src)
"Also, for CRED_TYPE_DOMAIN_PASSWORD, this member can only be read by the authentication packages." (src)
Workaround:
The TSSSP security package is supported only in Windows. So, we can just ask the user to re-enter the credentials and then use them for the authentication. This is what I've implemented in this fix.
Doc & references:
closes #152