Give PeImageReader the power to read certificate entries.

Give PeImageReader the power to read certificate entries.

BUG=462584
Committed: https://crrev.com/7177b69c48efb3da8ffcc0194dd0a463e58f4b32
Cr-Commit-Position: refs/heads/master@{#323667}

[grt (UTC plus 2), 2015-04-01 20:02:30]

grt@chromium.org changed reviewers:
+ mattm@chromium.org

[grt (UTC plus 2), 2015-04-01 20:02:31]

Hi Matt,

This new code is based on section 4.7 "The Attribute Certificate Table (Image
Only)" of the PE and COFF spec published here:
http://msdn.microsoft.com/library/windows/hardware/gg463119.aspx. Let me know if
you would prefer to only do an OWNERS stamp following another review. Thanks.

[mattm, 2015-04-01 22:49:52]

https://codereview.chromium.org/1047693002/diff/1/chrome/common/safe_browsing...
File chrome/common/safe_browsing/pe_image_reader_win.cc (right):

https://codereview.chromium.org/1047693002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/pe_image_reader_win.cc:184: data_size -=
(win_certificate->dwLength + 7) & ~0x7;
data needs to be incremented also?

https://codereview.chromium.org/1047693002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/pe_image_reader_win.cc:342: // The entry for the
certificate table is special in that its addres is a file
address

[grt (UTC plus 2), 2015-04-02 02:59:37]

PTAL. I'll try to add a test binary with multiple signers for this case
tomorrow.

https://codereview.chromium.org/1047693002/diff/1/chrome/common/safe_browsing...
File chrome/common/safe_browsing/pe_image_reader_win.cc (right):

https://codereview.chromium.org/1047693002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/pe_image_reader_win.cc:184: data_size -=
(win_certificate->dwLength + 7) & ~0x7;
On 2015/04/01 22:49:52, mattm wrote:
> data needs to be incremented also?

Yes. Nice catch.

https://codereview.chromium.org/1047693002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/pe_image_reader_win.cc:342: // The entry for the
certificate table is special in that its addres is a file
On 2015/04/01 22:49:52, mattm wrote:
> address

Done.

[grt (UTC plus 2), 2015-04-02 17:52:51]

On 2015/04/02 02:59:37, grt wrote:
> PTAL. I'll try to add a test binary with multiple signers for this case
> tomorrow.

New test added. I'll land the new test binary separately in
https://codereview.chromium.org/1058013002 if this looks good to you. Thanks.

[mattm, 2015-04-02 21:39:47]

lgtm

[grt (UTC plus 2), 2015-04-03 02:36:45]

The CQ bit was checked by grt@chromium.org

[grt (UTC plus 2), 2015-04-03 02:36:46]

The patchset sent to the CQ was uploaded after l-g-t-m from mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/1047693002/#ps80001
(title: "sync to position 323578")

[grt (UTC plus 2), 2015-04-03 02:37:40]

The CQ bit was unchecked by grt@chromium.org

[grt (UTC plus 2), 2015-04-03 02:40:26]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2015-04-03 04:43:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1047693002/80001

[commit-bot: I haz the power, 2015-04-03 07:49:35]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-04-03 20:34:23]

Patchset 5 (id:??) landed as
https://crrev.com/7177b69c48efb3da8ffcc0194dd0a463e58f4b32
Cr-Commit-Position: refs/heads/master@{#323667}