369028 - crash in [@ nsImapUrl::ParseNumBytes]

Status: VERIFIED FIXED

Description

[David :Bienvenu]

Created attachment 253689 [details] [diff] [review]
proposed fix

embedding a certain imap url string in a mail message can lead to a null ptr deref crash when we try to parse the url, just from displaying the message. We're not trying to run the url, just creating it. Patch upcoming.

Comment 1

[Daniel Veditz [:dveditz]]

Why the security flag? This appears to be a straightforward null deref unless I'm missing something.

Comment 2

[David :Bienvenu]

It's a DOS - if that doesn't warrant the security flag, then I clear it...

Comment 3

[Daniel Veditz [:dveditz]]

we don't need to hide crashers that can be easily cleared (by deleting the message with the content view). If it's something that can get stuck in there and crash the user until they hand-edit their mailbox or something equally persistent then that would be worth hiding until fixed.

Comment 4

[Jay Patel [:jay]]

Comment on attachment 253689 [details] [diff] [review]
proposed fix

approved for the 1.8.0 branch, a=dveditz for drivers.

We have not built TBird 1.5.0.10 yet, can you land this in the next couple of hours?

Comment 5

[David :Bienvenu]

Comment on attachment 253689 [details] [diff] [review]
proposed fix

clearing approval - my bad, this doesn't apply to the 1.5.0.x branch

Comment 6

[Jay Patel [:jay]]

David:  Any easy way to test this?  So we have a testcase that crashes?  If not, the patch looks trivial, so we can just mark this verified by code inspection.  Let me know.

Comment 7

[David :Bienvenu]

sorry, this turns out not to be an issue at all for 1.5.0.x - for 2.0, it's definitely fixed - as you say, the patch is trivial. The test case I have has private data in it, but basically, it was an imap log that had imap urls in it to fetch preview text, and creating those urls when the message was displayed was causing the crash.

Comment 8

[Marcia Knous [:marcia - needinfo? me]]

David: Since you have the test case in question, could you run it against the candidate build for a final verification?

(In reply to comment #7)
> sorry, this turns out not to be an issue at all for 1.5.0.x - for 2.0, it's
> definitely fixed - as you say, the patch is trivial. The test case I have has
> private data in it, but basically, it was an imap log that had imap urls in it
> to fetch preview text, and creating those urls when the message was displayed
> was causing the crash.
> 

Comment 9

[David :Bienvenu]

verified against candidate build.

Comment 10

[Marcia Knous [:marcia - needinfo? me]]

Per Comment 9, adding the branch verified keyword.