335334 - Fix string URI consumers to use CheckLoadURIStr

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

At least the following consumers in Firefox code use CheckLoadURI for strings, which we've discovered (in bug 334341) is unsafe:

nsFeedLoadListener::IsLinkValid
nsLivemarkLoadListener::IsLinkValid
<method name="onLinkAdded"> in tabbrowser.xml
<method name="open"> in text.xml
onDrop function in browser.js (two different onDrop functions, actually)

These should probably be switched to CheckLoadURIStr or something... And other consumers of CheckLoadURI should be checked over.

Comment 1

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

"open" in text.xml is only used as a safeguard against accidental data:/javascript: loads (kind of a misuse of checkLoadURI, I guess), so it isn't really "vulnerable" to anything.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Er... why does it care about javascript: or data:?  Does it care about jar:data:?  Or view-source:javascript:?

It does sound like the wrong check is being done here; could you file a separate bug on that with a description of what this code is trying to accomplish so we can figure out what check _should_ be happening?

Comment 3

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

(In reply to comment #2)
> Er... why does it care about javascript: or data:?  Does it care about
> jar:data:?  Or view-source:javascript:?

It cares about javascript: or data: because they are "dangerous" if loaded unknowingly. It's just a safeguard against situations like bug 315004. See bug 315166.

The fact that jar:data: or view-source:javascript: are blocked doesn't really matter: people who are using that binding can just use an onclick handler if they want to load a URI like that. The binding is meant to be used to load http[s]/ftp/etc links.

Comment 4

[Daniel Veditz [:dveditz]]

I'd really want this one, but we're running out of time in 1.8.0.4 (formerly .0.3)

Comment 5

[Alexander Sack]

Attachment: for ffox 1.5.0.3: browser.js, tabbrowser.xml and nsBookmarksFeedHandler.cpp

+ open in text.xml has no CheckLoadURI in ffox 1.5.0.3
 + nsLivemarkListener does to exist in ffox 1.5.0.3

Have no source tree for 1_8_BRANCH right now. If patch is ok I can do patches for other branches as well.

BTW, is there a way to test this on linux or is it a non-issue on unix archs?

Comment 6

[Alexander Sack]

Attachment: 2nd for ffox 1.5.0.3: browser.js, tabbrowser.xml and nsBookmarksFeedHandler.cpp

last one was an accident.

Comment 7

[Jay Patel [:jay]]

Missed the 1.8.0.5 train, we should look in this for the next release.

Comment 8

[Mike Connor [:mconnor]]

Need this for 1.8.0.6 and Fx2.

Dveditz, if you don't have the cycles, dump this to nobody@ and I'll find someone to fix this.

Comment 9

[Mike Schroepfer]

Gavin or Biesi do you have any cycles for this?

Comment 10

[Mike Schroepfer]

Resetting milestone to final.

Comment 11

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

Attachment: testcase

Comment 12

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

Attachment: patch

I've audited all of the checkLoadURI callers in /browser and /toolkit, and these were the only ones where the checked URIs where then passed on to a method that may fix up the URI. This makes urlSecurityCheck more flexible by taking an optional flags param, and uses it instead of explicit calls to checkLoadURI(Str). In the viewImage case, I decided to always pass DISALLOW_SCRIPT to match the current behavior (there's a bug in the code, this.docURL != gBrowser.currentURI is never false, since one is a URL string and the other an nsIURI).

Comment 13

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

The patch fixes the two tests in the attached testcase.

Comment 14

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

Attachment: real patch

Forgot a file in the diff.

Comment 15

[Mike Connor [:mconnor]]

Comment on attachment 234264 [details] [diff] [review]
real patch

good work, and nice catch on the viewImage bug!

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 234264 [details] [diff] [review]
real patch

Looks good for branch.  For trunk, I'd like us to use checkLoadURIStrWithPrincipal, since checkLoadURIStr will generally give the wrong answer (hopefully always too restrictive?), but I'm OK with a followup for that.

Comment 17

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

(In reply to comment #16)
> (From update of attachment 234264 [details] [diff] [review] [edit])
> Looks good for branch.  For trunk, I'd like us to use
> checkLoadURIStrWithPrincipal, since checkLoadURIStr will generally give the
> wrong answer (hopefully always too restrictive?), but I'm OK with a followup
> for that.

Checked this patch in on the trunk to bake. Does bug 327244 sufficiently cover the things you want to address on the trunk, or should I file another bug that depends on bug 348559?

mozilla/toolkit/content/contentAreaUtils.js 	1.85
mozilla/browser/base/content/browser.js 	1.688

Comment 18

[Boris Zbarsky [:bzbarsky]]

Bug 327244 is a tracking bug at the moment; we should probably have separate bugs for the code changes to stop using those functions.  I forgot that bug 348559 hasn't landed yet; a bug depending on it and blocking bug 327244 is a good idea.

Thanks!

Comment 19

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

(In reply to comment #18)
> Bug 327244 is a tracking bug at the moment; we should probably have separate
> bugs for the code changes to stop using those functions.  I forgot that bug
> 348559 hasn't landed yet; a bug depending on it and blocking bug 327244 is a
> good idea.

I talked to bz on IRC, and we decided that bug 342485 can be that bug.

Comment 20

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

Attachment: 1.8 branch patch

This includes the trivial fix for bug 349757, which the original patch caused. It's been on the trunk since Monday, Aug 21st.

Comment 21

[Mike Schroepfer]

Comment on attachment 235025 [details] [diff] [review]
1.8 branch patch

a=schrep for drivers - approving all [181approval pending] bugs now that tree is open.

Comment 22

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

Comment on attachment 235025 [details] [diff] [review]
1.8 branch patch

Sorry, I misunderstood the use of the [181approval pending] s/w marking.

Comment 23

[Mike Schroepfer]

Comment on attachment 235025 [details] [diff] [review]
1.8 branch patch

a=schrep for drivers for real this time :-).  Thanks  Gavin for resetting the approval request.

Comment 24

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

mozilla/browser/base/content/browser.js 	1.479.2.190
mozilla/toolkit/content/contentAreaUtils.js 	1.77.2.6

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 235025 [details] [diff] [review]
1.8 branch patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 26

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

Attachment: 1.8.0 branch patch

Comment 27

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

mozilla/browser/base/content/browser.js 	1.479.2.43.2.4
mozilla/toolkit/content/contentAreaUtils.js 	1.77.2.2.2.3

Comment 28

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.7) Gecko/20060906 Firefox/1.5.0.7