Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2926473002:
Mac Archive Type Sniffing (Closed)
DescriptionRather than simply relying on file extension (e.g. .DMG) to determine whether to extract DMG features from download, instead look at file metadata to check for existence of 'koly' signature which identifies Mac archive types.
BUG=600392
>Review-Url: https://codereview.chromium.org/2926473002
>Cr-Commit-Position: refs/heads/master@{#480631}
>Committed: >https://chromium.googlesource.com/chromium/src/+/6602ea616cce79869b300ffa82c3fc64fe422200
Review-Url: https://codereview.chromium.org/2926473002
Cr-Commit-Position: refs/heads/master@{#482463}
Committed: https://chromium.googlesource.com/chromium/src/+/177cde5a7f2d95de6601f01e9bee8fdedf13a96b
Patch Set 1 #Patch Set 2 : checkpoint #Patch Set 3 : koly check compiling, not yet tested #Patch Set 4 : removing unnecessary big endian conversions #Patch Set 5 : removing comments #Patch Set 6 : linker error with template stuff in posttaskandreplywithresult #Patch Set 7 : updated build file #Patch Set 8 : saving work #Patch Set 9 : tester compiler error with destructor #Patch Set 10 : adding tester file #Patch Set 11 : restructured tester to take place on file thread #Patch Set 12 : tests pass #Patch Set 13 : testing all the disk image formats #Patch Set 14 : added ParseTrailer() functionality to UDIFParser and switched to value-parameterized tests #Patch Set 15 : simplified unit test since not parsing actual compressed data in DMGs #
Total comments: 18
Patch Set 16 : added check that ParseTrailer() is only called once #
Total comments: 4
Patch Set 17 : addressing comments #
Total comments: 2
Patch Set 18 : removed GetTrailer() function from UDIFParser #
Total comments: 5
Patch Set 19 : adding tester for download protection service #
Total comments: 8
Patch Set 20 : moving UDIFResourceFile definition back into .cc file #Patch Set 21 : matching style #
Total comments: 11
Patch Set 22 : addressing comments #Patch Set 23 : more addressing comments #Patch Set 24 : minor edit to comment #
Total comments: 6
Patch Set 25 : adding comment to download protection unit test #
Total comments: 3
Patch Set 26 : simplifying code flow in download protection service unit test #Patch Set 27 : adding switch for either only parse UDIF trailer or completely unpack archive #Patch Set 28 : reading file directly #Patch Set 29 : adding signature check file read and removing mods to UDIFParser #Patch Set 30 : adding signature check file read and removing mods to UDIFParser #
Total comments: 10
Patch Set 31 : addressing comments #
Total comments: 6
Patch Set 32 : addressing comments. still have to rename mac_archive_type_sniffer #Patch Set 33 : changing name of MacArchiveTypeSniffer to DiskImageTypeSnifferMac #Patch Set 34 : changing name of tester #
Total comments: 12
Patch Set 35 : addressing comments #Patch Set 36 : removing redundant DCHECK #Patch Set 37 : revising DownloadProtectionServiceTest.TestDownloadItemDestroyed #Patch Set 38 : adding ifdef for Mac in tester #Patch Set 39 : debugging CQ failure #Patch Set 40 : checkpoint #Patch Set 41 : fixing bad commit #Patch Set 42 : resolving merge conflict #Patch Set 43 : avoiding multiple asynchronous calls in tester for mac #Messages
Total messages: 100 (61 generated)
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_...)
Description was changed from ========== archive type sniffing checkpoint comment BUG=600392 ========== to ========== Rather than simply relying on file extension (e.g. .DMG) to determine whether to extract DMG features from download, instead look at file metadata to check for existence of 'koly' signature which identifies Mac archive types. BUG=600392 ==========
mortonm@google.com changed reviewers: + jialiul@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 checked by jialiul@chromium.org to run a CQ dry run
Good job! Mostly nits. https://codereview.chromium.org/2926473002/diff/270001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2926473002/diff/270001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:801: } else { nit: curly braces are not required for single-line statements, if (download_file_has_koly_signature) StartExtractDmgFeatures(); else StartExtractFileFeatures(); https://codereview.chromium.org/2926473002/diff/270001/chrome/browser/safe_br... File chrome/browser/safe_browsing/mac_archive_type_sniffer.cc (right): https://codereview.chromium.org/2926473002/diff/270001/chrome/browser/safe_br... chrome/browser/safe_browsing/mac_archive_type_sniffer.cc:25: if (parser.GetTrailer() != NULL) { You can simply this as: return !!parser.GetTrailer(); https://codereview.chromium.org/2926473002/diff/270001/chrome/browser/safe_br... File chrome/browser/safe_browsing/mac_archive_type_sniffer.h (right): https://codereview.chromium.org/2926473002/diff/270001/chrome/browser/safe_br... chrome/browser/safe_browsing/mac_archive_type_sniffer.h:11: nit: please remove this extra blank line https://codereview.chromium.org/2926473002/diff/270001/chrome/browser/safe_br... chrome/browser/safe_browsing/mac_archive_type_sniffer.h:29: static bool FileIsArchiveType(const base::FilePath& dmg_file); nit: how about IsAppleDiskImage() to make it more specific? zip is a kind of archive too. https://codereview.chromium.org/2926473002/diff/270001/chrome/browser/safe_br... File chrome/browser/safe_browsing/mac_archive_type_sniffer_unittest.cc (right): https://codereview.chromium.org/2926473002/diff/270001/chrome/browser/safe_br... chrome/browser/safe_browsing/mac_archive_type_sniffer_unittest.cc:51: if (test_case.expected_results) { nit: no need {} around single line statement https://codereview.chromium.org/2926473002/diff/270001/chrome/browser/safe_br... chrome/browser/safe_browsing/mac_archive_type_sniffer_unittest.cc:92: // .dmg extension. Note that DownloadProtectionService does not check for You probably don't need to mention DownloadProtectionService here. Feel free to remove the second sentence. https://codereview.chromium.org/2926473002/diff/270001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.cc (right): https://codereview.chromium.org/2926473002/diff/270001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:31: // The following structures come from the analysis provided by Jonathan Levin Remove line 31-34. Already in .h file. https://codereview.chromium.org/2926473002/diff/270001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.h (right): https://codereview.chromium.org/2926473002/diff/270001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:78: #pragma pack(pop) https://codereview.chromium.org/2926473002/diff/270001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:105: // trailer. Otherwise returns NULL. s/NULL/nullptr https://codereview.chromium.org/2926473002/diff/290001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.cc (right): https://codereview.chromium.org/2926473002/diff/290001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:307: trailer_successfully_parsed_ = ParseTrailer(); A better way to do this: 1. Don't call ParseTrailer() in the constructor. 2. Make ParseTrailer() a public function 3. set trailer_successfully_parsed_ at the end of ParseTrailer() function. 4. call ParseTrailer() the first thing in ParseBlkx 5. In mac_archieve_type_sniffer, you do ... dmg::UDIFParser parser(&read_stream); return parser.ParseTrailer(); ... 6. you probably don't need GetTrailer() at all.
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_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2926473002/diff/270001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2926473002/diff/270001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:801: } else { On 2017/06/09 18:48:49, Jialiu Lin wrote: > nit: curly braces are not required for single-line statements, > > if (download_file_has_koly_signature) > StartExtractDmgFeatures(); > else > StartExtractFileFeatures(); Done. https://codereview.chromium.org/2926473002/diff/270001/chrome/browser/safe_br... File chrome/browser/safe_browsing/mac_archive_type_sniffer.cc (right): https://codereview.chromium.org/2926473002/diff/270001/chrome/browser/safe_br... chrome/browser/safe_browsing/mac_archive_type_sniffer.cc:25: if (parser.GetTrailer() != NULL) { On 2017/06/09 18:48:49, Jialiu Lin wrote: > You can simply this as: > > return !!parser.GetTrailer(); > Done. https://codereview.chromium.org/2926473002/diff/270001/chrome/browser/safe_br... File chrome/browser/safe_browsing/mac_archive_type_sniffer.h (right): https://codereview.chromium.org/2926473002/diff/270001/chrome/browser/safe_br... chrome/browser/safe_browsing/mac_archive_type_sniffer.h:11: On 2017/06/09 18:48:49, Jialiu Lin wrote: > nit: please remove this extra blank line Done. https://codereview.chromium.org/2926473002/diff/270001/chrome/browser/safe_br... chrome/browser/safe_browsing/mac_archive_type_sniffer.h:29: static bool FileIsArchiveType(const base::FilePath& dmg_file); On 2017/06/09 18:48:49, Jialiu Lin wrote: > nit: how about IsAppleDiskImage() to make it more specific? zip is a kind of > archive too. Done. https://codereview.chromium.org/2926473002/diff/270001/chrome/browser/safe_br... File chrome/browser/safe_browsing/mac_archive_type_sniffer_unittest.cc (right): https://codereview.chromium.org/2926473002/diff/270001/chrome/browser/safe_br... chrome/browser/safe_browsing/mac_archive_type_sniffer_unittest.cc:51: if (test_case.expected_results) { On 2017/06/09 18:48:49, Jialiu Lin wrote: > nit: no need {} around single line statement Done. https://codereview.chromium.org/2926473002/diff/270001/chrome/browser/safe_br... chrome/browser/safe_browsing/mac_archive_type_sniffer_unittest.cc:92: // .dmg extension. Note that DownloadProtectionService does not check for On 2017/06/09 18:48:49, Jialiu Lin wrote: > You probably don't need to mention DownloadProtectionService here. Feel free to > remove the second sentence. Done. https://codereview.chromium.org/2926473002/diff/270001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.cc (right): https://codereview.chromium.org/2926473002/diff/270001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:31: // The following structures come from the analysis provided by Jonathan Levin On 2017/06/09 18:48:49, Jialiu Lin wrote: > Remove line 31-34. Already in .h file. Done. https://codereview.chromium.org/2926473002/diff/270001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.h (right): https://codereview.chromium.org/2926473002/diff/270001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:78: On 2017/06/09 18:48:49, Jialiu Lin wrote: > #pragma pack(pop) Done. https://codereview.chromium.org/2926473002/diff/270001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:105: // trailer. Otherwise returns NULL. On 2017/06/09 18:48:49, Jialiu Lin wrote: > s/NULL/nullptr Done. https://codereview.chromium.org/2926473002/diff/290001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.cc (right): https://codereview.chromium.org/2926473002/diff/290001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:307: trailer_successfully_parsed_ = ParseTrailer(); On 2017/06/09 18:48:49, Jialiu Lin wrote: > A better way to do this: > 1. Don't call ParseTrailer() in the constructor. > 2. Make ParseTrailer() a public function > 3. set trailer_successfully_parsed_ at the end of ParseTrailer() function. > 4. call ParseTrailer() the first thing in ParseBlkx > 5. In mac_archieve_type_sniffer, you do > ... > dmg::UDIFParser parser(&read_stream); > return parser.ParseTrailer(); > ... > > 6. you probably don't need GetTrailer() at all. As we just discussed, I'm going to leave GetTrailer() in, since we will want to obtain the DMG signatures from the trailer in the near future. Regardless, I have moved ParseTrailer() out of the constructor and made it a public method.
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...
rsesek@chromium.org changed reviewers: + rsesek@chromium.org
https://codereview.chromium.org/2926473002/diff/290001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.cc (right): https://codereview.chromium.org/2926473002/diff/290001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:307: trailer_successfully_parsed_ = ParseTrailer(); On 2017/06/09 19:47:21, mortonm wrote: > On 2017/06/09 18:48:49, Jialiu Lin wrote: > > A better way to do this: > > 1. Don't call ParseTrailer() in the constructor. > > 2. Make ParseTrailer() a public function > > 3. set trailer_successfully_parsed_ at the end of ParseTrailer() function. > > 4. call ParseTrailer() the first thing in ParseBlkx > > 5. In mac_archieve_type_sniffer, you do > > ... > > dmg::UDIFParser parser(&read_stream); > > return parser.ParseTrailer(); > > ... > > > > 6. you probably don't need GetTrailer() at all. > > As we just discussed, I'm going to leave GetTrailer() in, since we will want to > obtain the DMG signatures from the trailer in the near future. Regardless, I > have moved ParseTrailer() out of the constructor and made it a public method. I'd strongly prefer to not expose the UDIF internal types. For extracting signatures, the DMGParser/UDIFParser could expose a method that just provides signature information.
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_...)
https://codereview.chromium.org/2926473002/diff/290001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.cc (right): https://codereview.chromium.org/2926473002/diff/290001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:307: trailer_successfully_parsed_ = ParseTrailer(); On 2017/06/09 20:02:32, Robert Sesek wrote: > On 2017/06/09 19:47:21, mortonm wrote: > > On 2017/06/09 18:48:49, Jialiu Lin wrote: > > > A better way to do this: > > > 1. Don't call ParseTrailer() in the constructor. > > > 2. Make ParseTrailer() a public function > > > 3. set trailer_successfully_parsed_ at the end of ParseTrailer() function. > > > 4. call ParseTrailer() the first thing in ParseBlkx > > > 5. In mac_archieve_type_sniffer, you do > > > ... > > > dmg::UDIFParser parser(&read_stream); > > > return parser.ParseTrailer(); > > > ... > > > > > > 6. you probably don't need GetTrailer() at all. > > > > As we just discussed, I'm going to leave GetTrailer() in, since we will want > to > > obtain the DMG signatures from the trailer in the near future. Regardless, I > > have moved ParseTrailer() out of the constructor and made it a public method. > > I'd strongly prefer to not expose the UDIF internal types. For extracting > signatures, the DMGParser/UDIFParser could expose a method that just provides > signature information. Ok. I've taken out GetTrailer(). We will add a GetCodeSignature() function in a later CL and accessor functions for any other part of the trailer that we need.
https://codereview.chromium.org/2926473002/diff/310001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2926473002/diff/310001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:476: // Checks for existence of "koly" signature even if file doesn't have You also need to add unittest in download_protection_service_unittest.cc to verify that a ClientDownloadReport is sent if a dmg pretends to be an other type. The following function for your reference: https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/download_pr...
https://codereview.chromium.org/2926473002/diff/330001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.h (right): https://codereview.chromium.org/2926473002/diff/330001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:26: #pragma pack(push, 1) Can this be left back in the .cc file now?
https://codereview.chromium.org/2926473002/diff/310001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2926473002/diff/310001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:476: // Checks for existence of "koly" signature even if file doesn't have On 2017/06/09 20:24:18, Jialiu Lin wrote: > You also need to add unittest in download_protection_service_unittest.cc to > verify that a ClientDownloadReport is sent if a dmg pretends to be an other > type. > > The following function for your reference: > https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/download_pr... Ok, I have added the test. Currently I only check whether the DownloadProtectionService determines that there is a valid archive in the download file or not. Let me know if you think there is any more extensive checking I should do.
https://codereview.chromium.org/2926473002/diff/330001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.h (right): https://codereview.chromium.org/2926473002/diff/330001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:26: #pragma pack(push, 1) On 2017/06/09 21:46:19, Robert Sesek wrote: > Can this be left back in the .cc file now? I kept it in the .h since we want to maintain a private field (trailer_) that stores the trailer for access in the future. For example, we will want to query the DMG signature (and possibly other fields) and would rather not have to re-parse the file if it had already been parsed. Does this make sense or is there a better way to do this?
https://codereview.chromium.org/2926473002/diff/330001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.h (right): https://codereview.chromium.org/2926473002/diff/330001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:26: #pragma pack(push, 1) On 2017/06/09 21:46:19, Robert Sesek wrote: > Can this be left back in the .cc file now? I kept it in the .h since we want to maintain a private field (trailer_) that stores the trailer for access in the future. For example, we will want to query the DMG signature (and possibly other fields) and would rather not have to re-parse the file if it had already been parsed. Does this make sense or is there a better way to do this? >> Nevermind, I'll switch the private member containing the entire trailer for an assortment of members representing the fields of the trailer that we want to be accessible to other classes. This way we can move the definition back in to the .cc
The CQ bit was checked by mortonm@google.com to run a CQ dry run
vakh@chromium.org changed reviewers: + vakh@chromium.org
https://codereview.chromium.org/2926473002/diff/350001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2926473002/diff/350001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:479: BrowserThread::PostTaskAndReplyWithResult( qq: does this mean that the method goes from being synchronous to async (due to PostTaskAndReplyWithResult)? https://codereview.chromium.org/2926473002/diff/350001/chrome/browser/safe_br... File chrome/browser/safe_browsing/mac_archive_type_sniffer.cc (right): https://codereview.chromium.org/2926473002/diff/350001/chrome/browser/safe_br... chrome/browser/safe_browsing/mac_archive_type_sniffer.cc:19: DCHECK(file.IsValid()); Should this be a DCHECK? Shouldn't this check be in release builds also? https://codereview.chromium.org/2926473002/diff/350001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.cc (right): https://codereview.chromium.org/2926473002/diff/350001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:29: #pragma pack(push, 1) Do we still need this preprocessor directive here? https://codereview.chromium.org/2926473002/diff/350001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:345: if (!trailer_successfully_parsed_ || !ParseBlkx()) shortcut this as: return trailer_successfully_parsed_ && ParseBlkx()
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2926473002/diff/350001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2926473002/diff/350001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:479: BrowserThread::PostTaskAndReplyWithResult( On 2017/06/12 16:20:28, vakh (Varun Khaneja) wrote: > qq: does this mean that the method goes from being synchronous to async (due to > PostTaskAndReplyWithResult)? Yes, the execution is asynchronous only for the purpose of doing the work on the FILE thread rather than the main UI thread. https://codereview.chromium.org/2926473002/diff/350001/chrome/browser/safe_br... File chrome/browser/safe_browsing/mac_archive_type_sniffer.cc (right): https://codereview.chromium.org/2926473002/diff/350001/chrome/browser/safe_br... chrome/browser/safe_browsing/mac_archive_type_sniffer.cc:19: DCHECK(file.IsValid()); On 2017/06/12 16:20:28, vakh (Varun Khaneja) wrote: > Should this be a DCHECK? Shouldn't this check be in release builds also? Done. https://codereview.chromium.org/2926473002/diff/350001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.cc (right): https://codereview.chromium.org/2926473002/diff/350001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:29: #pragma pack(push, 1) On 2017/06/12 16:20:28, vakh (Varun Khaneja) wrote: > Do we still need this preprocessor directive here? Yes -- I've moved the struct definition back into the .cc file and the directive is necessary to make the struct the correct size. https://codereview.chromium.org/2926473002/diff/350001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:345: if (!trailer_successfully_parsed_ || !ParseBlkx()) On 2017/06/12 16:20:28, vakh (Varun Khaneja) wrote: > shortcut this as: return trailer_successfully_parsed_ && ParseBlkx() Done.
https://codereview.chromium.org/2926473002/diff/330001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.h (right): https://codereview.chromium.org/2926473002/diff/330001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:26: #pragma pack(push, 1) On 2017/06/09 23:11:13, mortonm wrote: > On 2017/06/09 21:46:19, Robert Sesek wrote: > > Can this be left back in the .cc file now? > > I kept it in the .h since we want to maintain a private field (trailer_) that > stores the trailer for access in the future. For example, we will want to query > the DMG signature (and possibly other fields) and would rather not have to > re-parse the file if it had already been parsed. Does this make sense or is > there a better way to do this? > > >> Nevermind, I'll switch the private member containing the entire trailer for > an assortment of members representing the fields of the trailer that we want to > be accessible to other classes. This way we can move the definition back in to > the .cc You can also heap-allocate it and store it as a unique_ptr member, and only forward-declare the struct names. https://codereview.chromium.org/2926473002/diff/390001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2926473002/diff/390001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:4893: outputs = [ Need to update the list of test data output since you're generating more. https://codereview.chromium.org/2926473002/diff/390001/chrome/test/data/safe_... File chrome/test/data/safe_browsing/dmg/generate_test_data.sh (right): https://codereview.chromium.org/2926473002/diff/390001/chrome/test/data/safe_... chrome/test/data/safe_browsing/dmg/generate_test_data.sh:90: "${OUT_DIR}/mach_o_in_dmg_no_koly_signature.dmg" nit: indent continuation lines by 4 spaces so it's clear that this is not a new command https://codereview.chromium.org/2926473002/diff/390001/chrome/test/data/safe_... chrome/test/data/safe_browsing/dmg/generate_test_data.sh:103: nit: extra blank line https://codereview.chromium.org/2926473002/diff/390001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.h (right): https://codereview.chromium.org/2926473002/diff/390001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:53: bool ParseTrailer(); This should be private, callers should only call Parse(), and then a getter can be exposed to get trailer_successfuly_parsed().
https://codereview.chromium.org/2926473002/diff/330001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.h (right): https://codereview.chromium.org/2926473002/diff/330001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:26: #pragma pack(push, 1) On 2017/06/12 16:38:39, Robert Sesek wrote: > On 2017/06/09 23:11:13, mortonm wrote: > > On 2017/06/09 21:46:19, Robert Sesek wrote: > > > Can this be left back in the .cc file now? > > > > I kept it in the .h since we want to maintain a private field (trailer_) that > > stores the trailer for access in the future. For example, we will want to > query > > the DMG signature (and possibly other fields) and would rather not have to > > re-parse the file if it had already been parsed. Does this make sense or is > > there a better way to do this? > > > > >> Nevermind, I'll switch the private member containing the entire trailer for > > an assortment of members representing the fields of the trailer that we want > to > > be accessible to other classes. This way we can move the definition back in to > > the .cc > > You can also heap-allocate it and store it as a unique_ptr member, and only > forward-declare the struct names. Done. https://codereview.chromium.org/2926473002/diff/390001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2926473002/diff/390001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:4893: outputs = [ On 2017/06/12 16:38:39, Robert Sesek wrote: > Need to update the list of test data output since you're generating more. Done. https://codereview.chromium.org/2926473002/diff/390001/chrome/test/data/safe_... File chrome/test/data/safe_browsing/dmg/generate_test_data.sh (right): https://codereview.chromium.org/2926473002/diff/390001/chrome/test/data/safe_... chrome/test/data/safe_browsing/dmg/generate_test_data.sh:90: "${OUT_DIR}/mach_o_in_dmg_no_koly_signature.dmg" On 2017/06/12 16:38:39, Robert Sesek wrote: > nit: indent continuation lines by 4 spaces so it's clear that this is not a new > command Done. https://codereview.chromium.org/2926473002/diff/390001/chrome/test/data/safe_... chrome/test/data/safe_browsing/dmg/generate_test_data.sh:103: On 2017/06/12 16:38:39, Robert Sesek wrote: > nit: extra blank line Done. https://codereview.chromium.org/2926473002/diff/390001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.h (right): https://codereview.chromium.org/2926473002/diff/390001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:53: bool ParseTrailer(); On 2017/06/12 16:38:39, Robert Sesek wrote: > This should be private, callers should only call Parse(), and then a getter can > be exposed to get trailer_successfuly_parsed(). Yeah, that does seem like a better interface. One potential issue is that this code path is only concerned with determining whether the downloaded file has the "koly" trailer. If we call the whole Parse() code path (i.e. ParseTrailer() and ParseBlkx()) then for such files the entire unpacking of the DMG will happen twice - since we're not actually using the results of the unpacking here. I guess this isn't a big deal, since the only time this happens is for files without a disk-image-type extension (e.g. DMG) that do have a "koly" signature, which should be rare and possibly only done by malware. I went ahead and made the change anyway.
https://codereview.chromium.org/2926473002/diff/390001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.h (right): https://codereview.chromium.org/2926473002/diff/390001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:53: bool ParseTrailer(); On 2017/06/12 18:31:16, mortonm wrote: > On 2017/06/12 16:38:39, Robert Sesek wrote: > > This should be private, callers should only call Parse(), and then a getter > can > > be exposed to get trailer_successfuly_parsed(). > > Yeah, that does seem like a better interface. One potential issue is that this > code path is only concerned with determining whether the downloaded file has the > "koly" trailer. If we call the whole Parse() code path (i.e. ParseTrailer() and > ParseBlkx()) then for such files the entire unpacking of the DMG will happen > twice - since we're not actually using the results of the unpacking here. I > guess this isn't a big deal, since the only time this happens is for files > without a disk-image-type extension (e.g. DMG) that do have a "koly" signature, > which should be rare and possibly only done by malware. > > I went ahead and made the change anyway. Actually, there is another issue to consider here. In the current workflow, a file with a 'koly' signature and no disk-image-type extension will be fully unpacked on the file thread without any dedicated sandbox to contain the processing. This could be a security/reliability liability -- not sure. We think a possible way forward is to leave the ParseTrailer() function as public for now, as there is soon going to be a refactoring effort to move the classes defined in download_protection_service.cc (e.g. CheckClientDownloadRequest) to their own .h files. At that point, we can make the ParseTrailer() function private and declare CheckClientDownloadRequest as a friend class of UDIFParser.
https://codereview.chromium.org/2926473002/diff/390001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.h (right): https://codereview.chromium.org/2926473002/diff/390001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:53: bool ParseTrailer(); On 2017/06/12 20:04:01, mortonm wrote: > On 2017/06/12 18:31:16, mortonm wrote: > > On 2017/06/12 16:38:39, Robert Sesek wrote: > > > This should be private, callers should only call Parse(), and then a getter > > can > > > be exposed to get trailer_successfuly_parsed(). > > > > Yeah, that does seem like a better interface. One potential issue is that this > > code path is only concerned with determining whether the downloaded file has > the > > "koly" trailer. If we call the whole Parse() code path (i.e. ParseTrailer() > and > > ParseBlkx()) then for such files the entire unpacking of the DMG will happen > > twice - since we're not actually using the results of the unpacking here. I > > guess this isn't a big deal, since the only time this happens is for files > > without a disk-image-type extension (e.g. DMG) that do have a "koly" > signature, > > which should be rare and possibly only done by malware. > > > > I went ahead and made the change anyway. > > Actually, there is another issue to consider here. In the current workflow, a > file with a 'koly' signature and no disk-image-type extension will be fully > unpacked on the file thread without any dedicated sandbox to contain the > processing. This could be a security/reliability liability -- not sure. Yes, that's definitely a security issue. We shouldn't be parsing really any untrusted content in the browser process. > We think a possible way forward is to leave the ParseTrailer() function as > public for now, as there is soon going to be a refactoring effort to move the > classes defined in download_protection_service.cc (e.g. > CheckClientDownloadRequest) to their own .h files. At that point, we can make > the ParseTrailer() function private and declare CheckClientDownloadRequest as a > friend class of UDIFParser. OK. https://codereview.chromium.org/2926473002/diff/450001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.cc (right): https://codereview.chromium.org/2926473002/diff/450001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:363: if (trailer_.get() == nullptr) This will never happen (Chromium terminates on OOM). But you could lazily allocate the trailer_ member and use the presence of the object to indicate whether or not the trailer has been parsed. https://codereview.chromium.org/2926473002/diff/450001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.h (right): https://codereview.chromium.org/2926473002/diff/450001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:25: nit: no blank line (and sort by type name between class and struct) https://codereview.chromium.org/2926473002/diff/450001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:89: std::unique_ptr<UDIFResourceFile> trailer_; These two members need comments. https://codereview.chromium.org/2926473002/diff/470001/chrome/browser/safe_br... File chrome/browser/safe_browsing/mac_archive_type_sniffer.cc (right): https://codereview.chromium.org/2926473002/diff/470001/chrome/browser/safe_br... chrome/browser/safe_browsing/mac_archive_type_sniffer.cc:7: #include "chrome/utility/safe_browsing/mac/udif.h" The comment about sandboxing led me to realize: this is not going to pass check_deps. chrome/browser cannot include chrome/utility.
https://codereview.chromium.org/2926473002/diff/390001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.h (right): https://codereview.chromium.org/2926473002/diff/390001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:53: bool ParseTrailer(); On 2017/06/12 20:52:34, Robert Sesek wrote: > On 2017/06/12 20:04:01, mortonm wrote: > > On 2017/06/12 18:31:16, mortonm wrote: > > > On 2017/06/12 16:38:39, Robert Sesek wrote: > > > > This should be private, callers should only call Parse(), and then a > getter > > > can > > > > be exposed to get trailer_successfuly_parsed(). > > > > > > Yeah, that does seem like a better interface. One potential issue is that > this > > > code path is only concerned with determining whether the downloaded file has > > the > > > "koly" trailer. If we call the whole Parse() code path (i.e. ParseTrailer() > > and > > > ParseBlkx()) then for such files the entire unpacking of the DMG will happen > > > twice - since we're not actually using the results of the unpacking here. I > > > guess this isn't a big deal, since the only time this happens is for files > > > without a disk-image-type extension (e.g. DMG) that do have a "koly" > > signature, > > > which should be rare and possibly only done by malware. > > > > > > I went ahead and made the change anyway. > > > > Actually, there is another issue to consider here. In the current workflow, a > > file with a 'koly' signature and no disk-image-type extension will be fully > > unpacked on the file thread without any dedicated sandbox to contain the > > processing. This could be a security/reliability liability -- not sure. > > Yes, that's definitely a security issue. We shouldn't be parsing really any > untrusted content in the browser process. > > > We think a possible way forward is to leave the ParseTrailer() function as > > public for now, as there is soon going to be a refactoring effort to move the > > classes defined in download_protection_service.cc (e.g. > > CheckClientDownloadRequest) to their own .h files. At that point, we can make > > the ParseTrailer() function private and declare CheckClientDownloadRequest as > a > > friend class of UDIFParser. > > OK. As addressed in the later comment, we think we need to do the signature checking in a utility process anyway. https://codereview.chromium.org/2926473002/diff/450001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.cc (right): https://codereview.chromium.org/2926473002/diff/450001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.cc:363: if (trailer_.get() == nullptr) On 2017/06/12 20:52:34, Robert Sesek wrote: > This will never happen (Chromium terminates on OOM). But you could lazily > allocate the trailer_ member and use the presence of the object to indicate > whether or not the trailer has been parsed. I removed the null pointer check, but unless anyone has a strong preference will leave the UDIFResourceFile struct initialization the way it is, since I think that's a little more clear. https://codereview.chromium.org/2926473002/diff/450001/chrome/utility/safe_br... File chrome/utility/safe_browsing/mac/udif.h (right): https://codereview.chromium.org/2926473002/diff/450001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:25: On 2017/06/12 20:52:34, Robert Sesek wrote: > nit: no blank line (and sort by type name between class and struct) Done. https://codereview.chromium.org/2926473002/diff/450001/chrome/utility/safe_br... chrome/utility/safe_browsing/mac/udif.h:89: std::unique_ptr<UDIFResourceFile> trailer_; On 2017/06/12 20:52:34, Robert Sesek wrote: > These two members need comments. Done. https://codereview.chromium.org/2926473002/diff/470001/chrome/browser/safe_br... File chrome/browser/safe_browsing/mac_archive_type_sniffer.cc (right): https://codereview.chromium.org/2926473002/diff/470001/chrome/browser/safe_br... chrome/browser/safe_browsing/mac_archive_type_sniffer.cc:7: #include "chrome/utility/safe_browsing/mac/udif.h" On 2017/06/12 20:52:34, Robert Sesek wrote: > The comment about sandboxing led me to realize: this is not going to pass > check_deps. chrome/browser cannot include chrome/utility. Assuming we need to refactor to use a full-blown utility process to do the 4-byte koly signature check. Don't see another way around this, unless there's a way to issue IPC calls from the mojo client without actually creating a utility sandbox process to service them.
https://codereview.chromium.org/2926473002/diff/470001/chrome/browser/safe_br... File chrome/browser/safe_browsing/mac_archive_type_sniffer.cc (right): https://codereview.chromium.org/2926473002/diff/470001/chrome/browser/safe_br... chrome/browser/safe_browsing/mac_archive_type_sniffer.cc:7: #include "chrome/utility/safe_browsing/mac/udif.h" On 2017/06/12 21:39:49, mortonm wrote: > On 2017/06/12 20:52:34, Robert Sesek wrote: > > The comment about sandboxing led me to realize: this is not going to pass > > check_deps. chrome/browser cannot include chrome/utility. > > Assuming we need to refactor to use a full-blown utility process to do the > 4-byte koly signature check. Don't see another way around this, unless there's a > way to issue IPC calls from the mojo client without actually creating a utility > sandbox process to service them. Yeah, but I agree that seems heavyweight for just doing a file sniff. Another option: don't use UDIFParser in the browser process to perform this check. Simply seek to the trailer offset, read the uint32 and do the 'koly' check -- that's basically the same thing as https://cs.chromium.org/chromium/src/chrome/common/safe_browsing/mach_o_image... If it passes that, then spin up the utility process to do any additional scanning.
On 2017/06/13 15:52:55, Robert Sesek wrote: > https://codereview.chromium.org/2926473002/diff/470001/chrome/browser/safe_br... > File chrome/browser/safe_browsing/mac_archive_type_sniffer.cc (right): > > https://codereview.chromium.org/2926473002/diff/470001/chrome/browser/safe_br... > chrome/browser/safe_browsing/mac_archive_type_sniffer.cc:7: #include > "chrome/utility/safe_browsing/mac/udif.h" > On 2017/06/12 21:39:49, mortonm wrote: > > On 2017/06/12 20:52:34, Robert Sesek wrote: > > > The comment about sandboxing led me to realize: this is not going to pass > > > check_deps. chrome/browser cannot include chrome/utility. > > > > Assuming we need to refactor to use a full-blown utility process to do the > > 4-byte koly signature check. Don't see another way around this, unless there's > a > > way to issue IPC calls from the mojo client without actually creating a > utility > > sandbox process to service them. > > Yeah, but I agree that seems heavyweight for just doing a file sniff. Another > option: don't use UDIFParser in the browser process to perform this check. > Simply seek to the trailer offset, read the uint32 and do the 'koly' check -- > that's basically the same thing as > https://cs.chromium.org/chromium/src/chrome/common/safe_browsing/mach_o_image... > > If it passes that, then spin up the utility process to do any additional > scanning. Alright, that sounds good to me. @rsesek - are you ok with us leaving the changes to UDIFParser that separated the trailer parsing from the data block parsing? We will be doing further DMG metadata checks in the future and it may be desirable to be able to parse just the trailer without unpacking the entire archive.
On 2017/06/13 16:56:57, mortonm wrote: > On 2017/06/13 15:52:55, Robert Sesek wrote: > > > https://codereview.chromium.org/2926473002/diff/470001/chrome/browser/safe_br... > > File chrome/browser/safe_browsing/mac_archive_type_sniffer.cc (right): > > > > > https://codereview.chromium.org/2926473002/diff/470001/chrome/browser/safe_br... > > chrome/browser/safe_browsing/mac_archive_type_sniffer.cc:7: #include > > "chrome/utility/safe_browsing/mac/udif.h" > > On 2017/06/12 21:39:49, mortonm wrote: > > > On 2017/06/12 20:52:34, Robert Sesek wrote: > > > > The comment about sandboxing led me to realize: this is not going to pass > > > > check_deps. chrome/browser cannot include chrome/utility. > > > > > > Assuming we need to refactor to use a full-blown utility process to do the > > > 4-byte koly signature check. Don't see another way around this, unless > there's > > a > > > way to issue IPC calls from the mojo client without actually creating a > > utility > > > sandbox process to service them. > > > > Yeah, but I agree that seems heavyweight for just doing a file sniff. Another > > option: don't use UDIFParser in the browser process to perform this check. > > Simply seek to the trailer offset, read the uint32 and do the 'koly' check -- > > that's basically the same thing as > > > https://cs.chromium.org/chromium/src/chrome/common/safe_browsing/mach_o_image... > > > > If it passes that, then spin up the utility process to do any additional > > scanning. > > Alright, that sounds good to me. @rsesek - are you ok with us leaving the > changes to UDIFParser that separated the trailer parsing from the data block > parsing? We will be doing further DMG metadata checks in the future and it may > be desirable to be able to parse just the trailer without unpacking the entire > archive. I don't think it'd belong in the same CL as this, since it'd be unrelated to the thrust of the change (file type sniffing). It's ideal to keep changes as small as possible, with one logical change per CL. If the further metadata parsing is happening in the utility process, then there's not much to be gained by separating parsing of the trailer from the blkx. The blkx parsing does not actually unpack the entire archive; it's only loading the partition table and block metadata. The block data are only unpacked and decompressed when they're read.
On 2017/06/13 17:25:45, Robert Sesek wrote: > On 2017/06/13 16:56:57, mortonm wrote: > > On 2017/06/13 15:52:55, Robert Sesek wrote: > > > > > > https://codereview.chromium.org/2926473002/diff/470001/chrome/browser/safe_br... > > > File chrome/browser/safe_browsing/mac_archive_type_sniffer.cc (right): > > > > > > > > > https://codereview.chromium.org/2926473002/diff/470001/chrome/browser/safe_br... > > > chrome/browser/safe_browsing/mac_archive_type_sniffer.cc:7: #include > > > "chrome/utility/safe_browsing/mac/udif.h" > > > On 2017/06/12 21:39:49, mortonm wrote: > > > > On 2017/06/12 20:52:34, Robert Sesek wrote: > > > > > The comment about sandboxing led me to realize: this is not going to > pass > > > > > check_deps. chrome/browser cannot include chrome/utility. > > > > > > > > Assuming we need to refactor to use a full-blown utility process to do the > > > > 4-byte koly signature check. Don't see another way around this, unless > > there's > > > a > > > > way to issue IPC calls from the mojo client without actually creating a > > > utility > > > > sandbox process to service them. > > > > > > Yeah, but I agree that seems heavyweight for just doing a file sniff. > Another > > > option: don't use UDIFParser in the browser process to perform this check. > > > Simply seek to the trailer offset, read the uint32 and do the 'koly' check > -- > > > that's basically the same thing as > > > > > > https://cs.chromium.org/chromium/src/chrome/common/safe_browsing/mach_o_image... > > > > > > If it passes that, then spin up the utility process to do any additional > > > scanning. > > > > Alright, that sounds good to me. @rsesek - are you ok with us leaving the > > changes to UDIFParser that separated the trailer parsing from the data block > > parsing? We will be doing further DMG metadata checks in the future and it may > > be desirable to be able to parse just the trailer without unpacking the entire > > archive. > > I don't think it'd belong in the same CL as this, since it'd be unrelated to the > thrust of the change (file type sniffing). It's ideal to keep changes as small > as possible, with one logical change per CL. If the further metadata parsing is > happening in the utility process, then there's not much to be gained by > separating parsing of the trailer from the blkx. The blkx parsing does not > actually unpack the entire archive; it's only loading the partition table and > block metadata. The block data are only unpacked and decompressed when they're > read. Ok, I removed the changes to UDIFParser and now directly read the 'koly' signature on the file thread in the main browser process.
Looking good! Mostly nits. :-) https://codereview.chromium.org/2926473002/diff/560001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2926473002/diff/560001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:2407: if (is_mac) { nit: move this section right after line 2402. source usually goes before deps. https://codereview.chromium.org/2926473002/diff/560001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2926473002/diff/560001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:799: if (download_file_has_koly_signature) We would like to track some UMA stats here. e.g. we can add a UMA called "SBClientDownload.NoneMacArchiveExtensionHasKolySignature" with a boolean type. Similar to https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/download_pr... https://codereview.chromium.org/2926473002/diff/560001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service_unittest.cc (right): https://codereview.chromium.org/2926473002/diff/560001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service_unittest.cc:1447: Give a high level description of this test here or in code. https://codereview.chromium.org/2926473002/diff/560001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service_unittest.cc:1480: same here. https://codereview.chromium.org/2926473002/diff/560001/chrome/browser/safe_br... File chrome/browser/safe_browsing/mac_archive_type_sniffer.cc (right): https://codereview.chromium.org/2926473002/diff/560001/chrome/browser/safe_br... chrome/browser/safe_browsing/mac_archive_type_sniffer.cc:5: #define SIZE_KOLY_SIGNATURE_IN_BYTES 4 nit Can you use constant instead of defines? e.g. namespace safe_browsing { namespace { const int kSizeKolySignatureInByte = 4; const int kSizeKolyTrailer = 512; } // namespace ....
https://codereview.chromium.org/2926473002/diff/560001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2926473002/diff/560001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:2407: if (is_mac) { On 2017/06/13 20:55:38, Jialiu Lin wrote: > nit: move this section right after line 2402. source usually goes before deps. Done. https://codereview.chromium.org/2926473002/diff/560001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2926473002/diff/560001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:799: if (download_file_has_koly_signature) On 2017/06/13 20:55:38, Jialiu Lin wrote: > We would like to track some UMA stats here. e.g. we can add a UMA called > "SBClientDownload.NoneMacArchiveExtensionHasKolySignature" with a boolean type. > > Similar to > https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/download_pr... Done. https://codereview.chromium.org/2926473002/diff/560001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service_unittest.cc (right): https://codereview.chromium.org/2926473002/diff/560001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service_unittest.cc:1447: On 2017/06/13 20:55:38, Jialiu Lin wrote: > Give a high level description of this test here or in code. Done. https://codereview.chromium.org/2926473002/diff/560001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service_unittest.cc:1480: On 2017/06/13 20:55:38, Jialiu Lin wrote: > same here. Done. https://codereview.chromium.org/2926473002/diff/560001/chrome/browser/safe_br... File chrome/browser/safe_browsing/mac_archive_type_sniffer.cc (right): https://codereview.chromium.org/2926473002/diff/560001/chrome/browser/safe_br... chrome/browser/safe_browsing/mac_archive_type_sniffer.cc:5: #define SIZE_KOLY_SIGNATURE_IN_BYTES 4 On 2017/06/13 20:55:38, Jialiu Lin wrote: > nit Can you use constant instead of defines? > e.g. > namespace safe_browsing { > > namespace { > const int kSizeKolySignatureInByte = 4; > const int kSizeKolyTrailer = 512; > > } // namespace > > .... > > Done.
Last few comments! https://codereview.chromium.org/2926473002/diff/580001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2926473002/diff/580001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:2403: if (is_mac) { If you rename the file to archive_type_sniffer_mac, the build system will automatically exclude the files on non-Mac platforms. https://codereview.chromium.org/2926473002/diff/580001/chrome/browser/safe_br... File chrome/browser/safe_browsing/mac_archive_type_sniffer.cc (right): https://codereview.chromium.org/2926473002/diff/580001/chrome/browser/safe_br... chrome/browser/safe_browsing/mac_archive_type_sniffer.cc:13: const int kSizeKolyTrailer = 512; …InBytes just like the other https://codereview.chromium.org/2926473002/diff/580001/chrome/browser/safe_br... chrome/browser/safe_browsing/mac_archive_type_sniffer.cc:38: return (strncmp(data, "\x6b\x6f\x6c\x79", kSizeKolySignatureInBytes) == 0); strncmp is meant for null-terminated strings. Instead, you could do: (in the constants section): const uint32_t kKolySignature = 'koly'; And here: return memcmp(data, &kKolySignature, kSizeKolySignatureInBytes) == 0;
mortonm@google.com changed reviewers: + rkaplow@chromium.org
Adding rkaplow@ for OWNER of: tools/metrics/histograms/histograms.xml https://codereview.chromium.org/2926473002/diff/580001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2926473002/diff/580001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:2403: if (is_mac) { On 2017/06/13 22:18:56, Robert Sesek wrote: > If you rename the file to archive_type_sniffer_mac, the build system will > automatically exclude the files on non-Mac platforms. Done. https://codereview.chromium.org/2926473002/diff/580001/chrome/browser/safe_br... File chrome/browser/safe_browsing/mac_archive_type_sniffer.cc (right): https://codereview.chromium.org/2926473002/diff/580001/chrome/browser/safe_br... chrome/browser/safe_browsing/mac_archive_type_sniffer.cc:13: const int kSizeKolyTrailer = 512; On 2017/06/13 22:18:56, Robert Sesek wrote: > …InBytes just like the other Done. https://codereview.chromium.org/2926473002/diff/580001/chrome/browser/safe_br... chrome/browser/safe_browsing/mac_archive_type_sniffer.cc:38: return (strncmp(data, "\x6b\x6f\x6c\x79", kSizeKolySignatureInBytes) == 0); On 2017/06/13 22:18:56, Robert Sesek wrote: > strncmp is meant for null-terminated strings. Instead, you could do: > > (in the constants section): > const uint32_t kKolySignature = 'koly'; > > And here: return memcmp(data, &kKolySignature, kSizeKolySignatureInBytes) == 0; Done. I used a 4-element array of uint8_t for the constant instead of packing into a multi-byte integer type in order to avoid the big vs little endian problem -- since the koly trailer is formatted as big endian.
No more comments from me. LGTM :-)
lgtm https://codereview.chromium.org/2926473002/diff/640001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2926473002/diff/640001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:2405: "safe_browsing/disk_image_type_sniffer_mac.cc", Optional but recommended: Add a target for disk_image_type_sniffer in chrome/browser/safe_browsing/BUILD.gn and make that a dependency here. Ideally we should have done that for all the files in c/b/safe_browsing/ from the start and you are certainly following a precedent so I won't block you on this. https://codereview.chromium.org/2926473002/diff/640001/chrome/browser/safe_br... File chrome/browser/safe_browsing/disk_image_type_sniffer_mac_unittest.cc (right): https://codereview.chromium.org/2926473002/diff/640001/chrome/browser/safe_br... chrome/browser/safe_browsing/disk_image_type_sniffer_mac_unittest.cc:45: LOG(ERROR) << "start of test"; Remove LOG. Better yet define the << operator for ArchiveTestCase and then: const ArchiveTestCase& test_case = GetParam(); DVLOG(1) << "Test case: " << test_case; Simple example: https://cs.chromium.org/chromium/src/components/safe_browsing_db/v4_store.cc?... https://codereview.chromium.org/2926473002/diff/640001/chrome/browser/safe_br... chrome/browser/safe_browsing/disk_image_type_sniffer_mac_unittest.cc:50: bool result = DiskImageTypeSnifferMac::IsAppleDiskImage(path); ASSERT_EQ(test_case.expected_results, DiskImageTypeSnifferMac::IsAppleDiskImage(path));
LGTM with just a few nits. Thanks for bearing with us on the review :). Nice work! https://codereview.chromium.org/2926473002/diff/640001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2926473002/diff/640001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:2403: if (is_mac) { You can drop this and just include it in the sources list directly (since the filename exclusion will handle the platform condition for you). https://codereview.chromium.org/2926473002/diff/640001/chrome/browser/safe_br... File chrome/browser/safe_browsing/disk_image_type_sniffer_mac.cc (right): https://codereview.chromium.org/2926473002/diff/640001/chrome/browser/safe_br... chrome/browser/safe_browsing/disk_image_type_sniffer_mac.cc:12: const int kSizeKolySignatureInBytes = 4; This could now be declared as: constexpr size_t kSizeKolySignatureInBytes = sizeof(kKolySignature); https://codereview.chromium.org/2926473002/diff/640001/chrome/browser/safe_br... chrome/browser/safe_browsing/disk_image_type_sniffer_mac.cc:38: // Compare 4 bytes at given position in file to 'koly' in ascii. I'd remove this comment now that the signature is written in ASCII above, rather than as the HEX byte sequence.
https://codereview.chromium.org/2926473002/diff/640001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2926473002/diff/640001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:2403: if (is_mac) { On 2017/06/14 15:11:43, Robert Sesek wrote: > You can drop this and just include it in the sources list directly (since the > filename exclusion will handle the platform condition for you). Done. https://codereview.chromium.org/2926473002/diff/640001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:2405: "safe_browsing/disk_image_type_sniffer_mac.cc", On 2017/06/14 00:53:07, vakh (Varun Khaneja) wrote: > Optional but recommended: Add a target for disk_image_type_sniffer in > chrome/browser/safe_browsing/BUILD.gn and make that a dependency here. > > Ideally we should have done that for all the files in c/b/safe_browsing/ from > the start and you are certainly following a precedent so I won't block you on > this. Ok, sounds good. I'm not too familiar with the semantics of build files and what to add in chrome/browser/safe_browsing/BUILD.gn, but I will try to make the change before the code is finally submitted. https://codereview.chromium.org/2926473002/diff/640001/chrome/browser/safe_br... File chrome/browser/safe_browsing/disk_image_type_sniffer_mac.cc (right): https://codereview.chromium.org/2926473002/diff/640001/chrome/browser/safe_br... chrome/browser/safe_browsing/disk_image_type_sniffer_mac.cc:12: const int kSizeKolySignatureInBytes = 4; On 2017/06/14 15:11:43, Robert Sesek wrote: > This could now be declared as: > > constexpr size_t kSizeKolySignatureInBytes = sizeof(kKolySignature); Done. https://codereview.chromium.org/2926473002/diff/640001/chrome/browser/safe_br... chrome/browser/safe_browsing/disk_image_type_sniffer_mac.cc:38: // Compare 4 bytes at given position in file to 'koly' in ascii. On 2017/06/14 15:11:43, Robert Sesek wrote: > I'd remove this comment now that the signature is written in ASCII above, rather > than as the HEX byte sequence. Done. https://codereview.chromium.org/2926473002/diff/640001/chrome/browser/safe_br... File chrome/browser/safe_browsing/disk_image_type_sniffer_mac_unittest.cc (right): https://codereview.chromium.org/2926473002/diff/640001/chrome/browser/safe_br... chrome/browser/safe_browsing/disk_image_type_sniffer_mac_unittest.cc:45: LOG(ERROR) << "start of test"; On 2017/06/14 00:53:07, vakh (Varun Khaneja) wrote: > Remove LOG. > > Better yet define the << operator for ArchiveTestCase and then: > const ArchiveTestCase& test_case = GetParam(); > DVLOG(1) << "Test case: " << test_case; > > Simple example: > https://cs.chromium.org/chromium/src/components/safe_browsing_db/v4_store.cc?... Done. https://codereview.chromium.org/2926473002/diff/640001/chrome/browser/safe_br... chrome/browser/safe_browsing/disk_image_type_sniffer_mac_unittest.cc:50: bool result = DiskImageTypeSnifferMac::IsAppleDiskImage(path); On 2017/06/14 00:53:07, vakh (Varun Khaneja) wrote: > ASSERT_EQ(test_case.expected_results, > DiskImageTypeSnifferMac::IsAppleDiskImage(path)); 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 rsesek@chromium.org, vakh@chromium.org, jialiul@chromium.org Link to the patchset: https://codereview.chromium.org/2926473002/#ps660001 (title: "addressing comments")
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: 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 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: 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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_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: This issue passed the CQ dry run.
The CQ bit was checked by mortonm@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, vakh@chromium.org, rkaplow@chromium.org, jialiul@chromium.org Link to the patchset: https://codereview.chromium.org/2926473002/#ps720001 (title: "adding ifdef for Mac in tester")
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": 720001, "attempt_start_ts": 1497914666840070,
"parent_rev": "ddb4c9f9e13fe5b6ccd785a71fe08b11c65b8770", "commit_rev":
"6602ea616cce79869b300ffa82c3fc64fe422200"}
Message was sent while issue was closed.
Description was changed from ========== Rather than simply relying on file extension (e.g. .DMG) to determine whether to extract DMG features from download, instead look at file metadata to check for existence of 'koly' signature which identifies Mac archive types. BUG=600392 ========== to ========== Rather than simply relying on file extension (e.g. .DMG) to determine whether to extract DMG features from download, instead look at file metadata to check for existence of 'koly' signature which identifies Mac archive types. BUG=600392 Review-Url: https://codereview.chromium.org/2926473002 Cr-Commit-Position: refs/heads/master@{#480631} Committed: https://chromium.googlesource.com/chromium/src/+/6602ea616cce79869b300ffa82c3... ==========
Message was sent while issue was closed.
Committed patchset #38 (id:720001) as https://chromium.googlesource.com/chromium/src/+/6602ea616cce79869b300ffa82c3...
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) identified this CL at revision 480631 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
On 2017/06/20 at 01:21:10, findit-for-me wrote: > Findit (https://goo.gl/kROfz5) identified this CL at revision 480631 as the culprit for > failures in the build cycles as shown on: > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... This looks like it caused test failures. I've filed https://bugs.chromium.org/p/chromium/issues/detail?id=734858.
Message was sent while issue was closed.
Description was changed from ========== Rather than simply relying on file extension (e.g. .DMG) to determine whether to extract DMG features from download, instead look at file metadata to check for existence of 'koly' signature which identifies Mac archive types. BUG=600392 Review-Url: https://codereview.chromium.org/2926473002 Cr-Commit-Position: refs/heads/master@{#480631} Committed: https://chromium.googlesource.com/chromium/src/+/6602ea616cce79869b300ffa82c3... ========== to ========== Rather than simply relying on file extension (e.g. .DMG) to determine whether to extract DMG features from download, instead look at file metadata to check for existence of 'koly' signature which identifies Mac archive types. BUG=600392 Review-Url: https://codereview.chromium.org/2926473002 Cr-Commit-Position: refs/heads/master@{#480631} Committed: https://chromium.googlesource.com/chromium/src/+/6602ea616cce79869b300ffa82c3... ==========
Description was changed from ========== Rather than simply relying on file extension (e.g. .DMG) to determine whether to extract DMG features from download, instead look at file metadata to check for existence of 'koly' signature which identifies Mac archive types. BUG=600392 Review-Url: https://codereview.chromium.org/2926473002 Cr-Commit-Position: refs/heads/master@{#480631} Committed: https://chromium.googlesource.com/chromium/src/+/6602ea616cce79869b300ffa82c3... ========== to ========== Rather than simply relying on file extension (e.g. .DMG) to determine whether to extract DMG features from download, instead look at file metadata to check for existence of 'koly' signature which identifies Mac archive types. BUG=600392 >Review-Url: https://codereview.chromium.org/2926473002 >Cr-Commit-Position: refs/heads/master@{#480631} >Committed: >https://chromium.googlesource.com/chromium/src/+/6602ea616cce79869b300ffa82c3fc64fe422200 ==========
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.
Patchset #42 (id:800001) 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...
The CQ bit was checked by jialiul@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 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.
The CQ bit was checked by mortonm@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, vakh@chromium.org, rkaplow@chromium.org, jialiul@chromium.org Link to the patchset: https://codereview.chromium.org/2926473002/#ps840001 (title: "avoiding multiple asynchronous calls in tester for mac")
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": 1498520195728050,
"parent_rev": "0f7a19296f354d7ed937a0d161c08638f542d919", "commit_rev":
"177cde5a7f2d95de6601f01e9bee8fdedf13a96b"}
Message was sent while issue was closed.
Description was changed from ========== Rather than simply relying on file extension (e.g. .DMG) to determine whether to extract DMG features from download, instead look at file metadata to check for existence of 'koly' signature which identifies Mac archive types. BUG=600392 >Review-Url: https://codereview.chromium.org/2926473002 >Cr-Commit-Position: refs/heads/master@{#480631} >Committed: >https://chromium.googlesource.com/chromium/src/+/6602ea616cce79869b300ffa82c3fc64fe422200 ========== to ========== Rather than simply relying on file extension (e.g. .DMG) to determine whether to extract DMG features from download, instead look at file metadata to check for existence of 'koly' signature which identifies Mac archive types. BUG=600392 >Review-Url: https://codereview.chromium.org/2926473002 >Cr-Commit-Position: refs/heads/master@{#480631} >Committed: >https://chromium.googlesource.com/chromium/src/+/6602ea616cce79869b300ffa82c3fc64fe422200 Review-Url: https://codereview.chromium.org/2926473002 Cr-Commit-Position: refs/heads/master@{#482463} Committed: https://chromium.googlesource.com/chromium/src/+/177cde5a7f2d95de6601f01e9bee... ==========
Message was sent while issue was closed.
Committed patchset #43 (id:840001) as https://chromium.googlesource.com/chromium/src/+/177cde5a7f2d95de6601f01e9bee... |
