Open Bug 388674 Opened 19 years ago Updated 3 years ago

Need secure text to HTML converter

Categories

(MailNews Core :: Backend, defect)

Product:
MailNews Core
Component:
Backend
Type:
defect

Tracking

(Not tracked)

People

(Reporter: mnyromyr, Unassigned)

Details

We use mozTxtToHTMLConv to convert plain text into glyphs, emoticons and links - but only in the message pane, i.e. in an HTML environment in a kind of sandbox iframe. Albeit mozTxtToHTMLConv seems to be "secure" - it does not create harmful active content - in its current state of implementation, discussion in bug 23114 claims that this was no design goal and might change in future. So, if we want to beautify textual content in chrome, we either need to: (a) lock mozTxtToHTMLConv down to be security-aware, or (b) have a secure variation of it. The latter will definitely lead to duplicated code, without much real gain. FWIW, using a secure converter in a non-secure environment won't do any harm! So, actually, I think we should investigate (a): - thoroughly review mozTxtToHTMLConv wrt to security breaches - require security-aware reviews for future patches
Karsten, as I tried to explain to you in the other bug, it's not an implementation or arbitrary restriction of mozTXTToHTMLConv. I think the whole *idea* is fundamentally risky. You take *untrusted* content from spammers and virii, and create active code (HTML) from it, even as stream, and then you want to run that with system privileges. Sure, you can try to escape and make it safe and all, but one single oversight in one strange edge-case, and you have a remote arbitrary code execution bug (worst kind of bug, can install keyboard sniffer, read diary, do bank transactions etc.). In the message body, we have the protection from Gecko, it's treated like a webpage. If you do what you propose, your code takes the full responsibility for security.
> Karsten, as I tried to explain to you in the other bug, it's not an > implementation or arbitrary restriction of mozTXTToHTMLConv. I think the whole > *idea* is fundamentally risky. I do (and did) understand what you think, I just don't agree. I don't think that mere fear is a valid concept, but I concede that without in-depth review the current implementation cannot be used in such a way. > You take *untrusted* content from spammers and virii, First of all, this bug is not primarily about arbitrary content, "just" about the basic problem of a security-aware text to HTML transposition. If (!) we can create such a method, it should convert _any_ source, though. > and create active code (HTML) from it, No. These are just two different things. Most HTML tags are not active and can be used without risk; the converter won't be parsing arbitrary HTML! Especially glyph or emoticon conversion should be save: you take *known* plain text and map it fixedly to *known* *internal* imagery without interpreting arbitrary content. You can check at source review time if a particular conversion is safe or not! Example: house ==> <img src="chrome://comm/content/hoxxuse.gif" title="house"/> > even as stream, and then you want to run that with system privileges. I don't want to run anything. Creating src attributes (and the like) from random text would be hazardous indeed. In fact, generating _URIs_ is the most risky part here, because it actually does transform arbitrary content directly into something clickable (and that's what you proposed in bug 23114)! > one single oversight in one strange edge-case, and you have a remote > arbitrary code execution bug You're really on the wrong train here. I do consent wholeheartedly with that sentence, it just doesn't apply here! > (worst kind of bug, can install keyboard sniffer, read diary, do > bank transactions etc.). Yeah, not to forget terrorist attacks and general excommunication. :-/ Off-target scare crows. > If you do what you propose, your code takes the full responsibility > for security. Well, yes. That's why it should stick to harmless things like structs, glyphs and (emot)icons, and maybe(!) URLs...
> I don't think that mere fear is a valid concept, Now you're getting personal. "It is risky" != "fear". The statement "it is too risky" is a very valid and important concept and judgment. It is a reason to not implement some features. > > and create active code (HTML) from it, > No. These are just two different things. Most HTML tags are not active and can > be used without risk That's your misunderstanding. It's not just about the HTML that we explicitly create. It's about the source content that slips into the HTML accidently and can then be active. Miss some escaping or anything like that, and you have *arbitrary* HTML in there. The problem is not that *you* insert HTML, the problem is that the result is *used* as HTML, and contains parts of the untrusted content. The main danger is that this stuff is an HTML *stream* with trusted HTML and untrusted content mixed and that it will be parsed as one. See e.g. http://larholm.com/2007/07/10/internet-explorer-0day-exploit/ - different context, but also missing escaping. We though we were safe because we put the URL properly quoted as "%1", but didn't think that URL inserted would contain quotes, due to an MSIE bug. This is just a week old, remote execution bug, just to show how easy and often it happens in practice. > > even as stream, and then you want to run that with system privileges. > I don't want to run anything. When you create HTML, and put that in the system privileged XUL, you "run" the HTML with with system privileges. You may not *want* to have JS or bad links in there, but see above. --- I don't see this bug being necessary. Inserting textnodes and attributes in the DOM is far more secure, because you avoid the whole risk of something slipping through due to escaping etc.. You will not have smileys and *bold* in the headers, but they were just cuteness anyway. You can put the link and link text in via DOM, that's a bit more safe. You still have the problem to verify the URL, though, but at least you cut the largest problem short.
> The statement "it is too risky" is a very valid and important concept and > judgment. It is a reason to not implement some features. No doubt. But it needs proof. > That's your misunderstanding. Maybe. Maybe not. Let's try to resolve it, though. > It's not just about the HTML that we explicitly create. It's about the > source content that slips into the HTML accidently and can then be active. > Miss some escaping or anything like that, and you have *arbitrary* HTML in > there. The problem is not that *you* insert HTML, the problem is that the > result is *used* as HTML, and contains parts of the untrusted content. Mmh, I think one problem we have is the usage of 'HTML', 'source' and 'content'. AFAICS, we have three sorts of stuff here: 1. The plain text source content. 2. The HTML markup text substituted for (1). 3. The actual DOM elements created from (2) by parsing it. I'll use these for refrence in the following. > The main danger is that this stuff is an HTML *stream* with trusted HTML and > untrusted content mixed and that it will be parsed as one. I still fail to see that (see below). Maybe you can give an example fitting to this very bug? > See e.g. http://larholm.com/2007/07/10/internet-explorer-0day-exploit/ - > different context, but also missing escaping. This is off-target, again. > When you create HTML, and put that in the system privileged XUL, you "run" the > HTML with with system privileges. You may not *want* to have JS or bad links > in there, but see above. If I don't put unknown stuff from (1) into (2), how should (3) be unsafe? Again my example from comment #2: (1) = house (2) = <img src="chrome://comm/content/hoxxuse.gif" title="house"/> The 'house' in the title attribute is not the 'house' from (1), but any text we'd like to give to the user because we know we found 'house' in (1)! It could be anything else like 'building', but it makes sense to just show what we found. *We have absolute control about that stuff here.* (3) is not created by parsing anything in (1) Please give an example what will fail here. > I don't see this bug being necessary. Inserting textnodes and attributes in > the DOM is far more secure, because you avoid the whole risk of something > slipping through due to escaping etc.. You keep repeating yourself without proving how that should ever happen. > You still have the problem to verify the URL, though, but at least you cut > the largest problem short. The URI case is the only case I see where the untrusted content from (1) is used to create anything, so IMO it's the *only* risky part here...
> > The statement "it is too risky" is a very valid and important concept and > > judgment. > No doubt. But it needs proof. No, it's a judgment based on experience. I already gave proof. You > > the whole risk of something slipping through due to escaping etc.. > You keep repeating yourself without proving how that should ever happen. I already showed several times. E.g. with http://larholm.com/2007/07/10/internet-explorer-0day-exploit/ (which you chose to ignore) and bug 23114 comment 38. Again, it's not the HTML that you insert intentionally, it's the content which slips through and is unintentionally executed as HTML, and can then do anything it wants. In bug 23114, I also proposed how you can use mozTXTToHTMLConvto detect links and insert them in a way that avoids the problem, even showed the function, API and that uses it, but you ignored it. (It will not give you smiley in headers, but that wasn't the bug anyways.)
Ben, calm down. ;-) We don't want to flamewar, I'm sure. And I think I know now where our communication problem lies. *g* You're not actually 'afraid' of what mozTXTToHTMLConv does convert, but of those parts of (1) which do *not* get converted, right? So that evil content may get through to the HTML parser, because it has *not* been converted safely before? (That'd mean that the basic problem of bug 23114 is not mozTXTToHTMLConv, but the application of the HTML parser upon (2).) Okay, so to convert an arbitrary text (1) into something safely usable in chrome, we need to tokenize (1) into trusted and untrusted parts. Trusted parts can be turned safely into DOM elements by the HTML parser, while untrusted parts need to become #text nodes. This means basically we'd probably want to forfeit the intermediate step (2) and create (3) from (1) almost directly - this won't be a stream conversion any more. How could this be achieved? Writing a mozTXTToHTMLConv-like module would mean lots of duplicated code with only slightly different output (and the likelihood of severe differences in parsing over time). But mozTXTToHTMLConv could help here still: instead of returning a string, a yet-to-write enumerator (or something) could return an 'atom' fragment string and a flag, saying "I couldn't convert this atom, don't trust it" or "I could convert this atom, here's the result". ('Atom' is used here in the sense that any trusted fragment is as small as possible.) Trusted atoms wouldn't contain arbitrary parts of (1) (?but URIs do?). Consulers mustn't HTML-parse untrusted atoms.
> You're not actually 'afraid' of what mozTXTToHTMLConv does convert, but of > those parts of (1) which do *not* get converted, right? Very simply put, yes (roughly). > Okay, so to convert an arbitrary text (1) into something safely usable in > chrome, we need to tokenize (1) into trusted and untrusted parts. Trusted parts > can be turned safely into DOM elements by the HTML parser, while untrusted > parts need to become #text nodes. That's more or less what I suggested. Just that we don't have any HTML parser. You take the plaintext, let mozTXTToHTMLConv recognize the links in it, make textnodes out of the text between the links and the linktext, and make textnodes (e.g. createTextNode() inside <xul:description>) out of it. For the link, you create an <html:a href=""> or an XBL binding <link> or whatever and add the link target via setAttribute. This means there is not *parsing* involved, which means that no unexpected HTML/XUL content can come it, it's just textnodes. This is already possible with the current mozTXTToHTMLConv. It will not give you smileys and *bold*, as I said, but I don't consider them important, in headers. They are just cute UI niceness (and a matter of opinion) even in the body.
> let mozTXTToHTMLConv recognize the links in it I mean mozITXTToHTMLConv::findURLInPlaintext, which will basically tell you the start and end position of the linktext in the text, and the link target.
> That's more or less what I suggested. Yes, but with smilies and such. > This is already possible with the current mozTXTToHTMLConv. It will not give > you smileys and *bold*, as I said, but I don't consider them important, in > headers. They are just cute UI niceness (and a matter of opinion) even in the > body. Actually, niceness is a/the point. I'll have a look at how hard it'd be to have a derived module which does the node creation. Creating all this on the JS side isn't particularly suggesting performance... ;-)
> Actually, niceness is a/the point. Bug 23114 was about URLs, and I urge you to limit it at this for now. A smiley in the subject is not *that* important to be rendered as image, if you're honest. If you really insist on putting smileys in there right now, you can maybe add an API very much like findURLsInPlaintext, but for smileys and structs. It's not trivial, but probably possible. FWIW, if ever the headers are put in the body iframe (which means they can't do advanced stuff which accesses chrome, apart from context menus), the whole problem is void and you can the use HTML generation of mozTXTToHTMLConv without problem, because the header stuff is then untrusted as well.
> Bug 23114 was about URLs, and I urge you to limit it at this for now. Sure, but that's not the scope of this bug here - as you already stated, bug 23114 is solvable without greater changes. Removing the dependency. > A smiley in the subject is not *that* important to be rendered as image, if > you're honest. Yeah, that's why I spun off this bug for the more general issue.
No longer blocks: 23114
Product: Core → MailNews Core
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.