Hi Donat,
On 10/16/16 7:50 PM, Donat Enikeev wrote:
Hi Guys, Watched video recently from WineConf 2015, and resulting directions around opening community looks really promising, so I am performing 3rd attempt to be useful here :) The bug https://bugs.winehq.org/show_bug.cgi?id=30187 : Cisco IP Communicator failing to setup due to 'certmgr.exe' tool fails to install certificate to the system-wide trusted certificate store. Wine uses registry-based certificates stores by default, but treats HKLM\Root store as a special case. Whenever app opens such store, Crypt32 goes through hard-coded paths in rootstore.c: static const char * const CRYPT_knownLocations[] = { "/etc/ssl/certs/ca-certificates.crt", "/etc/ssl/certs", ... }; And adds all found certificates to the special store. In the context of bug, this special-case store doesn't support adding certificates, and thus Cisco IP fails to install. So a backward compatible patch (attached) that just fixes this bug looks straightforward: make a stores collection, add there HKLM\Root certificates registry store at first and then that system store with certificates from the environment, and return the collection. It will allow applications do whatever they used to with certificates, keeping all the linux certificates available for verification of any kind. The problem with this approach is that current wine crypt32 doesn't actually save certificates that were added to a collection of stores (the test for this attached), while Win does. Although it could be fixed with a different patch in one function, but I would like to hear your thoughts first around following questions:
- Does this backward-compatible patch-set make sense at all and worth
proceeding? Probably you have some ongoing activities 2. Do you still think that wine should import system certificates during HKLM\Root request at all, not just shipping with those from typical windows installation? 3. Have you considered different approach of utilizing system certificates in Wine? For example, import all system certificates to the *registry* during wine-prefix initialization process, and work with them from there in a way windows does (even native crypt32 will benefit from this approach). That will allow to unify and simplify crypt32 and remove all that arguable hard-coded paths in the code, and bring more familiar environment to the windows application and, at the same time, isolation.
First of all, I think we want to keep current solution of using host system's cert store and configuration as much as possible. I agree that hardcoded paths are not nice, but if we don't have better generic solution, we have to live with that (although it's possible to improve it in some cases, like it's done for Mac).
That said, I think that expressing system certificates in Wine registry would be the right solution. How about instead of current CRYPT_RootOpenStore call, we'd create registry entries expressing system certificates using REG_OPTION_VIOLATILE once for Wine session? Then we could use regular registry store for root store.
Thanks, Jacek * *
Hi Jacek,
I agree that hardcoded paths are not nice, but if we don't have better generic solution, we have to live with that
So, could expressing certificates into registry once and beforehand during wine-prefix creation - be that one "better generic solution"? I think I could spend some time on it, if that considered a right way to go
How about instead of current CRYPT_RootOpenStore call, we'd create registry entries expressing system certificates using REG_OPTION_VIOLATILE once for Wine session?
That will work as well, but would require another custom wine-specific piece of code for the certificates import using that flag for registry keys OR passing special flag to the existing Store management functions. In general, if sticking to the current way of implementation is the most reasonable option in the context, just proper handling of flags and priorities for collections is required (following Remarks at https://msdn.microsoft.com/en-us/library/windows/desktop/aa376022(v=vs.85).a... ), so the collection will forward added certificates to the appropriate store: registry one with RW access, not the memory based created by CRYPT_RootOpenStore with RO access
Considering these, what do you think?
Best, Donnie
On Вс, окт 16, 2016 at 10:07 , Jacek Caban jacek@codeweavers.com wrote:
Hi Donat,
On 10/16/16 7:50 PM, Donat Enikeev wrote:
Hi Guys, Watched video recently from WineConf 2015, and resulting directions around opening community looks really promising, so I am performing 3rd attempt to be useful here :) The bug https://bugs.winehq.org/show_bug.cgi?id=30187 : Cisco IP Communicator failing to setup due to 'certmgr.exe' tool fails to install certificate to the system-wide trusted certificate store. Wine uses registry-based certificates stores by default, but treats HKLM\Root store as a special case. Whenever app opens such store, Crypt32 goes through hard-coded paths in rootstore.c: static const char * const CRYPT_knownLocations[] = { "/etc/ssl/certs/ca-certificates.crt", "/etc/ssl/certs", ... }; And adds all found certificates to the special store. In the context of bug, this special-case store doesn't support adding certificates, and thus Cisco IP fails to install. So a backward compatible patch (attached) that just fixes this bug looks straightforward: make a stores collection, add there HKLM\Root certificates registry store at first and then that system store with certificates from the environment, and return the collection. It will allow applications do whatever they used to with certificates, keeping all the linux certificates available for verification of any kind. The problem with this approach is that current wine crypt32 doesn't actually save certificates that were added to a collection of stores (the test for this attached), while Win does. Although it could be fixed with a different patch in one function, but I would like to hear your thoughts first around following questions:
- Does this backward-compatible patch-set make sense at all and
worth proceeding? Probably you have some ongoing activities 2. Do you still think that wine should import system certificates during HKLM\Root request at all, not just shipping with those from typical windows installation? 3. Have you considered different approach of utilizing system certificates in Wine? For example, import all system certificates to the *registry* during wine-prefix initialization process, and work with them from there in a way windows does (even native crypt32 will benefit from this approach). That will allow to unify and simplify crypt32 and remove all that arguable hard-coded paths in the code, and bring more familiar environment to the windows application and, at the same time, isolation.
First of all, I think we want to keep current solution of using host system's cert store and configuration as much as possible. I agree that hardcoded paths are not nice, but if we don't have better generic solution, we have to live with that (although it's possible to improve it in some cases, like it's done for Mac).
That said, I think that expressing system certificates in Wine registry would be the right solution. How about instead of current CRYPT_RootOpenStore call, we'd create registry entries expressing system certificates using REG_OPTION_VIOLATILE once for Wine session? Then we could use regular registry store for root store.
Thanks, Jacek *
Hi Donat,
On 10/16/16 9:34 PM, Donat Enikeev wrote:
Hi Jacek,
I agree that hardcoded paths are not nice, but if we don't have better generic solution, we have to live with that
So, could expressing certificates into registry once and beforehand during wine-prefix creation - be that one "better generic solution"? I think I could spend some time on it, if that considered a right way to go
This solution would change nothing in that regards - you still need hardcoded list of paths to do the import. You would just use it slightly differently.
Also, I did not say to do that for wine prefix creation. User may intentionally remove a certificate in host system after wineserver is created and we should reflect that in Wine (not to mention moving wine prefix to another machine). That's why I suggested using REG_OPTION_VIOLATILE - so that registry keys would be gone when wineserver terminates (aka. Windows shutdown) and we'd recreate them the next time root store is created.
And yes, I would consider such solution better.
How about instead of current CRYPT_RootOpenStore call, we'd create registry entries expressing system certificates using REG_OPTION_VIOLATILE once for Wine session?
That will work as well, but would require another custom wine-specific piece of code for the certificates import using that flag for registry keys OR passing special flag to the existing Store management functions.
I'm not sure why you need more that we already have. We already have special handling for root store in CRYPT_SysRegOpenStoreW. You could just change it to call new import_system_certs function and continue like for other stores instead of calling CRYPT_RootOpenStore and returning.
In general, if sticking to the current way of implementation is the most reasonable option in the context, just proper handling of flags and priorities for collections is required (following Remarks at https://msdn.microsoft.com/en-us/library/windows/desktop/aa376022(v=vs.85).a... ), so the collection will forward added certificates to the appropriate store: registry one with RW access, not the memory based created by CRYPT_RootOpenStore with RO access
I would need deeper look at the code to comment that. However, note that with that we'd use registry stores that are already used for non-root cases. That said, I wouldn't be surprised if you find that existing code would needs fixes.
Thanks, Jacek
Hi Jacek,
Following your proposal over the previous patch, that it should be a step forward to the implementation closer to the original behaviour,
I've prepared different version of patch:
1. sligthly changed existing functions saving certs to the registry (to be able to pass the REG_OPTION_VOLATILE flag) 2. replaced the RootStore entity and everything related with new CRYPT_ImportSystemRootCertsToReg() that does actual import (once) into REG_OPTION_VOLATILE keys 3. start calling this import whenever an application is reaching necessity to work with HKLM\Root certs store as previously, however the latter call could be moved to the DllMain
Does it look better?
P.s. The CiscoIP and certmgr apps are working without problems, registry keys of system certs are not saved with the wine unload as expected; though tests are not ready yet
Best, Donnie
Signed-off-by: Donat Enikeev donat@enikeev.net --- dlls/crypt32/crypt32_private.h | 6 +- dlls/crypt32/main.c | 3 +- dlls/crypt32/regstore.c | 10 ++-- dlls/crypt32/rootstore.c | 121 +++++++++++++---------------------------- dlls/crypt32/store.c | 13 ++--- 5 files changed, 53 insertions(+), 100 deletions(-)
diff --git a/dlls/crypt32/crypt32_private.h b/dlls/crypt32/crypt32_private.h index ad4a827..8d415dc 100644 --- a/dlls/crypt32/crypt32_private.h +++ b/dlls/crypt32/crypt32_private.h @@ -336,7 +336,11 @@ WINECRYPT_CERTSTORE *CRYPT_FileNameOpenStoreA(HCRYPTPROV hCryptProv, DWORD dwFlags, const void *pvPara) DECLSPEC_HIDDEN; WINECRYPT_CERTSTORE *CRYPT_FileNameOpenStoreW(HCRYPTPROV hCryptProv, DWORD dwFlags, const void *pvPara) DECLSPEC_HIDDEN; -WINECRYPT_CERTSTORE *CRYPT_RootOpenStore(HCRYPTPROV hCryptProv, DWORD dwFlags) DECLSPEC_HIDDEN; + +BOOL CRYPT_ImportSystemRootCertsToReg(void) DECLSPEC_HIDDEN; +BOOL CRYPT_SerializeContextsToReg(HKEY key, DWORD dwFlags, const WINE_CONTEXT_INTERFACE *contextInterface, + HCERTSTORE memStore) DECLSPEC_HIDDEN; + BOOL CRYPT_IsCertificateSelfSigned(PCCERT_CONTEXT cert) DECLSPEC_HIDDEN;
/* Allocates and initializes a certificate chain engine, but without creating diff --git a/dlls/crypt32/main.c b/dlls/crypt32/main.c index 241a1d9..cf6c66a 100644 --- a/dlls/crypt32/main.c +++ b/dlls/crypt32/main.c @@ -49,7 +49,6 @@ BOOL WINAPI DllMain(HINSTANCE hInst, DWORD fdwReason, PVOID pvReserved) if (pvReserved) break; crypt_oid_free(); crypt_sip_free(); - root_store_free(); default_chain_engine_free(); if (hDefProv) CryptReleaseContext(hDefProv, 0); break; @@ -194,7 +193,7 @@ HCRYPTPROV WINAPI I_CryptGetDefaultCryptProv(DWORD reserved) BOOL WINAPI I_CryptReadTrustedPublisherDWORDValueFromRegistry(LPCWSTR name, DWORD *value) { - static const WCHAR safer[] = { + static const WCHAR safer[] = { 'S','o','f','t','w','a','r','e','\','P','o','l','i','c','i','e','s','\', 'M','i','c','r','o','s','o','f','t','\','S','y','s','t','e','m', 'C','e','r','t','i','f','i','c','a','t','e','s','\', diff --git a/dlls/crypt32/regstore.c b/dlls/crypt32/regstore.c index b79387f..750f315 100644 --- a/dlls/crypt32/regstore.c +++ b/dlls/crypt32/regstore.c @@ -179,7 +179,7 @@ static void CRYPT_RegReadFromReg(HKEY key, HCERTSTORE store) }
/* Hash is assumed to be 20 bytes in length (a SHA-1 hash) */ -static BOOL CRYPT_WriteSerializedToReg(HKEY key, const BYTE *hash, const BYTE *buf, +static BOOL CRYPT_WriteSerializedToReg(HKEY key, DWORD dwFlags, const BYTE *hash, const BYTE *buf, DWORD len) { WCHAR asciiHash[20 * 2 + 1]; @@ -188,7 +188,7 @@ static BOOL CRYPT_WriteSerializedToReg(HKEY key, const BYTE *hash, const BYTE *b BOOL ret;
CRYPT_HashToStr(hash, asciiHash); - rc = RegCreateKeyExW(key, asciiHash, 0, NULL, 0, KEY_ALL_ACCESS, NULL, + rc = RegCreateKeyExW(key, asciiHash, 0, NULL, dwFlags, KEY_ALL_ACCESS, NULL, &subKey, NULL); if (!rc) { @@ -205,7 +205,7 @@ static BOOL CRYPT_WriteSerializedToReg(HKEY key, const BYTE *hash, const BYTE *b return ret; }
-static BOOL CRYPT_SerializeContextsToReg(HKEY key, +BOOL CRYPT_SerializeContextsToReg(HKEY key, DWORD dwFlags, const WINE_CONTEXT_INTERFACE *contextInterface, HCERTSTORE memStore) { const void *context = NULL; @@ -232,7 +232,7 @@ static BOOL CRYPT_SerializeContextsToReg(HKEY key, { ret = contextInterface->serialize(context, 0, buf, &size); if (ret) - ret = CRYPT_WriteSerializedToReg(key, hash, buf, size); + ret = CRYPT_WriteSerializedToReg(key, dwFlags, hash, buf, size); } CryptMemFree(buf); } @@ -287,7 +287,7 @@ static BOOL CRYPT_RegWriteToReg(WINE_REGSTOREINFO *store) } LeaveCriticalSection(&store->cs); } - ret = CRYPT_SerializeContextsToReg(key, interfaces[i], + ret = CRYPT_SerializeContextsToReg(key, 0, interfaces[i], store->memStore); RegCloseKey(key); } diff --git a/dlls/crypt32/rootstore.c b/dlls/crypt32/rootstore.c index 5cee235..9ad330c 100644 --- a/dlls/crypt32/rootstore.c +++ b/dlls/crypt32/rootstore.c @@ -435,53 +435,6 @@ static BOOL import_certs_from_path(LPCSTR path, HCERTSTORE store, return ret; }
-static BOOL WINAPI CRYPT_RootWriteCert(HCERTSTORE hCertStore, - PCCERT_CONTEXT cert, DWORD dwFlags) -{ - /* The root store can't have certs added */ - return FALSE; -} - -static BOOL WINAPI CRYPT_RootDeleteCert(HCERTSTORE hCertStore, - PCCERT_CONTEXT cert, DWORD dwFlags) -{ - /* The root store can't have certs deleted */ - return FALSE; -} - -static BOOL WINAPI CRYPT_RootWriteCRL(HCERTSTORE hCertStore, - PCCRL_CONTEXT crl, DWORD dwFlags) -{ - /* The root store can have CRLs added. At worst, a malicious application - * can DoS itself, as the changes aren't persisted in any way. - */ - return TRUE; -} - -static BOOL WINAPI CRYPT_RootDeleteCRL(HCERTSTORE hCertStore, - PCCRL_CONTEXT crl, DWORD dwFlags) -{ - /* The root store can't have CRLs deleted */ - return FALSE; -} - -static void *rootProvFuncs[] = { - NULL, /* CERT_STORE_PROV_CLOSE_FUNC */ - NULL, /* CERT_STORE_PROV_READ_CERT_FUNC */ - CRYPT_RootWriteCert, - CRYPT_RootDeleteCert, - NULL, /* CERT_STORE_PROV_SET_CERT_PROPERTY_FUNC */ - NULL, /* CERT_STORE_PROV_READ_CRL_FUNC */ - CRYPT_RootWriteCRL, - CRYPT_RootDeleteCRL, - NULL, /* CERT_STORE_PROV_SET_CRL_PROPERTY_FUNC */ - NULL, /* CERT_STORE_PROV_READ_CTL_FUNC */ - NULL, /* CERT_STORE_PROV_WRITE_CTL_FUNC */ - NULL, /* CERT_STORE_PROV_DELETE_CTL_FUNC */ - NULL, /* CERT_STORE_PROV_SET_CTL_PROPERTY_FUNC */ - NULL, /* CERT_STORE_PROV_CONTROL_FUNC */ -}; - static const char * const CRYPT_knownLocations[] = { "/etc/ssl/certs/ca-certificates.crt", "/etc/ssl/certs", @@ -492,6 +445,7 @@ static const char * const CRYPT_knownLocations[] = { "/etc/security/cacerts", /* Android */ };
+/* This cert had been expired on 31.12.1999, shoult we delete it ? */ static const BYTE authenticode[] = { 0x30,0x82,0x03,0xd6,0x30,0x82,0x02,0xbe,0xa0,0x03,0x02,0x01,0x02,0x02,0x01,0x01, 0x30,0x0d,0x06,0x09,0x2a,0x86,0x48,0x86,0xf7,0x0d,0x01,0x01,0x04,0x05,0x00,0x30, @@ -555,6 +509,10 @@ static const BYTE authenticode[] = { 0x60,0xdc,0x82,0x1f,0xfb,0xce,0x74,0x06,0x7e,0xd6,0xf1,0xac,0x19,0x6a,0x4f,0x74, 0x5c,0xc5,0x15,0x66,0x31,0x6c,0xc1,0x62,0x71,0x91,0x0f,0x59,0x5b,0x7d,0x2a,0x82, 0x1a,0xdf,0xb1,0xb4,0xd8,0x1d,0x37,0xde,0x0d,0x0f }; + +/* Microsoft Root Authority + * Valid to 31.12.2020, please consider update after this date + */ static const BYTE rootauthority[] = { 0x30,0x82,0x04,0x12,0x30,0x82,0x02,0xfa,0xa0,0x03,0x02,0x01,0x02,0x02,0x0f,0x00, 0xc1,0x00,0x8b,0x3c,0x3c,0x88,0x11,0xd1,0x3e,0xf6,0x63,0xec,0xdf,0x40,0x30,0x0d, @@ -622,6 +580,10 @@ static const BYTE rootauthority[] = { 0x04,0xe4,0xa8,0x45,0x04,0xc8,0x5a,0x33,0x38,0x6e,0x4d,0x1c,0x0d,0x62,0xb7,0x0a, 0xa2,0x8c,0xd3,0xd5,0x54,0x3f,0x46,0xcd,0x1c,0x55,0xa6,0x70,0xdb,0x12,0x3a,0x87, 0x93,0x75,0x9f,0xa7,0xd2,0xa0 }; + +/* Microsoft Root Authority Certificate + * Valid to 09.05.2021, please consider update after this date + */ static const BYTE rootcertauthority[] = { 0x30,0x82,0x05,0x99,0x30,0x82,0x03,0x81,0xa0,0x03,0x02,0x01,0x02,0x02,0x10,0x79, 0xad,0x16,0xa1,0x4a,0xa0,0xa5,0xad,0x4c,0x73,0x58,0xf4,0x07,0x13,0x2e,0x65,0x30, @@ -790,55 +752,48 @@ static void read_trusted_roots_from_known_locations(HCERTSTORE store)
static HCERTSTORE create_root_store(void) { - HCERTSTORE root = NULL; HCERTSTORE memStore = CertOpenStore(CERT_STORE_PROV_MEMORY, X509_ASN_ENCODING, 0, CERT_STORE_CREATE_NEW_FLAG, NULL);
if (memStore) { - CERT_STORE_PROV_INFO provInfo = { - sizeof(CERT_STORE_PROV_INFO), - sizeof(rootProvFuncs) / sizeof(rootProvFuncs[0]), - rootProvFuncs, - NULL, - 0, - NULL - }; - read_trusted_roots_from_known_locations(memStore); add_ms_root_certs(memStore); - root = CRYPT_ProvCreateStore(0, memStore, &provInfo); } - TRACE("returning %p\n", root); - return root; + TRACE("returning %p\n", memStore); + return memStore; }
-static WINECRYPT_CERTSTORE *CRYPT_rootStore; +static LONG root_certs_imported = 0;
-WINECRYPT_CERTSTORE *CRYPT_RootOpenStore(HCRYPTPROV hCryptProv, DWORD dwFlags) +static const WCHAR certs_root_path[] = + {'S','o','f','t','w','a','r','e','\','M','i','c','r','o','s','o','f','t','\', + 'S','y','s','t','e','m','C','e','r','t','i','f','i','c','a','t','e','s','\', + 'R','o','o','t','\', 'C','e','r','t','i','f','i','c','a','t','e','s', 0}; + +BOOL CRYPT_ImportSystemRootCertsToReg(void) { - TRACE("(%ld, %08x)\n", hCryptProv, dwFlags); + HCERTSTORE store = NULL; + HKEY key; + BOOL ret = FALSE; + LONG rc;
- if (dwFlags & CERT_STORE_DELETE_FLAG) - { - WARN("root store can't be deleted\n"); - SetLastError(ERROR_ACCESS_DENIED); - return NULL; - } - if (!CRYPT_rootStore) - { - HCERTSTORE root = create_root_store(); + TRACE("\n"); + + if (root_certs_imported) return FALSE;
- InterlockedCompareExchangePointer((PVOID *)&CRYPT_rootStore, root, - NULL); - if (CRYPT_rootStore != root) - CertCloseStore(root, 0); + store = create_root_store(); + if (!store) return FALSE; + + rc = RegCreateKeyExW(HKEY_LOCAL_MACHINE, certs_root_path, 0, NULL, 0, KEY_ALL_ACCESS, NULL, &key, 0); + if (!rc) + { + ret = CRYPT_SerializeContextsToReg(key, REG_OPTION_VOLATILE, pCertInterface, store); + if (ret) + InterlockedIncrement(&root_certs_imported); + RegCloseKey(key); } - CRYPT_rootStore->vtbl->addref(CRYPT_rootStore); - return CRYPT_rootStore; -}
-void root_store_free(void) -{ - CertCloseStore(CRYPT_rootStore, 0); -} + CertCloseStore(store, 0); + return ret; +} \ No newline at end of file diff --git a/dlls/crypt32/store.c b/dlls/crypt32/store.c index 356712b..9e1980e 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(); break; case CERT_SYSTEM_STORE_CURRENT_USER: root = HKEY_CURRENT_USER; @@ -491,6 +485,7 @@ static WINECRYPT_CERTSTORE *CRYPT_SysRegOpenStoreW(HCRYPTPROV hCryptProv, KEY_ALL_ACCESS;
wsprintfW(storePath, fmt, base, storeName); + if (dwFlags & CERT_STORE_OPEN_EXISTING_FLAG) rc = RegOpenKeyExW(root, storePath, 0, sam, &key); else
Hi Donnie,
On 26.10.2016 21:29, Donat Enikeev wrote:
Hi Jacek,
Following your proposal over the previous patch, that it should be a step forward to the implementation closer to the original behaviour,
I've prepared different version of patch:
- sligthly changed existing functions saving certs to the registry (to be able to pass the REG_OPTION_VOLATILE flag)
- replaced the RootStore entity and everything related with new CRYPT_ImportSystemRootCertsToReg() that does actual import (once) into REG_OPTION_VOLATILE keys
- start calling this import whenever an application is reaching necessity to work with HKLM\Root certs store as previously, however the latter call could be moved to the DllMain
Does it look better?
Yes, it looks better. I have a few comments from a quick review.
static const char * const CRYPT_knownLocations[] = { "/etc/ssl/certs/ca-certificates.crt", "/etc/ssl/certs", @@ -492,6 +445,7 @@ static const char * const CRYPT_knownLocations[] = { "/etc/security/cacerts", /* Android */ };
+/* This cert had been expired on 31.12.1999, shoult we delete it ? */ static const BYTE authenticode[] = { 0x30,0x82,0x03,0xd6,0x30,0x82,0x02,0xbe,0xa0,0x03,0x02,0x01,0x02,0x02,0x01,0x01, 0x30,0x0d,0x06,0x09,0x2a,0x86,0x48,0x86,0xf7,0x0d,0x01,0x01,0x04,0x05,0x00,0x30, @@ -555,6 +509,10 @@ static const BYTE authenticode[] = { 0x60,0xdc,0x82,0x1f,0xfb,0xce,0x74,0x06,0x7e,0xd6,0xf1,0xac,0x19,0x6a,0x4f,0x74, 0x5c,0xc5,0x15,0x66,0x31,0x6c,0xc1,0x62,0x71,0x91,0x0f,0x59,0x5b,0x7d,0x2a,0x82, 0x1a,0xdf,0xb1,0xb4,0xd8,0x1d,0x37,0xde,0x0d,0x0f };
+/* Microsoft Root Authority
- Valid to 31.12.2020, please consider update after this date
- */
static const BYTE rootauthority[] = { 0x30,0x82,0x04,0x12,0x30,0x82,0x02,0xfa,0xa0,0x03,0x02,0x01,0x02,0x02,0x0f,0x00, 0xc1,0x00,0x8b,0x3c,0x3c,0x88,0x11,0xd1,0x3e,0xf6,0x63,0xec,0xdf,0x40,0x30,0x0d, @@ -622,6 +580,10 @@ static const BYTE rootauthority[] = { 0x04,0xe4,0xa8,0x45,0x04,0xc8,0x5a,0x33,0x38,0x6e,0x4d,0x1c,0x0d,0x62,0xb7,0x0a, 0xa2,0x8c,0xd3,0xd5,0x54,0x3f,0x46,0xcd,0x1c,0x55,0xa6,0x70,0xdb,0x12,0x3a,0x87, 0x93,0x75,0x9f,0xa7,0xd2,0xa0 };
+/* Microsoft Root Authority Certificate
- Valid to 09.05.2021, please consider update after this date
- */
Please avoid unrelated changes in part of code that you don't touch. The patch changes enough places already.
static const BYTE rootcertauthority[] = { 0x30,0x82,0x05,0x99,0x30,0x82,0x03,0x81,0xa0,0x03,0x02,0x01,0x02,0x02,0x10,0x79, 0xad,0x16,0xa1,0x4a,0xa0,0xa5,0xad,0x4c,0x73,0x58,0xf4,0x07,0x13,0x2e,0x65,0x30, @@ -790,55 +752,48 @@ static void read_trusted_roots_from_known_locations(HCERTSTORE store)
static HCERTSTORE create_root_store(void) {
HCERTSTORE root = NULL; HCERTSTORE memStore = CertOpenStore(CERT_STORE_PROV_MEMORY, X509_ASN_ENCODING, 0, CERT_STORE_CREATE_NEW_FLAG, NULL);
if (memStore) {
CERT_STORE_PROV_INFO provInfo = {sizeof(CERT_STORE_PROV_INFO),sizeof(rootProvFuncs) / sizeof(rootProvFuncs[0]),rootProvFuncs,NULL,0,NULL};read_trusted_roots_from_known_locations(memStore); add_ms_root_certs(memStore);root = CRYPT_ProvCreateStore(0, memStore, &provInfo);}
TRACE("returning %p\n", root);
return root;
- TRACE("returning %p\n", memStore);
- return memStore;
}
-static WINECRYPT_CERTSTORE *CRYPT_rootStore; +static LONG root_certs_imported = 0;
-WINECRYPT_CERTSTORE *CRYPT_RootOpenStore(HCRYPTPROV hCryptProv, DWORD dwFlags) +static const WCHAR certs_root_path[] =
- {'S','o','f','t','w','a','r','e','\','M','i','c','r','o','s','o','f','t','\',
- 'S','y','s','t','e','m','C','e','r','t','i','f','i','c','a','t','e','s','\',
- 'R','o','o','t','\', 'C','e','r','t','i','f','i','c','a','t','e','s', 0};
+BOOL CRYPT_ImportSystemRootCertsToReg(void) {
- TRACE("(%ld, %08x)\n", hCryptProv, dwFlags);
- HCERTSTORE store = NULL;
- HKEY key;
- BOOL ret = FALSE;
- LONG rc;
- if (dwFlags & CERT_STORE_DELETE_FLAG)
- {
WARN("root store can't be deleted\n");SetLastError(ERROR_ACCESS_DENIED);return NULL;- }
- if (!CRYPT_rootStore)
- {
HCERTSTORE root = create_root_store();
- TRACE("\n");
- if (root_certs_imported) return FALSE;
To guarantee that it's called only once per process, INIT_ONCE would be nicer. However, in this case, we'd like to have it done only once per wineserver session. For that CreateMutex seems like a good choice.
Also, there is no need to return BOOL if since return value is not used anyway.
Also, it seems that we don't really have tests using root store. It would be nice to have some.
Thanks, Jacek
Hi Jacek,
I've applied your recommendations and submitted new version to @wine-patches
Also, it seems that we don't really have tests using root store. It would be nice to have some.
Generic test for this and other common stores are submitted as well, it took slightly more time, as appeared that certs for HKCU\MY are not saved in the registry, but in %AppData%, even though it is explicitly declared to use registry physical store.
However, this test doesn't cover expressing of system certs into registry, as I hasn't figured out yet, how to do that in a portable way, so that tests are reliably working in
a) the read-only context, b) across distributions and c) in the future, considering certificates revocation and expiration
(ftr I have 173 certificates in /etc/ssl/certs/ca-certificates.crt shipped with distribution, not counting another hundreds of Mozilla)
Re-implementing certs import from scratch for the reference, so tests are well-independent, would have taken notable amount of time, while putting hacks into the code for the test-purposes only - looks redundantly (and such patches will not be accepted, I suppose :). I am still thinking on this, but probably you would have (or already have) ideas how to do that in more elegant way
Looking forward
Best, Donnie
On Чт, окт 27, 2016 at 2:15 , Jacek Caban jacek@codeweavers.com wrote:
Hi Donnie,
On 26.10.2016 21:29, Donat Enikeev wrote:
Hi Jacek,
Following your proposal over the previous patch, that it should be a step forward to the implementation closer to the original behaviour,
I've prepared different version of patch:
- sligthly changed existing functions saving certs to the registry
(to be able to pass the REG_OPTION_VOLATILE flag) 2. replaced the RootStore entity and everything related with new CRYPT_ImportSystemRootCertsToReg() that does actual import (once) into REG_OPTION_VOLATILE keys 3. start calling this import whenever an application is reaching necessity to work with HKLM\Root certs store as previously, however the latter call could be moved to the DllMain
Does it look better?
Yes, it looks better. I have a few comments from a quick review.
static const char * const CRYPT_knownLocations[] = { "/etc/ssl/certs/ca-certificates.crt", "/etc/ssl/certs", @@ -492,6 +445,7 @@ static const char * const CRYPT_knownLocations[] = { "/etc/security/cacerts", /* Android */ };
+/* This cert had been expired on 31.12.1999, shoult we delete it ? */ static const BYTE authenticode[] = {
0x30,0x82,0x03,0xd6,0x30,0x82,0x02,0xbe,0xa0,0x03,0x02,0x01,0x02,0x02,0x01,0x01,
0x30,0x0d,0x06,0x09,0x2a,0x86,0x48,0x86,0xf7,0x0d,0x01,0x01,0x04,0x05,0x00,0x30, @@ -555,6 +509,10 @@ static const BYTE authenticode[] = {
0x60,0xdc,0x82,0x1f,0xfb,0xce,0x74,0x06,0x7e,0xd6,0xf1,0xac,0x19,0x6a,0x4f,0x74,
0x5c,0xc5,0x15,0x66,0x31,0x6c,0xc1,0x62,0x71,0x91,0x0f,0x59,0x5b,0x7d,0x2a,0x82, 0x1a,0xdf,0xb1,0xb4,0xd8,0x1d,0x37,0xde,0x0d,0x0f };
+/* Microsoft Root Authority
- Valid to 31.12.2020, please consider update after this date
- */
static const BYTE rootauthority[] = {
0x30,0x82,0x04,0x12,0x30,0x82,0x02,0xfa,0xa0,0x03,0x02,0x01,0x02,0x02,0x0f,0x00,
0xc1,0x00,0x8b,0x3c,0x3c,0x88,0x11,0xd1,0x3e,0xf6,0x63,0xec,0xdf,0x40,0x30,0x0d, @@ -622,6 +580,10 @@ static const BYTE rootauthority[] = {
0x04,0xe4,0xa8,0x45,0x04,0xc8,0x5a,0x33,0x38,0x6e,0x4d,0x1c,0x0d,0x62,0xb7,0x0a,
0xa2,0x8c,0xd3,0xd5,0x54,0x3f,0x46,0xcd,0x1c,0x55,0xa6,0x70,0xdb,0x12,0x3a,0x87, 0x93,0x75,0x9f,0xa7,0xd2,0xa0 };
+/* Microsoft Root Authority Certificate
- Valid to 09.05.2021, please consider update after this date
- */
Please avoid unrelated changes in part of code that you don't touch. The patch changes enough places already.
static const BYTE rootcertauthority[] = {
0x30,0x82,0x05,0x99,0x30,0x82,0x03,0x81,0xa0,0x03,0x02,0x01,0x02,0x02,0x10,0x79,
0xad,0x16,0xa1,0x4a,0xa0,0xa5,0xad,0x4c,0x73,0x58,0xf4,0x07,0x13,0x2e,0x65,0x30, @@ -790,55 +752,48 @@ static void read_trusted_roots_from_known_locations(HCERTSTORE store)
static HCERTSTORE create_root_store(void) {
HCERTSTORE root = NULL; HCERTSTORE memStore = CertOpenStore(CERT_STORE_PROV_MEMORY, X509_ASN_ENCODING, 0, CERT_STORE_CREATE_NEW_FLAG, NULL);
if (memStore) {
CERT_STORE_PROV_INFO provInfo = {sizeof(CERT_STORE_PROV_INFO),sizeof(rootProvFuncs) / sizeof(rootProvFuncs[0]),rootProvFuncs,NULL,0,NULL};read_trusted_roots_from_known_locations(memStore); add_ms_root_certs(memStore);root = CRYPT_ProvCreateStore(0, memStore, &provInfo);}
TRACE("returning %p\n", root);
return root;
- TRACE("returning %p\n", memStore);
- return memStore;
}
-static WINECRYPT_CERTSTORE *CRYPT_rootStore; +static LONG root_certs_imported = 0;
-WINECRYPT_CERTSTORE *CRYPT_RootOpenStore(HCRYPTPROV hCryptProv, DWORD dwFlags) +static const WCHAR certs_root_path[] =
{'S','o','f','t','w','a','r','e','\','M','i','c','r','o','s','o','f','t','\',
'S','y','s','t','e','m','C','e','r','t','i','f','i','c','a','t','e','s','\',
- 'R','o','o','t','\',
'C','e','r','t','i','f','i','c','a','t','e','s', 0};
+BOOL CRYPT_ImportSystemRootCertsToReg(void) {
- TRACE("(%ld, %08x)\n", hCryptProv, dwFlags);
- HCERTSTORE store = NULL;
- HKEY key;
- BOOL ret = FALSE;
- LONG rc;
- if (dwFlags & CERT_STORE_DELETE_FLAG)
- {
WARN("root store can't be deleted\n");SetLastError(ERROR_ACCESS_DENIED);return NULL;- }
- if (!CRYPT_rootStore)
- {
HCERTSTORE root = create_root_store();
- TRACE("\n");
- if (root_certs_imported) return FALSE;
To guarantee that it's called only once per process, INIT_ONCE would be nicer. However, in this case, we'd like to have it done only once per wineserver session. For that CreateMutex seems like a good choice.
Also, there is no need to return BOOL if since return value is not used anyway.
Also, it seems that we don't really have tests using root store. It would be nice to have some.
Thanks, Jacek
-
Donat Enikeev -
Jacek Caban