Closed Bug 369028 Opened 15 years ago Closed 15 years ago

crash in [@ nsImapUrl::ParseNumBytes]

Categories

(MailNews Core :: Networking: IMAP, defect)

Product:
MailNews Core
Component:
Networking: IMAP
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: Bienvenu, Assigned: Bienvenu)

Details

(Keywords: crash, fixed1.8.1.2, verified1.8.1.3, Whiteboard: [sg:nse dos][needs testcase])

Crash Data

Attachments

(1 file)

proposed fix
793 bytes, patch
mscott
: superreview+
Details | Diff | Splinter Review
Attached patch proposed fix — — Splinter Review 
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)
Attachment #253689 - Flags: superreview?(mscott) → superreview+
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: fixed1.8.1.2
Resolution: --- → FIXED
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]
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 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+
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+
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]
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.
>
Summary: crash in nsImapUrl::ParseNumBytes() → crash in [@ nsImapUrl::ParseNumBytes]
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.