Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Starred by 4 users
Status: Fixed
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security



Sign in to add a comment
Security: UXSS introduced through bookmark containing user information
Reported by g_goo...@flamescape.com, Aug 18 2016 Back to list
VULNERABILITY DETAILS
Chrome's "Edit Bookmark" dialogue window strips the opening "http://" from the URL field, if it exists.  If the URI also contains user (auth) information, then saving the bookmark will change the URI scheme of the bookmark.  This bug can be exploited to introduce XSS into the currently open page whenever the bookmark is clicked.

VERSION
Chrome Version: 52.0.2743.116 stable
Operating System: Windows 10 Pro

REPRODUCTION CASE

1. The victim clicks on a specially crafted link whose URL contains malicious javascript disguised as user (auth) information:

   <a href='http://javascript:eval(atob("YWxlcnQoIlhTUyIp"))-"@example.com/#"'>Click Me!</a>

2. The browser loads the page at example.com.
   The victim's URL bar only displays the (innocuous looking) text: example.com/#"

3. The user, in an attempt to bookmark the page, performs the following actions:
   a. Click the star icon in the URL bar
   b. Click "Edit..." -or- Click "Choose another folder..." within the Folder drop-down
   c. Click "Save"

4. If the user then clicks on the bookmark, the injected javascript from Step 1 will be executed in the context of whichever domain is currently loaded in the active tab.


Please let me know if you require further information.

 
Cc: jialiul@chromium.org
Components: UI>Browser>Bookmarks UI>Browser>Bookmarks>Enhanced
Labels: Security_Severity-Medium Security_Impact-Stable M-54 Pri-2
Owner: ian...@chromium.org
Thanks for reporting, g_google@flamescape.com!

+ianwen@, could you help triage this issue? It looks similar to  bug 481015 .  Please feel free to suggest other owner. 


Status: Untriaged
Comment 3 by ian...@chromium.org, Aug 18 2016
Owner: f...@chromium.org
Hmm this is windows 10?

I am not sure who is working on bookmark for desktop now. Since it's a security issue, I would reassign it to felt@ and let her decide?
Labels: OS-All
it can be reproduced on linux as well. I haven't tested it on mobile, but I guess this issue is not specific to a particular OS.
+felt@, could you suggest a owner? 
Comment 5 by f...@chromium.org, Aug 19 2016
Cc: f...@chromium.org
Owner: shrike@chromium.org
Re #13: Feature teams are responsible for fixing the bugs in their areas of code. The security team is just here to triage and help provide advice if you need it. We wouldn't be able to scale up to fix all security bugs in all features. So in this case, someone on the bookmarks team ought to investigate this. :)

shrike@ - it looks like you have been fixing bookmarks-related bugs, do you know who owns bookmarks now?
Project Member Comment 6 by sheriffbot@chromium.org, Aug 19 2016
Labels: -Pri-2 Pri-1
Project Member Comment 7 by sheriffbot@chromium.org, Aug 19 2016
Status: Assigned
Comment 8 by shrike@chromium.org, Aug 23 2016
Owner: rdevlin....@chromium.org
I've only been doing UI work on bookmarks on the Mac. I'm not sure who would be a good person for this fix, but taking a stab at rdevlin.cronin@.

Owner: sky@chromium.org
@5, @8 it looks like sky@ owns bookmarks (components/bookmarks and c/b/bookmarks).  Over to him for triage.
Project Member Comment 10 by sheriffbot@chromium.org, Sep 2 2016
sky: 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
Project Member Comment 11 by sheriffbot@chromium.org, Sep 17 2016
sky: Uh oh! This issue still open and hasn't been updated in the last 29 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
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc?sq=package:chromium&dr=C&l=380

calls:
    url_tf_->SetText(chrome::FormatBookmarkURLForDisplay(url));

That, in turn, is commented thusly:
base::string16 FormatBookmarkURLForDisplay(const GURL& url) {
  // Because this gets re-parsed by FixupURL(), it's safe to omit the scheme
  // and trailing slash, and unescape most characters.  However, it's
  // important not to drop any username/password, or unescape anything that
  // changes the URL's meaning.
  return url_formatter::FormatUrl(
      url, url_formatter::kFormatUrlOmitAll &
               ~url_formatter::kFormatUrlOmitUsernamePassword,
      net::UnescapeRule::SPACES, nullptr, nullptr, nullptr);
}

Keeping the userinfo (username/password) is not compatible with omitting the scheme, because most schemes work like scheme://user:password@host/path and if you omit the scheme it becomes user:password@host, which, upon reinterpretation, is indistinguishable from scheme:user@host, because non-hierarchical schemes use ":" rather than "://" as the scheme delimiter.

To fix, we could change (url_formatter::kFormatUrlOmitAll & ~url_formatter::kFormatUrlOmitUsernamePassword) to kFormatUrlOmitTrailingSlashOnBareHostname, or we could more selectively do so iff the URL contains a userinfo component. I'm inclined to say that we should just always show the scheme, especially because today it's only omitted for HTTP (not HTTPS) and HTTPS is getting ever more common.
Owner: elawre...@chromium.org
Status: Started
Thanks for taking on this elawrence@. 
Project Member Comment 16 by bugdroid1@chromium.org, Oct 3 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fa34e547d6ee25ea0692436ba7462ed0a0ef45f4

commit fa34e547d6ee25ea0692436ba7462ed0a0ef45f4
Author: elawrence <elawrence@chromium.org>
Date: Mon Oct 03 18:41:02 2016

Prevent interpretating userinfo as url scheme when editing bookmarks

Chrome's Edit Bookmark dialog formats urls for display such that a
url of http://javascript:scripttext@host.com is later converted to a
javascript url scheme, allowing persistence of a script injection
attack within the user's bookmarks.

This fix prevents such misinterpretations by always showing the
scheme when a userinfo component is present within the url.

BUG= 639126 

Review-Url: https://codereview.chromium.org/2368593002
Cr-Commit-Position: refs/heads/master@{#422467}

[modify] https://crrev.com/fa34e547d6ee25ea0692436ba7462ed0a0ef45f4/chrome/browser/ui/bookmarks/bookmark_utils.cc
[modify] https://crrev.com/fa34e547d6ee25ea0692436ba7462ed0a0ef45f4/chrome/browser/ui/bookmarks/bookmark_utils.h
[modify] https://crrev.com/fa34e547d6ee25ea0692436ba7462ed0a0ef45f4/chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm
[modify] https://crrev.com/fa34e547d6ee25ea0692436ba7462ed0a0ef45f4/chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc

Status: Fixed
Project Member Comment 18 by sheriffbot@chromium.org, Oct 4 2016
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: Merge-Request-54
Externally reported, so requesting merge to M-54
Comment 20 by dimu@chromium.org, Oct 5 2016
Labels: -Merge-Request-54 Merge-Review-54 Hotlist-Merge-Review
[Automated comment] Less than 2 weeks to go before stable on M54, manual review required.
Labels: -Merge-Review-54 Merge-Approved-54
SGTM, approved for merging into M54
Project Member Comment 22 by bugdroid1@chromium.org, Oct 10 2016
Labels: -merge-approved-54 merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2775e31152857adc2bb9775b03212d1356541b4b

commit 2775e31152857adc2bb9775b03212d1356541b4b
Author: Andrew R. Whalley <awhalley@chromium.org>
Date: Mon Oct 10 21:47:53 2016

[merge to m54] Prevent interpretating userinfo as url scheme when editing bookmarks

Chrome's Edit Bookmark dialog formats urls for display such that a
url of http://javascript:scripttext@host.com is later converted to a
javascript url scheme, allowing persistence of a script injection
attack within the user's bookmarks.

This fix prevents such misinterpretations by always showing the
scheme when a userinfo component is present within the url.

BUG= 639126 

Review-Url: https://codereview.chromium.org/2368593002
Cr-Commit-Position: refs/heads/master@{#422467}
(cherry picked from commit fa34e547d6ee25ea0692436ba7462ed0a0ef45f4)

Review URL: https://codereview.chromium.org/2411473002 .

Cr-Commit-Position: refs/branch-heads/2840@{#708}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/2775e31152857adc2bb9775b03212d1356541b4b/chrome/browser/ui/bookmarks/bookmark_utils.cc
[modify] https://crrev.com/2775e31152857adc2bb9775b03212d1356541b4b/chrome/browser/ui/bookmarks/bookmark_utils.h
[modify] https://crrev.com/2775e31152857adc2bb9775b03212d1356541b4b/chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm
[modify] https://crrev.com/2775e31152857adc2bb9775b03212d1356541b4b/chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc

Labels: Release-0-M54
Labels: reward-topanel
Labels: CVE-2016-5191
Labels: -reward-topanel reward-unpaid reward-500
Nice one, the panel awarded $500 for this bug.  They noted it's not really a UXSS, and needs a fairly uncommon combination of user interaction.  Thanks for the report!  A member of our finance team will be in touch shortly.
Labels: reward-inprocess
Labels: -reward-unpaid
Project Member Comment 30 by bugdroid1@chromium.org, Oct 27 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2775e31152857adc2bb9775b03212d1356541b4b

commit 2775e31152857adc2bb9775b03212d1356541b4b
Author: Andrew R. Whalley <awhalley@chromium.org>
Date: Mon Oct 10 21:47:53 2016

[merge to m54] Prevent interpretating userinfo as url scheme when editing bookmarks

Chrome's Edit Bookmark dialog formats urls for display such that a
url of http://javascript:scripttext@host.com is later converted to a
javascript url scheme, allowing persistence of a script injection
attack within the user's bookmarks.

This fix prevents such misinterpretations by always showing the
scheme when a userinfo component is present within the url.

BUG= 639126 

Review-Url: https://codereview.chromium.org/2368593002
Cr-Commit-Position: refs/heads/master@{#422467}
(cherry picked from commit fa34e547d6ee25ea0692436ba7462ed0a0ef45f4)

Review URL: https://codereview.chromium.org/2411473002 .

Cr-Commit-Position: refs/branch-heads/2840@{#708}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/2775e31152857adc2bb9775b03212d1356541b4b/chrome/browser/ui/bookmarks/bookmark_utils.cc
[modify] https://crrev.com/2775e31152857adc2bb9775b03212d1356541b4b/chrome/browser/ui/bookmarks/bookmark_utils.h
[modify] https://crrev.com/2775e31152857adc2bb9775b03212d1356541b4b/chrome/browser/ui/cocoa/bookmarks/bookmark_editor_controller_unittest.mm
[modify] https://crrev.com/2775e31152857adc2bb9775b03212d1356541b4b/chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc

Project Member Comment 31 by sheriffbot@chromium.org, Jan 10 2017
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