Closed Bug 440812 Opened 16 years ago Closed 14 years ago

Remote code execution in Sage extension

Categories

(addons.mozilla.org :: Security, defect, P2)

Product:
addons.mozilla.org
Component:
Security
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: jwkbugzilla, Assigned: jorgev)

References

(
URL
)

Details

Attachments

(4 files, 1 obsolete file)

RSS feed for testing
326 bytes, application/xml
Details
Second RSS feed for testing
977 bytes, application/xml
Details
Third RSS feed for testing
238 bytes, application/xml
Details
Fourth RSS feed, with correct content-type
535 bytes, application/xml
Details
Attached file RSS feed for testing — 
Sage 1.4.1 will display RSS feeds in the browser by opening the following URL:

chrome://sage/content/feedsummary.html?uri=http://...

What happens here is the following: feedsummary.html will download the feed, parse it, generate HTML code for it and insert it as its own body. So the feed's HTML code is inserted into chrome context.

Of course there is an attempt to sanitize the HTML content, this on is extremely insufficient however. Instead of using Gecko's parser, Sage will its own simplehtmlparser.js that uses regular expressions and accepts just about anything as tag or attribute name. filterhtmlhandler.js will then apply some (blacklist) filters on those tag and attribute names.

I immediately saw two ways to fool the filters. One is supplying an attribute name that Sage will accept but Gecko won't:

<div bla"onmouseover="alert('xss')">

Sage will treat bla"onmouseover as attribute name and accept it (it doesn't start with "on"). Gecko however will make <div bla="" onmouseover="alert('xss')"> out of it. A simpler one:

<a href=" javascript:alert('xss')">

Sage only looks for "javascript:" at the beginning of the URL, Gecko will ignore whitespace however.

I am sure that there are far more ways to inject JavaScript here. As the attached testcase demonstrates, this JavaScript runs with chrome privileges - opening that RSS feed in Sage will display the value of Components.classes without throwing a security exception.

Sage should revert to displaying RSS feeds in unprivileged context, there is no way around that. And it should use proper sanitizing code of course.
Dan/Brandon, can you please advise as to the right course of action?

Sage has about 350k active users. Sage-Too (https://addons.mozilla.org/en-US/firefox/addon/7263) should probably be checked also to see if it has a similar vulnerability. It has about 30k active users.


From what I have been told, Sage-too uses file:/// URLs to display feeds which would mean that it isn't affected.
We will be rewriting the sanitizer to use the Gecko parser.  This will allow a  much more thorough sanitization.

This is not a new vulnerability.
The sanitizer isn't really the problem. Even good sanitizers might miss something. But this HTML code shouldn't be displayed in chrome context in the first place - then any issues of your sanitizers will really stay issues of your sanitizer. Simplest way to get there should be calling location.replace("data:text/html," + encodeURIComponent(htmlCode)) - this will display the HTML code but give it zero privileges because the URL of the document is a data: URL. Note that this should only be done for the top-level document, data: URLs loaded into frames will inherit the security context of their parent.
I think that it is possible to sufficiently sanitize the content for display in a privileged context. The possibilities it opens for the user experience are more than worth the effort as well as any remaining marginal security risk.
Made some progress on content sanitization over the weekend.  These holes in feed rendering should be closed now in our development builds:

http://downloads.mozdev.org/sage/sage_nightly.xpi
Peter, is there any update on this?
I'm halfway through a rewrite of the feed rendering code.  Hope to get a new release out and close this bug before the end of the year.
Component: Add-ons → Administration
QA Contact: add-ons → administration
Peter, what's the status of this?
I'll be releasing a FF 3.5 compatible version of Sage shortly.  I should be able to plug this hole while I'm at it.
Component: Administration → Add-on Security
QA Contact: administration → security
I see a new Sage up, is this fixed now?
Assignee: nobody → jorge
Priority: -- → P2
Target Milestone: --- → 5.4
The latest version of the add-on is still vulnerable. After reviewing these comments and the extension code, it is clear to me that the current effort to secure the add-on is misguided. You can't just rely on 'sanitizing' input and expect it to be "safe enough", specially when there are clearly safer alternatives.

I have sandboxed all versions of Sage except the latest one.

Peter, you have 2 weeks to correct your extension code to make it safe. You
should upload the update on AMO and let us know so that we can review it. If
you don't comply by then, your latest version will be sandboxed as well.
Since the vulnerability has become public (http://www.net-security.org/secworld.php?id=8527), I have sandboxed the latest version of the add-on. I'll close this in a week if there is no response from the author.
Jorge, I'm working on a solution, and I'll update this bug when it's ready.  Be aware that this vulnerability is specific to versions 1.4 and after, there should be no need to sandbox previous versions.
Why should this extension be allowed on a.m.o. at all? The "Let me install this experimental add-on" checkbox gives no indication there is a publicized security problem. Sorry to advocate, but I am mystified. Same comments here: http://forums.mozillazine.org/viewtopic.php?f=9&t=1603515
-> 5.5
The add-on is currently sandboxed, so it's not that visible to be a major threat. Still waiting for an update from Peter.
Target Milestone: 5.4 → 5.5
While sandboxing prevents some new installations, this doesn't help people already using the extension...
Yes, but I don't think blocklisting is a good idea either. The extension still has many users, and there's no evidence of malicious exploits in the wild.
Yes, having it fixed would be the ideal solution of course. Peter, can't you just replace these lines:

>						this.filterHtmlHandler.clear();
>						this.simpleHtmlParser.parse(item.getContent());
>						ds = this.filterHtmlHandler.toString();

By:

>            var sanitizer = Cc["@mozilla.org/feed-unescapehtml;1"].getService(Ci.nsIScriptableUnescapeHTML);
>            var fragment = sanitizer.parseFragment(item.getContent(), false, null, document.documentElement);
>            ds = new XMLSerializer().serializeToString(fragment);

This isn't perfect (parameters to parseFragment aren't quite correct) but it will work well enough. And then there is this line:

>            ds = SageUtils.htmlToText(item.getContent());

The result of htmlToText may be text but that doesn't mean that it can be used as HTML code safely. For example, item.getContent() might be "&lt;script&gt;alert('XSS')&lt;/script&gt;". SageUtils.htmlToText() will convert it into "<script>alert('XSS')</script>" and we have a problem. This is easily fixed by adding one line:

>            ds = ds.replace(/&/g, '&amp;').replace(/</g, '&lt;').replace(/>/g, '&gt;');

While I don't think that these changes are sufficient they should do to resolve the current situation. You can release that as Sage 1.4.4 and go back to making Sage 1.5 perfect.
Peter, can you please give us an update on this bug?
Target Milestone: 5.5 → 5.6
I've disabled the add-on due to a lack of response from the author. I'll consider blocklisting in the next cycle, given that the add-on has a substantial user base (137K active daily users).
Severity: critical → normal
Target Milestone: 5.6 → 5.7
Jorge: I'm planning on uploading a new version to AMO this evening.  It should address the security concerns.
Send it to me directly. I'll review it and enable your listing again.
Version 1.4.4 is now up on AMO: https://addons.mozilla.org/en-US/firefox/addons/versions/77#version-1.4.4

I performed a review on it, and it looks safe to me. This version implements a custom RSS parser, and it looks like it does its job well.

Wladimir, can you please have a look too?
The RSS parser doesn't worry me - worst case scenario is that it misinterprets the contents of the RSS feed. The critical part is replaceFeedItemKeyword function that returns HTML code to be inserted into the template. There is a number of different code paths there:

* Feed data passed through entityEncode() - safe.
* Feed data passed through nsIScriptableUnescapeHTML and then XMLSerializer - safe.
* Returning result of SageUtils.htmlToText() - vulnerable as noted in comment 20 already, an additional call to entityEncode() is be necessary.
* Returning result of item.getLink()/enc.getLink() - vulnerable if the feed has no <link> tag (in the other scenario the link is passed through nsIURIFixup which has the side-effect of converting dangerous characters), an additional call to entityEncode() is necessary.
* Returning result of formatter.formatDate() - safe, date from the feed is generally converted to its numeric representation first. An additional entityEncode() call would be advisable nevertheless, just so an unusual translation for a month name isn't interpreted as HTML code.
* Returning result of enc.getMimeType() - vulnerable, that's just an attribute value from the RSS feed which could be anything. An additional call to entityEncode() is necessary.
* Returning the result of formatFileSize() - vulnerable, if the "length" attribute isn't a number this function will simply concatenate it with the string " B". entityEncode() is one solution, however better pass the length through parseInt() first and ignore it if isNaN() returns true.

In other words, looks like I didn't inspect the code carefully enough before. I'll try to create a proof-of-concert for these vulnerabilities now...
(In reply to comment #26)
> * Returning the result of formatFileSize() - vulnerable, if the "length"
> attribute isn't a number this function will simply concatenate it with the
> string " B". entityEncode() is one solution, however better pass the length
> through parseInt() first and ignore it if isNaN() returns true.

This one is actually not exploitable - the length value is passed through an XPCOM interface that takes numbers, it gets implicitly converted there. Everything else is exploitable however. Add to this issues in replaceFeedKeyword function which I didn't notice at first:

* Returning feed.getLink() - vulnerable, similar to item.getLink() with the exception that the link is never passed through nsIURIFixup. Calling entityEncode() helps here.
* Returning feed.getDescription() - vulnerable, no sanitizing whatsoever here. This should get the same treatment as item description, in particular it should probably be converted to text if the user disallowed HTML...
Attaching new RSS feed for testing. It injects HTML through five different places in the feed, resulting in six alert messages. One alert will only show up if the option "Allow HTML tags" is switched off in Sage settings. I'll attach a separate feed to demonstrate the hole in feed link processing - if the feed has a link it will disable the vulnerabilities related to item and enclosure links.
That one now demonstrates the vulnerability in feed link processing only.
To test these feed without subscribing to them in Sage you can open the following addresses:

chrome://sage/content/feedsummary.html?uri=https://bugzilla.mozilla.org/attachment.cgi?id%3D425433
chrome://sage/content/feedsummary.html?uri=https://bugzilla.mozilla.org/attachment.cgi?id%3D425434
Thank you, Wladimir. Peter, you'll need to fix all of these vulnerable spots before your add-on can be made public again. Remember that *no* unprocessed HTML data should be inserted directly into XUL.
(In reply to comment #26)
> (in the other scenario the link is passed through nsIURIFixup
> which has the side-effect of converting dangerous characters)

Actually, this isn't quite correct - whether dangerous characters will be escaped depends on the protocol. I just verified that they won't be escaped for the mailto: protocol for example. So encoding the entities is still the best protection.
Thanks for the analysis Wladimir, I'll do another release to address these holes.
I've uploaded Sage 1.4.5 to AMO for review.  This should address the most recent security concerns.
Thank you. Looks good, from what I can tell all output is properly encoded. I would still recommend adding another layer of protection in a future version so that an issue like this can no longer happen. Unfortunately, the way your UI is built you cannot take advantage of the chrome-content boundary. Still, you can put an unprivileged iframe into feedsummary.html (file:/// protocol?) where you write the data into. Note that you cannot use data: or about:blank for this frame, those will inherit privileges from their parent document. Then you can restrict the frame as much as possible (https://developer.mozilla.org/En/Displaying_web_content_in_an_extension_without_security_issues#Restricting_what_the_document_containing_untrusted_data_can_do), all necessary JavaScript code should stay in feedsummary.html. If you need event handlers on some elements in the iframe, those can be attached from feedsummary.html as well. The iframe should really not be able to run JavaScript or actually do anything at all (other than displaying images maybe).
To explain my point about using a file:/// URL: you can have an empty HTML file under chrome://sage/content/dummy.html. You then resolve it using the chrome registry:

> var ioService = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);
> var chromeURI = ioService.newURI("chrome://sage/content/dummy.html", null, null);
> var registry = Cc["@mozilla.org/chrome/chrome-registry;1"].getService(Ci.nsIChromeRegistry);
> var unprivileged = registry.convertChromeURL(chromeURI).spec;

This will give you something like jar:file:///.../sage.jar!/content/dummy.html which you can set as location of the unprivileged frame. This location will no longer have chrome privileges even though it is located in your extension's directory.
Marking this as fixed, since we've finally produced a reasonably safe add-on for public consumption. Peter, please take Wladimir's advice (and the notes I sent you with the approval) for your next version.

Thank you all for your work in resolving this.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Peter, could you add release notes for this version? The release notes will show up when people get an update notification and click "Show information" - mentioning that a security issue was fixed might result in more updates.
Btw, there is already a comment claiming that Sage 1.4.5 is still vulnerable linking to http://himag.blog26.fc2.com/blog-entry-325.html?lang=en. I tried to leave a comment there but a Japanese CAPTCHA isn't only very effective against bots but against non-Japanese as well.

Point 1 is bogus from what I can tell. nsIScriptableUnescapeHTML.parseFragment removes all attributes but a few allowed ones, I just verified that in Firefox 3.0/3.5/3.6/3.7a2pre to be sure.

Point 2 is valid but minor - no URL validation in place. Still, given that Sage will even accept javascript: URLs and clicking them will execute code in chrome context this is still an issue to be solved ASAP. I think that all links should be passed to nsIIOService.newURI(). Any that trigger exceptions (malformed URLs) should be ignored. Same with any links where nsIURI.scheme is not "http" or "https" (is there any other scheme that should be allowed?). For the rest of them nsIURI.spec should be taken and used, in many cases it will not be identical with the original URL (some characters escaped, ports normalized etc).

Point 3 is about an additional layer of defense - what we already mentioned above. The solution proposed in this blog post will not work however, I hope Sage-Too or Sage++ aren't using it in the assumption that it will give them any protection. For a document that is already in the content area of the browser declaring a frame as content will have no effect whatsoever.
I'm already looking at the malicious URI issue.  I plan to use the ScriptSecurityManger to weed out individual URIs that inherit security context and I'll probably have to use a node filter to sanitize the document fragments since the ParanoidFragmentSinks give those URIs a pass when they're from a chrome document.
Any reason to concern ScriptSecurityManager with this? Why would feed/item links be anything other than http and https?

As to the sanitizing issue - looks like you are right, links aren't sanitized properly if you give the sanitizer a chrome document. But that should go away once you start using a file:/// URL for the feed content? I would certainly recommend against a node filter, getting it right is everything but trivial.
Attached file Forth RSS feed for testing (obsolete) — 
One more RSS feed for testing - clicking any of the links in the feed summary will currently execute JavaScript with chrome privileges. That's the feed link, item link, enclosure link and a link in the item description.
Peter, what's the status of this fix? The exploitability of this version is getting lots of attention, and I may need to sandbox it.
Jorge: I'm within a few days of a release that should take care of the remaining exploits.
Jorge: I've just uploaded Sage 1.4.6 to AMO for review.  This should deal with all the security issues surfaced so far.
Comment on attachment 425967 [details]
Forth RSS feed for testing

Trying to change content type in attachment #4 [details] [diff] [review]
Uploading again, to allow easy testing.
Attachment #425967 - Attachment is obsolete: true
Attachment #425967 - Attachment is patch: false
Attachment #425967 - Attachment mime type: text/plain → application/xml
Version 1.4.6 looks good, especially the use of DOM methods instead of innerHTML. One issue still - if somebody bookmarks mailto:test@example.com?"><script>alert("XSS")</script> and tries to open it via Sage, the URL chrome://sage/content/feedsummary.html?uri=mailto%3Atest%40example.com%3F%22%3E%3Cscript%3Ealert%28%22XSS%22%29%3C%2Fscript%3E will open and show an alert message. Once again an XSS allowing to execute code with chrome privileges. This line is responsible:

> document.write("<base href=\"" + feedSummary.uri + "\">");

You should escape HTML entities here...
Note that you can pass the base URL as third parameter to nsIIOService.newURI() in setURIAttributeSafe method - if you then set the attribute to feedURI.spec rather than the original value this should eliminate the need for a <base> tag.
Heh, the above is non-sense - I overlooked that feedURI isn't the same URL. Anyway, here is how I would change that function:

>	setURIAttributeSafe: function(element, attribute, uri) {
>		var secman = Cc["@mozilla.org/scriptsecuritymanager;1"].getService(Ci.nsIScriptSecurityManager);
>		var ios = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);
>
>		const flags = Ci.nsIScriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL;
>		try {
>			var feedURI = ios.newURI(this.uri, null, null);
>			var attrURI = ios.newURI(uri, null, feedURI);
>			secman.checkLoadURI(feedURI, attrURI, flags);
>
>			// If we got here then URI is valid and allowed to be loaded
>			element.setAttribute(attribute, attrURI.spec);
>		} catch (e) {
>			return;
>		}
>	},

Note that this code ignores the attribute if feedURI is null - this is a case that should never happen but better ignore the attribute than to accidentally have checks run against system principal.
Wladimir: Thanks for the feedback.  I'll be doing another release shortly.
I think that this should be public by now.
Flags: needinfo?(jorge)
Group: client-services-security
Flags: needinfo?(jorge)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: