crypt32 patch review

[Jacek Caban]

On 09/05/13 18:53, Juan Lang wrote:
> [+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
> <mailto: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 <http://winehq.org>,
> that won't be the case.

I'm not sure what's the state of testbot VMs, but I tested it
extensively locally. When tests are ran on a fresh Windows snapshot
(that doesn't have the Rapid SSL cert cached), tests work fine and I can
see HTTP download of the cert in Wireshark. All subsequent runs also
pass, but don't do any HTTP connections.

>  
>
>     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,

That I do not believe is dupe of 27168, at least according to your
comment 5 in bug 28004 (I didn't debug PartyPoker myself, I just found
it in bugzilla, saw that it's probably the same problem I'm fixing and
verified that it works with my patches).

> 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
> <http://msdn.microsoft.com/en-us/library/windows/desktop/aa377184%28v=vs.85%29.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

Thanks for the pointers. Looking at 27168 is something I hope to look at
at some point. I hit the problem already when working on schannel and
even in my current patch I had to work around it in one place (and
forgot to put a comment about it...).

>
> 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.

Looking at this again, I could probably just use CryptGetObjectUrl,
which will be more explicit and will avoid code duplication.

>
> 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.

Sure, I will change those.

>
> Thanks very much for working on this. Hope my comments are helpful,
> let me know if I can be of further assistance.

Thanks for the review. I will send updated patch.

Thanks,
Jacek

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20130906/e27b269d/attachment.html>