938988 - SecReview: FxOS POP3 support

Status: RESOLVED FIXED

(mozilla.org :: Security Assurance: Review Request, task)

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Security review to look at pop support landing in 1.3  (916080, 916083, 916088, 916090)

Comment 1

[Frederik Braun [:freddy]]

Hey Andrew,
I need some additional feedback (maybe a quick chat?) about the HTML fragments in pop3.js. The idea is to make sure that POP3 itself isn't touching the markup but the consumer of it is, so we can leverage existing defense mechanisms.

Comment 2

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

Marcus implemented the POP3 implementation so should be the point-man for this.  I reviewed and can comment too, but the security review is part of the fun of landing a giant patch! ;)

Pinging in #gelam for either of us when it's your daytime again is probably the best way since we both lurk there too and can see the discussion in scrollback.  (From IRC I understand it's now late in the Berlin area.)

The short answer is that we don't pass "showAttachmentLinks" to the MailParser, so the _joinHTMLAttachment method should never be called.

Code-reading-wise, it does look like _joinHTMLNodes is probably getting called, but that its byproducts are ignored.  Specifically, it mutates the contents of mailParserInstance.mailData.html[N].content, but mailData.html is populated by pushing an object whose content is just a reference to the mime node's 'content'.  So manipulating mailData.html[N].content does not affect mimeNode.content.  We just consume mimeNode.content.

(Also, one could argue that even if we consumed mailData.html[N].content that we still sanitize that, so it doesn't matter.)

Of course, unit tests speak louder than anything, so I would propose that we add a test case to test_mime.js in GELAM that is a multipart/mixed consisting of two text/html parts and that we ensure that we don't see any random joining HTML inserted.  We might even go further and have a structure like the following because MailParser even has a concept of levels for processing like this (which is really impressively thorough):
- multipart/mixed
  - text/html
  - multipart/mixed
    - text/html
    - text/html

Comment 3

[Marcus Cavanaugh [:mcav]]

Attachment: gelam.html

GELAM test_mime unit test to ensure that when we parse a MIME message with adjacent HTML nodes (nested in multipart/mixed for good measure), we don't include extra stuff in the content.

Comment 4

[Marcus Cavanaugh [:mcav]]

Yeah, Freddy, I'm happy to be the point man and try to answer any questions you have. Andrew's a busy man! I added a unit test per Andrew's comment, but if you have more questions on this or other stuff, feel free to ping me either here, or #gelam, or wherever.

Comment 5

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

Comment on attachment 8345575 [details]
gelam.html

You sir, are dangerously on the ball.

Great test, thanks for the ridiculously fast turnaround.

Comment 6

[Frederik Braun [:freddy]]

Thanks for following up so quickly. Adding a unit test was a great idea. 
Let's set this bug to Resolved/Fixed once the test has landed.

Comment 7

[Marcus Cavanaugh [:mcav]]

Landed.

https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/567dcc0942ce421d72fb99cb3439304ba88089a7