crypt32 patch review

[Juan Lang]

[+wine-devel]

Hi Jacek,
I've added the list so the discussion can take place in public.

On Tue, Sep 3, 2013 at 8:50 AM, Jacek Caban <jacek at codeweavers.com> wrote:

> I have a patch for crypt32. I'd appreciate your review before I submit
> it to Wine. It has high potential to be insecure... This should fix Wine

bug 28004 as well as some websites that don't send full cert chain in
> SSL/TLS handshake, but instead just their own certs. For that we need
> two changes. First, we need to look for issuers in global stores (as in
> those that are not passed to CertGetCertificateChain). When I did that,
>

I don't really see a security issue here. The global stores are implicitly
trusted, so neglecting to check them was probably an oversight on my part.
Your tests imply something is missing.

My only real question here is whether the test results are in any way
impacted by some unknown caching happening in Windows. Has this chain been
verified prior to running the test? I'm going to guess that the test bot
machines are "clean" prior to running the tests, but if they download their
test executables from winehq.org, that won't be the case.


> it came out that it's not enough for intermediate issuers. Those have to
> be downloaded if we have information about their location stored in
> validated cert. That's easy using cryptnet. Could you please review the
> attached patch?
>

+ for(i=0; i < info->cAccDescr; i++) {
+        if(!strcmp(szOID_PKIX_CA_ISSUERS,
info->rgAccDescr[i].pszAccessMethod)
+                && info->rgAccDescr[i].AccessLocation.dwAltNameChoice ==
CERT_ALT_NAME_URL) {

You explicitly make use of the AIA extension to find the location of
intermediate certs. This is reasonable, and it gets your test case to pass,
but it won't cover end certs that don't make use of the AIA extension. As I
said above, I wonder whether caching is impacting the test results. More:

The thing that would be also nice to have as follow up is to cache
> downloaded URLs. They are already cached in URL cache, but I believe
> that they should be also cached in some place like world collection or
> something like that. I'm not sure what would be appropriate. Do you have
> a suggestion?
>

Yes. I suspect the most relevant bug is actually 27168. 28004 is probably a
dupe of it, except that I'm not certain that PartyPoker uses chromium. In
brief, Microsoft's crypt32 appears to cache intermediate certs in a
PCCERT_CONTEXT's hCertStore. What I should be doing in crypt32, but am not,
is to have certs ref count the stores they're in, such that the last cert
to be closed causes its store to be freed, if the store is closed prior to
the cert being freed. This, combined with certificate chains being cached
in Microsoft's implementation, would allow intermediate certs to be found
without explicitly checking the AIA.

You can see how the certificate chain engine ought to support caching on
MSDN:
http://msdn.microsoft.com/en-us/library/windows/desktop/aa377184(v=vs.85).aspx

I never implemented chain caching, as I expected it'd be a performance
optimization and could be addressed at a later time, and never got back to
it.

In addition to bug 27168, you can see how Chromium is making use of
crypt32's certificate caching in
net::X509Certificate::CreateOSCertChainForCert, both its comments and
implementation:
http://src.chromium.org/viewvc/chrome/trunk/src/net/cert/x509_certificate.h
http://src.chromium.org/viewvc/chrome/trunk/src/net/cert/x509_certificate_win.cc

Back to your patch: I'm not entirely opposed to the explicit AIA approach,
but I think it's worth at least thinking about other methods. And, I'd
prefer to see the code block more explicitly separated from the surrounding
code, with either a comment or a clear function name to indicate what's
going on. As it is, one has to read quite a bit of CRYPT_FindIssuer to
realize that it's really looking for an AIA extension to find intermediate
issuers.

Last thing on the patch, some style nits. (You already remarked that it
needs splitting, and I agree.)

+    if(issuer) {

 Elsewhere in crypt32 I indent prior to open parentheses, so I worry this
will make it a little harder to maintain a consistent style. Please observe
the existing style, here and elsewhere.

-        issuer = CertFindCertificateInStore(store,
-         subject->dwCertEncodingType, 0, CERT_FIND_SUBJECT_NAME,
-         &subject->pCertInfo->Issuer, prevIssuer);
+        issuer = CRYPT_FindIssuer(engine, subject, store,
CERT_FIND_SUBJECT_NAME,
+                                  &subject->pCertInfo->Issuer, flags,
prevIssuer);

It might not be easy to guess my indenting style for continuation lines,
but I use a single additional space to indicate continuation through
crypt32. I'd appreciate observing the same style.

-     hAdditionalStore, &chain);
+                                            hAdditionalStore, dwFlags,
&chain);

Likewise, the indentation change here doesn't seem necessary.

Thanks very much for working on this. Hope my comments are helpful, let me
know if I can be of further assistance.
--Juan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20130905/dbd6f116/attachment.html>