Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2888853003:
Expand CRX verifier to verify CRX₃ files. (Closed)
DescriptionExpand CRX verifier to verify CRX₃ files.
BUG=720092
Review-Url: https://codereview.chromium.org/2888853003
Cr-Commit-Position: refs/heads/master@{#474394}
Committed: https://chromium.googlesource.com/chromium/src/+/11f044437561f28e16d0a2549714d860310fb3fd
Patch Set 1 #Patch Set 2 : Less copying, more reinterpreting. #
Total comments: 12
Patch Set 3 : Through #5 #
Total comments: 2
Patch Set 4 : Comment polish. #Patch Set 5 : Win compile #Patch Set 6 : Basic unit tests #Patch Set 7 : Tests #Patch Set 8 : Fix const name #
Total comments: 23
Patch Set 9 : Through #27 #Patch Set 10 : BUILD fix + comment. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 44 (32 generated)
The CQ bit was checked by waffles@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
waffles@chromium.org changed reviewers: + sorin@chromium.org
Sorin, PTAL. (I will add some end-to-end tests but I am waiting on the pinned key to make it to prod so I can snatch a correctly-signed CRX₃ to test the happy case with.) There's so many variables, I probably missed a const or two.
TY! small fry. https://codereview.chromium.org/2888853003/diff/20001/components/crx_file/crx... File components/crx_file/crx_verifier.cc (right): https://codereview.chromium.org/2888853003/diff/20001/components/crx_file/crx... components/crx_file/crx_verifier.cc:74: size_t len; not initialized https://codereview.chromium.org/2888853003/diff/20001/components/crx_file/crx... components/crx_file/crx_verifier.cc:125: kPublisherKeyHash, kPublisherKeyHash + arraysize(kPublisherKeyHash)); I've started using begin and end for initializers like these. https://codereview.chromium.org/2888853003/diff/20001/components/crx_file/crx... components/crx_file/crx_verifier.cc:129: std::vector<std::unique_ptr<crypto::SignatureVerifier>> verifiers = {}; do we need {} ? https://codereview.chromium.org/2888853003/diff/20001/components/crx_file/crx... components/crx_file/crx_verifier.cc:140: for (auto proof_type : proof_types) { maybe const auto& or auto& ? https://codereview.chromium.org/2888853003/diff/20001/components/crx_file/crx... components/crx_file/crx_verifier.cc:141: for (auto proof : proof_type.first()) { maybe const auto & if possible? https://codereview.chromium.org/2888853003/diff/20001/components/crx_file/crx... components/crx_file/crx_verifier.cc:158: v->VerifyUpdate(kSalt, arraysize(kSalt)); do we want arraysize here or sizeof? It won't make a difference but which one is more logically correct?
The CQ bit was checked by waffles@chromium.org to run a CQ dry run
https://codereview.chromium.org/2888853003/diff/20001/components/crx_file/crx... File components/crx_file/crx_verifier.cc (right): https://codereview.chromium.org/2888853003/diff/20001/components/crx_file/crx... components/crx_file/crx_verifier.cc:74: size_t len; On 2017/05/17 23:54:53, Sorin Jianu wrote: > not initialized Done, thanks; messed it up in the merge. https://codereview.chromium.org/2888853003/diff/20001/components/crx_file/crx... components/crx_file/crx_verifier.cc:125: kPublisherKeyHash, kPublisherKeyHash + arraysize(kPublisherKeyHash)); On 2017/05/17 23:54:53, Sorin Jianu wrote: > I've started using begin and end for initializers like these. Done, thanks! https://codereview.chromium.org/2888853003/diff/20001/components/crx_file/crx... components/crx_file/crx_verifier.cc:129: std::vector<std::unique_ptr<crypto::SignatureVerifier>> verifiers = {}; On 2017/05/17 23:54:53, Sorin Jianu wrote: > do we need {} ? Done. https://codereview.chromium.org/2888853003/diff/20001/components/crx_file/crx... components/crx_file/crx_verifier.cc:140: for (auto proof_type : proof_types) { On 2017/05/17 23:54:53, Sorin Jianu wrote: > maybe const auto& or auto& ? Done. https://codereview.chromium.org/2888853003/diff/20001/components/crx_file/crx... components/crx_file/crx_verifier.cc:141: for (auto proof : proof_type.first()) { On 2017/05/17 23:54:53, Sorin Jianu wrote: > maybe const auto & if possible? Done. https://codereview.chromium.org/2888853003/diff/20001/components/crx_file/crx... components/crx_file/crx_verifier.cc:158: v->VerifyUpdate(kSalt, arraysize(kSalt)); On 2017/05/17 23:54:53, Sorin Jianu wrote: > do we want arraysize here or sizeof? It won't make a difference but which one is > more logically correct? I think arraysize, since that is semantically consistent with other callers (all other callers of this method in the code base use std::string::size or std::vector<uint8_t>::size). Let me know if I'm confused.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm Thank you! https://codereview.chromium.org/2888853003/diff/40001/components/crx_file/crx... File components/crx_file/crx_verifier.cc (right): https://codereview.chromium.org/2888853003/diff/40001/components/crx_file/crx... components/crx_file/crx_verifier.cc:213: verifiers.push_back(base::MakeUnique<crypto::SignatureVerifier>()); we might be able to initialize the vector with the verifier and avoid the push_back call. Then the vector can be made const. Also, could use a type alias for std::vector<std::unique_ptr<crypto::SignatureVerifier>>
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
https://codereview.chromium.org/2888853003/diff/40001/components/crx_file/crx... File components/crx_file/crx_verifier.cc (right): https://codereview.chromium.org/2888853003/diff/40001/components/crx_file/crx... components/crx_file/crx_verifier.cc:213: verifiers.push_back(base::MakeUnique<crypto::SignatureVerifier>()); On 2017/05/18 00:53:31, Sorin Jianu wrote: > we might be able to initialize the vector with the verifier and avoid the > push_back call. Then the vector can be made const. > Also, could use a type alias for > std::vector<std::unique_ptr<crypto::SignatureVerifier>> Unfortunately I don't think you can use an initializer list with a move-only type. Re: type alias, done. Also in another place.
The CQ bit was checked by waffles@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by waffles@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by waffles@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by waffles@chromium.org to run a CQ dry run
waffles@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Devlin, you are an OWNER, please take a look!
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... File components/crx_file/crx3.proto (right): https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx3.proto:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... File components/crx_file/crx_verifier.cc (right): https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx_verifier.cc:74: bool ReadHashAndVerifyArchive(base::File* file, add a function comment https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx_verifier.cc:77: uint8_t buffer[1 << 12] = {}; Comment why 1 << 12? https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx_verifier.cc:108: static_cast<int>(header_size)) any risk of overflow? https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx_verifier.cc:135: required_key_set.insert(publisher_key); use emplace? required_key_set.emplace(std::begin(kPublisherKeyHash), std::end(kPublisherKeyHash)); Or use std::move(), if you prefer. https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx_verifier.cc:139: const std::vector<std::pair<std::function<const RepeatedProof&()>, std::function is banned in Chromium. Can you use base::Callback<> here? https://chromium-cpp.appspot.com https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx_verifier.cc:165: std::unique_ptr<crypto::SignatureVerifier> v = optional: you could use auto here since base::MakeUnique<T> is defined to return a std::unique_ptr<T>. https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx_verifier.cc:179: verifiers.push_back(std::move(v)); can we reserve() verifiers? https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... File components/crx_file/crx_verifier_unittest.cc (right): https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx_verifier_unittest.cc:39: class CrxVerifierTest : public testing::Test { if we don't need to provide custom behavior, prefer: using CrxVerifierTest = testing::Test;
The CQ bit was checked by waffles@chromium.org to run a CQ dry run
https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... File components/crx_file/crx3.proto (right): https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx3.proto:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/05/23 20:32:40, Devlin (catching up) wrote: > nit: no (c) Done. https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... File components/crx_file/crx_verifier.cc (right): https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx_verifier.cc:74: bool ReadHashAndVerifyArchive(base::File* file, On 2017/05/23 20:32:40, Devlin (catching up) wrote: > add a function comment Done. https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx_verifier.cc:77: uint8_t buffer[1 << 12] = {}; On 2017/05/23 20:32:40, Devlin (catching up) wrote: > Comment why 1 << 12? I don't know what to write in the comment. This constant was inherited from the previous code, which predated my contributions to this project. Do you have a recommendation for a different constant? https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx_verifier.cc:108: static_cast<int>(header_size)) On 2017/05/23 20:32:40, Devlin (catching up) wrote: > any risk of overflow? I don't think we can compile Chrome on any machine where int is less than 19 bits. Let me know if you disagree. https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx_verifier.cc:135: required_key_set.insert(publisher_key); On 2017/05/23 20:32:40, Devlin (catching up) wrote: > use emplace? > required_key_set.emplace(std::begin(kPublisherKeyHash), > std::end(kPublisherKeyHash)); > > Or use std::move(), if you prefer. Thanks, done. https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx_verifier.cc:139: const std::vector<std::pair<std::function<const RepeatedProof&()>, On 2017/05/23 20:32:40, Devlin (catching up) wrote: > std::function is banned in Chromium. Can you use base::Callback<> here? > > https://chromium-cpp.appspot.com Hacked on this for a while - does this look better, or do you still prefer the use of Callback? https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx_verifier.cc:165: std::unique_ptr<crypto::SignatureVerifier> v = On 2017/05/23 20:32:40, Devlin (catching up) wrote: > optional: you could use auto here since base::MakeUnique<T> is defined to return > a std::unique_ptr<T>. Done. https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx_verifier.cc:179: verifiers.push_back(std::move(v)); On 2017/05/23 20:32:40, Devlin (catching up) wrote: > can we reserve() verifiers? Done. https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... File components/crx_file/crx_verifier_unittest.cc (right): https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx_verifier_unittest.cc:39: class CrxVerifierTest : public testing::Test { On 2017/05/23 20:32:41, Devlin (catching up) wrote: > if we don't need to provide custom behavior, prefer: > using CrxVerifierTest = testing::Test; Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... File components/crx_file/crx_verifier.cc (right): https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx_verifier.cc:77: uint8_t buffer[1 << 12] = {}; On 2017/05/23 23:05:13, waffles wrote: > On 2017/05/23 20:32:40, Devlin (catching up) wrote: > > Comment why 1 << 12? > > I don't know what to write in the comment. This constant was inherited from the > previous code, which predated my contributions to this project. Do you have a > recommendation for a different constant? Apparently this was added 8 years ago here: https://codereview.chromium.org/126014 I still have no idea where the 1 << 12 came from, but it seems to have been working, so I guess it's fine. https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx_verifier.cc:108: static_cast<int>(header_size)) On 2017/05/23 23:05:13, waffles wrote: > On 2017/05/23 20:32:40, Devlin (catching up) wrote: > > any risk of overflow? > > I don't think we can compile Chrome on any machine where int is less than 19 > bits. Let me know if you disagree. Ah, missed the check on line 105. I wonder if this would be clearer with something like: if (header_size > kMaxHeaderSize) return VerifierResult::ERROR_HEADER_INVALID; // This cast is safe because of the range check above. int header_size_int = static_cast<int>(header_size); That would also save us from doing the cast in ReadAndHashBuffer(). https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx_verifier.cc:139: const std::vector<std::pair<std::function<const RepeatedProof&()>, On 2017/05/23 23:05:13, waffles wrote: > On 2017/05/23 20:32:40, Devlin (catching up) wrote: > > std::function is banned in Chromium. Can you use base::Callback<> here? > > > > https://chromium-cpp.appspot.com > > Hacked on this for a while - does this look better, or do you still prefer the > use of Callback? This is fine. I think base::Callback is mildly more readable, but probably not worth the overhead.
https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... File components/crx_file/crx_verifier.cc (right): https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx_verifier.cc:108: static_cast<int>(header_size)) On 2017/05/24 17:10:54, Devlin (catching up) wrote: > On 2017/05/23 23:05:13, waffles wrote: > > On 2017/05/23 20:32:40, Devlin (catching up) wrote: > > > any risk of overflow? > > > > I don't think we can compile Chrome on any machine where int is less than 19 > > bits. Let me know if you disagree. > > Ah, missed the check on line 105. I wonder if this would be clearer with > something like: > if (header_size > kMaxHeaderSize) > return VerifierResult::ERROR_HEADER_INVALID; > // This cast is safe because of the range check above. > int header_size_int = static_cast<int>(header_size); > > That would also save us from doing the cast in ReadAndHashBuffer(). I added a comment. (I hate introducing names... >_>) It would be nice to instead write a static_assert that kMaxHeaderSize can fit in an "int", but I'm not sure how to safely write that assert. (The cast in RAHB is on the buffer, not the length, but maybe I misunderstand what you mean.) https://codereview.chromium.org/2888853003/diff/140001/components/crx_file/cr... components/crx_file/crx_verifier.cc:139: const std::vector<std::pair<std::function<const RepeatedProof&()>, On 2017/05/24 17:10:54, Devlin (catching up) wrote: > On 2017/05/23 23:05:13, waffles wrote: > > On 2017/05/23 20:32:40, Devlin (catching up) wrote: > > > std::function is banned in Chromium. Can you use base::Callback<> here? > > > > > > https://chromium-cpp.appspot.com > > > > Hacked on this for a while - does this look better, or do you still prefer the > > use of Callback? > > This is fine. I think base::Callback is mildly more readable, but probably not > worth the overhead. Great. Yeah, I started to use base::Callback, but I had to have a static_cast to the full function signature anyways (because the function is overloaded), and at that point I decided there was no need to wrap in callbacks, since I needed the other less-readable lines anyways...
The CQ bit was checked by waffles@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by waffles@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sorin@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2888853003/#ps180001 (title: "BUILD fix + comment.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1495655032818520,
"parent_rev": "66112904ff5090216cde196b83f0d44b6ae42e0f", "commit_rev":
"11f044437561f28e16d0a2549714d860310fb3fd"}
Message was sent while issue was closed.
Description was changed from ========== Expand CRX verifier to verify CRX₃ files. BUG=720092 ========== to ========== Expand CRX verifier to verify CRX₃ files. BUG=720092 Review-Url: https://codereview.chromium.org/2888853003 Cr-Commit-Position: refs/heads/master@{#474394} Committed: https://chromium.googlesource.com/chromium/src/+/11f044437561f28e16d0a2549714... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/11f044437561f28e16d0a2549714... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
