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