crash in [@ nsImapUrl::ParseNumBytes]

VERIFIED FIXED

Status

MailNews Core
Networking: IMAP
--
critical
VERIFIED FIXED
10 years ago
8 years ago

People

(Reporter: Bienvenu, Assigned: Bienvenu)

Tracking

({crash, fixed1.8.1.2, verified1.8.1.3})

Trunk
x86
Windows XP
crash, fixed1.8.1.2, verified1.8.1.3

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:nse dos][needs testcase], crash signature)

Attachments

(1 attachment)

(Assignee)

Description

10 years ago
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.
Attachment #253689 - Flags: superreview?(mscott)

Updated

10 years ago
Attachment #253689 - Flags: superreview?(mscott) → superreview+
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: fixed1.8.1.2
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Attachment #253689 - Flags: approval1.8.0.11?
Why the security flag? This appears to be a straightforward null deref unless I'm missing something.
Whiteboard: [sg:nse]
(Assignee)

Comment 2

10 years ago
It's a DOS - if that doesn't warrant the security flag, then I clear it...
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.
Group: security
Severity: normal → critical
Keywords: crash
Whiteboard: [sg:nse] → [sg:nse dos]

Comment 4

10 years ago
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?
Attachment #253689 - Flags: approval1.8.0.11? → approval1.8.0.10+
(Assignee)

Comment 5

10 years ago
Comment on attachment 253689 [details] [diff] [review]
proposed fix

clearing approval - my bad, this doesn't apply to the 1.5.0.x branch
Attachment #253689 - Flags: approval1.8.0.10+

Comment 6

10 years ago
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.
Whiteboard: [sg:nse dos] → [sg:nse dos][needs testcase]
(Assignee)

Comment 7

10 years ago
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.
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.
> 

Updated

10 years ago
Summary: crash in nsImapUrl::ParseNumBytes() → crash in [@ nsImapUrl::ParseNumBytes]
(Assignee)

Comment 9

10 years ago
verified against candidate build.
Status: RESOLVED → VERIFIED
Per Comment 9, adding the branch verified keyword.
Keywords: verified1.8.1.3
Product: Core → MailNews Core
Crash Signature: [@ nsImapUrl::ParseNumBytes]
You need to log in before you can comment on or make changes to this bug.