Include the full certificate chain in the download pingback.

Include the full certificate chain in the download pingback.

BUG=102540
TEST=SignatureUtilWinTest

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109807

[noelutz, 2011-11-12 00:03:42]

lgtm

[mattm, 2011-11-12 03:15:36]

lgtm

http://codereview.chromium.org/8536035/diff/1/chrome/browser/safe_browsing/do...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

http://codereview.chromium.org/8536035/diff/1/chrome/browser/safe_browsing/do...
chrome/browser/safe_browsing/download_protection_service.cc:406:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
Probably want to move to doing this with WorkerPool so we don't block up the
file thread for too long.  Can do that in a separate CL though.

[Brian Ryner, 2011-11-12 19:00:39]

So just to clarify, you're suggesting that we read the cert (CryptQueryObject)
on the file thread, then do the rest of the work on another thread?

On 2011/11/12 03:15:36, mattm wrote:
> lgtm
> 
>
http://codereview.chromium.org/8536035/diff/1/chrome/browser/safe_browsing/do...
> File chrome/browser/safe_browsing/download_protection_service.cc (right):
> 
>
http://codereview.chromium.org/8536035/diff/1/chrome/browser/safe_browsing/do...
> chrome/browser/safe_browsing/download_protection_service.cc:406:
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
> Probably want to move to doing this with WorkerPool so we don't block up the
> file thread for too long.  Can do that in a separate CL though.

[commit-bot: I haz the power, 2011-11-12 19:00:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryner@chromium.org/8536035/1

[commit-bot: I haz the power, 2011-11-12 20:02:25]

Change committed as 109807

[Ryan Sleevi, 2011-11-12 22:06:17]

Just some drive-by comments. I wasn't aware there was CryptoAPI usage outside of
net/ && crypto/.

http://codereview.chromium.org/8536035/diff/1/chrome/browser/safe_browsing/do...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

http://codereview.chromium.org/8536035/diff/1/chrome/browser/safe_browsing/do...
chrome/browser/safe_browsing/download_protection_service.cc:406:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
On 2011/11/12 03:15:36, mattm wrote:
> Probably want to move to doing this with WorkerPool so we don't block up the
> file thread for too long.  Can do that in a separate CL though.

drive-by: +1 to this. These calls can take upwards of 20 seconds, depending on
how the system is configured, due to things like AIA chasing (which can still
happen for WinVerifyTrust/Authenticode sigs, IIRC).

http://codereview.chromium.org/8536035/diff/1/chrome/browser/safe_browsing/si...
File chrome/browser/safe_browsing/signature_util_win.cc (right):

http://codereview.chromium.org/8536035/diff/1/chrome/browser/safe_browsing/si...
chrome/browser/safe_browsing/signature_util_win.cc:169:
wintrust_data.dwStateAction = WTD_STATEACTION_IGNORE;
FWIW, you could save yourself the chain extraction here by passing
WTD_STATEACTION_VERIFY

This will cause hWVTStateData to receive the verification state data, which will
include all of the signer (and cross-signer and timestamp) certificate chains.

Pass the returned handle to WTHelperProvDataFromStateData, which will return a
CRYPT_PROVIDER_DATA.

You can then EITHER enumerate the data there directly OR call
WTHelperGetProvSignerFromChain to enumerate the signers.

The big benefit here is that it will cache all of the certificate path building
/ verification, rather than requiring you to re-parse/re-verify the entire file.

[Brian Ryner, 2011-11-14 02:55:43]

Thanks for the comments Ryan.  Responses inline.

On 2011/11/12 22:06:17, Ryan Sleevi wrote:
> drive-by: +1 to this. These calls can take upwards of 20 seconds, depending on
> how the system is configured, due to things like AIA chasing (which can still
> happen for WinVerifyTrust/Authenticode sigs, IIRC).

Hm, can you explain a little bit more of what's involved here?  20 seconds is
much longer than we'd like this check to take.  Is this still true with
revocation checking disabled or is it unrelated?

http://codereview.chromium.org/8536035/diff/1/chrome/browser/safe_browsing/si...
> File chrome/browser/safe_browsing/signature_util_win.cc (right):
> 
>
http://codereview.chromium.org/8536035/diff/1/chrome/browser/safe_browsing/si...
> chrome/browser/safe_browsing/signature_util_win.cc:169:
> wintrust_data.dwStateAction = WTD_STATEACTION_IGNORE;
> FWIW, you could save yourself the chain extraction here by passing
> WTD_STATEACTION_VERIFY
> 
> This will cause hWVTStateData to receive the verification state data, which
will
> include all of the signer (and cross-signer and timestamp) certificate chains.
> 
> Pass the returned handle to WTHelperProvDataFromStateData, which will return a
> CRYPT_PROVIDER_DATA.
> 
> You can then EITHER enumerate the data there directly OR call
> WTHelperGetProvSignerFromChain to enumerate the signers.
> 
> The big benefit here is that it will cache all of the certificate path
building
> / verification, rather than requiring you to re-parse/re-verify the entire
file.

Thanks, I'll look into this.  I saw the hWVTStateData argument to WinVerifyTrust
but it wasn't clear to me how to use it.

[Ryan Sleevi, 2011-11-14 03:13:18]

On 2011/11/14 02:55:43, Brian Ryner wrote:
> Thanks for the comments Ryan.  Responses inline.
> 
> On 2011/11/12 22:06:17, Ryan Sleevi wrote:
> > drive-by: +1 to this. These calls can take upwards of 20 seconds, depending
on
> > how the system is configured, due to things like AIA chasing (which can
still
> > happen for WinVerifyTrust/Authenticode sigs, IIRC).
> 
> Hm, can you explain a little bit more of what's involved here?  20 seconds is
> much longer than we'd like this check to take.  Is this still true with
> revocation checking disabled or is it unrelated?
> 

It's unrelated. While the expectation is that a signed executable will have all
of the certificate change embedded as a PKCS#7 signerInfo, it's possible to
avoid doing this. Because you're using CertGetCertificateChain on top of the
CMSG (rather than the WTHelper routines), this means that AIA fetching will be
enabled by default.

The thing that controls this is the HCERTCHAINENGINE you're passing in. The NULL
chain engine (the default) will have URL fetching enabled. You can override this
by creating a CERT_CHAIN_ENGINE_CONFIG that passes
CERT_CHAIN_CACHE_ONLY_URL_RETRIEVAL, so as to disable AIA fetching.
Alternatively, you can pass CERT_CHAIN_CACHE_ONLY_URL_RETRIEVAL as a flag to
CertGetCertificateChain().

Even still though, the logical "Root" store includes the AUTHROOT system, which
may make a ping to Microsoft to download a root on the fly.  You could use
CERT_CHAIN_DISABLE_AUTH_ROOT_UPDATE to disable that.

Additionally, though documented as a revocation aspect, I believe that
CERT_CHAIN_REVOCATION_ACCUMULATIVE_TIMEOUT /
CERT_CHAIN_PARA.dwUrlRetrievalTimeout will affect the AIA fetches (in addition
to CRL/OCSP status checks, which are not enabled with your current flags).

But the easiest way to side-step all of that is to use the WinVerifyTrust
hWVTStateData. 

Just realize that the WVT call can still be multi-second blocking - it has to
read (and hash) the entire fire, to build a chain it may consult the various
default System stores, which may be backed by (Registry, LDAP, Smartcard, other
latency-ridden pieces). Since you're not showing a UI, it may make sense to
delegate this call entirely to a worker thread.

Alternatively, slap some metrics around here to figure out the time to execute.
With about:tracking/about:tracking2 now, perhaps it will be easier to see how
long these block.