Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
561613002:
Support SHA-256 on pre-Vista Windows clients (Closed)
|
Created:
6 years, 3 months ago by Ryan Sleevi Modified:
6 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, agl Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionUse sidestep to detour CertVerifyCertificateSignatureEx on Windows versions earlier than Vista (excluding XP SP3), adding in SHA-256 support by deferring to NSS.
The canonical path to supporting SHA-256 is to install XP SP3 or the appropriate hotfixes for XP x64 / Windows Server 2003. However, as not all users may do so, and there's enough of a usability hurdle, provide an interception hook until we fully drop support for these systems.
XP is already outside of MSFT EOL (April 2014). Windows Server 2003 is EOL'd July 2015.
BUG=401365
Committed: https://crrev.com/25e2bc0a95354727342757ba71582bf4de638fe8
Cr-Commit-Position: refs/heads/master@{#296335}
Patch Set 1 #Patch Set 2 : Cleaned up patch #Patch Set 3 : Dead code #
Total comments: 23
Patch Set 4 : Feedback #Patch Set 5 : Attempt to reset net_test_suite #
Messages
Total messages: 26 (6 generated)
rsleevi@chromium.org changed reviewers: + cpu@chromium.org, shrikant@chromium.org
cpu, shrikant: Below is a WIP that tests out on my XP SP3 VM (and, incidentally, works for my Windows 7 machine). There's the 'obvious' issue of DEPS layering (probably solvable by moving sidestep to base/win to go with PEImage?), but otherwise this works. Next is to find an XP SP2 VM (I don't have one, and it turns out I let my MSDN expire), copy net_unittests.exe and twitter.pem to it, and run CertVerifyProcTest.SHA256 on it. Noting that this is just a rough POC, please offer suggestions for anything hinky you see.
On 2014/09/10 04:55:02, Ryan Sleevi wrote: > cpu, shrikant: Below is a WIP that tests out on my XP SP3 VM (and, incidentally, > works for my Windows 7 machine). There's the 'obvious' issue of DEPS layering > (probably solvable by moving sidestep to base/win to go with PEImage?), but > otherwise this works. > > Next is to find an XP SP2 VM (I don't have one, and it turns out I let my MSDN > expire), copy net_unittests.exe and twitter.pem to it, and run > CertVerifyProcTest.SHA256 on it. > > Noting that this is just a rough POC, please offer suggestions for anything > hinky you see. Woot, usage looks correct as per our discussion yesterday. Yesterday I have prepared brand new XP sp2 vm , I will check your patch on it.
rsleevi@chromium.org changed reviewers: + davidben@chromium.org, jam@chromium.org - cpu@chromium.org
jam: As we discussed this morning, the //content changes shrikant: Please take a look at the //content side and make sure you're happy / I haven't missed anything obvious. david: Normally, I'd kick something like this to wtc, but uh, it's on you now. Be aggressive and methodical, and if you see missed opportunities for tests or documentation, let me know and push back.
On 2014/09/11 20:17:36, Ryan Sleevi wrote: > jam: As we discussed this morning, the //content changes > shrikant: Please take a look at the //content side and make sure you're happy / > I haven't missed anything obvious. > > david: Normally, I'd kick something like this to wtc, but uh, it's on you now. > Be aggressive and methodical, and if you see missed opportunities for tests or > documentation, let me know and push back. nit: If you want to keep all interception logic into browser_main_runner, think about moving out decision for calling original_stub to browser_main_runner as well. One way would be to return enum from net::CryptVerifyCertificateSignatureExHook and based on that either call original_stub or return value returned by CryptVerifyCertificateSignatureExHook. otherwise, lgtm!
On 2014/09/11 21:53:27, Shrikant Kelkar wrote: > On 2014/09/11 20:17:36, Ryan Sleevi wrote: > > jam: As we discussed this morning, the //content changes > > shrikant: Please take a look at the //content side and make sure you're happy > / > > I haven't missed anything obvious. > > > > david: Normally, I'd kick something like this to wtc, but uh, it's on you now. > > Be aggressive and methodical, and if you see missed opportunities for tests or > > documentation, let me know and push back. > > nit: If you want to keep all interception logic into browser_main_runner, think > about moving out decision for calling original_stub to browser_main_runner as > well. One way would be to return enum from > net::CryptVerifyCertificateSignatureExHook and based on that either call > original_stub or return value returned by CryptVerifyCertificateSignatureExHook. > > otherwise, lgtm! I considered that, but that means splitting up a lot of the logic - in particular, determining the effective subject param and the determining the signature alg, since we only want to redirect SHA-2. The current approach also makes it easier to unit test the logic. Depending on David's reaction, I may stub in an extra redirect function for the //net unit test so we can make sure when/how the redirect is called. But thanks for looking!
https://codereview.chromium.org/561613002/diff/40001/content/browser/browser_... File content/browser/browser_main_runner.cc (right): https://codereview.chromium.org/561613002/diff/40001/content/browser/browser_... content/browser/browser_main_runner.cc:60: // Interception on x64 is not supported. nit: move this to net::sha256_interception::IsNeeded() so that there's only one place that knows when this is needed, instead of two? https://codereview.chromium.org/561613002/diff/40001/content/browser/browser_... content/browser/browser_main_runner.cc:105: &old_protect)); that's a lot of lines to patch the method. can you use base::win::IATPatchFunction instead? it's much smaller. i.e. look at https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ch...
https://codereview.chromium.org/561613002/diff/40001/content/browser/browser_... File content/browser/browser_main_runner.cc (right): https://codereview.chromium.org/561613002/diff/40001/content/browser/browser_... content/browser/browser_main_runner.cc:60: // Interception on x64 is not supported. On 2014/09/12 04:13:08, jam wrote: > nit: move this to net::sha256_interception::IsNeeded() so that there's only one > place that knows when this is needed, instead of two? The logic behind this is that the function would totally work for x64 too, but sidestep can't handle it. In other places in Chrome (e.g. NtDLL patching), we do implement x64 support, but by manually constructing the sidestep. Since its a limitation of the sandbox, not net, that's why I left it here. https://codereview.chromium.org/561613002/diff/40001/content/browser/browser_... content/browser/browser_main_runner.cc:105: &old_protect)); On 2014/09/12 04:13:08, jam wrote: > that's a lot of lines to patch the method. can you use > base::win::IATPatchFunction instead? it's much smaller. i.e. look at > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ch... Nope. I worked with Carlos and Shrikant on this. Because of how and when its called, it has to be sidestepped. In particular, we are intercepting an in-module call (crypt32 CertGetCerificateChain to crypt32 CryptVerifyCertificateSignatureEx), so this has to be sidestepped, as the IAT is entirely bypassed for that call.
https://codereview.chromium.org/561613002/diff/40001/content/browser/browser_... File content/browser/browser_main_runner.cc (right): https://codereview.chromium.org/561613002/diff/40001/content/browser/browser_... content/browser/browser_main_runner.cc:86: return; If this return fires, we'll leave cert_verify_signature_ptr writable. https://codereview.chromium.org/561613002/diff/40001/content/browser/browser_... content/browser/browser_main_runner.cc:149: InstallSha256LegacyHooks(); Perhaps add tests the exercise the patched version? Like an end-to-end CertVerifier test on XP that a SHA-256 chain is fine. Or perhaps even have tests that unconditionally patch and run on all versions of Windows, though that might be bothering with things we don't want to. (As far as I can tell, it should work anyway on 32-bit?) https://codereview.chromium.org/561613002/diff/40001/net/cert/sha256_legacy_s... File net/cert/sha256_legacy_support_win.cc (right): https://codereview.chromium.org/561613002/diff/40001/net/cert/sha256_legacy_s... net/cert/sha256_legacy_support_win.cc:18: #include "crypto/scoped_nss_types.h" Hrm. This is going to be somewhat a nuisance for the Windows OpenSSL build since there'll be a period of time when we'll want both ends working. Maybe we need all of net/cert/sha256_legacy_support_win{,_nss,_openssl,_unittest}.cc to exist. (Probably can also split that off when we get to it; I don't even know if the Windows OpenSSL build even compiles right now.) https://codereview.chromium.org/561613002/diff/40001/net/cert/sha256_legacy_s... net/cert/sha256_legacy_support_win.cc:80: return &cert->pCertInfo->SubjectPublicKeyInfo; I'm assuming it's safe to assume cert->pCertInfo isn't NULL, and other fields dereferenced below. https://codereview.chromium.org/561613002/diff/40001/net/cert/sha256_legacy_s... net/cert/sha256_legacy_support_win.cc:85: PCCERT_CONTEXT cert = chain->rgpChain[0]->rgpElement[0]->pCertContext; Is it possible for chain->cChain or chain->rgpChain->cElement to be zero? The documentation in both cases says ...[0] is the end one, so probably not, though maybe worth checking anyway. https://codereview.chromium.org/561613002/diff/40001/net/cert/sha256_legacy_s... net/cert/sha256_legacy_support_win.cc:110: // this path. DCHECK_EQ(VERSION_SERVER_2003, os_info->version()) or a comment? It took me a bit to realize what versions hit this codepath at all. Dunno if this is clearer: if (os_info->version() == base::win::VERSION_XP) { if (os_info->service_pack().major >= 3) return false; // SP3 added SHA-256 support return true; } else if (os_info->version() == base::win::VERSION_SERVER_2003) { // Just assume it's needed. While it's possible to hotfix in support via // patches from Microsoft, or to install a third-party CSP that was signed by // Microsoft, the only reason to use that implementation would be to allow // administrators to disable SHA-2 by policy, and doing so would just force // this path. return true; } // New enough to have SHA-256 support or too old to matter. return false; https://codereview.chromium.org/561613002/diff/40001/net/cert/sha256_legacy_s... net/cert/sha256_legacy_support_win.cc:143: // which all of the supported subject types match. If the signature (I was unable to find a reference for this for the CRYPT_VERIFY_CERT_SIGN_SUBJECT_BLOB type, but I'm not familiar with Windows APIs. I did verify that NSS's template matches the other two per RFC and MSDN.) https://codereview.chromium.org/561613002/diff/40001/net/cert/sha256_legacy_s... net/cert/sha256_legacy_support_win.cc:177: if (!issuer_public_key) { Comment about blahChain[0] above aside, should this have a NOTREACHED()? Maybe move this above and go back to the original function if this fails, if you're adding additional checks up there. https://codereview.chromium.org/561613002/diff/40001/net/cert/sha256_legacy_s... net/cert/sha256_legacy_support_win.cc:197: SetLastError(static_cast<DWORD>(NTE_BAD_ALGID)); I don't have a better suggestion out of the ones this function is documented to emit, but this error code doesn't especially match. https://codereview.chromium.org/561613002/diff/40001/net/test/net_test_suite.cc File net/test/net_test_suite.cc (right): https://codereview.chromium.org/561613002/diff/40001/net/test/net_test_suite.... net/test/net_test_suite.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Did this file change, or did Rietveld get confused? Diffstat says +67,-67 so something stupid with line endings?
Something to note: sidestep's implementation does have this comment here. https://chromium.googlesource.com/chromium/chromium/+/trunk/sandbox/win/src/s... // TODO(V7:joi) Siggi and I just had a discussion and decided that both // patching and unpatching are actually unsafe. We also discussed a // method of making it safe, which is to freeze all other threads in the // process, check their thread context to see if their eip is currently // inside the block of instructions we need to copy to the stub, and if so // wait a bit and try again, then unfreeze all threads once we've patched. // Not implementing this for now since we're only using SideStep for unit // testing, but if we ever use it for production code this is what we // should do. (I'm guessing it not being safe comes from us not knowing if Windows starts a process up with multiple threads, or will do so in the future? We at least don't have to worry about it being future-proof.)
agl@chromium.org changed reviewers: + agl@chromium.org
Note: if this isn't going to make it for Chrome 39 please let me know and I'll patch NSS not to advertise SHA-256 support in the signature algorithms.
lgtm
https://codereview.chromium.org/561613002/diff/40001/content/browser/browser_... File content/browser/browser_main_runner.cc (right): https://codereview.chromium.org/561613002/diff/40001/content/browser/browser_... content/browser/browser_main_runner.cc:149: InstallSha256LegacyHooks(); On 2014/09/12 22:51:17, David Benjamin wrote: > Perhaps add tests the exercise the patched version? Like an end-to-end > CertVerifier test on XP that a SHA-256 chain is fine. Or perhaps even have tests > that unconditionally patch and run on all versions of Windows, though that might > be bothering with things we don't want to. (As far as I can tell, it should work > anyway on 32-bit?) Considering that the XP bots we have are SP3, I don't think we'll actually end up testing anything. Which is why I made the patched-function test unconditional, but the patch-testing is no-opy. Note that once https://codereview.chromium.org/515813002/ lands (which would be after this, to be on the safeside), ALL of our SSL tests will exercise this path, even if it won't be hit. https://codereview.chromium.org/561613002/diff/40001/net/cert/sha256_legacy_s... File net/cert/sha256_legacy_support_win.cc (right): https://codereview.chromium.org/561613002/diff/40001/net/cert/sha256_legacy_s... net/cert/sha256_legacy_support_win.cc:18: #include "crypto/scoped_nss_types.h" On 2014/09/12 22:51:17, David Benjamin wrote: > Hrm. This is going to be somewhat a nuisance for the Windows OpenSSL build since > there'll be a period of time when we'll want both ends working. Maybe we need > all of net/cert/sha256_legacy_support_win{,_nss,_openssl,_unittest}.cc to exist. > > (Probably can also split that off when we get to it; I don't even know if the > Windows OpenSSL build even compiles right now.) Right, that was my plan as well. https://codereview.chromium.org/561613002/diff/40001/net/cert/sha256_legacy_s... net/cert/sha256_legacy_support_win.cc:80: return &cert->pCertInfo->SubjectPublicKeyInfo; On 2014/09/12 22:51:17, David Benjamin wrote: > I'm assuming it's safe to assume cert->pCertInfo isn't NULL, and other fields > dereferenced below. Correct. Windows will crash too. https://codereview.chromium.org/561613002/diff/40001/net/cert/sha256_legacy_s... net/cert/sha256_legacy_support_win.cc:85: PCCERT_CONTEXT cert = chain->rgpChain[0]->rgpElement[0]->pCertContext; On 2014/09/12 22:51:17, David Benjamin wrote: > Is it possible for chain->cChain or chain->rgpChain->cElement to be zero? The > documentation in both cases says ...[0] is the end one, so probably not, though > maybe worth checking anyway. Nope. Windows will crash if they are as well, but the API contract is that they won't be. https://codereview.chromium.org/561613002/diff/40001/net/cert/sha256_legacy_s... net/cert/sha256_legacy_support_win.cc:110: // this path. On 2014/09/12 22:51:17, David Benjamin wrote: > DCHECK_EQ(VERSION_SERVER_2003, os_info->version()) or a comment? > > It took me a bit to realize what versions hit this codepath at all. Dunno if > this is clearer: > > if (os_info->version() == base::win::VERSION_XP) { > if (os_info->service_pack().major >= 3) > return false; // SP3 added SHA-256 support > return true; > } else if (os_info->version() == base::win::VERSION_SERVER_2003) { > // Just assume it's needed. While it's possible to hotfix in support via > // patches from Microsoft, or to install a third-party CSP that was signed by > // Microsoft, the only reason to use that implementation would be to allow > // administrators to disable SHA-2 by policy, and doing so would just force > // this path. > return true; > } > > // New enough to have SHA-256 support or too old to matter. > return false; This is now unnecessary in ToT, due to base/win/win_util.h MaybeHasSHA256Support() https://codereview.chromium.org/561613002/diff/40001/net/cert/sha256_legacy_s... net/cert/sha256_legacy_support_win.cc:143: // which all of the supported subject types match. If the signature On 2014/09/12 22:51:17, David Benjamin wrote: > (I was unable to find a reference for this for the > CRYPT_VERIFY_CERT_SIGN_SUBJECT_BLOB > type, but I'm not familiar with Windows APIs. I did verify that NSS's template > matches the other two per RFC and MSDN.) Yeah, like most of CryptoAPI, WinCrypt.h has better documentation. But this is the generic SignedData structure from X.500 https://codereview.chromium.org/561613002/diff/40001/net/cert/sha256_legacy_s... net/cert/sha256_legacy_support_win.cc:177: if (!issuer_public_key) { On 2014/09/12 22:51:17, David Benjamin wrote: > Comment about blahChain[0] above aside, should this have a NOTREACHED()? Maybe > move this above and go back to the original function if this fails, if you're > adding additional checks up there. We've already determined this is a SHA-2 signature, so we're doomed either way. https://codereview.chromium.org/561613002/diff/40001/net/cert/sha256_legacy_s... net/cert/sha256_legacy_support_win.cc:197: SetLastError(static_cast<DWORD>(NTE_BAD_ALGID)); On 2014/09/12 22:51:17, David Benjamin wrote: > I don't have a better suggestion out of the ones this function is documented to > emit, but this error code doesn't especially match. It doesn't match the documentation, but it does match Windows ;) https://codereview.chromium.org/561613002/diff/40001/net/test/net_test_suite.cc File net/test/net_test_suite.cc (right): https://codereview.chromium.org/561613002/diff/40001/net/test/net_test_suite.... net/test/net_test_suite.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/09/12 22:51:17, David Benjamin wrote: > Did this file change, or did Rietveld get confused? Diffstat says +67,-67 so > something stupid with line endings? The latter.
On 2014/09/12 22:59:25, David Benjamin wrote: > Something to note: sidestep's implementation does have this comment here. > > https://chromium.googlesource.com/chromium/chromium/+/trunk/sandbox/win/src/s... > > // TODO(V7:joi) Siggi and I just had a discussion and decided that both > // patching and unpatching are actually unsafe. We also discussed a > // method of making it safe, which is to freeze all other threads in the > // process, check their thread context to see if their eip is currently > // inside the block of instructions we need to copy to the stub, and if so > // wait a bit and try again, then unfreeze all threads once we've patched. > // Not implementing this for now since we're only using SideStep for unit > // testing, but if we ever use it for production code this is what we > // should do. > > (I'm guessing it not being safe comes from us not knowing if Windows starts a > process up with multiple threads, or will do so in the future? We at least don't > have to worry about it being future-proof.) Right. That's why I spent a lot of time talking with Shrikant and Carlos. This are more of an academic/theorhetical nature, and inherent in _any_ patching solution. We just assume it's safe because of word-alignment being atomic on x86 processors.
David: PTAL.
lgtm
The CQ bit was checked by rsleevi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/561613002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/59812) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rsleevi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/561613002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as 22b6fdc3e78beaea14da1db0a7fd04b1da7fbe56
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/25e2bc0a95354727342757ba71582bf4de638fe8 Cr-Commit-Position: refs/heads/master@{#296335} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
