New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 729809 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

FullHashToHashPrefix does not calculate the hash prefix correctly

Project Member Reported by vakh@chromium.org, Jun 5 2017

Issue description

https://cs.chromium.org/chromium/src/components/safe_browsing_db/v4_protocol_manager_util.cc?l=302&rcl=f7b7f0ee69149ccc0bf5a903d6086b114626dda8

Is:
*hash_prefix = full_hash.substr(prefix_size);

Should be:
*hash_prefix = full_hash.substr(0, prefix_size);

This code path is only used for permissions stuff so the rest of PVer4 launch is unaffected.
 
Is it possible to merge this fix to M60 (it's pretty tiny). That would let us have permissions blacklisting available earlier.

Comment 2 by vakh@chromium.org, Jun 6 2017

Labels: Merge-Request-60
Requesting merge to M60 since it is super tiny and risk-free.
Here's the CL under review: https://chromium-review.googlesource.com/c/524892/
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1e66b2698e1e71e9266c761f3fdac8b458c720f4

commit 1e66b2698e1e71e9266c761f3fdac8b458c720f4
Author: Varun Khaneja <vakh@chromium.org>
Date: Tue Jun 06 15:24:56 2017

Use the substr API correctly in FullHashToHashPrefix.

This code path is only used for permissions blacklists so the rest of
PVer4 launch can progress as-is.

Bug:  729809 
Change-Id: Iec6af24fadae59c6e16c109c0990bc6159575e60
Reviewed-on: https://chromium-review.googlesource.com/524892
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Reviewed-by: Nathan Parker <nparker@chromium.org>
Commit-Queue: Varun Khaneja <vakh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#477292}
[modify] https://crrev.com/1e66b2698e1e71e9266c761f3fdac8b458c720f4/components/safe_browsing_db/v4_protocol_manager_util.cc
[modify] https://crrev.com/1e66b2698e1e71e9266c761f3fdac8b458c720f4/components/safe_browsing_db/v4_protocol_manager_util_unittest.cc

Labels: -Merge-Request-60
Did sheriffbot not pick this up because the label was added prior to the commit landing? I'll remove and re-add the label.
Labels: Merge-Request-60
Requesting merge of c#3 to M60. It's a tiny fix with an associated unittest that fixes permissions blacklisting and lets us have that capability as a defensive measure against permissions abuse (a fairly large case of which cropped up last weekend).
Project Member

Comment 6 by sheriffbot@chromium.org, Jun 7 2017

Labels: -Merge-Request-60 Hotlist-Merge-Approved Merge-Approved-60
Your change meets the bar and is auto-approved for M60. Please go ahead and merge the CL to branch 3112 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Please merge the patch to M60 branch(3112),Beta RC cut is scheduled @ 4.00 PM PST today(06/07).

Project Member

Comment 8 by bugdroid1@chromium.org, Jun 7 2017

Labels: -merge-approved-60 merge-merged-3112
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/203a0320bd08e0a1832c2692309783de3aa66aab

commit 203a0320bd08e0a1832c2692309783de3aa66aab
Author: Jialiu Lin <jialiul@chromium.org>
Date: Wed Jun 07 19:03:58 2017

Use the substr API correctly in FullHashToHashPrefix.

This code path is only used for permissions blacklists so the rest of
PVer4 launch can progress as-is.

Bug:  729809 
Change-Id: Iec6af24fadae59c6e16c109c0990bc6159575e60
Reviewed-on: https://chromium-review.googlesource.com/524892
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Reviewed-by: Nathan Parker <nparker@chromium.org>
Commit-Queue: Varun Khaneja <vakh@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#477292}
Review-Url: https://codereview.chromium.org/2928793002 .
Cr-Commit-Position: refs/branch-heads/3112@{#228}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}

[modify] https://crrev.com/203a0320bd08e0a1832c2692309783de3aa66aab/components/safe_browsing_db/v4_protocol_manager_util.cc
[modify] https://crrev.com/203a0320bd08e0a1832c2692309783de3aa66aab/components/safe_browsing_db/v4_protocol_manager_util_unittest.cc

Comment 9 by vakh@chromium.org, Jun 8 2017

Status: Fixed (was: Started)

Sign in to add a comment