The default bug view has changed. See this FAQ.

Fix/disable goURI ?

RESOLVED FIXED in mozilla1.4alpha

Status

()

Core
Document Navigation
RESOLVED FIXED
15 years ago
9 years ago

People

(Reporter: Heikki Toivonen (remove -bugzilla when emailing directly), Assigned: Mitchell Stoltz (not reading bugmail))

Tracking

Trunk
mozilla1.4alpha
x86
Windows 2000
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:mustfix], patch)

Attachments

(4 attachments)

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.
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]
(Assignee)

Comment 4

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

Comment 5

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

Comment 6

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

Comment 7

15 years ago
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

15 years ago
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.

Updated

15 years ago
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla1.2final

Comment 10

15 years ago
Created attachment 103898 [details] [diff] [review]
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.
Adding Mitch to Cc. Mitch, there is a patch here that you should check out.

Comment 12

15 years ago
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
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)
(Assignee)

Comment 14

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

Comment 15

14 years ago
do we have a plan?
(Assignee)

Comment 16

14 years ago
Created attachment 114384 [details]
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.
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).
(Assignee)

Comment 18

14 years ago
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
Status: ASSIGNED → NEW
Target Milestone: mozilla1.2final → mozilla1.4alpha
(Assignee)

Comment 19

14 years ago
Created attachment 114805 [details] [diff] [review]
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?
(Assignee)

Updated

14 years ago
Attachment #114805 - Flags: superreview?(jst)
Attachment #114805 - Flags: review?(alecf)

Comment 20

14 years ago
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.
(Assignee)

Updated

14 years ago
Attachment #114805 - Flags: superreview?(jst) → superreview?(heikki)
(Assignee)

Updated

14 years ago
Whiteboard: [sg:mustfix] → [sg:mustfix], patch
Created attachment 114844 [details] [diff] [review]
Same as above with GoIndex() renamed to Go().
(Assignee)

Comment 23

14 years ago
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+
(Assignee)

Comment 24

14 years ago
Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Attachment #114805 - Flags: superreview?(heikki)
Opening.
Group: security

Updated

9 years ago
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.