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 KR 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
Re: black-box implementation of CryptProtectData/CryptUnprotectData
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 [EMAIL PROTECTED]
Re: black-box implementation of CryptProtectData/CryptUnprotectData
On Sun, Apr 03, 2005 at 11:04:53PM -0500, James Hawkins wrote: On Apr 3, 2005 10:12 PM, Kees Cook [EMAIL PROTECTED] wrote: To store the triplets, these functions use the registry: Registry Layout: HKEY_CURRENT_USER\Software\Wine\Crypt\ProtectData\Map\[index] Cipher: HEX string Entropy: HEX string DataDescription: WCHAR Plain: HEX string I'll have to be honest that I haven't looked over the code yet, but I do have a concern. Do these values have to be stored under Software\Wine? That location is for wine-specific configuration options and not api use, unless I'm mistaken. Do we know if this data is stored in the registry in windows, and if so where is it stored? (because I know it's not Software\Wine ;-) Windows doesn't store the results anywhere: it's just a symmetric crypto function. Since we don't know the function, we have to store the original data somewhere so we can return it later. Since this is entirely a Wine-only implementation of the encryption, I wanted to put it somewhere in the registry totally separate from all the other keys. Within the Wine tree seemed like the best place. -- Kees Cook@outflux.net
Re: black-box implementation of CryptProtectData/CryptUnprotectData
On Sun, Apr 03, 2005 at 11:56:34PM -0500, James Hawkins wrote: we do it should be encapsulated. What I'm getting at is that just because this implementation is wine-specific doesn't mean some of the implementation data should go in Software\Wine. Software\Wine is where the configurations of the wine program itself, not its implementation, are contained. See what I mean? btw it's great that Sure, that's fine by me. I wasn't really sure where to put it, but it's easy to change; it's just a path at the top of the file. In looking around at other examples, it seemed the most sensible. Is there a normal implementation-data-storage area for Wine? I just wanted to avoid clashing with any other registry items. I'm open to any suggestions. :) you wrote this code and submitted it. Sending the code in is the big first step. I just think we should get a community opinion on this matter (ya never know, everyone may disagree with me!). Keep up the good coding. Thanks! -- Kees Cook@outflux.net
Re: black-box implementation of CryptProtectData/CryptUnprotectData
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 KR 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
Re: black-box implementation of CryptProtectData/CryptUnprotectData
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
Re: black-box implementation of CryptProtectData/CryptUnprotectData
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
Re: black-box implementation of CryptProtectData/CryptUnprotectData
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
Re: black-box implementation of CryptProtectData/CryptUnprotectData
On Apr 3, 2005 10:12 PM, Kees Cook [EMAIL PROTECTED] wrote: To store the triplets, these functions use the registry: Registry Layout: HKEY_CURRENT_USER\Software\Wine\Crypt\ProtectData\Map\[index] Cipher: HEX string Entropy: HEX string DataDescription: WCHAR Plain: HEX string I'll have to be honest that I haven't looked over the code yet, but I do have a concern. Do these values have to be stored under Software\Wine? That location is for wine-specific configuration options and not api use, unless I'm mistaken. Do we know if this data is stored in the registry in windows, and if so where is it stored? (because I know it's not Software\Wine ;-) -- James Hawkins
Re: black-box implementation of CryptProtectData/CryptUnprotectData
On Apr 3, 2005 11:44 PM, Kees Cook [EMAIL PROTECTED] wrote: Windows doesn't store the results anywhere: it's just a symmetric crypto function. Since we don't know the function, we have to store the original data somewhere so we can return it later. Since this is entirely a Wine-only implementation of the encryption, I wanted to put it somewhere in the registry totally separate from all the other keys. Within the Wine tree seemed like the best place. I understand your logic behind the decision, but while it is true that the details of wine's implementation of the 'encryption' is different from that of windows', these details aren't a trait of wine itself. Wine is an implementation of the Windows api, but the details of how we do it should be encapsulated. What I'm getting at is that just because this implementation is wine-specific doesn't mean some of the implementation data should go in Software\Wine. Software\Wine is where the configurations of the wine program itself, not its implementation, are contained. See what I mean? btw it's great that you wrote this code and submitted it. Sending the code in is the big first step. I just think we should get a community opinion on this matter (ya never know, everyone may disagree with me!). Keep up the good coding. -- James Hawkins
Re: black-box implementation of CryptProtectData/CryptUnprotectData
On Apr 4, 2005 12:00 AM, Kees Cook [EMAIL PROTECTED] wrote: Sure, that's fine by me. I wasn't really sure where to put it, but it's easy to change; it's just a path at the top of the file. In looking around at other examples, it seemed the most sensible. Is there a normal implementation-data-storage area for Wine? I just wanted to avoid clashing with any other registry items. I'm open to any suggestions. :) I wish I could give a definitive answer, but I think waiting for the weekend to be over so more of the developers will be able to give input is a good idea. It could even turn out that the best place to put it actually is Software\Wine, so we'll see what happens. -- James Hawkins
