adding news feed reader causes trouble

RESOLVED FIXED in Firefox 2

Status

()

defect
P2
normal
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: guninski, Assigned: mano)

Tracking

({fixed1.8.1})

Trunk
Firefox 2
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate] post ff1.5)

Attachments

(2 attachments)

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.
Flags: blocking-firefox2?
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
Flags: blocking-firefox2? → blocking-firefox2+
This definitely blocks FF2/RC2.  If we can't bullet-proof the registerContentHandler from unsafe content we should disable it...
Taking.
Assignee: nobody → mano
Target Milestone: --- → Firefox 2
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.
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.
Whiteboard: [sg:moderate]
Target Milestone: Firefox 2 → ---
Status: NEW → ASSIGNED
OS: Linux → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → Firefox 2
(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.
(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 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
Attachment #240203 - Flags: review?(mconnor) → review+
Comment on attachment 240203 [details] [diff] [review]
patch

If this is the fix lets get it in the branch asap.  Approved for RC2.
Attachment #240203 - Flags: approval1.8.1+
1.8: mozilla/browser/components/feeds/src/WebContentConverter.js 1.1.2.16
Keywords: fixed1.8.1
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?
Attachment #240203 - Flags: review+
(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.
This is only fixed on branch for now.
Whiteboard: [sg:moderate] → [sg:moderate] post ff1.5
Fixed on trunk too (see bug 349380).
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Group: security
You need to log in before you can comment on or make changes to this bug.