278772 - Various Firefox code incorrectly constructs URIs

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Unless you're implementing a protocol, creating an nsIURI object by
instantiating an nsStandardURL and setting the spec is almost certainly the
wrong thing to do.  This will lead to bugs for various URIs, ranging from data:
and javascript: to jar: (and we don't really want to break signed jars any more
than we have to, please).  Especially troubling, to me, are the cases where the
incorrectly-created URI object is then used in security checks.  This could
cause real issues in both directions (checks failing when they should succeed,
and the other way around).

The URL in the url field shows a number of hits for the contractid in
browser/base/content/browser.js, browser/base/content/contentAreaUtils.js,
browser/base/content/metaData.js, etc; some spot-testing shows that every single
one I checked on should be using newURI or newFileURI on the nsIIOService instead.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Bug 278773 is the equivalent toolkit bug.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Requesting blocking status, since these incorrectly constructed objects are used
for security checks...

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Attachment: Patch

Changes most of the bad calls.

I'm not sure when these calls could fail.

Comment 4

[Boris Zbarsky [:bzbarsky]]

newURI can fail any time (OOM, if nothing else, but also some particularly bogus
http URIs will cause it to throw, iirc).  Of course those caused the SetSpec
calls that used to be here to throw too.

Comment 5

[Daniel Veditz [:dveditz]]

The problem wasn't that setting the spec on a standard URL would fail, it was
that not all URLs are correctly handled by nsStandardURL. One old problem, for
example, was cleverly-formatted javascript: urls interpreted as having a "host"
and using that to slip past same-origin checks.

The IOService will make sure we use the correct nsIURI implementation for each
protocol.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 175259 [details] [diff] [review]
Patch

browser/base/content/browser.js
@@ -2711,19 +2714,21 @@ function BrowserToolboxCustomizeDone(aTo

   var url = getWebNavigation().currentURI.spec;
   if (gURLBar) {
     gURLBar.value = url;
-    var uri = Components.classes["@mozilla.org/network/standard-url;1"]
-			 .createInstance(Components.interfaces.nsIURI);
-    uri.spec = url;
+
+    var uri = Components.classes["@mozilla.org/network/io-service;1"]
+			 .getService(Components.interfaces.nsIIOService)
+			 .newURI(url, null, null);

errr... why not just reuse getWebNavigation().currentURI here? use .clone() if
needed.

this construction of URIs without an origin charset isn't so great... it seems
that in most cases you have one (document.characterSet, with the right
document)

+			   .newURI(this.imageURL, null, null);

this could get an nsIURI from the nsIImageLoadingContent instead...

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #6)
> @@ -2711,19 +2714,21 @@ function BrowserToolboxCustomizeDone(aTo
> errr... why not just reuse getWebNavigation().currentURI here? use .clone() if
> needed.

Turns out that one can be removed, see bug 283522.

> this construction of URIs without an origin charset isn't so great... it seems
> that in most cases you have one (document.characterSet, with the right
> document)

> +			   .newURI(this.imageURL, null, null);
> 
> this could get an nsIURI from the nsIImageLoadingContent instead...

Ok, new patch coming up.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Attachment: Patch v2

Comment 9

[Darin Fisher]

Comment on attachment 175496 [details] [diff] [review]
Patch v2

sr=darin FWIW

Comment 10

[Asaf Romano (gone)]

Checking in base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.393; previous revision: 1.392
done
Checking in base/content/contentAreaUtils.js;
/cvsroot/mozilla/browser/base/content/contentAreaUtils.js,v  <-- 
contentAreaUtils.js
new revision: 1.63; previous revision: 1.62
done
Checking in base/content/pageReport.js;
/cvsroot/mozilla/browser/base/content/pageReport.js,v  <--  pageReport.js
new revision: 1.13; previous revision: 1.12
done
Checking in base/content/search.xml;
/cvsroot/mozilla/browser/base/content/search.xml,v  <--  search.xml
new revision: 1.23; previous revision: 1.22
done
Checking in components/history/content/history.js;
/cvsroot/mozilla/browser/components/history/content/history.js,v  <--  history.js
new revision: 1.45; previous revision: 1.44
done