sage-extension (1.4.3-5) new_xss_fix.patch

Summary

 chrome/sage.jar!/content/createhtml.js        |   69 ++++++++++++++++++++++----
 chrome/sage.jar!/content/filterhtmlhandler.js |    2 
 2 files changed, 60 insertions(+), 11 deletions(-)

    
download this patch

Patch contents

Description: Fix RSS Feeds Cross Domain Scripting Vulnerability
 CVE-2009-4102 Cross Domain Scripting vulnerability. Don't trust HTML in titles,
 descriptions. Don't allow 'strange' (i.e. javascript:, data:) URLs in Links.
 CVE-2006-4712 (Regression), some of the old test cases no longer passed due to
 problem with htmlToText.
Bug-Debian: http://bugs.debian.org/559267
Author: Alan Woodland <awoodland@debian.org>

--- sage-1.4.3.orig/chrome/sage.jar!/content/createhtml.js
+++ sage-1.4.3/chrome/sage.jar!/content/createhtml.js
@@ -133,29 +133,37 @@ var CreateHTML = {
 
 		switch (s) {
 			case "**TITLE**":
-				return this.entityEncode(feed.getTitle());
+                                // Entity encode is correct here - we shouldn't let any HTML through
+				return this.entityEncode(SageUtils.htmlToText(feed.getTitle()));
 
 			case "**LINK**":
-				return feed.getLink();
+            // Partial fix for CVE-2009-4102
+                                // Clean href is correct here - there is HTML in what gets returned by getLink, but it's all Sage generated and anything which can break out of it should be escaped
+				return this.cleanHref(feed.getLink());
 				break;
 
 			case "**AUTHOR**":
 				if (feed.hasAuthor()) {
-					return "<div class=\"feed-author\">" + this.entityEncode(feed.getAuthor()) + "</div>";
+                                        // Entity encode is correct - we don't want any HTML back from this
+					return "<div class=\"feed-author\">" + this.entityEncode(SageUtils.htmlToText(feed.getAuthor())) + "</div>";
 				}
 				return "";
 
 			case "**DESCRIPTION**":
 				if (feed.hasDescription()) {
-					return feed.getDescription();
+				 	 // Entity encode call is Partial fix for CVE-2009-4102
+                                         // EntityEncode + htmlToText is too strict really, but not broken. Ideally we should honour the 'display HTML' preference here.
+					 return this.entityEncode(SageUtils.htmlToText(feed.getDescription()));
 				}
 				return "";
 
 /*
 			case "**LOGOLINK**":
+                                // need to be sure we can't escape the href="..." part this gets enclosed in
 				return feed.getLogo().link;
 
 			case "**LOGOALT**":
+                                // need to be sure we can't escape the alt="..."
 				return feed.getLogo().alt;
 
 			case "**COPYRIGHT**":
@@ -185,6 +193,7 @@ var CreateHTML = {
 				return "";
 */
 			case "**ITEMS**":
+                                // Correct - getItemsHtml is already escaped/quoted internally
 				return this.getItemsHtml(feed);
 		}
 
@@ -199,6 +208,7 @@ var CreateHTML = {
 		}
 		var sb = [];
 		for (var i = 0; i < feed.getItemCount(); i++) {
+                        // Correct - already quoted/escaped
 			sb.push(this.getItemHtml(feed, feed.getItem(i), i));
 		}
 		return sb.join("");
@@ -216,20 +226,26 @@ var CreateHTML = {
 				return i  +1;
 
 			case "**LINK**":
-				return item.getLink();
+			   // Partial fix for CVE-2009-4102
+                           // Correct - be careful of breaking out of the href="..." though
+			   return this.cleanHref(item.getLink());
 
 			case "**TITLE**":
 				if (item.hasTitle()) {
-					return this.entityEncode(item.getTitle());
+                                        // correct - this doesn't let any HTML through
+					return this.entityEncode(SageUtils.htmlToText(item.getTitle()));
 				} else if (item.getTitle()) {
-					return this.entityEncode(item.getTitle());
+                                        // correct - no HTML through
+					return this.entityEncode(SageUtils.htmlToText(item.getTitle()));
 				} else {
+                                        // No HTML here eitther, but it's not input anyway
 					return this.entityEncode(strRes.GetStringFromName("feed_item_no_title"));
 				}
 
 			case "**AUTHOR**":
 				if (item.hasAuthor()) {
-					return "<div class=\"item-author\">" + this.entityEncode(item.getAuthor()) + "</div>";
+                                        // Correct - no HTML permitted here
+					return "<div class=\"item-author\">" + this.entityEncode(SageUtils.htmlToText(item.getAuthor())) + "</div>";
 				}
 				return "";
 
@@ -239,10 +255,13 @@ var CreateHTML = {
 					var ds;
 					if (allowEContent) {
 						this.filterHtmlHandler.clear();
+                                                // Correct provided simpleHtmlParser works
 						this.simpleHtmlParser.parse(item.getContent());
 						ds = this.filterHtmlHandler.toString();
 					} else {
-						ds = SageUtils.htmlToText(item.getContent());
+						 // Entity encode call is fix for regression from CVE-2006-4712
+                                                 // Correct
+						 ds = this.entityEncode(SageUtils.htmlToText(item.getContent()));
 					}
 					return "<div class=\"item-desc\">" + ds + "</div>";
 				}
@@ -253,6 +272,7 @@ var CreateHTML = {
 					var twelveHourClock = SageUtils.getSagePrefValue(SageUtils.PREF_TWELVE_HOUR_CLOCK);
 					var formatter = Components.classes["@sage.mozdev.org/sage/dateformatter;1"].getService(Components.interfaces.sageIDateFormatter);
 					formatter.setFormat(formatter.FORMAT_LONG, formatter.ABBREVIATED_FALSE, twelveHourClock ? formatter.CLOCK_12HOUR : formatter.CLOCK_24HOUR);
+                                        // Correct provided formatDate isn't buggy
 					var dateString = formatter.formatDate(item.getPubDate());
 					return "<div class=\"item-pubDate\">" + dateString + "</div>";
 				}
@@ -260,6 +280,7 @@ var CreateHTML = {
 
 			case "**ENCLOSURE**":
 				if (item.hasEnclosure()) {
+					// ??
 					var enc = item.getEnclosure();
 					function createDescriptionFromURL(url) {
 						var array = url.split("/");
@@ -270,7 +291,8 @@ var CreateHTML = {
 						return description;
 					}
 					return "<div class=\"item-enclosure\">" +
-						"<a href=\"" + enc.getLink() + "\" title=\"" +
+                                                // correct
+						"<a href=\"" + cleanHref(enc.getLink()) + "\" title=\"" +
 						createDescriptionFromURL(enc.getLink()) +
 						"\"><img src=\"" +
 							(enc.hasMimeType() ?
@@ -291,6 +313,33 @@ var CreateHTML = {
 		return dirService.get(aProp, Components.interfaces.nsILocalFile);
 	},
 	
+	// Partial fix for CVE-2009-4102
+	cleanHref: function(aUrl)
+	{
+		 // We only want to allow http, ftp, news and mailto before :
+		 var ltype = aUrl.split(":")[0];
+                 // Make it greedy so there cannot be any surplus :'s left after filtering
+                 // This was an error in my original patch
+		 aUrl = aUrl.replace(/^.*:/, "");
+	 	 switch(ltype.toLowerCase())
+       {
+		 case "http":
+		    aUrl = ltype + ":" + aUrl;
+   		 break;
+		 case "nntp":
+		    aUrl = ltype + ":" + aUrl;
+   		 break;
+		 case "mailto":
+		    aUrl = ltype + ":" + aUrl;
+   		 break;
+		 case "ftp":
+		    aUrl = ltype + ":" + aUrl;
+   		 break;
+       }
+		 // Did I miss some safe ones?
+		 return aUrl
+	},
+
 	entityEncode: function(aStr)
 	{
 		function replacechar(match) {
--- sage-1.4.3.orig/chrome/sage.jar!/content/filterhtmlhandler.js
+++ sage-1.4.3/chrome/sage.jar!/content/filterhtmlhandler.js
@@ -139,7 +139,7 @@ FilterHtmlHandler.prototype = {
 		} else if (nl.substr(0,2) == "on") {
 			//noop
 		} else if ((nl == "href" || nl == "src" || nl == "data" || nl == "codebase") &&
-				 /^javascript\:/i.test(vl)) {
+				 /^javascript\:/i.test(vl) && /^data\:/i.test(vl)) {
 			//noop
 		} else if (tl == "img" && nl == "src" &&
 							 vl.substring(0, 7) == "mailto:") {