161546 - javascript: urls from history window/sidebar run in context of current page

Status: RESOLVED FIXED

(Core Graveyard :: History: Global, defect)

Description

[Jesse Ruderman]

Steps to reproduce:
1. Type javascript:alert(document.cookie);"<title>haha</title>" into the address
bar.
2. Open the history window or sidebar.
3. Make sure the results aren't grouped by day.
4. Sort by last visited.
5. Click the javascript: url.

Result: the javascript: url runs in a recently used browser window and shows
your cookie for that window.

Expected: the javascript: url should encounter a security error when it tries to
access the cookie.

This is a minor security hole, made slightly worse by bug 161531 and bug 151024.

Possible fixes:

1. Hide javascript: urls from history window but leave them in autocomplete. 
Since many users' only interaction with history is to cover their tracks, I
don't like this solution.  It would make Select All, Delete appear to clear
history but actually leave some URLs behind.

2. Leave javascript: urls in the history window but make them run in a new
security domain rather than the domain of a currently loaded page.

Note the current behavior is correct for the address bar and bookmarks.  A fix
for bug 161531 will keep the least trusted javascript: urls out of history,
making this less of a problem for the address bar with autocomplete.  But since
the history sidebar only displays titles, I think this bug should be fixed anyway.

This bug also applies to data: urls.

Comment 1

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Making security sensitive.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sounds like this should be fixable by copying the behaviour of bookmarks.

Comment 3

[Mitchell Stoltz (not reading bugmail)]

It currently has the same behavior as bookmarks. Bookmarks run in the context of
the current page, and that's by design. The security risk there is one we've
decided is accptable. The risk is a little bit higher here because URLs are
added to history automatically, while bookmarks are explicitly created. We
should make javascript URLs from history run in a safe context or possibly not
run at all.

Comment 4

[Mitchell Stoltz (not reading bugmail)]

I'll take this; I think I know how to fix it.

Comment 5

[Stephen P. Morse]

I just posted a patch to bug 161531 which will prevent javascript: and data:
urls from going into global history unless they were typed in by the user.  That
should solve the problem presented here.

However I do not understand the following comment in the description above:

  A fix for bug 161531 will keep the least trusted javascript: urls out of
  history, making this less of a problem for the address bar with autocomplete.
  But since the history sidebar only displays titles, I think this bug should
  be fixed anyway.

Please explain the significance of the history sidebar, and tell why the fix for
bug 161531 isn't sufficient.

Comment 6

[Mitchell Stoltz (not reading bugmail)]

to nisheeth

Comment 7

[Nisheeth Ranjan]

The fix for bug 161531 didn't get checked in.  I've taken that bug over as well
as bug 151024 mentioned in Jesse's first comment above.  More on this as I get
my head around this better...

Comment 8

[Nisheeth Ranjan]

Attachment: First attempt

This patch disables running of js and data urls from the history sidebar and
history window.  It turns out that this is much cleaner than trying to make js
urls in history sidebar/window execute with their own security principal in the
document context.  I don't think users will want to run js urls from history. 
If they really want to save js urls, they can save them in their bookmark
folders.

Comments?

Comment 9

[Nisheeth Ranjan]

Comment on attachment 118372 [details] [diff] [review]
First attempt

Please see comment #8 that describes this patch. Alec, please r=.  Heikki,
please sr=.

Thanks!

Comment 10

[Nisheeth Ranjan]

A code level description for the reviewers:  I basically check in OpenURL() of
history.js whether the url is a js or data url.  If it is, I bail out without
loading the url.

The user experience sucks with this patch, though.  Users see the js and data
urls in the sidebar/window but when they click on it, nothing happens.  Maybe a
better solution would be to gray out js/data urls in the history tree view?  Or
maybe we could weed out js/data urls from the tree view altogether?  I'm not
sure how the RDF backend to tree view hook up works, but, if it is easy to
discard js/data urls as the tree view is populated, lets do it.

A lot of these ideas came out of my conversation with jst.  CCing Dan and him.

Comment 11

[Alec Flett]

huh? in what sidebar/window?
I don't think its reasonable to try to poke this in the UI. That's the whole
point of the "hidden" attribute, so they don't appear in the UI. If they're
appearing in the UI then "hidden" flag is getting un-set somehow.

Comment 12

[Nisheeth Ranjan]

alecf, the hiding will only happen for js urls that are clicked when the patch
you reviewed for bug 197127 is checked in.  typed js urls get unhidden because
we want them to show up in the autocomplete dropdown.  when that happens they
can also be seen in the history sidebar/window.  but we don't want the user to
be able to run those urls.  this patch makes that happen.

"history sidebar/window" is shorthand for the history sidebar and the history
window that can be accessed from the Go menu...

Comment 13

[Nisheeth Ranjan]

Attachment: 2nd attempt at fix

Had an AIM chat with Alec and cleared up the confusion around this bug.

This second version of the patch prevents JS and data urls from executing in
the history sidebar or window and throws an alert if the user clicks on such
urls in the sidebar/window.

Jesse, I understand the risks of JS urls, but why is the loading of data urls a
security risk?

Comment 14

[Nisheeth Ranjan]

Attachment: 3rd attempt at fix

I had put up patch 118453 while I created a fresh build.  On testing the patch
in the fresh build, I found and fixed a few nits.  This is the updated patch. 
No new functionality.

Comment 15

[Nisheeth Ranjan]

Comment on attachment 118487 [details] [diff] [review]
3rd attempt at fix

Alec, I AIM'd you about this patch earlier today.  Please r=.  See comment 13
for more details.  Heikki, please sr.

Thanks!

Comment 16

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 118487 [details] [diff] [review]
3rd attempt at fix

sr=heikki. I can live with this for now, but ideally we should go with graying
out these entries and not reacting to clicks etc. Alerts are always annoying,
and often ignored. If alecf gives r and you check in, please open an new bug
(still secret until this is published) about a better way to implement this.

Comment 17

[Nisheeth Ranjan]

Agreed, I've filed bug 199225 and marked it security sensitive

Comment 18

[Alec Flett]

Comment on attachment 118487 [details] [diff] [review]
3rd attempt at fix

r=alecf, but let me suggest a better string - we don't like using "you" in the
UI:

For security reasons, javascript urls cannot be loaded from the history window
or sidebar.

Comment 19

[Nisheeth Ranjan]

Comment on attachment 118487 [details] [diff] [review]
3rd attempt at fix

Please allow me to check in a security fix to 1.4 alpha.  The fix doesn't allow
the user to load JS and data urls from within the history  sidebar or history
window.  When a user clicks on such urls, an alert pops up with an appropriate
error message.	Its been reviewed by alecf and super reviewed by heikki.  I've
run the pre-checkin tests and also made sure that other types of urls can be
loaded properly from the history window and sidebar.

Thanks!

Comment 20

[Nisheeth Ranjan]

alec, thanks for the review.  yes, i'll make that change to the error message.

Comment 21

[Alec Flett]

Comment on attachment 118372 [details] [diff] [review]
First attempt

clearing reviews for obsolete patches

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 118487 [details] [diff] [review]
3rd attempt at fix

a=asa (on behalf of drivers) for checkin to 1.4alpha.

Comment 23

[Nisheeth Ranjan]

Fix checked into trunk...  About to attach patch that actually got checked in
with the new error message suggested by alec.

Comment 24

[Nisheeth Ranjan]

Attachment: Final patch with changed error message

This is the patch that I checked into the trunk.  The only difference between
this patch and the r/sr/a'd patch is the text of the error message in the
alert.