Closed Bug 901894 Opened 11 years ago Closed 7 years ago

Centralize HTML sanitization Code

Categories

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

defect
Not set
normal

Tracking

(blocking-b2g:-)

RESOLVED INCOMPLETE
blocking-b2g -

People

(Reporter: freddy, Unassigned)

References

Details

(Keywords: sec-moderate, wsec-xss)

I've come to notice that there are stray bits and pieces of HTML sanitization code, e.g. in the mail parser: ./js/ext/mailparser/mailparser.js-1539-MailParser.prototype._joinHTMLAttachment = function(htmlNode, attachment){ ./js/ext/mailparser/mailparser.js-1540- var inserted = false, ./js/ext/mailparser/mailparser.js-1541- fname = attachment.generatedFileName.replace(/</g, "&lt;").replace(/>/g, "&gt;").replace(/"/g, "&quot;"), ./js/ext/mailparser/mailparser.js-1542- cid, newHTML; ./js/ext/mailparser/mailparser.js-1543- ./js/ext/mailparser/mailparser.js:1544: cid = attachment.cid || (attachment.cid = attachment.generatedFileName+"@node"); ./js/ext/mailparser/mailparser.js-1545- newHTML = "\n<div class=\"mailparser-attachment\"><a href=\"cid:"+cid+"\">&lt;" + fname + "&gt;</a></div>"; While it might be convenient to add a .replace().replace (line 1541), it's still incorrect: Escaping depends highly on the context this is being used in and you should have a centralized set of escape methods to be used in handling user input (e.g. attachment file names and CIDs). I suggest escapeforHTML, escapeforAttribute, escapeforURL. I am setting this bug to security-sensitivealthough I could find no method to create an email that contains a malicious cid. It might just be me and the vulnerability is definitely there.. A cid that injects the class name "card" to the a object would make it use up the whole screen and hard to miss when touching the screen.
mailparser is an upstream lib that is currently only used for header parsing. The logic in question is not used because we get our structural information directly from the IMAP server's BODYSTRUCTURE results. We will, however, start to need mailparser's full functionality for POP3, although that might be the time for us to make a transition to Thunderbird's new JSMIME parser since that reduces Mozilla's net security surface area and results in various synergies as long as Thunderbird is a going concern. I agree that the HTML handling there could be cleaned up. More significantly, I don't think we want any of the HTML touching that mailparser is doing. Most of that code is conflating display logic with parsing logic. (The charset logic might be okay? I would need to look into that more.) Please note that all the code under js/ext/ is actually from the back-end repo https://github.com/mozilla-b2g/gaia-email-libs-and-more and that most of the bits there get concatenated. While it can be handy to grep in the 'ext' sub-tree because it does rule our files that are totally unused (our build step traces actually used dependencies at a file granularity), the line numbers are unreliable, etc.
blocking-
blocking-b2g: koi? → -
Triage: This bug has been added to the v1.2 backlog but is not blocking release
Group: core-security
Group: b2g-core-security
Group: core-security → b2g-core-security
FirefoxOS is no longer under active development.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INCOMPLETE
Group: b2g-core-security
You need to log in before you can comment on or make changes to this bug.