Last Comment Bug 369028 - crash in [@ nsImapUrl::ParseNumBytes]
: crash in [@ nsImapUrl::ParseNumBytes]
Status: VERIFIED FIXED
[sg:nse dos][needs testcase]
: crash, fixed1.8.1.2, verified1.8.1.3
Product: MailNews Core
Classification: Components
Component: Networking: IMAP (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: ---
Assigned To: David :Bienvenu
: grylchan
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-02-01 16:44 PST by David :Bienvenu
Modified: 2009-01-22 10:17 PST (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed fix (793 bytes, patch)
2007-02-01 16:44 PST, David :Bienvenu
mscott: superreview+
Details | Diff | Splinter Review

Description David :Bienvenu 2007-02-01 16:44:11 PST
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] 2007-02-12 18:36:19 PST
Why the security flag? This appears to be a straightforward null deref unless I'm missing something.
Comment 2 David :Bienvenu 2007-02-12 19:09:18 PST
It's a DOS - if that doesn't warrant the security flag, then I clear it...
Comment 3 Daniel Veditz [:dveditz] 2007-02-12 22:34:16 PST
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] 2007-02-13 12:33:06 PST
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 2007-02-13 12:45:45 PST
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] 2007-02-14 15:49:06 PST
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 2007-02-14 16:04:35 PST
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 - use ni] 2007-04-02 12:30:57 PDT
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 2007-04-04 07:17:49 PDT
verified against candidate build.
Comment 10 Marcia Knous [:marcia - use ni] 2007-04-05 11:23:30 PDT
Per Comment 9, adding the branch verified keyword.

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