Closed Bug 899070 Opened 11 years ago Closed 11 years ago

wrapTextIntoSafeHTMLString should really return a safe html string

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::E-Mail
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:koi+, firefox-esr24 unaffected, b2g18 fixed, b2g-v1.1hd fixed, b2g-v1.2 fixed)

Status:
RESOLVED FIXED
Milestone:
1.2 FC (16sep)
Project Flags:
blocking-b2g koi+
Tracking Flags:
Tracking Status
firefox-esr24 --- unaffected
b2g18 --- fixed
b2g-v1.1hd --- fixed
b2g-v1.2 --- fixed

People

(Reporter: freddy, Assigned: mcav)

References

Details

(Keywords: sec-moderate, wsec-xss)

Attachments

(1 file, 1 obsolete file)

pull-request.html
126 bytes, text/html
asuth
: review+
freddy
: review+
praghunath
: approval-mozilla-b2g18+
Details 
Gaia HTML Sanitizer bypass when generating HTML email replies:

As far as I understand `htmlMsg' is always null, so emails are *never* being replied to in HTML (see https://github.com/mozilla-b2g/gaia-email-libs-and-more/blob/38ecd12fa242489a1f990821102546e16bacdd4f/data/lib/mailapi/mailchew.js#L135)
This is good. If there was a case in which it wasn't null the function `wrapTextIntoSafeHTMLString' would be used (see https://github.com/mozilla-b2g/gaia-email-libs-and-more/blob/master/data/lib/mailapi/htmlchew.js#L544) which doesn't correctly sanitize HTML.
Images, scripts, iframes and all other HTML tags are *not* being filtered here.
Blocks: 754742
Component: General → Gaia::E-Mail
Oh dear.  So, Vivien's proof-of-concept worker thread patch completely defeated the security benefits of wrapTextIntoSafeHTMLString and I clearly did not catch it in my review.  Previously we built a DOM tree and effected escaping of content by creating text nodes; we then grabbed the outerHTML of that DOM tree and the content would have been effectively neutered.

The evil hunk in question is here:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/2189c381e943ceab9ffeea49f84de05ae7c7a982#L3L531


The bad news is that 'htmlMsg' can in fact become non-null.  We describe our compose state by a text portion and an optional HTML portion that follows the text portion.  If you're replying to an HTML message even though we don't let you compose in HTML, we still want to be able to include the HTML message that you are replying to.  A nefarious message would consist of a multi-part message consisting of a text/html body part followed by a text/html body part.  This frequently happens in mailing lists.  In that case, we will then try and put that text part in the reply too because we were trying to simplify our compose state and generated message.

The good news is that we still put that HTML content in the exact same type of iframe we use for message display.

The technical risk profile is then that an attacker can compose a message where they are able to inject arbitrary HTML into the body of an HTML5 document (<!doctype html>) in an iframe with sandbox='allow-same-origin' (but not script) that is created via document.write() where <body> is already open at the time of writing the string.  Which is to say, it probably can't put stuff that would go in the <head> in there unless document.write has semantics that allow for something like that.

The extra privilege from having bypassed the sanitizer is that it is able to:
- exploit any other exploits that our sanitizer is protecting us from; sub-iframes can be created, for one.
- cause network traffic that would make it obvious that the user is drafting a reply to the message in question
- be annoying by being able to create <video> and <audio> tags or use CSS animations.  This could also increase device resource consumption a bit.
We'll want to fix this and try and to ideally uplift it to v1.1.  The fix is very localized since it's just fixing the one function to do what it previously did (escape HTML; we don't need to sanitize because we don't want anything in there to be treated as HTML).
(In reply to Andrew Sutherland (:asuth) from comment #1)
> A nefarious message would consist of a
> multi-part message consisting of a text/html body part followed by a
> text/html body part.

Brain typo here: I mean a text/html body part followed by a text/plain body part.
blocking-b2g: --- → leo?
I built something to test solely the sanitizing function as implemented in htmlchew.js in the browser.

This does includes putting the sanitized output into a sandbox, but it does not include the CSP. Most of the sources I pulled in and the glue I had to put on top is roughly explain in main.js.

Sadly, this cannot be attached as bugzilla disallows zip file attachments. The source includes the bleach.js library and its dependencies so it's about 9MB unzipped. I will gladly share it upon request though.
Unfortunately this late in the v1.1 cycle we can't block on a sec-moderate.
blocking-b2g: leo? → -
blocking-b2g: - → koi?
Blocks: 897524
Attached file Pull Request (obsolete) — 
Assignee: nobody → mcavanaugh
Status: NEW → ASSIGNED

        Attachment #790440 -
        Flags: review?(bugmail)

    
    

    
  
Comment on attachment 790440 [details]
Pull Request

Great first pass!  I suggest we do the following:

- Let's escape double-quotes in the attribute values (the [i+1] ones) to &quot;.  Although we control those values, it's safer to do that than write a comment explaining that the caller has to be careful.

- Add some limited coverage for the attribute escaping.

- Let's put the the new test case in its own file, something like "test_html_escaping_unit.js".  You'll need to add the file to test/test-files.json, it should be a "noserver" variant with the others down at the bottom.

nb: I recommend using https://addons.mozilla.org/en-US/firefox/addon/github-tweaks-for-bugzilla/ for attaching pull requests.  in this case your attachment's mime-type was text/plain, so auto-clicky magic doesn't work.

        Attachment #790440 -
        Flags: review?(bugmail)

    
    

    
  
Comment on attachment 790440 [details]
Pull Request

I've updated the pull request https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/233 to address that. I unwittingly squashed the commits so you'll only see the updated commit, but next time I'll not do that.

I tried to use the github-tweaks plugin but it gave me a 500 error; next time I'll investigate to see if I can figure out why that happened. Also I'm adding +r to you again to indicate an updated patch, let me know if I'm not supposed to do that. Thanks!

        Attachment #790440 -
        Flags: review?(bugmail)

    
    

    
  
Comment on attachment 790440 [details]
Pull Request

looks good.  Feel free to merge to gaia-email-libs-and-more.  You will then also want to do the "make install-into-gaia" dance after checking out 'master' with your change merged in and generate a pull request against gaia which you can then land after making sure that only your changes are in there.  (It's possible some previous landings were not propagated, in which case you would ideally generate a separate pull request for those that correspond just to that commit.)  If you want me or anyone else to eyeball things for you to make sure that your changes to gaia are just what they should be, feel free.

        Attachment #790440 -
        Flags: review?(bugmail) → review+

    
    

    
  
Comment on attachment 790440 [details]
Pull Request

Why do we go back to naively replacing > and < again? Blacklisting certain characters never works out.
Sorry, but I think we need to use bleach for this. There's a reason we have a security tested library in the email app after all...

        Attachment #790440 -
        Flags: review-

    
    

    
  
I'm not sure exactly what you're proposing in regards to bleach.  Do you mean exporting and then using its:

function escapeHTMLEntities(text) {
  return text.replace(/[<>"']|&(?![#a-zA-Z0-9]+;)/g, function(c) {
    return '&#' + c.charCodeAt(0) + ';';
  });
}

method instead of our replace chain?  It definitely escapes more stuff; does that translate to additional safety?  If so, I'd love if you could provide a comment block we can label the method with which explains what bad things could otherwise get through, and then we could add explicit test cases to bleach.js for those dangerous characters/sequences.


In terms of the parser, we're turning plaintext into HTML and don't want to do any parsing.  We could try and drive the parser's perceived parse events directly, but that still seems like it obscures the intent.  For example, if we were to call HTMLSanitizer.text() it just calls that method above in the good case, or does something incorrect in all other cases.  And our attribute escaping logic is identical to what's done in this patch.  (We always use double-quotes for attributes.)
Flags: needinfo?(fbraun)

    
    

    
  
triage: working on this for 1.2
blocking-b2g: koi? → koi+

    
    

    
  
Frederik, ping on comment 11 about how you want to address this.

    
    

    
  
Sorry for the long wait.
Yes, I suggest having one function that deals with these characters and use it whenever needed. escapeHTMLEntities looks like a good fit.
Flags: needinfo?(fbraun)

    
    

    
  
(In reply to Frederik Braun [:freddyb] from comment #14)
> Sorry for the long wait.
> Yes, I suggest having one function that deals with these characters and use
> it whenever needed. escapeHTMLEntities looks like a good fit.

I found the following explanation that I think is worth linking to:
https://www.owasp.org/index.php/XSS_%28Cross_Site_Scripting%29_Prevention_Cheat_Sheet#RULE_.231_-_HTML_Escape_Before_Inserting_Untrusted_Data_into_HTML_Element_Content

The policy it describes is very similar to what our escapeHTMLEntities method implements with the following differences:
- escapeHTMLEntities passes through existing, valid entities.  This is correct for its use but not text/plain-to-text/html; there can already be valid entities in the HTML text we are trying to keep safe.  (And our failure to do this before caused problems.)
- escapeHTMLEntities do not escape "/".


I would propose that we rename our escapeHTMLEntities method to something like "escapeHTMLTextKeepingExistingEntities" then add the following methods to bleach.js and use those:
- escapePlaintextIntoElementContext(): Do the [&<>"'/] escaping.
- escapePlaintextIntoDoubleQuotedAttributePayload(): Escape double-quotes

Alternatively, for the latter we could have escapePlaintextIntoAttribute() and implement it according to https://www.owasp.org/index.php/XSS_%28Cross_Site_Scripting%29_Prevention_Cheat_Sheet#RULE_.232_-_Attribute_Escape_Before_Inserting_Untrusted_Data_into_HTML_Common_Attributes so that it covers all of the varying quoted/unquoted variations.

    
    

    
  
I was hesitant to suggest something that would require more engineering, but your plan sounds great!

    
    

    
  
Sounds good to me, will implement and ping when done.

    
  
Depends on: 907805

    
    

    
  

      
      
      
      

        Attached file
          
        pull-request.html
        — 
      

    


  
This patch swaps out the escaping with new functions in bleach.js as indicated in bug 907805, which this bug now depends on.

I'm not sure how you handle submodule references; I assume we'll need to update that reference in this PR as soon as the other one gets through.

        Attachment #790440 -
        Attachment is obsolete: true

        Attachment #793557 -
        Flags: superreview?(fbraun)

        Attachment #793557 -
        Flags: review?(bugmail)

    
  

        Attachment #793557 -
        Flags: superreview?(fbraun) → review?(fbraun)

    
    

    
  
Comment on attachment 793557 [details]
pull-request.html

Yep

        Attachment #793557 -
        Flags: review?(fbraun) → review+

    
    

    
  
Comment on attachment 793557 [details]
pull-request.html

Looks great, thank you.  Please be sure to update the git submodule hash ref to the post-merge-to-worker-thread-friendly bleach.js hash of 9fec169e1e7a6fd3d773b3120f7d48e474f16239

(Since nothing else happened in the interim, it's fine, but it's safest hygiene-wise.)

        Attachment #793557 -
        Flags: review?(bugmail) → review+

    
    

    
  
Er, and feel free to rebase/squash the commits if you want.  While I really appreciate having the separate commits still existing while I'm doing my review since it makes looking at deltas easier, I don't have a problem with squashing history at the author's discretion.  (On larger patches, I tend to leave more history around so I can see the progression of my logic or just because it's easier to find correlated changes in a series of 100 line commits rather than in a single 1500 line commit.)

    
    

    
  
Comment on attachment 793557 [details]
pull-request.html

Okay, I've updated the submodule reference and squashed the commits for the pull request.

    
    

    
  
Landed on gaia master: https://github.com/mozilla-b2g/gaia/commit/43cf923c3020d77a9fa962cc0c30fd27f647660d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2 FC (16sep)

    
    

    
  
Comment on attachment 793557 [details]
pull-request.html

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
:pauljt requests uplift of this (gaia, not mozilla-b2g18; we lack a better flag) on https://bugzilla.mozilla.org/show_bug.cgi?id=897524#c32 in order to plug an iframe vulnerability related to the e-mail app where the other potential uplifts are much more complicated than this one (which is easy to uplift).

User impact if declined: Attackers creating specially crafted multipart e-mail bodies can cause some JS code to be run if the user tries to reply to that message.  See above or other bugs for a more thorough analysis of the sadness.

Testing completed: This fix has been on v1.2 and everything later for some time.
Risk to taking this patch (and alternatives if risky): Low risk.

        Attachment #793557 -
        Flags: approval-mozilla-b2g18?

        Attachment #793557 -
        Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+

    
    

    
  
v1-train: c434fe9a0e823029796805e141cfa983cda2d246

          status-b2g18:
          affected → fixed

          status-b2g-v1.1hd:
          --- → affected
Group: b2g-core-security → core-security
Group: core-security

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: