[PATCH 2/2] crypt32: importing system root certs into volatile registry keys instead of dedicated RootStore

[Donat Enikeev]

Hi Jacek,

Thanks for the feedback

Re: semaphores, ok, will re-master

> I'm a bit nervous about implications of this patch. This will allow 
> changing the store, which was not possible earlier. Let me explain. 
> If user has sufficient rights:
> 
> - On Windows: warning dialogs appear when application tries to modify 
> root store. It's allowed, but only with user confirmation

According to my experiments and tests submitted with the patch (and run 
successfully across the windows versions), that is not exactly true.

With Administrative privileges on Windows, no warning appears when the 
application tries to install certificate to the Local*Machine* stores 
(including Root).

However it appears, when application tries to do the same with 
Current*User*\Root (therefore the test case is commented there as there 
is no way to suppress, and tests will hang)

So, considering that default Wine behavior (please correct me if not) - 
applications have Administrative rights, in a meaning that there is no 
strong access-control and they are allowed to do whatever they can - 
let me please rephrase your before/after patch list

*Current User* Root store modification

- On Windows: warning dialogs appear when application tries to modify 
*USER* root store. It's allowed, but only with user confirmation

- On current Wine: it's allowed without warning

- After the patch: it's allowed without warning

*Local Machine* Root store

- On Windows: it's allowed with no warning for a user with 
administrative privileges

- On current Wine: it's not allowed

- After the patch: it's allowed with no warning.

Thus, the patch makes things closer to Windows, and reaches the initial 
goal of the patch - to fix the bug with Cisco IP communicator that 
expects exactly such behavior.

Does it make sense?

If you still think it is better to do a step away with additional 
warning, we could add something very simple like this for LocalMachine 
and CurrentUser both

    typedef INT (WINAPI *MessageBoxA)(HWND,LPCSTR,LPCSTR,UINT);
    HMODULE hUser32 = LoadLibraryA("user32");
    MessageBoxA pMessageBoxA = (void *)GetProcAddress(hUser32, 
"MessageBoxA");

    if (pMessageBoxA && ( pMessageBoxA(NULL,
            "You are about to install a certificate from a 
certification authority (CA) to the Wine prefix. If you install this 
certificate, Wine will automatically trust any certificate issued by 
this CA. If you proceed you aknowledge this security risk.\nDo you want 
to install this certificate?",
	    "Security Warning"
            MB_OKCANCEL | MB_ICONWARNING | MB_TASKMODAL) != IDOK))
        return FALSE;

Please share your thoughts

Best,
Donnie

On Ср, ноя 2, 2016 at 7:58 , Jacek Caban <jacek at codeweavers.com> 
wrote:
> Hi Donat,
> 
> 
> The patch looks mostly good, I have just two final comments.
> 
> 
> On 10/28/16 5:16 PM, Donat Enikeev wrote:
>> -WINECRYPT_CERTSTORE *CRYPT_RootOpenStore(HCRYPTPROV hCryptProv, 
>> DWORD dwFlags)
>> +void CRYPT_ImportSystemRootCertsToReg(void)
>>   {
>> -    TRACE("(%ld, %08x)\n", hCryptProv, dwFlags);
>> +    HCERTSTORE store = NULL;
>> +    HKEY key;
>> +    LONG rc;
>> +    HANDLE mutex;
>>   -    if (dwFlags & CERT_STORE_DELETE_FLAG)
>> +    TRACE("\n");
>> +
>> +    mutex = CreateMutexW( NULL, FALSE, mutex_nameW);
>> +    if (!mutex)
> 
> There is a race in here. Sorry about wrong suggestion earlier, 
> CreateSemaphore seems more appropriate here. If semaphore is already 
> created, we need to wait for it to be satisfied. Otherwise we could 
> try to read partially processed registries.
> 
>>       {
>> -        WARN("root store can't be deleted\n");
>> -        SetLastError(ERROR_ACCESS_DENIED);
>> -        return NULL;
>> +        ERR("Failed to create mutex, %08x\n", GetLastError());
>> +        return;
>>       }
>> -    if (!CRYPT_rootStore)
>> +    if ( GetLastError() == ERROR_ALREADY_EXISTS )
>> +        return;
>> +
>> +    if (!(store = create_root_store()))
>> +        return;
>> +
>> +    rc = RegCreateKeyExW(HKEY_LOCAL_MACHINE, certs_root_pathW, 0, 
>> NULL, 0, KEY_ALL_ACCESS, NULL, &key, 0);
>> +    if (!rc)
>>       {
>> -        HCERTSTORE root = create_root_store();
>> +       if (!CRYPT_SerializeContextsToReg(key, REG_OPTION_VOLATILE, 
>> pCertInterface, store))
>> +            WARN("Failed to import system certs into registry, 
>> %08x\n", GetLastError());
>>   -        InterlockedCompareExchangePointer((PVOID 
>> *)&CRYPT_rootStore, root,
>> -         NULL);
>> -        if (CRYPT_rootStore != root)
>> -            CertCloseStore(root, 0);
>> +       RegCloseKey(key);
>>       }
>> -    CRYPT_rootStore->vtbl->addref(CRYPT_rootStore);
>> -    return CRYPT_rootStore;
>> -}
>>   -void root_store_free(void)
>> -{
>> -    CertCloseStore(CRYPT_rootStore, 0);
>> -}
>> +    CertCloseStore(store, 0);
>> +}
>> \ No newline at end of file
>> diff --git a/dlls/crypt32/store.c b/dlls/crypt32/store.c
>> index 356712b..19eca22 100644
>> --- a/dlls/crypt32/store.c
>> +++ b/dlls/crypt32/store.c
>> @@ -424,21 +424,15 @@ static WINECRYPT_CERTSTORE 
>> *CRYPT_SysRegOpenStoreW(HCRYPTPROV hCryptProv,
>>           SetLastError(E_INVALIDARG);
>>           return NULL;
>>       }
>> -    /* FIXME:  In Windows, the root store (even the current user 
>> location) is
>> -     * protected:  adding to it or removing from it present a user 
>> interface,
>> -     * and the keys are owned by the system process, not the 
>> current user.
>> -     * Wine's registry doesn't implement access controls, so a 
>> similar
>> -     * mechanism isn't possible yet.
>> -     */
>> -    if ((dwFlags & CERT_SYSTEM_STORE_LOCATION_MASK) ==
>> -     CERT_SYSTEM_STORE_LOCAL_MACHINE && !lstrcmpiW(storeName, 
>> rootW))
>> -        return CRYPT_RootOpenStore(hCryptProv, dwFlags);
>>         switch (dwFlags & CERT_SYSTEM_STORE_LOCATION_MASK)
>>       {
>>       case CERT_SYSTEM_STORE_LOCAL_MACHINE:
>>           root = HKEY_LOCAL_MACHINE;
>>           base = CERT_LOCAL_MACHINE_SYSTEM_STORE_REGPATH;
>> +        /* If the HKLM\Root certs are requested, expressing system 
>> certs into the registry */
>> +        if (!lstrcmpiW(storeName, rootW))
>> +            CRYPT_ImportSystemRootCertsToReg();
> 
> I'm a bit nervous about implications of this patch. This will allow 
> changing the store, which was not possible earlier. Let me explain. 
> If user has sufficient rights:
> 
> - On Windows: warning dialogs appear when application tries to modify 
> root store. It's allowed, but only with user confirmation (which 
> makes me think it's not a common way for installing new certs).
> 
> - On current Wine: it's not allowed.
> 
> - With your patch: it's allowed.
> 
> Ideally we'd do what Windows does, but that's a separated issue. I'd 
> propose to change the patch to simply behave like current Wine does 
> in the regards. It should be as simple as:
> 
> dwFlags |= CERT_STORE_READONLY_FLAG;
> 
> Does that sounds good to you?
> 
> Thanks,
> Jacek