<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 09/05/13 18:53, Juan Lang wrote:<br>
</div>
<blockquote
cite="mid:CAP8JoYiTNgmc+eu+hvXPk2E7rBxGZja-f0Zi+E2Hexm=F89x_A@mail.gmail.com"
type="cite">
<div dir="ltr">
<div>[+wine-devel]</div>
<div><br>
</div>
Hi Jacek,
<div>I've added the list so the discussion can take place in
public.</div>
<div><br>
</div>
<div class="gmail_extra">
<div class="gmail_quote">On Tue, Sep 3, 2013 at 8:50 AM, Jacek
Caban <span dir="ltr"><<a moz-do-not-send="true"
href="mailto:jacek@codeweavers.com" target="_blank">jacek@codeweavers.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">I
have a patch for crypt32. I'd appreciate your review
before I submit<br>
it to Wine. It has high potential to be insecure... This
should fix Wine</blockquote>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">bug
28004 as well as some websites that don't send full cert
chain in<br>
SSL/TLS handshake, but instead just their own certs. For
that we need<br>
two changes. First, we need to look for issuers in global
stores (as in<br>
those that are not passed to CertGetCertificateChain).
When I did that,<br>
</blockquote>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>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 <a
moz-do-not-send="true" href="http://winehq.org"
target="_blank">winehq.org</a>, that won't be the case.</div>
</div>
</div>
</div>
</blockquote>
<br>
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.<br>
<br>
<blockquote
cite="mid:CAP8JoYiTNgmc+eu+hvXPk2E7rBxGZja-f0Zi+E2Hexm=F89x_A@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">it
came out that it's not enough for intermediate issuers.
Those have to<br>
be downloaded if we have information about their location
stored in<br>
validated cert. That's easy using cryptnet. Could you
please review the<br>
attached patch?<br>
</blockquote>
<div><br>
</div>
+ for(i=0; i < info->cAccDescr; i++) {<br>
+ if(!strcmp(szOID_PKIX_CA_ISSUERS,
info->rgAccDescr[i].pszAccessMethod)<br>
+ &&
info->rgAccDescr[i].AccessLocation.dwAltNameChoice ==
CERT_ALT_NAME_URL) {</div>
<div class="gmail_quote"><br>
</div>
<div class="gmail_quote">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:</div>
<div class="gmail_quote"><br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">The
thing that would be also nice to have as follow up is to
cache<br>
downloaded URLs. They are already cached in URL cache, but
I believe<br>
that they should be also cached in some place like world
collection or<br>
something like that. I'm not sure what would be
appropriate. Do you have<br>
a suggestion?<br>
</blockquote>
<div><br>
</div>
<div>Yes. I suspect the most relevant bug is actually 27168.
28004 is probably a dupe of it,</div>
</div>
</div>
</div>
</blockquote>
<br>
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).<br>
<br>
<blockquote
cite="mid:CAP8JoYiTNgmc+eu+hvXPk2E7rBxGZja-f0Zi+E2Hexm=F89x_A@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> 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.</div>
<div><br>
</div>
<div>You can see how the certificate chain engine ought to
support caching on MSDN:</div>
<div><a moz-do-not-send="true"
href="http://msdn.microsoft.com/en-us/library/windows/desktop/aa377184%28v=vs.85%29.aspx">http://msdn.microsoft.com/en-us/library/windows/desktop/aa377184(v=vs.85).aspx</a><br>
</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>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:</div>
<div><a moz-do-not-send="true"
href="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.h</a><br>
</div>
<div><a moz-do-not-send="true"
href="http://src.chromium.org/viewvc/chrome/trunk/src/net/cert/x509_certificate_win.cc">http://src.chromium.org/viewvc/chrome/trunk/src/net/cert/x509_certificate_win.cc</a></div>
</div>
</div>
</div>
</blockquote>
<br>
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...).<br>
<br>
<blockquote
cite="mid:CAP8JoYiTNgmc+eu+hvXPk2E7rBxGZja-f0Zi+E2Hexm=F89x_A@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><br>
</div>
<div>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.</div>
</div>
</div>
</div>
</blockquote>
<br>
Looking at this again, I could probably just use CryptGetObjectUrl,
which will be more explicit and will avoid code duplication.<br>
<br>
<blockquote
cite="mid:CAP8JoYiTNgmc+eu+hvXPk2E7rBxGZja-f0Zi+E2Hexm=F89x_A@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><br>
</div>
<div>Last thing on the patch, some style nits. (You already
remarked that it needs splitting, and I agree.)</div>
<div> </div>
+ if(issuer) {
<div><br>
</div>
<div> 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.</div>
<div><br>
</div>
- issuer = CertFindCertificateInStore(store,<br>
- subject->dwCertEncodingType, 0,
CERT_FIND_SUBJECT_NAME,<br>
- &subject->pCertInfo->Issuer,
prevIssuer);<br>
+ issuer = CRYPT_FindIssuer(engine, subject, store,
CERT_FIND_SUBJECT_NAME,<br>
+
&subject->pCertInfo->Issuer, flags, prevIssuer);</div>
<div class="gmail_quote"><br>
</div>
<div class="gmail_quote">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.</div>
<div class="gmail_extra"><br>
</div>
- hAdditionalStore, &chain);<br>
+ hAdditionalStore,
dwFlags, &chain);</div>
<div class="gmail_extra"><br>
</div>
<div class="gmail_extra">
Likewise, the indentation change here doesn't seem necessary.</div>
</div>
</blockquote>
<br>
Sure, I will change those.<br>
<br>
<blockquote
cite="mid:CAP8JoYiTNgmc+eu+hvXPk2E7rBxGZja-f0Zi+E2Hexm=F89x_A@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra"><br>
</div>
<div class="gmail_extra">Thanks very much for working on this.
Hope my comments are helpful, let me know if I can be of
further assistance.</div>
</div>
</blockquote>
<br>
Thanks for the review. I will send updated patch.<br>
<br>
Thanks,<br>
Jacek<br>
<br>
</body>
</html>