Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8528043:
Get all of the certificate data for downloads from the WinVerifyTrust result. (Closed)
|
Created:
9 years, 1 month ago by Brian Ryner Modified:
9 years, 1 month ago CC:
chromium-reviews, mattm Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionGet all of the certificate data for downloads from the WinVerifyTrust result.
This should speed up the certificate extraction significantly.
BUG=102540
TEST=SignatureUtilWinTest.* pass
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110151
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address review comments #
Messages
Total messages: 10 (0 generated)
Nice work - great to see code disappear! :-) LGTM. A few comments below, but take em or leave em. http://codereview.chromium.org/8528043/diff/1/chrome/browser/safe_browsing/si... File chrome/browser/safe_browsing/signature_util_win.cc (right): http://codereview.chromium.org/8528043/diff/1/chrome/browser/safe_browsing/si... chrome/browser/safe_browsing/signature_util_win.cc:45: wintrust_data.dwProvFlags = 0; // don't set any flags FWIW, you could also considere adding WTD_CACHE_ONLY_URL_RETRIEVAL here to disable AIA fetching. This should ensure that the certificate chain submitted is only what is actually present in the executable. This could result in false negatives, if the code signing actually failed to include the whole chain and the user didn't already have it, but that's generally an indication that someone is doing something 'silly' with code-signing, rather than "As intended". After all, it'd be no different than the user trying to verify the signature while offline. http://codereview.chromium.org/8528043/diff/1/chrome/browser/safe_browsing/si... chrome/browser/safe_browsing/signature_util_win.cc:61: for (size_t i = 0; i < prov_data->csSigners; ++i) { nit: DWORD http://codereview.chromium.org/8528043/diff/1/chrome/browser/safe_browsing/si... chrome/browser/safe_browsing/signature_util_win.cc:64: for (size_t j = 0; j < cert_chain_context->cChain; ++j) { nit: DWORD http://codereview.chromium.org/8528043/diff/1/chrome/browser/safe_browsing/si... chrome/browser/safe_browsing/signature_util_win.cc:68: for (size_t k = 0; k < simple_chain->cElement; ++k) { nit: DWORD
http://codereview.chromium.org/8528043/diff/1/chrome/browser/safe_browsing/si... File chrome/browser/safe_browsing/signature_util_win.cc (right): http://codereview.chromium.org/8528043/diff/1/chrome/browser/safe_browsing/si... chrome/browser/safe_browsing/signature_util_win.cc:53: signature_info->set_trusted( Should we not set trusted at all if the file has no signature? Also, it might be interested to return a more nuanced answer. According to [0] we could know whether the certificate was explicitly distrusted for example as opposed to not trusted. [0] http://msdn.microsoft.com/en-us/library/windows/desktop/aa382384(v=vs.85).aspx
http://codereview.chromium.org/8528043/diff/1/chrome/browser/safe_browsing/si... File chrome/browser/safe_browsing/signature_util_win.cc (right): http://codereview.chromium.org/8528043/diff/1/chrome/browser/safe_browsing/si... chrome/browser/safe_browsing/signature_util_win.cc:45: wintrust_data.dwProvFlags = 0; // don't set any flags On 2011/11/15 00:27:12, Ryan Sleevi wrote: > FWIW, you could also considere adding WTD_CACHE_ONLY_URL_RETRIEVAL here to > disable AIA fetching. This should ensure that the certificate chain submitted is > only what is actually present in the executable. > > This could result in false negatives, if the code signing actually failed to > include the whole chain and the user didn't already have it, but that's > generally an indication that someone is doing something 'silly' with > code-signing, rather than "As intended". After all, it'd be no different than > the user trying to verify the signature while offline. Done, but I have a question. The docs for this ( http://msdn.microsoft.com/en-us/library/windows/desktop/aa388205(v=vs.85).aspx ) say that this value isn't supported on Win2k and WinXP. Any idea what the behavior will be on those operating systems? Will the whole WinVerifyTrust call fail or will it just ignore the flag? http://codereview.chromium.org/8528043/diff/1/chrome/browser/safe_browsing/si... chrome/browser/safe_browsing/signature_util_win.cc:53: signature_info->set_trusted( On 2011/11/15 00:29:47, noelutz wrote: > Should we not set trusted at all if the file has no signature? Sure, done. > Also, it might be interested to return a more nuanced answer. According to [0] > we could know whether the certificate was explicitly distrusted for example as > opposed to not trusted. > > [0] > http://msdn.microsoft.com/en-us/library/windows/desktop/aa382384%28v=vs.85%29... Interesting, but in the interest of simplicity I'd like to hold off on this until we know for sure what we need. http://codereview.chromium.org/8528043/diff/1/chrome/browser/safe_browsing/si... chrome/browser/safe_browsing/signature_util_win.cc:61: for (size_t i = 0; i < prov_data->csSigners; ++i) { On 2011/11/15 00:27:12, Ryan Sleevi wrote: > nit: DWORD Done. http://codereview.chromium.org/8528043/diff/1/chrome/browser/safe_browsing/si... chrome/browser/safe_browsing/signature_util_win.cc:64: for (size_t j = 0; j < cert_chain_context->cChain; ++j) { On 2011/11/15 00:27:12, Ryan Sleevi wrote: > nit: DWORD Done. http://codereview.chromium.org/8528043/diff/1/chrome/browser/safe_browsing/si... chrome/browser/safe_browsing/signature_util_win.cc:68: for (size_t k = 0; k < simple_chain->cElement; ++k) { On 2011/11/15 00:27:12, Ryan Sleevi wrote: > nit: DWORD Done.
lgtm http://codereview.chromium.org/8528043/diff/1/chrome/browser/safe_browsing/si... File chrome/browser/safe_browsing/signature_util_win.cc (right): http://codereview.chromium.org/8528043/diff/1/chrome/browser/safe_browsing/si... chrome/browser/safe_browsing/signature_util_win.cc:53: signature_info->set_trusted( On 2011/11/15 01:05:37, Brian Ryner wrote: > On 2011/11/15 00:29:47, noelutz wrote: > > Should we not set trusted at all if the file has no signature? > > Sure, done. > > > Also, it might be interested to return a more nuanced answer. According to > [0] > > we could know whether the certificate was explicitly distrusted for example as > > opposed to not trusted. > > > > [0] > > > http://msdn.microsoft.com/en-us/library/windows/desktop/aa382384%2528v=vs.85%... > > Interesting, but in the interest of simplicity I'd like to hold off on this > until we know for sure what we need. SGTM
http://codereview.chromium.org/8528043/diff/1/chrome/browser/safe_browsing/si... File chrome/browser/safe_browsing/signature_util_win.cc (right): http://codereview.chromium.org/8528043/diff/1/chrome/browser/safe_browsing/si... chrome/browser/safe_browsing/signature_util_win.cc:45: wintrust_data.dwProvFlags = 0; // don't set any flags On 2011/11/15 01:05:37, Brian Ryner wrote: > On 2011/11/15 00:27:12, Ryan Sleevi wrote: > > FWIW, you could also considere adding WTD_CACHE_ONLY_URL_RETRIEVAL here to > > disable AIA fetching. This should ensure that the certificate chain submitted > is > > only what is actually present in the executable. > > > > This could result in false negatives, if the code signing actually failed to > > include the whole chain and the user didn't already have it, but that's > > generally an indication that someone is doing something 'silly' with > > code-signing, rather than "As intended". After all, it'd be no different than > > the user trying to verify the signature while offline. > > Done, but I have a question. The docs for this ( > http://msdn.microsoft.com/en-us/library/windows/desktop/aa388205%28v=vs.85%29... ) > say that this value isn't supported on Win2k and WinXP. Any idea what the > behavior will be on those operating systems? Will the whole WinVerifyTrust call > fail or will it just ignore the flag? To be honest, I don't recall, it's been a while since I've touched code that used WVT. I seem to recall passing it, and this was code that ran on Windows 95-Vista (95 via IE3, which first included CryptoAPI and Authenticode). It might be safer to omit entirely, or I can try to follow-up in a few days after digging in to XP vanilla again.
I guess the XP test build bots will catch it if there's a problem that keeps WinVerifyTrust from working? On 2011/11/15 01:20:01, Ryan Sleevi wrote: > http://codereview.chromium.org/8528043/diff/1/chrome/browser/safe_browsing/si... > File chrome/browser/safe_browsing/signature_util_win.cc (right): > > http://codereview.chromium.org/8528043/diff/1/chrome/browser/safe_browsing/si... > chrome/browser/safe_browsing/signature_util_win.cc:45: wintrust_data.dwProvFlags > = 0; // don't set any flags > On 2011/11/15 01:05:37, Brian Ryner wrote: > > On 2011/11/15 00:27:12, Ryan Sleevi wrote: > > > FWIW, you could also considere adding WTD_CACHE_ONLY_URL_RETRIEVAL here to > > > disable AIA fetching. This should ensure that the certificate chain > submitted > > is > > > only what is actually present in the executable. > > > > > > This could result in false negatives, if the code signing actually failed to > > > include the whole chain and the user didn't already have it, but that's > > > generally an indication that someone is doing something 'silly' with > > > code-signing, rather than "As intended". After all, it'd be no different > than > > > the user trying to verify the signature while offline. > > > > Done, but I have a question. The docs for this ( > > > http://msdn.microsoft.com/en-us/library/windows/desktop/aa388205%2528v=vs.85%... > ) > > say that this value isn't supported on Win2k and WinXP. Any idea what the > > behavior will be on those operating systems? Will the whole WinVerifyTrust > call > > fail or will it just ignore the flag? > > To be honest, I don't recall, it's been a while since I've touched code that > used WVT. I seem to recall passing it, and this was code that ran on Windows > 95-Vista (95 via IE3, which first included CryptoAPI and Authenticode). > > It might be safer to omit entirely, or I can try to follow-up in a few days > after digging in to XP vanilla again.
I'll go ahead and submit and keep an eye on the XP bots.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bryner@chromium.org/8528043/4002
Change committed as 110151 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
