Re: black-box implementation of CryptProtectData/CryptUnprotectData

[Mike McCormack]

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