Re: [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

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

[Donat Enikeev]

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(a)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

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

[Donat Enikeev]

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(a)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(a)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

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

[Jacek Caban]

On 02.11.2016 23:11, Donat Enikeev wrote:
> 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.

I meant a race like this:
1. process 1 calls CRYPT_ImportSystemRootCertsToReg, creates a mutex and
starts to process system certs
2. process 2 calls CRYPT_ImportSystemRootCertsToReg, mutex exists so
reads certs from registry which is empty at this point
3. process 1 writes to registry

See [1] for an example how this may be handled.

Thanks,
Jacek

[1]
http://source.winehq.org/git/wine.git/blob/cc361eefcef4e1fe0f15ecbf1aae65bcd...

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

[Donat Enikeev]

Hi Jacek,
> I meant a race like this:

Didn't look from this angle. I was able to reproduce the race with some 
tricks and did additional reading afterwards. Apologies for excess 
questions and thanks for the help and the link.

I have changed and re-submitted both patches

Best,
Donnie

On Чт, ноя 3, 2016 at 3:33 , Jacek Caban <jacek(a)codeweavers.com> 
wrote:
> On 02.11.2016 23:11, Donat Enikeev wrote:
> > 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.
>
> I meant a race like this:
> 1. process 1 calls CRYPT_ImportSystemRootCertsToReg, creates a mutex 
> and
> starts to process system certs
> 2. process 2 calls CRYPT_ImportSystemRootCertsToReg, mutex exists so
> reads certs from registry which is empty at this point
> 3. process 1 writes to registry
>
> See [1] for an example how this may be handled.
>
> Thanks,
> Jacek
>
> [1]
> http://source.winehq.org/git/wine.git/blob/cc361eefcef4e1fe0f15ecbf1aae65bcd...

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

[Jacek Caban]

On 02.11.2016 19:49, Donat Enikeev 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.

You're right, sorry. The patch is fine in this regard then.

Thanks,
Jacek