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

[Jacek Caban]

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