Re: Imort system certificates in crypt32@Wine

[Jacek Caban]

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