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