Re: black-box implementation of CryptProtectData/CryptUnprotectData
Kees Cook wrote:
This patch implements a functional replacement for crypt32.dll's CryptProtectData and CryptUnprotectData. It does _not_ perform any encrypt/decryption, but rather tracks the cipher/entropy/plain triplets so that programs depending on the calls will operate correctly.
Perhaps you could make it work "right" by using a key stored in ssh-agent?
diff -u -p -u -p -r1.92 ChangeLog
Just writing a ChangeLog entry like this is OK: ChangeLog: * Added black-box implementation of CryptProtectData/CryptUnprotectData. You don't need to try patch ChangeLog, because it's going to change alot as patches are applied. Just write the message you want to put in there.
+ hr = HRESULT_FROM_WIN32(RegOpenKeyExW(HKEY_CURRENT_USER, wszProtectDataMap, 0, KEY_READ, &hkeyMap)); + if (!SUCCEEDED(hr))
Why do you convert the error code to a HRESULT here? Since you don't do it elsewhere in your code, why not compare the returned value to ERROR_SUCCESS, like you do below?
+ if (RegEnumKeyExW(hkeyMap, dwIndex, wszIndexKey, &dwIndexKeyLength, NULL, NULL, NULL, NULL) != ERROR_SUCCESS) + break;
Personally, I prefer the following, as it makes the lines shorter, makes it easier to add a printf("%ld\n",r); and makes the comparison more obvious. r = RegEnumKeyExW(hkeyMap, ... if( r != ERROR_SUCCES ) break;
+ if (WINE_TRACE_ON(crypt)) + wine_dbg_printf("\tChecking Map Index %s\n", debugstr_w(wszIndexKey));
You don't need the WINE_TRACE_ON() check, because TRACE already does that for the default debug channel, so the following is the same: TRACE("\tChecking Map Index %s\n", debugstr_w(wszIndexKey);
+ if (!bFound) { + TRACE("no matches\n"); + }
Sometimes you used K&R style brackets and indenting, sometimes you used ANSI C style. It's better to choose one or the other and stick to it.
+ return SUCCEEDED(RegSetValueExW(hkeyOpen,wszName,0,dwType, + pData.pbData,pData.cbData));
The SUCCEEDED() macro is only for HRESULT values, so the above is going to succeed in alot of cases where it shouldn't.
+ } + + DWORD dwDisposition;
In an effort to maintain portability, we don't use C99 style variable declarations. Mike
On Monday 04 April 2005 08:01, Mike McCormack wrote:
Kees Cook wrote:
This patch implements a functional replacement for crypt32.dll's CryptProtectData and CryptUnprotectData. It does _not_ perform any encrypt/decryption, but rather tracks the cipher/entropy/plain triplets so that programs depending on the calls will operate correctly.
Perhaps you could make it work "right" by using a key stored in ssh-agent?
Having a "correct" implementation for this would be cool for rsaenh, too. Currently persistent RSA keys are stored in the registry in plaintext. Bye, -- Michael Jung mjung(a)iss.tu-darmstadt.de
On Mon, Apr 04, 2005 at 03:01:53PM +0900, Mike McCormack wrote:
Perhaps you could make it work "right" by using a key stored in ssh-agent?
Well, by working "right", it means that taking a cipher/entropy from Windows and calling CryptUnprotectData on it in Wine would return the plain text. This isn't going to be possible until we know what Windows keys off of to tie it to a machine and user. I figure the first step is to make the functions work within Wine, then if the encryption is ever understood, the calls can be replaced.
You don't need to try patch ChangeLog, because it's going to change alot as patches are applied. Just write the message you want to put in there.
Okay, cool.
+ hr = HRESULT_FROM_WIN32(RegOpenKeyExW(HKEY_CURRENT_USER, wszProtectDataMap, 0, KEY_READ, &hkeyMap)); + if (!SUCCEEDED(hr))
Why do you convert the error code to a HRESULT here? Since you don't do it elsewhere in your code, why not compare the returned value to ERROR_SUCCESS, like you do below?
Well, mostly I was copying from other examples I found, especially the filtergraph code in dlls/quartz. I'm happy to change that, of course. :)
Personally, I prefer the following, as it makes the lines shorter, makes it easier to add a printf("%ld\n",r); and makes the comparison more obvious.
r = RegEnumKeyExW(hkeyMap, ... if( r != ERROR_SUCCES ) break;
Okay, I can clean this up.
You don't need the WINE_TRACE_ON() check, because TRACE already does that for the default debug channel, so the following is the same:
Actually, I did that to avoid the line prefix that "TRACE" adds. All the stuff where I call the dbg functions directly are part of helper functions, and seeing their names is confusing while watching a Protect/Unprotect session.
Sometimes you used K&R style brackets and indenting, sometimes you used ANSI C style. It's better to choose one or the other and stick to it.
Sorry about that. I tried to stick to what seemed to be the wine style, with the braces on separate lines. However, that's not what I'm used to, so a few of mine snuck in. :)
+ return SUCCEEDED(RegSetValueExW(hkeyOpen,wszName,0,dwType, + pData.pbData,pData.cbData));
The SUCCEEDED() macro is only for HRESULT values, so the above is going to succeed in alot of cases where it shouldn't.
Doesn't RegSetValueExW return an HRESULT?
In an effort to maintain portability, we don't use C99 style variable declarations.
Ah, dang. I tried to clean those up too when I was reading the Patch how-to. I'll clean all this up, thanks very much! BTW: what is your opinion on where to store the triplets in the Registry? -- Kees Cook @outflux.net
Kees Cook wrote:
Actually, I did that to avoid the line prefix that "TRACE" adds. All the stuff where I call the dbg functions directly are part of helper functions, and seeing their names is confusing while watching a Protect/Unprotect session.
It's probably better to keep it consistent with what the rest of Wine does.
Doesn't RegSetValueExW return an HRESULT?
No, it returns a Win32 error code.
BTW: what is your opinion on where to store the triplets in the Registry?
If there's no equivilent keys in Windows, then under the Wine key sounds ok to me. It seems like you need to investigate what it does on Windows and the MSDN description of the function a bit more. The description on MSDN indicated that they used a per user key generated when the user logs in. Mike
On Tue, Apr 05, 2005 at 01:07:14AM +0900, Mike McCormack wrote:
It's probably better to keep it consistent with what the rest of Wine does.
I'd really like to push back on this. The traces become unreadable as the various function names change. I think the debugging as I have it is more useful than how it looks with only "TRACE" calls. The top-level function spits out a "TRACE" to identify the caller, and then all the helper functions report the data structures.
It seems like you need to investigate what it does on Windows and the MSDN description of the function a bit more. The description on MSDN indicated that they used a per user key generated when the user logs in.
I already have, and decided it was best to avoid a more detailed investigation for fear of DMCA joy. They key against at least user, machine, and time, since multiple calls with the same plain/entropy produces different ciphers. My implementation intentionally avoids any encryption at all. :) I like to think of it as a good "first step" to getting the real functions. With what I've got, a program can run normally. I'll be sending "version 2" of my patch in a little while. It's got your suggestions incorporated, and a small bug fix. -- Kees Cook @outflux.net
Kees Cook wrote:
It's probably better to keep it consistent with what the rest of Wine does.
I'd really like to push back on this. The traces become unreadable as the various function names change. I think the debugging as I have it is more useful than how it looks with only "TRACE" calls. The top-level function spits out a "TRACE" to identify the caller, and then all the helper functions report the data structures.
Well, I'm not the one who decides. Alexandre doesn't always have the time to explain why he likes or dislikes each patch, so I'm only guessing what might keep your patch from being applied :)
I'll be sending "version 2" of my patch in a little while. It's got your suggestions incorporated, and a small bug fix.
Great! Mike
participants (3)
-
Kees Cook -
Michael Jung -
Mike McCormack