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@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