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

Issue metadata

Status: Fixed
Owner:
Closed: Aug 31
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug-Security
Team-Security-UX



Sign in to add a comment
link

Issue 844881: Security: Address spoofing in Omnibox

Reported by chromium...@gmail.com, May 19 2018

Issue description

VERSION
Chrome Version: Version 68.0.3435.0 (Official Build) canary (64-bit)
Operating System: Mac

REPRODUCTION CASE

(repro  issue 672847 )

1. Load the test case 
2. Click on the button
3. Observe
 
Screen Shot 2018-05-19 at 20.40.41.png
47.9 KB View Download
poc.html
397 bytes View Download

Comment 1 by chromium...@gmail.com, May 19 2018

MacOS Sierra 10.12.6.

Comment 2 by mmoroz@google.com, May 21 2018

Components: UI>Browser>Navigation UI>Browser>Omnibox UI>Security>UrlFormatting
Labels: M-68 Security_Severity-Medium Security_Impact-Head OS-Mac
Owner: kenrb@chromium.org
Status: Assigned (was: Unconfirmed)
Confirmed, I'be able to reproduce this using the Canary build on Mac.
Screen Shot 2018-05-21 at 11.14.37 AM.png
47.0 KB View Download

Comment 3 by mmoroz@google.com, May 21 2018

Note that on Stable build I'm seeing "spoof" text just for a second or less, that the amazon page starts to load and get rendered.

Comment 4 by sheriffbot@chromium.org, May 22 2018

Project Member
Labels: ReleaseBlock-Stable
This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 5 by sheriffbot@chromium.org, May 22 2018

Project Member
Labels: Pri-1

Comment 6 by kenrb@chromium.org, May 22 2018

Cc: fsam...@chromium.org creis@chromium.org
Labels: -Security_Impact-Head -ReleaseBlock-Stable
Comment 3: On Canary, I can only repro this if I opt out of the Site Isolation trial. With SI enabled, the Amazon navigation completes successfully overriding the spoof text. That might just be a matter of timing, since I think the PoC assumes the navigation will take more than one second.

I don't believe this is a regression, but rather a somewhat more convincing example of issue 776402. In that one, it repeatedly navigates between the attacker page and a victim page, causing the spoof page to remain visible but the URL in the omnibox flickers. This is similar, repeatedly navigating to reset the spoof timer, but it goes to a page that responds with an HTTP 204 (No Content), which prevents the URL from updating at all.

Maybe we need to do something like not reset the timer on the navigation in that case?

Comment 7 by sheriffbot@chromium.org, May 23 2018

Project Member
Labels: Security_Impact-Head

Comment 8 by sheriffbot@chromium.org, May 23 2018

Project Member
Labels: ReleaseBlock-Stable
This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 9 by kenrb@chromium.org, May 23 2018

Labels: -Security_Impact-Head -ReleaseBlock-Stable Security_Impact-Stable

Comment 10 by kenrb@chromium.org, May 25 2018

Labels: OS-Chrome OS-Linux OS-Windows
My assessment in comment 6 is wrong, and this might be a regression (although it's hard to test for certain).

The timer is firing for the page but the surface is not getting cleared. There has been a fair amount of refactoring around this code lately, but so far I haven't been able to find any changes that fix the bug when I revert them.

I'm trying to figure out if the surface isn't getting cleared properly, or if maybe the compositor is locked so the frame eviction isn't followed by a commit.

Fady, I wonder if we could figure out if there is a way to write better regression tests for the timeout mechanism. Right now there are some unit tests but nothing that actually verifies there are only white pixels on the screen after the timer fires.

Comment 11 by chromium...@gmail.com, May 27 2018

I can only repro this on Mac.

Comment 12 by kenrb@chromium.org, May 28 2018

I have been able to repro it on Mac, Linux and Windows. A fix is in progress.

Comment 13 by palmer@chromium.org, Jun 11 2018

#12: Any news? :)

Comment 14 by sheriffbot@chromium.org, Jun 12 2018

Project Member
kenrb: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 15 by kenrb@chromium.org, Jun 12 2018

Status: Started (was: Assigned)
I uploaded a fix for this a while ago, but writing a good test to cover it is more difficult and I have been sidetracked by site isolation bugs.

I hope to be getting back to this soon.

https://chromium-review.googlesource.com/c/chromium/src/+/1075512

Comment 16 by chromium...@gmail.com, Jul 13 2018

Any update on this bug?

Comment 17 by mmoroz@chromium.org, Aug 7

kenrb@, friendly ping from the security sheriff. I see you've mentioned a fix in c#12, did you upload it for a review / landed?

Comment 18 by kenrb@chromium.org, Aug 16

Cc: samans@chromium.org
We think this was likely fixed by r577029, which would be on Canary and Dev channels now.

Unfortunately I haven't been able to immediately verify, because recent releases all seem to complete the navigation to amazon.com when I try the repro case provided. In that case the timer doesn't fire, so we can't observe the changed eviction code.

If anyone is able to verify on Dev or Canary I'd appreciate it -- we would want to see the spoof text appear for ~4 seconds and then disappear. If the Amazon navigation completes then it neither affirms nor refutes that the problem is fixed.

Comment 19 by chromium...@gmail.com, Aug 17

I'm still able to repro this on Canary 70.0.3525.0 on Mac.

Comment 20 by kenrb@chromium.org, Aug 22

Cc: ccameron@chromium.org
We really need a way to comprehensively test the timeout behavior. :/

It now appears that the original bug was fixed in r577029, but the behavior was separately regressed by ccameron in r569513.

The spoof text isn't being removed because the timer isn't starting. The timer isn't starting because the top-level RWHV doesn't get a new Surface ID when the Amazon navigation commits, and we return early at https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_impl.cc?l=1211&rcl=3ea4abbfded911cb15cc62f93519564983f41faf.

I'm not clear on why that is happening. Possibly because the initial page load is a javascript: URL that doesn't have a navigation commit?

fsamuel and ccameron: I haven't followed all the changes related to surface synchronization. Do either of you have thoughts on the above?

Comment 21 by ccameron@chromium.org, Aug 22

From the comments in the patch:

The fix is to change RHWI::DidNavigate to only start the timer when
RWHV::DidNavigate changes the surface id. Two reasons:
  - If we didn't change the surface id, it was because this was the
    first navigation, and so we don't need to clear the frame.

So, is that statement about "first navigation" not true?

Comment 22 by kenrb@chromium.org, Aug 22

That's what I think might be the case. It doesn't appear that loading a javascript: URL causes a navigation commit (which kind of makes sense, since you shouldn't get navigation history from running a bookmarklet, for instance). In this case there is a window.open("javascript://document.write('...')"), so we get a compositor frame from a previously existing document that didn't have a navigation.

Based on that comment I surmise this was an unexpected scenario.

Comment 23 by chromium...@gmail.com, Aug 22

Kenrb@, it sounds like you can no longer repro this issue on latest Canary?

Comment 24 by kenrb@chromium.org, Aug 23

I am able to repro this on Mac. The repro case often navigates to amazon.com and then displays the Amazon page, in which case the problem isn't visible. Right now only on Mac with Site Isolation disabled have I been able to make that not happen. As I mentioned, the stale content timer does not start after the Amazon navigation commits, so the spoof text doesn't get cleared.

Comment 25 by bugdroid1@chromium.org, Aug 28

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

commit 7da6c3419fd172405bcece1ae4ec6ec8316cd345
Author: Ken Buchanan <kenrb@chromium.org>
Date: Tue Aug 28 23:05:52 2018

Start rendering timer after first navigation

Currently the new content rendering timer in the browser process,
which clears an old page's contents 4 seconds after a navigation if the
new page doesn't draw in that time, is not set on the first navigation
for a top-level frame.

This is problematic because content can exist before the first
navigation, for instance if it was created by a javascript: URL.

This CL removes the code that skips the timer activation on the first
navigation.

Bug:  844881 
Change-Id: I19b3ad1ff62c69ded3a5f7b1c0afde191aaf4584
Reviewed-on: https://chromium-review.googlesource.com/1188589
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: ccameron <ccameron@chromium.org>
Commit-Queue: Ken Buchanan <kenrb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586913}
[modify] https://crrev.com/7da6c3419fd172405bcece1ae4ec6ec8316cd345/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/7da6c3419fd172405bcece1ae4ec6ec8316cd345/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc

Comment 26 by chromium...@gmail.com, Aug 29

Thanks for the fix :)

Comment 27 by kenrb@chromium.org, Aug 29

The fix hasn't made it to Canary yet, hopefully it will roll out tomorrow and we can verify it.

Comment 28 by kenrb@chromium.org, Aug 31

chromium.khalil@: Can you try this out on an updated Canary and see if you can still repro? The behavior looks correct to me right now.

Comment 29 by chromium...@gmail.com, Aug 31

Double-check.

I’m not able to repro this on thr lastest version of Canary on Mac. Fixed.

Comment 30 by kenrb@chromium.org, Aug 31

Labels: reward-topanel
Status: Fixed (was: Started)
Thanks!

Sorry it took so long to finally resolve this.

Comment 31 by sheriffbot@chromium.org, Sep 1

Project Member
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify

Comment 32 by awhalley@chromium.org, Sep 11

Labels: -reward-topanel reward-unpaid reward-3000
*** Boilerplate reminders! ***
Please do NOT publicly disclose details until a fix has been released to all our users. Early public disclosure may cancel the provisional reward. Also, please be considerate about disclosure when the bug affects a core library that may be used by other products. Please do NOT share this information with third parties who are not directly involved in fixing the bug. Doing so may cancel the provisional reward. Please be honest if you have already disclosed anything publicly or to third parties. Lastly, we understand that some of you are not interested in money. We offer the option to donate your reward to an eligible charity. If you prefer this option, let us know and we will also match your donation - subject to our discretion. Any rewards that are unclaimed after 12 months will be donated to a charity of our choosing.
*********************************

Comment 33 by awhalley@google.com, Sep 11

Nice spoof! $3,000 for this one!

Comment 34 by mcnee@chromium.org, Sep 11

Cc: kenrb@chromium.org mcnee@chromium.org wjmaclean@chromium.org
 Issue 871844  has been merged into this issue.

Comment 35 by awhalley@chromium.org, Sep 11

Labels: -reward-unpaid reward-inprocess

Comment 36 by awhalley@google.com, Oct 15

Labels: -M-68 Release-0-M70 M-70

Comment 37 by awhalley@chromium.org, Oct 16

Labels: CVE-2018-17467 CVE_description-missing

Comment 38 by kenrb@chromium.org, Oct 17

 Issue 893978  has been merged into this issue.

Comment 39 by sheriffbot@chromium.org, Oct 26

Project Member
Labels: Merge-Request-71

Comment 40 by sheriffbot@chromium.org, Oct 26

Project Member
Labels: -Merge-Request-71 Hotlist-Merge-Review Merge-Review-71
This bug requires manual review: M71 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 41 by kenrb@chromium.org, Oct 26

Is that a bug in sheriffbot? The fix to this bug doesn't need a merge anywhere.

Comment 42 by awhalley@google.com, Oct 26

Labels: -Hotlist-Merge-Review -Merge-Review-71

Comment 43 by awhalley@chromium.org, Nov 12

Labels: -CVE_description-missing CVE_description-submitted

Comment 44 by sheriffbot@chromium.org, Dec 8

Project Member
Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment