Last Comment Bug 163549 - Fix/disable goURI ?
: Fix/disable goURI ?
Status: RESOLVED FIXED
[sg:mustfix], patch
:
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: x86 Windows 2000
: -- normal (vote)
: mozilla1.4alpha
Assigned To: Mitchell Stoltz (not reading bugmail)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-08-19 16:08 PDT by Heikki Toivonen (remove -bugzilla when emailing directly)
Modified: 2008-07-31 02:42 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
strawman (2.61 KB, patch)
2002-10-23 14:05 PDT, Alec Flett
no flags Details | Diff | Splinter Review
Testcase - shows if you've visited mozilla.org in the current window (486 bytes, text/html)
2003-02-13 17:10 PST, Mitchell Stoltz (not reading bugmail)
no flags Details
Preliminary patch - remove goURI (2.88 KB, patch)
2003-02-18 10:51 PST, Mitchell Stoltz (not reading bugmail)
alecf: review+
Details | Diff | Splinter Review
Same as above with GoIndex() renamed to Go(). (6.31 KB, patch)
2003-02-18 17:40 PST, Johnny Stenback (:jst, jst@mozilla.com)
security-bugs: review+
hjtoi-bugzilla: superreview+
Details | Diff | Splinter Review

Description Heikki Toivonen (remove -bugzilla when emailing directly) 2002-08-19 16:08:16 PDT
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) 2002-08-28 14:36:35 PDT
heikki, Which group/person can provide info on the usage patterns of goURI()?
Comment 2 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-08-28 14:49:46 PDT
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) 2002-10-15 08:58:41 PDT
Why not just apply the same-origin check?
Comment 4 Mitchell Stoltz (not reading bugmail) 2002-10-17 17:04:05 PDT
Gerardo, can you collect data on how often and where history.goURI() is used?
Comment 5 Mitchell Stoltz (not reading bugmail) 2002-10-17 17:08:10 PDT
Gerardo, can you collect data on how often and where history.goURI() is used?
Comment 6 Alec Flett 2002-10-17 17:15:17 PDT
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 2002-10-21 15:06:26 PDT
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 2002-10-21 15:25:23 PDT
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) 2002-10-21 15:29:59 PDT
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 2002-10-23 14:05:09 PDT
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.
Comment 11 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-10-23 14:09:32 PDT
Adding Mitch to Cc. Mitch, there is a patch here that you should check out.
Comment 12 Alec Flett 2002-11-01 17:31:45 PST
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] 2002-11-07 13:22:07 PST
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) 2002-11-25 16:20:38 PST
Let's remove the goURI function, or make it inaccessible from content.
Comment 15 chris hofmann 2003-02-11 15:54:05 PST
do we have a plan?
Comment 16 Mitchell Stoltz (not reading bugmail) 2003-02-13 17:10:16 PST
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.
Comment 17 Johnny Stenback (:jst, jst@mozilla.com) 2003-02-13 17:45:07 PST
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) 2003-02-14 14:35:34 PST
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) 2003-02-18 10:51:50 PST
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?
Comment 20 Alec Flett 2003-02-18 12:04:10 PST
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, jst@mozilla.com) 2003-02-18 13:27:15 PST
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, jst@mozilla.com) 2003-02-18 17:40:14 PST
Created attachment 114844 [details] [diff] [review]
Same as above with GoIndex() renamed to Go().
Comment 23 Mitchell Stoltz (not reading bugmail) 2003-02-18 18:11:57 PST
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) 2003-02-25 11:26:27 PST
Fix checked in.
Comment 25 Christopher Aillon (sabbatical, not receiving bugmail) 2003-11-24 07:30:43 PST
Opening.

Note You need to log in before you can comment on or make changes to this bug.