Validate safe_browsing::dmg::UDIFBlock data before attempting to read at its offsets.

Validate safe_browsing::dmg::UDIFBlock data before attempting to read at its offsets.

This change also validates that the blkx plist does not run past the end of the
file.

BUG=627355
TEST=Clusterfuzz coverage.
R=mark@chromium.org

Committed: https://crrev.com/41b7abbdb0a34bc77373673dd1400dfdf8c4d84c
Cr-Commit-Position: refs/heads/master@{#404862}

[Robert Sesek, 2016-07-12 14:44:36]

Patchset #1 (id:1) has been deleted

[Mark Mentovai, 2016-07-12 15:22:23]

LGTM

https://codereview.chromium.org/2141963002/diff/20001/chrome/utility/safe_bro...
File chrome/utility/safe_browsing/mac/udif.cc (right):

https://codereview.chromium.org/2141963002/diff/20001/chrome/utility/safe_bro...
chrome/utility/safe_browsing/mac/udif.cc:218: chunk->compressed_length >
block_size.ValueOrDie()) {
Should this be checking that the chunk_end_offset > block_size?

https://codereview.chromium.org/2141963002/diff/20001/chrome/utility/safe_bro...
chrome/utility/safe_browsing/mac/udif.cc:240: const uint16_t sector_size_;
Better if this is last, but I don’t think it needs to be a member at all. Pass
it into ParseBlockData(), the only place it’s used, instead of the constructor.

[Robert Sesek, 2016-07-12 16:16:35]

https://codereview.chromium.org/2141963002/diff/20001/chrome/utility/safe_bro...
File chrome/utility/safe_browsing/mac/udif.cc (right):

https://codereview.chromium.org/2141963002/diff/20001/chrome/utility/safe_bro...
chrome/utility/safe_browsing/mac/udif.cc:218: chunk->compressed_length >
block_size.ValueOrDie()) {
On 2016/07/12 15:22:23, Mark Mentovai wrote:
> Should this be checking that the chunk_end_offset > block_size?

No, because of the way these values are nested. It should be
>block_size+(start_sector*sector_size). But per offline investigation, the UDRO
image tests fail if we perform this validation. These offsets appear to be
absolute in the file. We don't need to validate that it's within the file bounds
because the seek or read will fail if we attempt to access that.

https://codereview.chromium.org/2141963002/diff/20001/chrome/utility/safe_bro...
chrome/utility/safe_browsing/mac/udif.cc:240: const uint16_t sector_size_;
On 2016/07/12 15:22:23, Mark Mentovai wrote:
> Better if this is last, but I don’t think it needs to be a member at all. Pass
> it into ParseBlockData(), the only place it’s used, instead of the
constructor.

Done.

[Robert Sesek, 2016-07-12 16:17:16]

rsesek@chromium.org changed reviewers:
+ nparker@chromium.org

[Robert Sesek, 2016-07-12 16:17:17]

+nparker for OWNERS

[Mark Mentovai, 2016-07-12 16:17:57]

LGTM

[Nathan Parker, 2016-07-12 21:13:52]

lgtm

[Robert Sesek, 2016-07-12 21:14:15]

The CQ bit was checked by rsesek@chromium.org

[commit-bot: I haz the power, 2016-07-12 21:16:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 00:10:24]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-07-13 00:11:06]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 00:13:04]

Description was changed from

==========
Validate safe_browsing::dmg::UDIFBlock data before attempting to read at its
offsets.

This change also validates that the blkx plist does not run past the end of the
file.

BUG=627355
TEST=Clusterfuzz coverage.
R=mark@chromium.org
==========

to

==========
Validate safe_browsing::dmg::UDIFBlock data before attempting to read at its
offsets.

This change also validates that the blkx plist does not run past the end of the
file.

BUG=627355
TEST=Clusterfuzz coverage.
R=mark@chromium.org

Committed: https://crrev.com/41b7abbdb0a34bc77373673dd1400dfdf8c4d84c
Cr-Commit-Position: refs/heads/master@{#404862}
==========

[commit-bot: I haz the power, 2016-07-13 00:13:05]

Patchset 2 (id:??) landed as
https://crrev.com/41b7abbdb0a34bc77373673dd1400dfdf8c4d84c
Cr-Commit-Position: refs/heads/master@{#404862}