163549 - Fix/disable goURI ?

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

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

goURI() can be abused to check if the user has visited a URL during this
session. This is slightly more serious than 147777.

I see 3 potential fixes:

1) Limit goURI() to divulge information only for same origin/normal security
policies

2) disable goURI() so that it always fails

3) make goURI() throw exception

I think we should evaluate how commonly goURI() is used on the web, and what is
the usage pattern to determine the best fix, if anything can even be done.

Comment 1

[Radha on family leave (not reading bugmail)]

heikki, Which group/person can provide info on the usage patterns of goURI()?

Comment 2

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

QA might be able to help. Ideally we'd need a web search engine to search the
code/markup on webpages, not just visible content. We could really use such a
thingwith other bugs as well, where we need to evaluate how common some usage
pattern is.

A poor man's approach would be to do a webcrawl of top100 sites or something and
store everything on local disc, and then search that with grep etc.

Comment 3

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

Why not just apply the same-origin check?

Comment 4

[Mitchell Stoltz (not reading bugmail)]

Gerardo, can you collect data on how often and where history.goURI() is used?

Comment 5

[Mitchell Stoltz (not reading bugmail)]

Gerardo, can you collect data on how often and where history.goURI() is used?

Comment 6

[Alec Flett]

taking for now, while radha is gone. Just so we're all on the same page, this is
nsHistory::GoURI(), right?

Comment 7

[Prashant Desale]

Mitch,

I searched for string "history.go" which is part of string "history.goURI()"
almost in half of the top100 sites.
My search was mainly for any .htm, .html, .js, .shtm, .shtml files which contain
string  "history.go". 
I searched for almost half of the top100 sites (all index pages, & scripts
related to index pages) & I did not find any file that contains string history.go

Comment 8

[Alec Flett]

so now that I've looked at this a bit more (I wasn't familiar with the intended
behavior) - what exactly do we want to do with this bug? if the current and
target sites are not the same, should we throw an exception? make it a noop?
Someone help me out here as I've been asked to help out but wasn't around for
the original inception of this bug.

Comment 9

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

I think in normal same origin violations we throw the error from CAPS, so I
guess we should then do that here as well.

Comment 10

[Alec Flett]

Attachment: strawman

I have NO idea if this is the right fix. In fact that whole FindInReadable
seems a bit suspect. Someone want to take a look? I stole some of it from
nsLocation.cpp which makes a similar check.

I also wonder if this fix belongs in nsDocShell.cpp instead, but I don't know
the code well enough to know.

Comment 11

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

Adding Mitch to Cc. Mitch, there is a patch here that you should check out.

Comment 12

[Alec Flett]

Is anyone reading this? If we're going to fix this security bug, I'm going to
need reviews or some feedback on the patch I attached over a week ago. Is this
the right thing?

Comment 13

[Daniel Veditz [:dveditz]]

So why did we add goURI in the first place? This is not a classic Navigator
history method and isn't documented by MS as an IE method
http://msdn.microsoft.com/workshop/author/dhtml/reference/objects/obj_history.asp

I also can't find any place in LXR where we use it, although the history.go()
method was changed to optionally take a string so I guess I'll have to look for
that too. Overloading go wasn't such a hot idea, there are pages which might
have relied on automatic JS string->int conversion and used a string integer
gotten from the user or some other data; now that string would be assumed to be
a URI.

Can we just rip out goURI?

Is CheckLoadURI really the right thing? Why not the UniversalBrowserRead check
used for the other questionable properties?

My recommendation would be to stop overloading go(), then add a capabilities
check for goURI to all.js (have to do both)

Comment 14

[Mitchell Stoltz (not reading bugmail)]

Let's remove the goURI function, or make it inaccessible from content.

Comment 15

[chris hofmann]

do we have a plan?

Comment 16

[Mitchell Stoltz (not reading bugmail)]

Attachment: Testcase - shows if you've visited mozilla.org in the current window

Here's a testcase. I still recommend we pull this function out, or make it
chrome-only.

Comment 17

[Johnny Stenback (:jst)]

Are these functions even used in Mozilla (from JS, that is)? If not, we can mark
them [noscript]. But then again history.goUri() doesn't give you anything that
history.go() doesn't, does it? So getting rid of history.goUri() wouldn't really
help, or would it?

The only reason goUri() and goIndex() exist in nsIDOMHistory is to let non-JS
callers do what JS callers can do with history.go(), and I don't think we can
get rid of history.go(), there's to much history there (no pun intended).

Comment 18

[Mitchell Stoltz (not reading bugmail)]

Per discussion with jst and Alec, I'm going to remove goURI() and the URL-string
version of go(). Taking bug, patch to follow.

Comment 19

[Mitchell Stoltz (not reading bugmail)]

Attachment: Preliminary patch - remove goURI

This patch removes goURI and the string argument case of go(). I'm not sure
what go() should do with a string argument now - in this patch it returns
NS_OK. Should it return an error, and if so, which error?

Comment 20

[Alec Flett]

Comment on attachment 114805 [details] [diff] [review]
Preliminary patch - remove goURI

r=alecf (plus whichever return value you decide on)

Comment 21

[Johnny Stenback (:jst)]

I'd suggest that we'd go ahead and rename goIndex() in nsIDOMHistory to simply
go() to match up with the JS version, the only reason goIndex() exists is that
history.go() could take either a string or a number before this patch, but now
that it no longer can take anything other than a number, we could rename
goIndex() to go() and we'd have language parity. We should leave go() in
nsIDOMJSHistory though since currently history.go() is callable w/o arguments
and I'd rather leave that as is, since who knows what's out on the crazy web
these days, top sites might fall appart if we change that (unlikely, but I've
been bit by stuff like this enough times to prefere to not change stuff like this).

IOW, the patch looks good, but rename nsIDOMHistory::GoIndex() to
nsIDOMHistory::Go() in addition to the other change.

Comment 22

[Johnny Stenback (:jst)]

Attachment: Same as above with GoIndex() renamed to Go().

Comment 23

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 114844 [details] [diff] [review]
Same as above with GoIndex() renamed to Go().

r=mstoltz, over to heikki for sr.

Comment 24

[Mitchell Stoltz (not reading bugmail)]

Fix checked in.

Comment 25

[Christopher Aillon (sabbatical, not receiving bugmail)]

Opening.