Skip to content
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

fix windows credentials #1770

Closed
wants to merge 2 commits into from
Closed

fix windows credentials #1770

wants to merge 2 commits into from

Conversation

weihongx9315
Copy link

Completed the credential implementation of the sandbox, after the fix, the password can be saved correctly when the rdp is connected

@isaak654 isaak654 requested a review from DavidXanatos April 6, 2022 10:51
Copy link
Member

@DavidXanatos DavidXanatos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just removing the error messages from the A versions is wrong as they are not properly redirected as the w versions is the tools for which you fixed this using the A versions?
I think first we need to find out why sandboxie redirects CredWrite to use WriteItem instead and then add a proper implementation for the A functions. Can you please privide a couple test cases so i can test the fix in praxis?

@weihongx9315
Copy link
Author

just removing the error messages from the A versions is wrong as they are not properly redirected as the w versions is the tools for which you fixed this using the A versions? I think first we need to find out why sandboxie redirects CredWrite to use WriteItem instead and then add a proper implementation for the A functions. Can you please privide a couple test cases so i can test the fix in praxis?

CredWriteA/W is not the same as other win32 api(such as getaddrinfo/GetAddrInfoW),other A api internal calls W api,but CredWriteA not.Sbie redirects this api's purpose is not for 'redirect',it implements a whole of credential methods,so it just use WriteItem to do it.But the question is,when it saves credential username and password,it writes wrong password...
To test this is very simple,you can use mstsc.exe to connect another windows PC and choose save password,you can see it doesn't work~

@APMichael
Copy link
Contributor

I am not sure if this information is helpful for this:

"... Note: Sandboxie stores credentials in the sandboxed protected storage ..."
https://sandboxie-plus.com/sandboxie/opencredentials/

"... The Sandboxie PStore is stored in the file SbiePst.dat in the sandboxed Windows folder ..."
https://sandboxie-plus.com/sandboxie/protectedstorage/

@weihongx9315
Copy link
Author

I am not sure if this information is helpful for this:

"... Note: Sandboxie stores credentials in the sandboxed protected storage ..." https://sandboxie-plus.com/sandboxie/opencredentials/

"... The Sandboxie PStore is stored in the file SbiePst.dat in the sandboxed Windows folder ..." https://sandboxie-plus.com/sandboxie/protectedstorage/

sbie implement the credentials itself,but it doesn't work as normal .That's why I fixed it,before this commit , you can test mstsc.exe to save password

@isaak654 isaak654 requested review from DavidXanatos and removed request for DavidXanatos April 7, 2022 16:42
@isaak654
Copy link
Collaborator

After discussing with the lead developer, any contribution to this pull request would be appreciated, including but not limited to:
@diversenok @typpos @flamencist @mitchcapper @7eRoM

@isaak654 isaak654 added the Help Wanted Extra help is needed label Apr 13, 2022
@DavidXanatos
Copy link
Member

@weihongx9315 this is still an open issue but it requriers as said a more in depth analysis and a propper fix, to just get it to work you can use OpenCredentials=y

What sandboxie does is quite more elaborate, it changes a cred entry which would be saved by windows globally into a sort that gets saved to a file a file which is then sandboxed, so the saved credentials are contained within the sandbox.
It needs to be investigated why this mechanism no longer works and how it can be repaired, as we don't want things to be saved globally by default.

@DavidXanatos
Copy link
Member

DavidXanatos commented May 28, 2022

A workable fix will be included in the next build v1.1.1, the issue was with Cred_GetName, it seems that it's not correct to save the type as part of the name, for now i just disabled adding type to the name string, but presumably the fix should be properly applied when names are compared and allow 1 as wildcard for any when reading credentials.

@isaak654 isaak654 added Status: Fixed in Next Build Fixed in the next Sandboxie version and removed Help Wanted Extra help is needed labels May 28, 2022
@weihongx9315
Copy link
Author

A workable fix will be included in the next build v1.1.1, the issue was with Cred_GetName, it seems that it's not correct to save the type as part of the name, for now i just disabled adding type to the name string, but presumably the fix should be properly applied when names are compared and allow 1 as wildcard for any when reading credentials.

Yes,the real reason is Cred_GetName,and my approach is to modify this, so that the credentials will not leak out of the sandbox. This function just write the wrong credentials,so we just need to fix this

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Fixed in Next Build Fixed in the next Sandboxie version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants