New issue
Advanced search Search tips
Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Feature



Sign in to add a comment
link

Issue 909778: Inspect the file contents within RAR files

Reported by drubery@chromium.org, Nov 28 Project Member

Issue description

Currently we only ping SB with the file names within the RAR. We should extract the files so that we can send lengths and digests as well.
 

Comment 1 by vakh@chromium.org, Nov 28

Labels: -Type-Bug Type-Feature

Comment 2 by bugdroid1@chromium.org, Nov 30

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aa21b3f577d2d912e7d3898af5ee42e01a00cacd

commit aa21b3f577d2d912e7d3898af5ee42e01a00cacd
Author: Daniel Rubery <drubery@chromium.org>
Date: Fri Nov 30 21:44:20 2018

Add util method to update ArchiveAnalyzerResults for a single file

This CL breaks out the code that inspects an individual file within a
ZIP archive. This will be shared between the ZIP and RAR inspection,
when RAR files begin doing content inspection.

Bug: 909778
Change-Id: I7acf1cabd472f112f2ed7c31735688cae7a6d122
Reviewed-on: https://chromium-review.googlesource.com/c/1354103
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Reviewed-by: Brian White <bcwhite@chromium.org>
Reviewed-by: Jay Civelli <jcivelli@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612799}
[modify] https://crrev.com/aa21b3f577d2d912e7d3898af5ee42e01a00cacd/chrome/common/safe_browsing/BUILD.gn
[modify] https://crrev.com/aa21b3f577d2d912e7d3898af5ee42e01a00cacd/chrome/common/safe_browsing/archive_analyzer_results.cc
[modify] https://crrev.com/aa21b3f577d2d912e7d3898af5ee42e01a00cacd/chrome/common/safe_browsing/archive_analyzer_results.h
[modify] https://crrev.com/aa21b3f577d2d912e7d3898af5ee42e01a00cacd/chrome/common/safe_browsing/zip_analyzer.cc
[modify] https://crrev.com/aa21b3f577d2d912e7d3898af5ee42e01a00cacd/chrome/services/file_util/public/cpp/sandboxed_zip_analyzer_unittest.cc
[modify] https://crrev.com/aa21b3f577d2d912e7d3898af5ee42e01a00cacd/chrome/utility/safe_browsing/mac/BUILD.gn
[modify] https://crrev.com/aa21b3f577d2d912e7d3898af5ee42e01a00cacd/tools/metrics/histograms/histograms.xml

Comment 3 by bugdroid1@chromium.org, Dec 3

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/98e53045947734df30e2634b5e38250350de186c

commit 98e53045947734df30e2634b5e38250350de186c
Author: Guido Urdaneta <guidou@chromium.org>
Date: Mon Dec 03 13:45:14 2018

Revert "Add util method to update ArchiveAnalyzerResults for a single file"

This reverts commit aa21b3f577d2d912e7d3898af5ee42e01a00cacd.

Reason for revert: Suspect of introducing consistent failure on Mac ASAN 64 tests.

First failed run: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac%20ASan%2064%20Tests%20%281%29/46988

Sample logs:
unit_tests Run on OS: 'Mac-10.13'
Shard duration: 0:07:56.378119
failures:
FileAnalyzerTest.ArchivedArchiveSetForZipNoArchive
FileAnalyzerTest.ArchivedExecutableFalseForZipNoExecutable
DownloadProtectionServiceTest.CheckClientDownloadZip
SandboxedZipAnalyzerTest.ZippedAppWithUnsignedAndSignedExecutable
SandboxedZipAnalyzerTest.NoBinaries
FileAnalyzerTest.ArchivedBinariesSkipsSafeFiles

==34531==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x700015467c14 at pc 0x00013e99c44d bp 0x700015467950 sp 0x7000154670f8
READ of size 5 at 0x700015467c14 thread T16
    #0 0x13e99c44c in __asan_after_dynamic_init ??:0:0
    #1 0x10e3ac931 in safe_browsing::UpdateArchiveAnalyzerResultsWithFile(base::FilePath, base::File*, bool, safe_browsing::ArchiveAnalyzerResults*) ??:0:0
    #2 0x10e3a6707 in safe_browsing::zip_analyzer::AnalyzeZipFile(base::File, base::File, safe_browsing::ArchiveAnalyzerResults*) ??:0:0
    #3 0x115236586 in SafeArchiveAnalyzer::AnalyzeZipFile(base::File, base::File, base::OnceCallback<void (safe_browsing::ArchiveAnalyzerResults const&)>) ??:0:0
    #4 0x10bc0b332 in chrome::mojom::SafeArchiveAnalyzerStubDispatch::AcceptWithResponder(chrome::mojom::SafeArchiveAnalyzer*, mojo::Message*, std::__1::unique_ptr<mojo::MessageReceiverWithStatus, std::__1::default_delete<mojo::MessageReceiverWithStatus> >) ??:0:0
    #5 0x115234a40 in chrome::mojom::SafeArchiveAnalyzerStub<mojo::RawPtrImplRefTraits<chrome::mojom::SafeArchiveAnalyzer> >::AcceptWithResponder(mojo::Message*, std::__1::unique_ptr<mojo::MessageReceiverWithStatus, std::__1::default_delete<mojo::MessageReceiverWithStatus> >) ??:0:0
    #6 0x1165eeb32 in mojo::InterfaceEndpointClient::HandleValidatedMessage(mojo::Message*) ??:0:0
    #7 0x1165ed1e0 in mojo::FilterChain::Accept(mojo::Message*) ??:0:0
    #8 0x1165f278b in mojo::InterfaceEndpointClient::HandleIncomingMessage(mojo::Message*) ??:0:0
    #9 0x116605686 in mojo::internal::MultiplexRouter::ProcessIncomingMessage(mojo::internal::MultiplexRouter::MessageWrapper*, mojo::internal::MultiplexRouter::ClientCallBehavior, base::SequencedTaskRunner*) ??:0:0
    #10 0x11660372b in mojo::internal::MultiplexRouter::Accept(mojo::Message*) ??:0:0
    #11 0x1165ed1e0 in mojo::FilterChain::Accept(mojo::Message*) ??:0:0
    #12 0x1165dcea8 in mojo::Connector::ReadSingleMessage(unsigned int*) ??:0:0
    #13 0x1165df32a in mojo::Connector::ReadAllAvailableMessages() ??:0:0
    #14 0x1165deda1 in mojo::Connector::OnHandleReadyInternal(unsigned int) ??:0:0
    #15 0x10d429d44 in mojo::SimpleWatcher::DiscardReadyState(base::RepeatingCallback<void (unsigned int)> const&, unsigned int, mojo::HandleSignalsState const&) ??:0:0
    #16 0x11a802cf5 in mojo::SimpleWatcher::OnHandleReady(int, unsigned int, mojo::HandleSignalsState const&) ??:0:0
    #17 0x11a803cbd in void base::internal::Invoker<base::internal::BindState<void (mojo::SimpleWatcher::*)(int, unsigned int, mojo::HandleSignalsState const&), base::WeakPtr<mojo::SimpleWatcher>, int, unsigned int, mojo::HandleSignalsState>, void ()>::RunImpl<void (mojo::SimpleWatcher::* const&)(int, unsigned int, mojo::HandleSignalsState const&), std::__1::tuple<base::WeakPtr<mojo::SimpleWatcher>, int, unsigned int, mojo::HandleSignalsState> const&, 0ul, 1ul, 2ul, 3ul>(void (mojo::SimpleWatcher::* const&)(int, unsigned int, mojo::HandleSignalsState const&), std::__1::tuple<base::WeakPtr<mojo::SimpleWatcher>, int, unsigned int, mojo::HandleSignalsState> const&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul>) ??:0:0
    #18 0x11890131c in base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*) ??:0:0
    #19 0x118995271 in base::MessageLoopImpl::RunTask(base::PendingTask*) ??:0:0
    #20 0x11899691f in base::MessageLoopImpl::DoWork() ??:0:0
    #21 0x118999eb3 in base::MessagePumpDefault::Run(base::MessagePump::Delegate*) ??:0:0
    #22 0x118994259 in base::MessageLoopImpl::Run(bool) ??:0:0
    #23 0x118a49a1c in base::RunLoop::Run() ??:0:0
    #24 0x118b962e2 in base::Thread::Run(base::RunLoop*) ??:0:0
    #25 0x118b96e9d in base::Thread::ThreadMain() ??:0:0
    #26 0x118c72a0d in base::(anonymous namespace)::ThreadFunc(void*) ??:0:0
    #27 0x7fff74564660 in _pthread_body ??:0:0
    #28 0x7fff7456450c in _pthread_start ??:0:0


Original change's description:
> Add util method to update ArchiveAnalyzerResults for a single file
> 
> This CL breaks out the code that inspects an individual file within a
> ZIP archive. This will be shared between the ZIP and RAR inspection,
> when RAR files begin doing content inspection.
> 
> Bug: 909778
> Change-Id: I7acf1cabd472f112f2ed7c31735688cae7a6d122
> Reviewed-on: https://chromium-review.googlesource.com/c/1354103
> Commit-Queue: Daniel Rubery <drubery@chromium.org>
> Reviewed-by: Brian White <bcwhite@chromium.org>
> Reviewed-by: Jay Civelli <jcivelli@chromium.org>
> Reviewed-by: Varun Khaneja <vakh@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#612799}

TBR=jcivelli@chromium.org,bcwhite@chromium.org,vakh@chromium.org,drubery@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 909778
Change-Id: I92e193707da4a28cee94aa1277a6ca567650760f
Reviewed-on: https://chromium-review.googlesource.com/c/1358491
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613073}
[modify] https://crrev.com/98e53045947734df30e2634b5e38250350de186c/chrome/common/safe_browsing/BUILD.gn
[modify] https://crrev.com/98e53045947734df30e2634b5e38250350de186c/chrome/common/safe_browsing/archive_analyzer_results.cc
[modify] https://crrev.com/98e53045947734df30e2634b5e38250350de186c/chrome/common/safe_browsing/archive_analyzer_results.h
[modify] https://crrev.com/98e53045947734df30e2634b5e38250350de186c/chrome/common/safe_browsing/zip_analyzer.cc
[modify] https://crrev.com/98e53045947734df30e2634b5e38250350de186c/chrome/services/file_util/public/cpp/sandboxed_zip_analyzer_unittest.cc
[modify] https://crrev.com/98e53045947734df30e2634b5e38250350de186c/chrome/utility/safe_browsing/mac/BUILD.gn
[modify] https://crrev.com/98e53045947734df30e2634b5e38250350de186c/tools/metrics/histograms/histograms.xml

Comment 4 by bugdroid1@chromium.org, Dec 7

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2c065bb11d7a6e0227c0d32e043e458e1ef76091

commit 2c065bb11d7a6e0227c0d32e043e458e1ef76091
Author: Daniel Rubery <drubery@chromium.org>
Date: Thu Dec 06 20:57:26 2018

Reland: Add util method to update ArchiveAnalyzerResults for a single file

This CL breaks out the code that inspects an individual file within a
ZIP archive. This will be shared between the ZIP and RAR inspection,
when RAR files begin doing content inspection.

This is a reland of
https://chromium-review.googlesource.com/c/chromium/src/+/1354103
The OOB read has been fixed between patchsets 1 and 2.

Bug: 909778
Change-Id: I707364516217136ee95174bbae930b5df0cf54ed
TBR: jcivelli@
Reviewed-on: https://chromium-review.googlesource.com/c/1359037
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Reviewed-by: Brian White <bcwhite@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614486}
[modify] https://crrev.com/2c065bb11d7a6e0227c0d32e043e458e1ef76091/chrome/common/safe_browsing/BUILD.gn
[modify] https://crrev.com/2c065bb11d7a6e0227c0d32e043e458e1ef76091/chrome/common/safe_browsing/archive_analyzer_results.cc
[modify] https://crrev.com/2c065bb11d7a6e0227c0d32e043e458e1ef76091/chrome/common/safe_browsing/archive_analyzer_results.h
[modify] https://crrev.com/2c065bb11d7a6e0227c0d32e043e458e1ef76091/chrome/common/safe_browsing/zip_analyzer.cc
[modify] https://crrev.com/2c065bb11d7a6e0227c0d32e043e458e1ef76091/chrome/services/file_util/public/cpp/sandboxed_zip_analyzer_unittest.cc
[modify] https://crrev.com/2c065bb11d7a6e0227c0d32e043e458e1ef76091/chrome/utility/safe_browsing/mac/BUILD.gn
[modify] https://crrev.com/2c065bb11d7a6e0227c0d32e043e458e1ef76091/tools/metrics/histograms/histograms.xml

Comment 5 by bugdroid1@chromium.org, Dec 10

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ec3682e3c9061c10f26ea9e5cdcf3c53f3f74387

commit ec3682e3c9061c10f26ea9e5cdcf3c53f3f74387
Author: Daniel Rubery <drubery@chromium.org>
Date: Mon Dec 10 22:47:32 2018

Extract the contents of RAR files and report hashes and signatures

Bug: 909778
Change-Id: Id1f5cb828b8ad5cc13dcff571e026dd69b3fbc76
Reviewed-on: https://chromium-review.googlesource.com/c/1327238
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615291}
[modify] https://crrev.com/ec3682e3c9061c10f26ea9e5cdcf3c53f3f74387/chrome/common/safe_browsing/BUILD.gn
[modify] https://crrev.com/ec3682e3c9061c10f26ea9e5cdcf3c53f3f74387/chrome/common/safe_browsing/rar_analyzer.cc
[modify] https://crrev.com/ec3682e3c9061c10f26ea9e5cdcf3c53f3f74387/chrome/common/safe_browsing/rar_analyzer.h
[modify] https://crrev.com/ec3682e3c9061c10f26ea9e5cdcf3c53f3f74387/chrome/services/file_util/DEPS
[modify] https://crrev.com/ec3682e3c9061c10f26ea9e5cdcf3c53f3f74387/chrome/services/file_util/public/cpp/sandboxed_rar_analyzer.cc
[modify] https://crrev.com/ec3682e3c9061c10f26ea9e5cdcf3c53f3f74387/chrome/services/file_util/public/cpp/sandboxed_rar_analyzer.h
[modify] https://crrev.com/ec3682e3c9061c10f26ea9e5cdcf3c53f3f74387/chrome/services/file_util/public/cpp/sandboxed_rar_analyzer_unittest.cc
[modify] https://crrev.com/ec3682e3c9061c10f26ea9e5cdcf3c53f3f74387/chrome/services/file_util/public/mojom/safe_archive_analyzer.mojom
[modify] https://crrev.com/ec3682e3c9061c10f26ea9e5cdcf3c53f3f74387/chrome/services/file_util/safe_archive_analyzer.cc
[modify] https://crrev.com/ec3682e3c9061c10f26ea9e5cdcf3c53f3f74387/chrome/services/file_util/safe_archive_analyzer.h
[modify] https://crrev.com/ec3682e3c9061c10f26ea9e5cdcf3c53f3f74387/components/safe_browsing/features.cc
[modify] https://crrev.com/ec3682e3c9061c10f26ea9e5cdcf3c53f3f74387/components/safe_browsing/features.h
[modify] https://crrev.com/ec3682e3c9061c10f26ea9e5cdcf3c53f3f74387/third_party/unrar/README.chromium
[modify] https://crrev.com/ec3682e3c9061c10f26ea9e5cdcf3c53f3f74387/third_party/unrar/patches/5.6.8.patch
[modify] https://crrev.com/ec3682e3c9061c10f26ea9e5cdcf3c53f3f74387/third_party/unrar/src/archive.cpp
[modify] https://crrev.com/ec3682e3c9061c10f26ea9e5cdcf3c53f3f74387/third_party/unrar/src/archive.hpp
[modify] https://crrev.com/ec3682e3c9061c10f26ea9e5cdcf3c53f3f74387/third_party/unrar/src/extract.cpp
[modify] https://crrev.com/ec3682e3c9061c10f26ea9e5cdcf3c53f3f74387/third_party/unrar/src/file.cpp
[modify] https://crrev.com/ec3682e3c9061c10f26ea9e5cdcf3c53f3f74387/third_party/unrar/src/unrar_wrapper.h

Comment 6 by bugdroid1@chromium.org, Jan 4

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2c3b194535c675bb42b060deae3cf79e06f2725a

commit 2c3b194535c675bb42b060deae3cf79e06f2725a
Author: Daniel Rubery <drubery@chromium.org>
Date: Fri Jan 04 19:16:52 2019

Define NOVOLUME for unrar code

To prevent attempting to extract other volumes within the sandbox, this
CL puts unrar in NOVOLUME mode. I also added a quick test to ensure we
still detect the presence of files in NOVOLUME mode, so this should be
safe.

Note: The generation of multipart rar files seems a little finicky. The
following two commands worked for me.

dd if=/dev/urandom of=random.exe bs=1k count=1
rar a -v1b multipart.rar random.exe

Bug: 917679, 909778
Change-Id: I472b0e959d7538dc9136597ada631707b044e765
Reviewed-on: https://chromium-review.googlesource.com/c/1393803
Reviewed-by: Jay Civelli <jcivelli@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620026}
[modify] https://crrev.com/2c3b194535c675bb42b060deae3cf79e06f2725a/chrome/services/file_util/public/cpp/sandboxed_rar_analyzer_unittest.cc
[add] https://crrev.com/2c3b194535c675bb42b060deae3cf79e06f2725a/chrome/test/data/safe_browsing/rar/multipart.part0001.rar
[add] https://crrev.com/2c3b194535c675bb42b060deae3cf79e06f2725a/chrome/test/data/safe_browsing/rar/multipart.part0002.rar
[modify] https://crrev.com/2c3b194535c675bb42b060deae3cf79e06f2725a/third_party/unrar/BUILD.gn

Comment 7 by bugdroid1@chromium.org, Jan 17

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/541e742e2d500e85328013b1d3524e71596277da

commit 541e742e2d500e85328013b1d3524e71596277da
Author: Daniel Rubery <drubery@chromium.org>
Date: Thu Jan 17 19:16:28 2019

Add maximum inspection size for RAR archives

Finch showed a significant difference in inspection time when enabling
content inspection for RAR archives (http://shortn/_64qaXWf78L). I
expect this is because RARs archives tend to be very large, so
unpacking the archive takes a lot of time. This CL has the RAR
analyzer fall back to the old behavior for large (> 50MB) archives.

Bug: 909778
Change-Id: I5a42c4921d7858cdd8e68d629b6326c450b41d75
Reviewed-on: https://chromium-review.googlesource.com/c/1415253
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623787}
[modify] https://crrev.com/541e742e2d500e85328013b1d3524e71596277da/chrome/browser/resources/safe_browsing/download_file_types.asciipb
[modify] https://crrev.com/541e742e2d500e85328013b1d3524e71596277da/chrome/browser/safe_browsing/download_protection/file_analyzer_unittest.cc
[modify] https://crrev.com/541e742e2d500e85328013b1d3524e71596277da/chrome/common/safe_browsing/rar_analyzer.cc

Comment 8 by drubery@chromium.org, Jan 31

Metrics now all look good.

- SBClientDownload.RarFileArchivedBinariesCount appears to have no significant change (which is good, we aren't losing any binaries)
- SBClientDownload.RarFileHasExecutable has risen slightly. In light of the previous metric, I'm inclined to call that noise.
- SBClientDownload.RarFileSuccess has dropped slightly. Our failure rate has gone from 0.22% to 0.33%, but that's still a very low failure rate.
- SBClientDownload.ExtractRarFeaturesTime has risen across the board (as expected), with the 50th percentile going from 255 to 359 ms.

It looks like the old and new extraction methods find roughly the same binaries, with the new one being a little slower. Since we aren't egregiously slower, and the new method computes hashes of binaries, I'll roll out to Beta.

Sign in to add a comment