Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2934373002:
Record Code Signature of Downloaded DMG files (Closed)
DescriptionRecord Code Signature of Downloaded DMG files: As of MacOS 10.11.5, DMG file
metadata includes a signature for the disk image itself, in addition to code
signatures on individual executables within the archive. This change records
the DMG signature for uploading via SafeBrowsing pings.
BUG=627605
Review-Url: https://codereview.chromium.org/2934373002
Cr-Commit-Position: refs/heads/master@{#486012}
Committed: https://chromium.googlesource.com/chromium/src/+/276eacb1c63fa58b1fce7e668690c2b53a853f3b
Patch Set 1 : added sanity checks for UDIF signature parsing #Patch Set 2 : compiles #Patch Set 3 : tests passing in download protection service and sandbox analyzer #Patch Set 4 : adding signature in file #Patch Set 5 : debugging #Patch Set 6 : debugging weird proto stuff #Patch Set 7 : tests pass #Patch Set 8 : cleanup, still need to fix file paths #Patch Set 9 : adjusted test file path names #
Total comments: 18
Patch Set 10 : comments partially addressed #Patch Set 11 : addressing comments #Patch Set 12 : rebase update #Patch Set 13 : updating UMA XML #Patch Set 14 : simplifying data structures in download_protection_service.cc #
Total comments: 18
Patch Set 15 : addressing comments #
Total comments: 18
Patch Set 16 : mods to Makefile and checking in signed DMG #Patch Set 17 : addressing comments #Patch Set 18 : removing Chrome binary and signature file #Patch Set 19 : generate DMG with executable inside so that tests pass #Patch Set 20 : rebase for trybots #
Total comments: 2
Patch Set 21 : addressing comment #
Total comments: 4
Patch Set 22 : minor mods to XML #
Total comments: 7
Patch Set 23 : addressing comments #Patch Set 24 : rebase update #Patch Set 25 : rebase update #Patch Set 26 : correcting rebase mixup #Messages
Total messages: 120 (94 generated)
Description was changed from ========== first commit BUG=627605 ========== to ========== As of MacOS 10.11.5, DMG file metadata includes a signature for the disk image itself, in addition to code signatures on individual executables within the archive. This change records the DMG signature for uploading via SafeBrowsing pings. BUG=627605 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #2 (id:160001) has been deleted
Patchset #2 (id:180001) has been deleted
Patchset #2 (id:200001) has been deleted
Patchset #2 (id:220001) has been deleted
Patchset #2 (id:240001) has been deleted
Patchset #2 (id:260001) has been deleted
Patchset #3 (id:300001) has been deleted
The CQ bit was checked by mortonm@google.com 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...
mortonm@google.com changed reviewers: + jialiul@chromium.org, rsesek@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
I assume we will want to use a smaller DMG than Chrome for the tester, but wasn't sure what we want here. Should I generate a signed DMG myself with the available fake private key and codesign configuration used to test signature extraction on Mac?
The CQ bit was checked by mortonm@google.com 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-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...)
mortonm@, please don't use chrome dmg. We don't want to expose chrome executable in chromium repository. rsesek@, do you have any testing dmgs that has a valid signature?
On 2017/06/26 18:28:36, Jialiu Lin wrote: > rsesek@, do you have any testing dmgs that has a valid signature? No, but one can be created as discussed on the internal thread. Using the artifacts in this directory, https://cs.chromium.org/chromium/src/chrome/test/data/safe_browsing/mach_o/RE..., you can codesign a DMG (it's the same command for codesigning an app). Just add a new target to the Makefile to produce and then sign the DMG, and check in that change as well as the resulting binary. (We don't want to run codesign as a buildstep). https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/dmg_iterator.h (right): https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/dmg_iterator.h:42: uint8_t* GetDmgSignatureData(); naming: GetCodeSignature() Using a vector will also clean up this interface into one method call. https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/dmg_iterator.h:42: uint8_t* GetDmgSignatureData(); This method needs a comment. https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.cc (right): https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:572: trailer.code_signature_offset + trailer.code_signature_length <= Use safe_numerics for this math. There are examples in this file. https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:573: (uint64_t)trailer_start) { C-style casts are banned by the styleguide. https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:575: uint8_t* temp_data = new uint8_t[temp_size]; This is leaked. Rather than tracking size and bytes as two separate fields, use a std::vector. That will also fix the ownership issue. https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:577: if (temp_data == nullptr) { This doesn't happen in Chromium (failure to allocate is fatal). https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:590: if (!stream_->Read(temp_data, temp_size, &bytes_read)) { … and then using a vector, you std::vector<unit8_t>::resize to the code_signature_length and then read directly into the vector member. https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.h (right): https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:57: uint8_t* GetDmgSignatureData(); This should return |const std::vector<uint8_t>&|.
The CQ bit was checked by mortonm@google.com 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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mortonm@google.com 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-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Still have to generate the signed DMG for testing purposes (instead of using Chrome executable) but figured I'd respond to the coding comments. https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/dmg_iterator.h (right): https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/dmg_iterator.h:42: uint8_t* GetDmgSignatureData(); On 2017/06/26 18:56:33, Robert Sesek wrote: > This method needs a comment. Done. https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/dmg_iterator.h:42: uint8_t* GetDmgSignatureData(); On 2017/06/26 18:56:33, Robert Sesek wrote: > naming: GetCodeSignature() > > Using a vector will also clean up this interface into one method call. Done. https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.cc (right): https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:572: trailer.code_signature_offset + trailer.code_signature_length <= On 2017/06/26 18:56:33, Robert Sesek wrote: > Use safe_numerics for this math. There are examples in this file. Done. https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:573: (uint64_t)trailer_start) { On 2017/06/26 18:56:33, Robert Sesek wrote: > C-style casts are banned by the styleguide. Done. https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:575: uint8_t* temp_data = new uint8_t[temp_size]; On 2017/06/26 18:56:33, Robert Sesek wrote: > This is leaked. Rather than tracking size and bytes as two separate fields, use > a std::vector. That will also fix the ownership issue. Done. https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:577: if (temp_data == nullptr) { On 2017/06/26 18:56:33, Robert Sesek wrote: > This doesn't happen in Chromium (failure to allocate is fatal). So do you think I should put a cap on the size of signature field specified by file metadata for which we try to allocate a buffer? Otherwise our analysis could be evaded by simply setting the value at offset trailer.code_signature_length to be arbitrarily large. On the other hand this may make the DMG un-openable on Mac, so maybe we don't care. https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:590: if (!stream_->Read(temp_data, temp_size, &bytes_read)) { On 2017/06/26 18:56:33, Robert Sesek wrote: > … and then using a vector, you std::vector<unit8_t>::resize to the > code_signature_length and then read directly into the vector member. Done. https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.h (right): https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:57: uint8_t* GetDmgSignatureData(); On 2017/06/26 18:56:33, Robert Sesek wrote: > This should return |const std::vector<uint8_t>&|. Done.
Patchset #14 (id:540001) has been deleted
Patchset #14 (id:560001) has been deleted
Looking great! My comments are mostly nits. I'll take another look after you updating the testing file. https://codereview.chromium.org/2934373002/diff/580001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2934373002/diff/580001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:818: #if defined(OS_MACOSX) You probably don't need the #if/#endif here, since this function will only be called on Mac OS, due to https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/download_pr... https://codereview.chromium.org/2934373002/diff/580001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:820: disk_image_signature_ = std::unique_ptr<std::vector<uint8_t>>( change "std::unique_ptr<std::vector<uint8_t>>(new std::vector<uint8_t>(results.signature_blob));" into "base::MakeUnique<std::vector<uint8_t>>(results.signature_blob);" MakeUnique<T>(args) should be preferred over WrapUnique(new T(args)): bare calls to `new` should be treated with scrutiny. https://codereview.chromium.org/2934373002/diff/580001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service_unittest.cc (right): https://codereview.chromium.org/2934373002/diff/580001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service_unittest.cc:1478: EXPECT_EQ(GetClientDownloadRequest()->udif_code_signature().length(), EXPECT_EQ(expected_value, actual_value); https://codereview.chromium.org/2934373002/diff/580001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service_unittest.cc:1489: EXPECT_EQ(signature.length(), (uint64_t)9454); EXPECT_EQ(expected_value, actual_value); https://codereview.chromium.org/2934373002/diff/580001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service_unittest.cc:1491: EXPECT_EQ( nit: how about EXPECT_EQ(signature, GetClientDownloadRequest()->udif_code_signature()); https://codereview.chromium.org/2934373002/diff/580001/chrome/browser/safe_br... File chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac_unittest.cc (right): https://codereview.chromium.org/2934373002/diff/580001/chrome/browser/safe_br... chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac_unittest.cc:153: EXPECT_EQ(results.signature_blob.size(), (uint64_t)0); EXPECT_EQ(expected_value, actual_value); https://codereview.chromium.org/2934373002/diff/580001/chrome/browser/safe_br... chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac_unittest.cc:168: EXPECT_EQ(results.signature_blob.size(), (uint64_t)9454); EXPECT_EQ(expected_value, actual_value); https://codereview.chromium.org/2934373002/diff/580001/chrome/browser/safe_br... chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac_unittest.cc:180: EXPECT_EQ(signature.length(), (uint64_t)9454); EXPECT_EQ(expected_value, actual_value); https://codereview.chromium.org/2934373002/diff/580001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.h (right): https://codereview.chromium.org/2934373002/diff/580001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:85: std::vector<uint8_t> Nit: Shorten the comment to make it fit into one line. std::vector<unit8_t> signature_blob_; // DMG signature.
https://codereview.chromium.org/2934373002/diff/580001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2934373002/diff/580001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:818: #if defined(OS_MACOSX) On 2017/06/28 00:23:34, Jialiu Lin wrote: > You probably don't need the #if/#endif here, since this function will only be > called on Mac OS, due to > https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/download_pr... Done. https://codereview.chromium.org/2934373002/diff/580001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:820: disk_image_signature_ = std::unique_ptr<std::vector<uint8_t>>( On 2017/06/28 00:23:34, Jialiu Lin wrote: > change "std::unique_ptr<std::vector<uint8_t>>(new > std::vector<uint8_t>(results.signature_blob));" into > "base::MakeUnique<std::vector<uint8_t>>(results.signature_blob);" > > MakeUnique<T>(args) should be preferred over WrapUnique(new T(args)): bare calls > to `new` should be treated with scrutiny. Done. https://codereview.chromium.org/2934373002/diff/580001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service_unittest.cc (right): https://codereview.chromium.org/2934373002/diff/580001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service_unittest.cc:1478: EXPECT_EQ(GetClientDownloadRequest()->udif_code_signature().length(), On 2017/06/28 00:23:34, Jialiu Lin wrote: > EXPECT_EQ(expected_value, actual_value); Done. https://codereview.chromium.org/2934373002/diff/580001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service_unittest.cc:1489: EXPECT_EQ(signature.length(), (uint64_t)9454); On 2017/06/28 00:23:35, Jialiu Lin wrote: > EXPECT_EQ(expected_value, actual_value); Done. https://codereview.chromium.org/2934373002/diff/580001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service_unittest.cc:1491: EXPECT_EQ( On 2017/06/28 00:23:34, Jialiu Lin wrote: > nit: how about > EXPECT_EQ(signature, GetClientDownloadRequest()->udif_code_signature()); Done. https://codereview.chromium.org/2934373002/diff/580001/chrome/browser/safe_br... File chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac_unittest.cc (right): https://codereview.chromium.org/2934373002/diff/580001/chrome/browser/safe_br... chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac_unittest.cc:153: EXPECT_EQ(results.signature_blob.size(), (uint64_t)0); On 2017/06/28 00:23:35, Jialiu Lin wrote: > EXPECT_EQ(expected_value, actual_value); Done. https://codereview.chromium.org/2934373002/diff/580001/chrome/browser/safe_br... chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac_unittest.cc:168: EXPECT_EQ(results.signature_blob.size(), (uint64_t)9454); On 2017/06/28 00:23:35, Jialiu Lin wrote: > EXPECT_EQ(expected_value, actual_value); Done. https://codereview.chromium.org/2934373002/diff/580001/chrome/browser/safe_br... chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac_unittest.cc:180: EXPECT_EQ(signature.length(), (uint64_t)9454); On 2017/06/28 00:23:35, Jialiu Lin wrote: > EXPECT_EQ(expected_value, actual_value); Done. https://codereview.chromium.org/2934373002/diff/580001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.h (right): https://codereview.chromium.org/2934373002/diff/580001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:85: std::vector<uint8_t> On 2017/06/28 00:23:35, Jialiu Lin wrote: > Nit: Shorten the comment to make it fit into one line. > std::vector<unit8_t> signature_blob_; // DMG signature. Done.
Really nice! Hopefully you, me, and Greg can figure out a solution to the keychain signing problem :\. Also: the "Subject" field in the codereview tool does not make it into the git commit message. So please copy it to be the first line of the CL description. This issue doesn't exist in Gerrit ;-). https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.cc (right): https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:577: if (temp_data == nullptr) { On 2017/06/27 16:36:23, mortonm wrote: > On 2017/06/26 18:56:33, Robert Sesek wrote: > > This doesn't happen in Chromium (failure to allocate is fatal). > > So do you think I should put a cap on the size of signature field specified by > file metadata for which we try to allocate a buffer? Otherwise our analysis > could be evaded by simply setting the value at offset > trailer.code_signature_length to be arbitrarily large. > > On the other hand this may make the DMG un-openable on Mac, so maybe we don't > care. I could see putting a cap on the signature_length, but I don't know what value to use. 10K, 1M, etc? It wouldn't totally thwart analysis, though, right? Just the outer code signature extraction. https://codereview.chromium.org/2934373002/diff/600001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service_unittest.cc (right): https://codereview.chromium.org/2934373002/diff/600001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service_unittest.cc:1478: EXPECT_EQ((uint64_t)9454, Remove the cast and just use |9454u|. https://codereview.chromium.org/2934373002/diff/600001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service_unittest.cc:1489: EXPECT_EQ((uint64_t)9454, signature.length()); Same. https://codereview.chromium.org/2934373002/diff/600001/chrome/browser/safe_br... File chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac_unittest.cc (right): https://codereview.chromium.org/2934373002/diff/600001/chrome/browser/safe_br... chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac_unittest.cc:153: EXPECT_EQ((uint64_t)0, results.signature_blob.size()); Same, just use a 'u' suffix. https://codereview.chromium.org/2934373002/diff/600001/chrome/browser/safe_br... chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac_unittest.cc:168: EXPECT_EQ((uint64_t)9454, results.signature_blob.size()); Same. https://codereview.chromium.org/2934373002/diff/600001/chrome/browser/safe_br... chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac_unittest.cc:178: EXPECT_EQ((uint64_t)9454, from_file.length()); Same. https://codereview.chromium.org/2934373002/diff/600001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/dmg_iterator.h (right): https://codereview.chromium.org/2934373002/diff/600001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/dmg_iterator.h:40: // Returns the raw code signature file metadata. "This will be empty for DMGs that are not signed." https://codereview.chromium.org/2934373002/diff/600001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.cc (right): https://codereview.chromium.org/2934373002/diff/600001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:587: if (bytes_read != trailer.code_signature_length) DLOGing here would be helpful too, I think. https://codereview.chromium.org/2934373002/diff/600001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.h (right): https://codereview.chromium.org/2934373002/diff/600001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:55: const std::vector<uint8_t>& GetDmgSignatureData(); Use the same name as in DMGIterator (GetCodeSignature). https://codereview.chromium.org/2934373002/diff/600001/components/safe_browsi... File components/safe_browsing/csd.proto (right): https://codereview.chromium.org/2934373002/diff/600001/components/safe_browsi... components/safe_browsing/csd.proto:511: optional bytes udif_code_signature = 40; Comment?
Description was changed from ========== As of MacOS 10.11.5, DMG file metadata includes a signature for the disk image itself, in addition to code signatures on individual executables within the archive. This change records the DMG signature for uploading via SafeBrowsing pings. BUG=627605 ========== to ========== Record Code Signature of Downloaded DMG files: As of MacOS 10.11.5, DMG file metadata includes a signature for the disk image itself, in addition to code signatures on individual executables within the archive. This change records the DMG signature for uploading via SafeBrowsing pings. BUG=627605 ==========
https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.cc (right): https://codereview.chromium.org/2934373002/diff/440001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:577: if (temp_data == nullptr) { On 2017/06/28 18:21:06, Robert Sesek wrote: > On 2017/06/27 16:36:23, mortonm wrote: > > On 2017/06/26 18:56:33, Robert Sesek wrote: > > > This doesn't happen in Chromium (failure to allocate is fatal). > > > > So do you think I should put a cap on the size of signature field specified by > > file metadata for which we try to allocate a buffer? Otherwise our analysis > > could be evaded by simply setting the value at offset > > trailer.code_signature_length to be arbitrarily large. > > > > On the other hand this may make the DMG un-openable on Mac, so maybe we don't > > care. > > I could see putting a cap on the signature_length, but I don't know what value > to use. 10K, 1M, etc? It wouldn't totally thwart analysis, though, right? Just > the outer code signature extraction. As of now, if the metadata in trailer.code_signature_length has a huge value (e.g. 5GB), the underlying memory allocator for the vector will likely fail for the call to signature_blob_.resize(trailer.code_signature_length). I'm guessing the exception thrown (http://www.cplusplus.com/reference/vector/vector/resize/) will crash the utility process. Of course this necessitates the file being 5GB as well. I'm just gonna leave the code as is for now since this evasion would require a very large DMG, which in a soon-to-come CL would not be unpacked by the DMG analyzer anyway. https://codereview.chromium.org/2934373002/diff/600001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service_unittest.cc (right): https://codereview.chromium.org/2934373002/diff/600001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service_unittest.cc:1478: EXPECT_EQ((uint64_t)9454, On 2017/06/28 18:21:06, Robert Sesek wrote: > Remove the cast and just use |9454u|. Done. https://codereview.chromium.org/2934373002/diff/600001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service_unittest.cc:1489: EXPECT_EQ((uint64_t)9454, signature.length()); On 2017/06/28 18:21:06, Robert Sesek wrote: > Same. Done. https://codereview.chromium.org/2934373002/diff/600001/chrome/browser/safe_br... File chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac_unittest.cc (right): https://codereview.chromium.org/2934373002/diff/600001/chrome/browser/safe_br... chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac_unittest.cc:153: EXPECT_EQ((uint64_t)0, results.signature_blob.size()); On 2017/06/28 18:21:06, Robert Sesek wrote: > Same, just use a 'u' suffix. Done. https://codereview.chromium.org/2934373002/diff/600001/chrome/browser/safe_br... chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac_unittest.cc:168: EXPECT_EQ((uint64_t)9454, results.signature_blob.size()); On 2017/06/28 18:21:06, Robert Sesek wrote: > Same. Done. https://codereview.chromium.org/2934373002/diff/600001/chrome/browser/safe_br... chrome/browser/safe_browsing/sandboxed_dmg_analyzer_mac_unittest.cc:178: EXPECT_EQ((uint64_t)9454, from_file.length()); On 2017/06/28 18:21:06, Robert Sesek wrote: > Same. Done. https://codereview.chromium.org/2934373002/diff/600001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/dmg_iterator.h (right): https://codereview.chromium.org/2934373002/diff/600001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/dmg_iterator.h:40: // Returns the raw code signature file metadata. On 2017/06/28 18:21:06, Robert Sesek wrote: > "This will be empty for DMGs that are not signed." Done. https://codereview.chromium.org/2934373002/diff/600001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.cc (right): https://codereview.chromium.org/2934373002/diff/600001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:587: if (bytes_read != trailer.code_signature_length) On 2017/06/28 18:21:06, Robert Sesek wrote: > DLOGing here would be helpful too, I think. Done. https://codereview.chromium.org/2934373002/diff/600001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.h (right): https://codereview.chromium.org/2934373002/diff/600001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:55: const std::vector<uint8_t>& GetDmgSignatureData(); On 2017/06/28 18:21:06, Robert Sesek wrote: > Use the same name as in DMGIterator (GetCodeSignature). Done. https://codereview.chromium.org/2934373002/diff/600001/components/safe_browsi... File components/safe_browsing/csd.proto (right): https://codereview.chromium.org/2934373002/diff/600001/components/safe_browsi... components/safe_browsing/csd.proto:511: optional bytes udif_code_signature = 40; On 2017/06/28 18:21:06, Robert Sesek wrote: > Comment? Done.
mortonm@google.com changed reviewers: + kerrnel@chromium.org
The CQ bit was checked by mortonm@google.com 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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mortonm@google.com 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-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mortonm@google.com 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-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mortonm@google.com 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.
mortonm@google.com changed reviewers: + asvitkine@chromium.org
mortonm@google.com changed reviewers: - asvitkine@chromium.org
mortonm@google.com changed reviewers: + rkaplow@chromium.org
adding reviewers kerrnel@ and rkaplow@ for OWNER of tools/metrics/histograms/histograms.xml
c/b/safe_browsing/* LGTM c/u/safe_browsing/* LGTM components/safe_browsing/* LGTM I'll rely on kerrnel@ and rsesek@ to review chrome/test/data/safe_browsing/*
https://codereview.chromium.org/2934373002/diff/700001/chrome/test/data/safe_... File chrome/test/data/safe_browsing/mach_o/Makefile (right): https://codereview.chromium.org/2934373002/diff/700001/chrome/test/data/safe_... chrome/test/data/safe_browsing/mach_o/Makefile:10: pre-build = security import codesign.key; security import codesign.crt I would make this && because if one fails, the other won' be useful.
https://codereview.chromium.org/2934373002/diff/700001/chrome/test/data/safe_... File chrome/test/data/safe_browsing/mach_o/Makefile (right): https://codereview.chromium.org/2934373002/diff/700001/chrome/test/data/safe_... chrome/test/data/safe_browsing/mach_o/Makefile:10: pre-build = security import codesign.key; security import codesign.crt On 2017/06/30 18:16:22, Greg K wrote: > I would make this && because if one fails, the other won' be useful. Done.
lgtm https://codereview.chromium.org/2934373002/diff/720001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2934373002/diff/720001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:47874: + specific way: - Not used at all during whole day. - Started, but failed to if you're reformatting the text, this is a bit unreadable now. https://codereview.chromium.org/2934373002/diff/720001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:65069: + Records whether a given download file is a cryptographically signed DMG it is probably obvious for anyone looking, but might want to mention this is Mac only
Patchset #22 (id:740001) has been deleted
https://codereview.chromium.org/2934373002/diff/720001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2934373002/diff/720001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:47874: + specific way: - Not used at all during whole day. - Started, but failed to On 2017/06/30 20:31:40, rkaplow wrote: > if you're reformatting the text, this is a bit unreadable now. Sorry, didn't notice the formatting tool went in and changed that, I changed it back. https://codereview.chromium.org/2934373002/diff/720001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:65069: + Records whether a given download file is a cryptographically signed DMG On 2017/06/30 20:31:40, rkaplow wrote: > it is probably obvious for anyone looking, but might want to mention this is Mac > only Done.
Almost there! Just a few nits about the Makefile. https://codereview.chromium.org/2934373002/diff/760001/chrome/test/data/safe_... File chrome/test/data/safe_browsing/mach_o/Makefile (right): https://codereview.chromium.org/2934373002/diff/760001/chrome/test/data/safe_... chrome/test/data/safe_browsing/mach_o/Makefile:9: # Funcitons to add and remove key and cert to users keychain. Can you add some more comments here about why these are necessary? It'd also be nice to update the README in this directory discussing the issue you found with codesign. (Something like: "The signed binaries will import a temporary codesigning identity into the current user's keychain. This is because the `codesign -k` flag, to use a specific keychain file, is no longer respected. The temporary identity will be removed after the test binary is signed.") https://codereview.chromium.org/2934373002/diff/760001/chrome/test/data/safe_... chrome/test/data/safe_browsing/mach_o/Makefile:68: $(call pre-build) Makefiles should use tabs (here and throughout the newly added lines). Tip: In Rietveld, the diff shows tabs as red » characters. https://codereview.chromium.org/2934373002/diff/760001/chrome/test/data/safe_... chrome/test/data/safe_browsing/mach_o/Makefile:122: $(call post-build) Tabs here.
On 2017/07/05 18:53:12, Robert Sesek wrote: > Almost there! Just a few nits about the Makefile. > > https://codereview.chromium.org/2934373002/diff/760001/chrome/test/data/safe_... > File chrome/test/data/safe_browsing/mach_o/Makefile (right): > > https://codereview.chromium.org/2934373002/diff/760001/chrome/test/data/safe_... > chrome/test/data/safe_browsing/mach_o/Makefile:9: # Funcitons to add and remove > key and cert to users keychain. > Can you add some more comments here about why these are necessary? > > It'd also be nice to update the README in this directory discussing the issue > you found with codesign. (Something like: "The signed binaries will import a > temporary codesigning identity into the current user's keychain. This is because > the `codesign -k` flag, to use a specific keychain file, is no longer respected. > The temporary identity will be removed after the test binary is signed.") > > https://codereview.chromium.org/2934373002/diff/760001/chrome/test/data/safe_... > chrome/test/data/safe_browsing/mach_o/Makefile:68: $(call pre-build) > Makefiles should use tabs (here and throughout the newly added lines). > > Tip: In Rietveld, the diff shows tabs as red » characters. > > https://codereview.chromium.org/2934373002/diff/760001/chrome/test/data/safe_... > chrome/test/data/safe_browsing/mach_o/Makefile:122: $(call post-build) > Tabs here. Thanks Robert! mortonm@ is OOO for a conference in Germany this week. So he probably won't reply until next week. :-)
https://codereview.chromium.org/2934373002/diff/760001/chrome/test/data/safe_... File chrome/test/data/safe_browsing/mach_o/Makefile (right): https://codereview.chromium.org/2934373002/diff/760001/chrome/test/data/safe_... chrome/test/data/safe_browsing/mach_o/Makefile:9: # Funcitons to add and remove key and cert to users keychain. On 2017/07/05 18:53:11, Robert Sesek wrote: > Can you add some more comments here about why these are necessary? > > It'd also be nice to update the README in this directory discussing the issue > you found with codesign. (Something like: "The signed binaries will import a > temporary codesigning identity into the current user's keychain. This is because > the `codesign -k` flag, to use a specific keychain file, is no longer respected. > The temporary identity will be removed after the test binary is signed.") Done. https://codereview.chromium.org/2934373002/diff/760001/chrome/test/data/safe_... chrome/test/data/safe_browsing/mach_o/Makefile:68: $(call pre-build) On 2017/07/05 18:53:11, Robert Sesek wrote: > Makefiles should use tabs (here and throughout the newly added lines). > > Tip: In Rietveld, the diff shows tabs as red » characters. I double checked, and all the places that are missing the indent arrows do indeed use tab characters - so not sure how to explain the missing arrows in Rietveld. https://codereview.chromium.org/2934373002/diff/760001/chrome/test/data/safe_... chrome/test/data/safe_browsing/mach_o/Makefile:122: $(call post-build) On 2017/07/05 18:53:11, Robert Sesek wrote: > Tabs here. Same as above.
LGTM https://codereview.chromium.org/2934373002/diff/760001/chrome/test/data/safe_... File chrome/test/data/safe_browsing/mach_o/Makefile (right): https://codereview.chromium.org/2934373002/diff/760001/chrome/test/data/safe_... chrome/test/data/safe_browsing/mach_o/Makefile:68: $(call pre-build) On 2017/07/10 16:31:48, mortonm wrote: > On 2017/07/05 18:53:11, Robert Sesek wrote: > > Makefiles should use tabs (here and throughout the newly added lines). > > > > Tip: In Rietveld, the diff shows tabs as red » characters. > > I double checked, and all the places that are missing the indent arrows do > indeed use tab characters - so not sure how to explain the missing arrows in > Rietveld. Acknowledged. Thanks for checking!
The CQ bit was checked by mortonm@google.com 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-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 mortonm@google.com 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 mortonm@google.com to run a CQ dry run
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...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mortonm@google.com 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-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 mortonm@google.com 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-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 mortonm@google.com 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_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 mortonm@google.com 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_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by mortonm@google.com 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_...)
ping kerrnel@~ Any more comments?
The CQ bit was checked by mortonm@google.com 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...
On 2017/07/12 01:54:06, Jialiu Lin wrote: > ping kerrnel@~ Any more comments? Robert got everything, his LGTM stands. Thanks, Greg
On 2017/07/12 16:35:32, Greg K wrote: > On 2017/07/12 01:54:06, Jialiu Lin wrote: > > ping kerrnel@~ Any more comments? > > Robert got everything, his LGTM stands. > > Thanks, > > Greg Actually there is one thing I just noticed, please format the CL description to be 80 characters wide. - Greg
Description was changed from ========== Record Code Signature of Downloaded DMG files: As of MacOS 10.11.5, DMG file metadata includes a signature for the disk image itself, in addition to code signatures on individual executables within the archive. This change records the DMG signature for uploading via SafeBrowsing pings. BUG=627605 ========== to ========== Record Code Signature of Downloaded DMG files: As of MacOS 10.11.5, DMG file metadata includes a signature for the disk image itself, in addition to code signatures on individual executables within the archive. This change records the DMG signature for uploading via SafeBrowsing pings. BUG=627605 ==========
On 2017/07/12 16:40:11, Greg K wrote: > On 2017/07/12 16:35:32, Greg K wrote: > > On 2017/07/12 01:54:06, Jialiu Lin wrote: > > > ping kerrnel@~ Any more comments? > > > > Robert got everything, his LGTM stands. > > > > Thanks, > > > > Greg > > Actually there is one thing I just noticed, please format the CL description to > be 80 characters wide. > > - Greg Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/07/12 17:06:22, mortonm wrote: > On 2017/07/12 16:40:11, Greg K wrote: > > On 2017/07/12 16:35:32, Greg K wrote: > > > On 2017/07/12 01:54:06, Jialiu Lin wrote: > > > > ping kerrnel@~ Any more comments? > > > > > > Robert got everything, his LGTM stands. > > > > > > Thanks, > > > > > > Greg > > > > Actually there is one thing I just noticed, please format the CL description > to > > be 80 characters wide. > > > > - Greg > > Done. LGTM
The CQ bit was checked by mortonm@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jialiul@chromium.org, rkaplow@chromium.org, rsesek@chromium.org Link to the patchset: https://codereview.chromium.org/2934373002/#ps840001 (title: "correcting rebase mixup")
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": 840001, "attempt_start_ts": 1499881038671040,
"parent_rev": "b3a69b95bdaa7e2df899e7bb02b0c7680748bdd4", "commit_rev":
"276eacb1c63fa58b1fce7e668690c2b53a853f3b"}
Message was sent while issue was closed.
Description was changed from ========== Record Code Signature of Downloaded DMG files: As of MacOS 10.11.5, DMG file metadata includes a signature for the disk image itself, in addition to code signatures on individual executables within the archive. This change records the DMG signature for uploading via SafeBrowsing pings. BUG=627605 ========== to ========== Record Code Signature of Downloaded DMG files: As of MacOS 10.11.5, DMG file metadata includes a signature for the disk image itself, in addition to code signatures on individual executables within the archive. This change records the DMG signature for uploading via SafeBrowsing pings. BUG=627605 Review-Url: https://codereview.chromium.org/2934373002 Cr-Commit-Position: refs/heads/master@{#486012} Committed: https://chromium.googlesource.com/chromium/src/+/276eacb1c63fa58b1fce7e668690... ==========
Message was sent while issue was closed.
Committed patchset #26 (id:840001) as https://chromium.googlesource.com/chromium/src/+/276eacb1c63fa58b1fce7e668690... |
