[PATCH 2/2] crypt32: importing system root certs into volatile registry keys instead of dedicated RootStore
Donat Enikeev
donat at enikeev.net
Wed Nov 2 13:49:57 CDT 2016
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
More information about the wine-devel
mailing list