1203905 - Account setup (self) XSS in Gaia E-Mail

Status: RESOLVED FIXED

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

Description

[Frederik Braun [:freddy]]

Attachment: xss.png

STR:
- Set up manual account
- Pick <s> as domain name for the mail server
- Observe markup injection

This in itself is low-severity, as I can not imagine someone typing in an XSS vector as the email server name :-)

Comment 1

[Frederik Braun [:freddy]]

I am concerned that the error dialog may be used for other things that *do* involve remote email content. This will increase the security rating substantially.

Comment 2

[Frederik Braun [:freddy]]

Sorry, wrong component.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Wuh-oh, we're using mozL10n.setAttributes() and that value is an explicit argument that we assumed either would be sanitized or only ever applied with textContent.  We intentionally changed to that from directly assigning to .textContent before because it allows us to get dynamic language changes from free.

Our string, which can be found at https://github.com/mozilla-b2g/gaia/blob/master/apps/email/locales/email.en-US.properties#L94 is:
setup-error-unresponsive-server=Unable to establish a connection with "{{server}}". There may be a problem with the network.

I'm assuming the problem is in l10n's translateElement.  At https://github.com/mozilla-b2g/gaia/blob/master/shared/js/l10n.js#L1941 it seems to perform the interpolation of the l10n.args values into a single string before it then assigns the value in a single go to innerHTML at https://github.com/mozilla-b2g/gaia/blob/master/shared/js/l10n.js#L1952.  But this is just based on a quick skim; needinfo'ing :gandalf.

Unfortunately, there is a case where we allow attacker-controlled data to be displayed.  When the user clicks on a sanitized link we prompt them to confirm they want to go to that URL since we don't trust the browser to save them from spoofing.  (We have bugs/discussion on this.)  We use mozL10n.setAttributes on the string found at https://github.com/mozilla-b2g/gaia/blob/master/apps/email/locales/email.en-US.properties#L609 which is
browse-to-url-prompt = Browse to URL?: {{ url }}

More unfortunately, the sanitizer pass only escapes double-quotes to prevent attribute escapes.  Arguably, we could improve our sanitizer here by adding a helper that escapes dangerous characters into URI-encoded form.  Like escapeDangerousURICharacters().  The counter-argument is that it might help attackers obscure wacky URIs.  But the counter-counter is that our transform is destructive, only experts would would care, and all the %-encoding would likely still help make the URI look suspicious, which is our goal.  Our goal with our sanitized HTML form is that that it be as inert as possible, requiring us to take proactive steps to do something dangerous.  While the ext-href attribute we create is safe when the HTML is treated as an entire block, arguably this encoding would make things safer.

ni'ing :freddyb for thoughts on the additional sanitization steps too.  And maybe bumping the severity of this.  Actually, bugzilla is being very broken about needinfo's and security bits right now.  Just adding cc's as a first step.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

I just double checked the documentation at https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices#How_to_write_code_that_operates_on_user-provided_strings_or_l10nIDs although there may be others, which makes no mention about needing to sanitize the input.

Above that, though, I see mention of "DOM Overlay" functionality whose documentation appears to be bug 994357.  Unfortunately, at bug 994357 comment 16 it seems like there is a use case where it's explicitly desired for the arguments to be able to include HTML.

The good news is that doing further investigation with that knowledge, I see that l10n.js has a white-list of legal HTML which is all it won't strip from the resulting template.  The whitelist is at https://github.com/mozilla-b2g/gaia/blob/master/shared/js/l10n.js#L1527 and explicitly includes <s>.  So more nefarious things can be ruled out.  It just means there's lot of temporary wacky UI breakage that's accidentally possible if the caller doesn't manually HTML-escape every argument it provides to mozL10n.  Which seems like a bad default API stance, since allowing the arguments to include HTML seems like a rich/complex feature that should need to be opted into much more explicitly.

Maybe this is addressed in l20n.js?

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

So, the good news is that we do control what can be injected, and we do have a whitelist, so there's no risk of putting an iframe there or setting onclick attributes.

But, since the <s> comes from the variable it should be escaped. In bug 1190539 we disallowed HTML in l10n variables precisely for that reason. :stas ?

Comment 6

[Staś Małolepszy :stas]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #5)
 
> But, since the <s> comes from the variable it should be escaped. In bug
> 1190539 we disallowed HTML in l10n variables precisely for that reason.
> :stas ?

Yes, this is correct.  :asuth's intuition is also right:  it would be bad for the API to allow HTML in interpolated variables.  The way DOM overlays were designed, they offer an alternative to passing in HTML strings by the developer.  As far as external input goes, we shouldn't allow HTML at all there.

Freddy, is this bug against master or an older branch?

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

I backported the args escaping patch to l10n.js in gaia master today (bug 1190539). It should be fixed now, please verify.