Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2874503002:
Refactor CRX verification in preparation to support CRX₃ files. (Closed)
DescriptionRefactor CRX verification in preparation to support CRX₃ files.
Clarified that the crx_file.* refers to CRX₂, moved verification out of the CRX₂ code to allow callers to specify CRX₃-permissive policies.
Also fixed a potential crash in extension_creator.cc.
BUG=720092
Review-Url: https://codereview.chromium.org/2874503002
Cr-Commit-Position: refs/heads/master@{#473780}
Committed: https://chromium.googlesource.com/chromium/src/+/5918d5fbdda9a61e2b83735975fcd7ebbf4d4645
Patch Set 1 : Base CL #
Total comments: 1
Patch Set 2 : No subclass #
Total comments: 76
Patch Set 3 : Remove the class #Patch Set 4 : Fix string constructor #Patch Set 5 : Do not reorder histogram #
Total comments: 6
Patch Set 6 : through #44 #
Total comments: 8
Patch Set 7 : through #51 #Dependent Patchsets: Messages
Total messages: 74 (56 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...
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
waffles@chromium.org changed reviewers: + sorin@chromium.org
Sorin, PTAL. In the meantime I think I'll add some tests.
The CQ bit was checked by waffles@chromium.org to run a CQ dry run
Updated the CL after our conversations yesterday.
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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Thank you! https://codereview.chromium.org/2874503002/diff/40001/components/crx_file/crx... File components/crx_file/crx2_file.h (right): https://codereview.chromium.org/2874503002/diff/40001/components/crx_file/crx... components/crx_file/crx2_file.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. do we retain the original copyright year? https://codereview.chromium.org/2874503002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/extension_creator.cc (right): https://codereview.chromium.org/2874503002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/extension_creator.cc:255: crx_file::Crx2File::Error error; initialize? https://codereview.chromium.org/2874503002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/extension_creator.cc:256: std::unique_ptr<crx_file::Crx2File> crx( consider using the assignment syntax for initialization if possible. https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a... https://codereview.chromium.org/2874503002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/extension_creator.cc:256: std::unique_ptr<crx_file::Crx2File> crx( Could use auto. https://codereview.chromium.org/2874503002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/extension_creator.cc:259: LOG(ERROR) << "cannot create Crx2FileHeader: " << error; If this error handling sufficient? It looks to me that the the execution flow continues and the code may crash @261. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... File components/crx_file/crx2_file.cc (right): https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx2_file.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... File components/crx_file/crx2_file.h (right): https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx2_file.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx2_file.h:50: // Returns a null scoped_ptr if erroneous values of |key_size| and/or Returns null if... https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... File components/crx_file/crx_verifier.cc (right): https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:25: static const uint32_t kMaxPublicKeySize = 1 << 16; static not needed here, also maybe use constexpr. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:34: // We assume a char is 8 bits long. we can compile time assert on the size of the char type. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:40: // Returns UINT32_MAX in the case of an unexpected EOF or read error. Can the hash on an int32 it be UINT32_MAX? https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:43: uint8_t buffer[4]; not initialized https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:43: uint8_t buffer[4]; 4 as a magic number used a several places, maybe use some sizeofs etc. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:56: key_hashes_.push_back(key_hash); We could handle the case where the key_hash lenght is different than crypto::kSHA256Length and not even allow setting up the object with invalid data members. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:91: char buffer[kCrx2FileHeaderMagicSize]; init? https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:92: int read = file.ReadAtCurrentPos(buffer, kCrx2FileHeaderMagicSize); const? https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:92: int read = file.ReadAtCurrentPos(buffer, kCrx2FileHeaderMagicSize); Maybe rename read to num_bytes or something more suggestive. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:102: uint32_t version = ReadAndHashLittleEndianUInt32(&file, file_hash.get()); const? https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:114: uint8_t final_hash[crypto::kSHA256Length]; init https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:121: return Result::ERROR_FILE_HASH_FAILED; It would be nice if the entire if can be rewritten just as: if (!crypto::SecureMemEqual(final_hash, expected_hash_.data(), crypto::kSHA256Length)) return Result::ERROR_FILE_HASH_FAILED; https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:135: uint32_t key_size = ReadAndHashLittleEndianUInt32(file, hash); some const here and below. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:148: std::unique_ptr<crypto::SecureHash> sha256( prefer = syntax https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:168: size_t len; init https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:174: std::string public_key_bytes = const? https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:174: std::string public_key_bytes = can you use a ctor or initializer instead of initializing with a copy ctor from a temp object? https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... File components/crx_file/crx_verifier.h (right): https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:25: // only, use a Crx3Verifier. comment seems obsolete. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:28: // Constructs a new CrxVerifier that verifies the file is a well-formed Comment seems appropriate as a class comment. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:38: // RequireKeyProof accumulate additional requirements. Is there any reason the caller wants to accumulate these instead of providing them at once? https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:43: // RequireFileHash replace previous expected hashes. Why would the caller want to replace the file hash? Can an instance of this class be used to verify more than one file? If yes, what is the motivation for this? https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:52: void RequirePublisherProof(); Should this function return an error to indicate this? https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:61: void GetPublicKey(std::string* public_key); why not return by value? https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:61: void GetPublicKey(std::string* public_key); const? https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:68: void GetCrxId(std::string* crx_id); return by value? https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:68: void GetCrxId(std::string* crx_id); const? https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:70: enum class Result { types usually declared toward the beginning of the section? Maybe move both the type and the function toward the top. Also, could add more errors to handle calling functions which currently return void. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:88: std::string* crx_id_ = nullptr; why pointers? https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:91: std::vector<uint8_t> expected_hash_; Is this type copyable? https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:94: Result VerifyCrx3(base::File* file, crypto::SecureHash* hash) const; Member function declaration precede data in class definitions, afair. https://codereview.chromium.org/2874503002/diff/120001/extensions/browser/san... File extensions/browser/sandboxed_unpacker.cc (right): https://codereview.chromium.org/2874503002/diff/120001/extensions/browser/san... extensions/browser/sandboxed_unpacker.cc:608: CrxVerifier::Result result = verifier.Verify(crx_path); const?
The CQ bit was checked by waffles@chromium.org to run a CQ dry run
https://codereview.chromium.org/2874503002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/extension_creator.cc (right): https://codereview.chromium.org/2874503002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/extension_creator.cc:255: crx_file::Crx2File::Error error; On 2017/05/15 19:49:51, Sorin Jianu wrote: > initialize? Done. https://codereview.chromium.org/2874503002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/extension_creator.cc:256: std::unique_ptr<crx_file::Crx2File> crx( On 2017/05/15 19:49:51, Sorin Jianu wrote: > consider using the assignment syntax for initialization if possible. > https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a... Done. https://codereview.chromium.org/2874503002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/extension_creator.cc:256: std::unique_ptr<crx_file::Crx2File> crx( On 2017/05/15 19:49:51, Sorin Jianu wrote: > Could use auto. Done. https://codereview.chromium.org/2874503002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/extension_creator.cc:259: LOG(ERROR) << "cannot create Crx2FileHeader: " << error; On 2017/05/15 19:49:51, Sorin Jianu wrote: > If this error handling sufficient? It looks to me that the the execution flow > continues and the code may crash @261. Hm, I agree. Seems strange. PTAL? https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... File components/crx_file/crx2_file.cc (right): https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx2_file.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2017/05/15 19:49:52, Sorin Jianu wrote: > 2017 Done. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... File components/crx_file/crx2_file.h (right): https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx2_file.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2017/05/15 19:49:52, Sorin Jianu wrote: > 2017 Are we supposed to update the Copyright when moving files? Anyways, done. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx2_file.h:50: // Returns a null scoped_ptr if erroneous values of |key_size| and/or On 2017/05/15 19:49:52, Sorin Jianu wrote: > Returns null if... Done. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... File components/crx_file/crx_verifier.cc (right): https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:25: static const uint32_t kMaxPublicKeySize = 1 << 16; On 2017/05/15 19:49:52, Sorin Jianu wrote: > static not needed here, also maybe use constexpr. Done. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:34: // We assume a char is 8 bits long. On 2017/05/15 19:49:52, Sorin Jianu wrote: > we can compile time assert on the size of the char type. Done. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:40: // Returns UINT32_MAX in the case of an unexpected EOF or read error. On 2017/05/15 19:49:52, Sorin Jianu wrote: > Can the hash on an int32 it be UINT32_MAX? I don't understand what you mean. Can you rephrase? Tried to clarify the comment. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:43: uint8_t buffer[4]; On 2017/05/15 19:49:52, Sorin Jianu wrote: > not initialized Done. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:43: uint8_t buffer[4]; On 2017/05/15 19:49:52, Sorin Jianu wrote: > 4 as a magic number used a several places, maybe use some sizeofs etc. Do you feel strongly about this? The return expression becomes more complex to read if it avoids the magic numbers. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:56: key_hashes_.push_back(key_hash); On 2017/05/15 19:49:52, Sorin Jianu wrote: > We could handle the case where the key_hash lenght is different than > crypto::kSHA256Length and not even allow setting up the object with invalid data > members. Obsolete. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:91: char buffer[kCrx2FileHeaderMagicSize]; On 2017/05/15 19:49:52, Sorin Jianu wrote: > init? Done. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:92: int read = file.ReadAtCurrentPos(buffer, kCrx2FileHeaderMagicSize); On 2017/05/15 19:49:52, Sorin Jianu wrote: > const? Acknowledged. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:92: int read = file.ReadAtCurrentPos(buffer, kCrx2FileHeaderMagicSize); On 2017/05/15 19:49:52, Sorin Jianu wrote: > Maybe rename read to num_bytes or something more suggestive. Inlined it. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:102: uint32_t version = ReadAndHashLittleEndianUInt32(&file, file_hash.get()); On 2017/05/15 19:49:52, Sorin Jianu wrote: > const? Done. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:114: uint8_t final_hash[crypto::kSHA256Length]; On 2017/05/15 19:49:52, Sorin Jianu wrote: > init Done. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:121: return Result::ERROR_FILE_HASH_FAILED; On 2017/05/15 19:49:52, Sorin Jianu wrote: > It would be nice if the entire if can be rewritten just as: > if (!crypto::SecureMemEqual(final_hash, expected_hash_.data(), > crypto::kSHA256Length)) > return Result::ERROR_FILE_HASH_FAILED; Acknowledged. Unfortunately we have to tolerate the case that no expected_hash_ was passed, even in the new format. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:135: uint32_t key_size = ReadAndHashLittleEndianUInt32(file, hash); On 2017/05/15 19:49:52, Sorin Jianu wrote: > some const here and below. Done. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:148: std::unique_ptr<crypto::SecureHash> sha256( On 2017/05/15 19:49:52, Sorin Jianu wrote: > prefer = syntax Done. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:168: size_t len; On 2017/05/15 19:49:52, Sorin Jianu wrote: > init Done. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:174: std::string public_key_bytes = On 2017/05/15 19:49:52, Sorin Jianu wrote: > can you use a ctor or initializer instead of initializing with a copy ctor from > a temp object? Done. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.cc:174: std::string public_key_bytes = On 2017/05/15 19:49:52, Sorin Jianu wrote: > const? Done. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... File components/crx_file/crx_verifier.h (right): https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:25: // only, use a Crx3Verifier. On 2017/05/15 19:49:53, Sorin Jianu wrote: > comment seems obsolete. Done. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:28: // Constructs a new CrxVerifier that verifies the file is a well-formed On 2017/05/15 19:49:53, Sorin Jianu wrote: > Comment seems appropriate as a class comment. Acknowledged. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:38: // RequireKeyProof accumulate additional requirements. On 2017/05/15 19:49:53, Sorin Jianu wrote: > Is there any reason the caller wants to accumulate these instead of providing > them at once? Obsolete. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:43: // RequireFileHash replace previous expected hashes. On 2017/05/15 19:49:53, Sorin Jianu wrote: > Why would the caller want to replace the file hash? Can an instance of this > class be used to verify more than one file? If yes, what is the motivation for > this? Obsolete. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:52: void RequirePublisherProof(); On 2017/05/15 19:49:53, Sorin Jianu wrote: > Should this function return an error to indicate this? Not sure what you mean. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:61: void GetPublicKey(std::string* public_key); On 2017/05/15 19:49:53, Sorin Jianu wrote: > const? Explained this in person. Anyways, obsolete. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:61: void GetPublicKey(std::string* public_key); On 2017/05/15 19:49:53, Sorin Jianu wrote: > why not return by value? Explained this in person. Anyways, obsolete. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:68: void GetCrxId(std::string* crx_id); On 2017/05/15 19:49:53, Sorin Jianu wrote: > const? Explained this in person. Anyways, obsolete. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:68: void GetCrxId(std::string* crx_id); On 2017/05/15 19:49:53, Sorin Jianu wrote: > return by value? Explained this in person. Anyways, obsolete. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:70: enum class Result { On 2017/05/15 19:49:53, Sorin Jianu wrote: > types usually declared toward the beginning of the section? Maybe move both the > type and the function toward the top. Also, could add more errors to handle > calling functions which currently return void. Done. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:88: std::string* crx_id_ = nullptr; On 2017/05/15 19:49:53, Sorin Jianu wrote: > why pointers? Explained this in person. Anyways, obsolete. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:91: std::vector<uint8_t> expected_hash_; On 2017/05/15 19:49:53, Sorin Jianu wrote: > Is this type copyable? Assuming you are referring to Verifier, obsolete. https://codereview.chromium.org/2874503002/diff/120001/components/crx_file/cr... components/crx_file/crx_verifier.h:94: Result VerifyCrx3(base::File* file, crypto::SecureHash* hash) const; On 2017/05/15 19:49:52, Sorin Jianu wrote: > Member function declaration precede data in class definitions, afair. Obsolete. https://codereview.chromium.org/2874503002/diff/120001/extensions/browser/san... File extensions/browser/sandboxed_unpacker.cc (right): https://codereview.chromium.org/2874503002/diff/120001/extensions/browser/san... extensions/browser/sandboxed_unpacker.cc:608: CrxVerifier::Result result = verifier.Verify(crx_path); On 2017/05/15 19:49:53, Sorin Jianu wrote: > const? 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_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm ty! https://codereview.chromium.org/2874503002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/extension_creator.cc (right): https://codereview.chromium.org/2874503002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/extension_creator.cc:17: #include "base/memory/ptr_util.h" we don't need this anymore now. https://codereview.chromium.org/2874503002/diff/180001/components/crx_file/cr... File components/crx_file/crx_verifier.cc (right): https://codereview.chromium.org/2874503002/diff/180001/components/crx_file/cr... components/crx_file/crx_verifier.cc:154: return VerifierResult::ERROR_HEADER_INVALID; we could say result = VerifierResult::ERROR_HEADER_INVALID; then the code returns naturally due to the if statement below. https://codereview.chromium.org/2874503002/diff/180001/extensions/browser/san... File extensions/browser/sandboxed_unpacker.cc (right): https://codereview.chromium.org/2874503002/diff/180001/extensions/browser/san... extensions/browser/sandboxed_unpacker.cc:610: std::vector<uint8_t> hash = {}; is there any reason to use an initialization list?
lgtm ty!
The CQ bit was checked by waffles@chromium.org to run a CQ dry run
https://codereview.chromium.org/2874503002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/extension_creator.cc (right): https://codereview.chromium.org/2874503002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/extension_creator.cc:17: #include "base/memory/ptr_util.h" On 2017/05/16 20:54:50, Sorin Jianu wrote: > we don't need this anymore now. Done. https://codereview.chromium.org/2874503002/diff/180001/components/crx_file/cr... File components/crx_file/crx_verifier.cc (right): https://codereview.chromium.org/2874503002/diff/180001/components/crx_file/cr... components/crx_file/crx_verifier.cc:154: return VerifierResult::ERROR_HEADER_INVALID; On 2017/05/16 20:54:50, Sorin Jianu wrote: > we could say result = VerifierResult::ERROR_HEADER_INVALID; > then the code returns naturally due to the if statement below. Done. https://codereview.chromium.org/2874503002/diff/180001/extensions/browser/san... File extensions/browser/sandboxed_unpacker.cc (right): https://codereview.chromium.org/2874503002/diff/180001/extensions/browser/san... extensions/browser/sandboxed_unpacker.cc:610: std::vector<uint8_t> hash = {}; On 2017/05/16 20:54:50, Sorin Jianu wrote: > is there any reason to use an initialization list? Done.
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: + rdevlin.cronin@chromium.org
+rdevlin.cronin; PTAL at extensions/ and chrome/extensions.
lgtm ty!
Can you expand the CL description a bit? Some of the changes in extensions land seem to imply a bit more than a simple refactor (which is fine, just might warrant more than a one-line description). https://codereview.chromium.org/2874503002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/extension_creator.cc (right): https://codereview.chromium.org/2874503002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/extension_creator.cc:256: auto crx = nit: IMO, this reduces readability, since it's not clear what type Create() returns. auto should be used with, e.g., base::MakeUnique<> which is understood to return a unique_ptr (and will be replaced by std::make_unique<>), but for things like this, prefer having the full type. https://codereview.chromium.org/2874503002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/extension_creator.cc:260: return false; this is a behavior change - maybe worth mentioning in the cl description? https://codereview.chromium.org/2874503002/diff/200001/extensions/browser/san... File extensions/browser/sandboxed_unpacker.cc (right): https://codereview.chromium.org/2874503002/diff/200001/extensions/browser/san... extensions/browser/sandboxed_unpacker.cc:10: #include <memory> memory's in the header, so no need for it here. https://codereview.chromium.org/2874503002/diff/200001/extensions/browser/san... File extensions/browser/sandboxed_unpacker_unittest.cc (right): https://codereview.chromium.org/2874503002/diff/200001/extensions/browser/san... extensions/browser/sandboxed_unpacker_unittest.cc:173: "0000000000000000000000000000000000000000000000000000000000000000"); I assume the fact that this is 64 characters is not a coincidence. :) This would be clearer (and shorter) as std::string(64, '0').
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...)
Description was changed from ========== Refactor CRX verification in preparation to support CRX₃ files. BUG=720092 ========== to ========== Refactor CRX verification in preparation to support CRX₃ files. Clarified that the crx_file.* refers to CRX₂, moved verification out of the CRX₂ code to allow callers to specify CRX₃-permissive policies. Also fixed a potential crash in extension_creator.cc. BUG=720092 ==========
The CQ bit was checked by waffles@chromium.org to run a CQ dry run
https://codereview.chromium.org/2874503002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/extension_creator.cc (right): https://codereview.chromium.org/2874503002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/extension_creator.cc:256: auto crx = On 2017/05/16 22:55:02, Devlin (catching up) wrote: > nit: IMO, this reduces readability, since it's not clear what type Create() > returns. auto should be used with, e.g., base::MakeUnique<> which is understood > to return a unique_ptr (and will be replaced by std::make_unique<>), but for > things like this, prefer having the full type. Done. https://codereview.chromium.org/2874503002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/extension_creator.cc:260: return false; On 2017/05/16 22:55:02, Devlin (catching up) wrote: > this is a behavior change - maybe worth mentioning in the cl description? Done. https://codereview.chromium.org/2874503002/diff/200001/extensions/browser/san... File extensions/browser/sandboxed_unpacker.cc (right): https://codereview.chromium.org/2874503002/diff/200001/extensions/browser/san... extensions/browser/sandboxed_unpacker.cc:10: #include <memory> On 2017/05/16 22:55:02, Devlin (catching up) wrote: > memory's in the header, so no need for it here. Done. https://codereview.chromium.org/2874503002/diff/200001/extensions/browser/san... File extensions/browser/sandboxed_unpacker_unittest.cc (right): https://codereview.chromium.org/2874503002/diff/200001/extensions/browser/san... extensions/browser/sandboxed_unpacker_unittest.cc:173: "0000000000000000000000000000000000000000000000000000000000000000"); On 2017/05/16 22:55:02, Devlin (catching up) wrote: > I assume the fact that this is 64 characters is not a coincidence. :) This > would be clearer (and shorter) as std::string(64, '0'). Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
extensions lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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 Link to the patchset: https://codereview.chromium.org/2874503002/#ps220001 (title: "through #51")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
waffles@chromium.org changed reviewers: + rockot@chromium.org
Ken, PTAL. (Is there a more current owner, by the way?)
On 2017/05/18 at 00:14:03, waffles wrote: > Ken, PTAL. (Is there a more current owner, by the way?) LGTM. Sorry I didn't see this sooner! I'll update the OWNERs file there to just alias back to extensions/OWNERS
The CQ bit was checked by waffles@chromium.org
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": 220001, "attempt_start_ts": 1495497979143870,
"parent_rev": "f32a232b18f75e5af490a160486468a55ef85641", "commit_rev":
"77d79a37c889a7d8cf58b3037d9e38eff6d73083"}
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1495497979143870,
"parent_rev": "2cbad67882de4eae81e58c0aed19419f2af31803", "commit_rev":
"5918d5fbdda9a61e2b83735975fcd7ebbf4d4645"}
Message was sent while issue was closed.
Description was changed from ========== Refactor CRX verification in preparation to support CRX₃ files. Clarified that the crx_file.* refers to CRX₂, moved verification out of the CRX₂ code to allow callers to specify CRX₃-permissive policies. Also fixed a potential crash in extension_creator.cc. BUG=720092 ========== to ========== Refactor CRX verification in preparation to support CRX₃ files. Clarified that the crx_file.* refers to CRX₂, moved verification out of the CRX₂ code to allow callers to specify CRX₃-permissive policies. Also fixed a potential crash in extension_creator.cc. BUG=720092 Review-Url: https://codereview.chromium.org/2874503002 Cr-Commit-Position: refs/heads/master@{#473780} Committed: https://chromium.googlesource.com/chromium/src/+/5918d5fbdda9a61e2b83735975fc... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:220001) as https://chromium.googlesource.com/chromium/src/+/5918d5fbdda9a61e2b83735975fc... |
