Closed Bug 163549 Opened 20 years ago Closed 20 years ago

Fix/disable goURI ?


(Core :: DOM: Navigation, defect)

Windows 2000
Not set





(Reporter: hjtoi-bugzilla, Assigned: security-bugs)


(Whiteboard: [sg:mustfix], patch)


(4 files)

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

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.
heikki, Which group/person can provide info on the usage patterns of goURI()?
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.
Target Milestone: --- → Future
Why not just apply the same-origin check?
Whiteboard: [sg:mustfix]
Gerardo, can you collect data on how often and where history.goURI() is used?
Gerardo, can you collect data on how often and where history.goURI() is used?
taking for now, while radha is gone. Just so we're all on the same page, this is
nsHistory::GoURI(), right?
Assignee: radha → alecf

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

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.
I think in normal same origin violations we throw the error from CAPS, so I
guess we should then do that here as well.
Target Milestone: Future → mozilla1.2final
Attached patch strawmanSplinter Review
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.
Adding Mitch to Cc. Mitch, there is a patch here that you should check out.
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?
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

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)
Let's remove the goURI function, or make it inaccessible from content.
do we have a plan?
Here's a testcase. I still recommend we pull this function out, or make it
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).
Per discussion with jst and Alec, I'm going to remove goURI() and the URL-string
version of go(). Taking bug, patch to follow.
Assignee: alecf → mstoltz
Target Milestone: mozilla1.2final → mozilla1.4alpha
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?
Attachment #114805 - Flags: superreview?(jst)
Attachment #114805 - Flags: review?(alecf)
Comment on attachment 114805 [details] [diff] [review]
Preliminary patch - remove goURI

r=alecf (plus whichever return value you decide on)
Attachment #114805 - Flags: review?(alecf) → review+
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.
Attachment #114805 - Flags: superreview?(jst) → superreview?(heikki)
Whiteboard: [sg:mustfix] → [sg:mustfix], patch
Comment on attachment 114844 [details] [diff] [review]
Same as above with GoIndex() renamed to Go().

r=mstoltz, over to heikki for sr.
Attachment #114844 - Flags: superreview?(heikki)
Attachment #114844 - Flags: review+
Attachment #114844 - Flags: superreview?(heikki) → superreview+
Fix checked in.
Closed: 20 years ago
Resolution: --- → FIXED
Attachment #114805 - Flags: superreview?(heikki)
Component: History: Session → Document Navigation
QA Contact: claudius → docshell
You need to log in before you can comment on or make changes to this bug.