354316 - adding news feed reader causes trouble

Status: RESOLVED FIXED

(Firefox :: Security, defect, P2)

Description

[georgi - hopefully not receiving bugspam]

adding news feed reader causes trouble

navigator.registerContentHandler("application/vnd.mozilla.maybe.feed",
"javascript:try { f00C }

causes chrome execution if the feed reader is accepted and later used for
reading.

not sure if 2.0 is safe - the out of the box exploit doesn't work, but the js
url is visible in about:config.

Comment 1

[georgi - hopefully not receiving bugspam]

Attachment: the best feed reader in the world

Comment 2

[Daniel Veditz [:dveditz]]

There is absolutely zero sanity checking of any arguments passed in from a potentially hostile web page. Content types are limited to a predefined set of "feed" types so that effectively cleanses that parameter for now, but I'd still be happier seeing an incoming check
    if (contentType.match(/[^-a-z0-9./]/i) )
         return; // reject if not alphnum, '-', '/' or '.'

so we're covered if the feed restriction is ever lifted.

Similarly we must do a whitelist check of /^https?:\/\//i for the URI. This is a --WEB-- application feature, the provider must be on the --WEB-- not some other protocol.

Similarly for the title. These things are stored in prefs, shown on dialogs, etc. and can screw us all over the place if not sanitized

Comment 3

[Mike Schroepfer]

This definitely blocks FF2/RC2.  If we can't bullet-proof the registerContentHandler from unsafe content we should disable it...

Comment 4

[Asaf Romano (gone)]

Taking.

Comment 5

[Daniel Veditz [:dveditz]]

The spec also says one of the few thrown exceptions from registerContentHandler is if the URI does not contain "%s" -- we're not checking that that exists. (This is not a security consideration though, filed bug 354366 so it can be considered separately.)

The spec also has rules about converting the outgoing replacement URI to punycode, utf-8, and then escaping non-alphanumeric characters. I don't see where that's happening either.

Comment 6

[Daniel Veditz [:dveditz]]

The javascript feed gets added, and the user can choose it as the default, and the javascript URI will then get run for feeds. It does not, however, have chrome privileges in FF2 -- the attempt to access Components.classes throws.

Comment 7

[Asaf Romano (gone)]

Removing [e-mail] from the CC list... ;)

Comment 8

[Asaf Romano (gone)]

Attachment: patch

Comment 9

[georgi - hopefully not receiving bugspam]

(In reply to comment #5)
> is if the URI does not contain "%s" -- we're not checking that that exists.
'%s' is present in the exploit - it is in a js comment near the end of the js url.

Comment 10

[georgi - hopefully not receiving bugspam]

(In reply to comment #6)
> It does not, however, have
> chrome privileges in FF2 -- the attempt to access Components.classes throws.
> 

couldn't execute it in ff2 - had a heavy error about not being able to execute 'javascript:/favico' or something like this. and it screw the whole feeds reading process persistent over restart.

Comment 11

[Mike Connor [:mconnor]]

Comment on attachment 240203 [details] [diff] [review]
patch

ok, r=me, please add a comment indicating that any changes to this should be vetted for security

also, please add a comment about the need to filter better on the contentType if and when we do more than just feeds

Comment 12

[Mike Schroepfer]

Comment on attachment 240203 [details] [diff] [review]
patch

If this is the fix lets get it in the branch asap.  Approved for RC2.

Comment 13

[Asaf Romano (gone)]

1.8: mozilla/browser/components/feeds/src/WebContentConverter.js 1.1.2.16

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 240203 [details] [diff] [review]
patch

This should do the trick. filed bug 354443 on the type/title sanitization.

I wonder if the try/catch around makeURI should be removed. If we're throwing on one type of bad URI then why not on another?

Comment 15

[georgi - hopefully not receiving bugspam]

(In reply to comment #13)
> 1.8: mozilla/browser/components/feeds/src/WebContentConverter.js 1.1.2.16
> 

is this supposed to be fixed?
just pulled and build trunk. the patch isn't there.

Comment 16

[Asaf Romano (gone)]

This is only fixed on branch for now.

Comment 17

[Asaf Romano (gone)]

Fixed on trunk too (see bug 349380).