[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 17:11:00 CDT 2016
Hi Jacek,
Regarding thread safe part, to use CreateSemaphore instead of
CreateMutex
After a deeper thinking and code/wiki reading, it would be better to
get some clarify first, I believe, probably I missed something on the
way.
The initial idea of that part was to import system root certificates
into registry once per user session, as I could hardly imagine a case,
when we need immediate (in magnitude of minutes) reaction to the root
certificate untimely revocation (this happens rarely, loudly) or
issuing (this with certain inertia).
Thus, by proposing mutex usage previously first time, did you mean that
each crypt32 instance should try to safely import certificates each
time, whenever it is attached during a session?
If once-per-session is still reasonably fine, could you please clarify
a reason the InterlockedIncrement() from the previous patch is worse
for this case than CreateMutex/CreateSemaphore? Or point to some
previous discussion or send link to a code, that will clarify the
picture.
It will help me a lot, as the InterlockedIncrement looks atomic and
doing what is necessary (at least according to my understanding how
this part would work)
Looking forward
Best,
Donnie
On Ср, ноя 2, 2016 at 9:49 , Donat Enikeev <donat at enikeev.net>
wrote:
> 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