Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(261)

Issue 1679873005: Switch SignatureVerifier to taking an algorithm enum. (Closed)

Created:
4 years, 10 months ago by davidben
Modified:
4 years, 9 months ago
CC:
asvitkine+watch_chromium.org, cbentzel+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Switch SignatureVerifier to taking an algorithm enum. The existing API and implementation were problematic for several reasons. - It is very unclear what algorithms were supported. - Everyone was using it as an enum anyway, but it required copy-and-pasting giant strings all over the codebase. - The API is dangerous. Anyone not using it as an enum (i.e. taking an AlgorithmIdentifier from another source) opens themselves up to accepting any random algorithm and parameters the underlying implementation knew how to parse. - It relies on EVP_get_digestbyobj extracting the hash for RSA-PKCS1-FOO signature OIDs. This is weird and, for EVP_get_digestbyobj, Chromium appears to be one of the only two consumers still relying on this. This is a remnant of OpenSSL's old EVP_Sign* APIs. - The old EVP_get_digestbyobj implementation failed to check that ECDSA public keys weren't used for an RSA algorithm, etc. - The old EVP_get_digestbyobj implementation happily accepted OIDs for hashes as signature algorithm OIDs. This removes a use of openssl/x509.h from //crypto. BUG=499653 Committed: https://crrev.com/9c97a36e56031b246276e28f2f22f9f13d9a005a Cr-Commit-Position: refs/heads/master@{#379014}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : fix iOS build #

Unified diffs Side-by-side diffs Delta from patch set Stats (+109 lines, -288 lines) Patch
M chrome/browser/extensions/install_signer.cc View 1 2 2 chunks +1 line, -2 lines 0 comments Download
M components/crx_file.gypi View 1 chunk +0 lines, -1 line 0 comments Download
M components/crx_file/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
D components/crx_file/constants.h View 1 chunk +0 lines, -21 lines 0 comments Download
M components/crx_file/crx_file.cc View 1 2 2 chunks +3 lines, -5 lines 0 comments Download
M components/policy/core/common/cloud/cloud_policy_validator.cc View 3 chunks +6 lines, -24 lines 0 comments Download
M components/update_client/client_update_protocol_ecdsa.cc View 1 2 3 2 chunks +3 lines, -19 lines 0 comments Download
M components/update_client/component_unpacker.cc View 1 chunk +0 lines, -1 line 0 comments Download
M components/variations/variations_seed_store.cc View 2 chunks +4 lines, -20 lines 0 comments Download
M crypto/ec_signature_creator_unittest.cc View 1 chunk +1 line, -17 lines 0 comments Download
M crypto/signature_creator_unittest.cc View 4 chunks +6 lines, -22 lines 0 comments Download
M crypto/signature_verifier.h View 1 4 chunks +11 lines, -23 lines 0 comments Download
M crypto/signature_verifier_nss.cc View 1 2 3 4 5 4 chunks +16 lines, -30 lines 0 comments Download
M crypto/signature_verifier_openssl.cc View 1 2 3 4 6 chunks +30 lines, -31 lines 0 comments Download
M crypto/signature_verifier_unittest.cc View 8 chunks +22 lines, -41 lines 0 comments Download
M extensions/browser/verified_contents.cc View 2 chunks +1 line, -9 lines 0 comments Download
M net/quic/crypto/proof_verifier_chromium.cc View 1 2 3 1 chunk +5 lines, -21 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 28 (13 generated)
davidben
How much do you hate me for this one, Ryan? :-) I think, given how ...
4 years, 9 months ago (2016-03-02 00:49:43 UTC) #5
davidben
On 2016/03/02 00:49:43, davidben wrote: > How much do you hate me for this one, ...
4 years, 9 months ago (2016-03-02 00:55:24 UTC) #7
Ryan Sleevi
LGTM & your changes for PSS SGTM
4 years, 9 months ago (2016-03-02 19:25:17 UTC) #8
Alexei Svitkine (slow)
components/variations lgtm
4 years, 9 months ago (2016-03-02 19:40:27 UTC) #10
davidben
+rockot for chrome/browser/extensions, components/crx_file, and extensions +cschuet for components/policy +sorin for components/update_client
4 years, 9 months ago (2016-03-02 20:31:06 UTC) #12
Sorin Jianu
lgtm component updater lgtm thank you!
4 years, 9 months ago (2016-03-03 00:50:30 UTC) #13
Ken Rockot(use gerrit already)
said files lgtm
4 years, 9 months ago (2016-03-03 01:10:37 UTC) #14
cschuet (SLOW)
On 2016/03/03 01:10:37, Ken Rockot wrote: > said files lgtm lgtm
4 years, 9 months ago (2016-03-03 07:27:44 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1679873005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1679873005/100001
4 years, 9 months ago (2016-03-03 13:25:43 UTC) #17
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/175913)
4 years, 9 months ago (2016-03-03 13:37:44 UTC) #19
davidben
+blundell for components/crx_file.gypi
4 years, 9 months ago (2016-03-03 13:42:53 UTC) #21
blundell
lgtm
4 years, 9 months ago (2016-03-03 15:03:04 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1679873005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1679873005/100001
4 years, 9 months ago (2016-03-03 15:46:13 UTC) #24
commit-bot: I haz the power
Committed patchset #6 (id:100001)
4 years, 9 months ago (2016-03-03 16:18:39 UTC) #26
commit-bot: I haz the power
4 years, 9 months ago (2016-03-03 16:19:29 UTC) #28
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/9c97a36e56031b246276e28f2f22f9f13d9a005a
Cr-Commit-Position: refs/heads/master@{#379014}

Powered by Google App Engine
This is Rietveld 408576698