328454 - mailbox protocol in email message causes crash

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[georgi - hopefully not receiving bugspam]

in a message:

<iframe src="mailbox://[e-mail]/?create>f00kb1l"></iframe>

crashes thunderbird and suite with a stack that may be the result of null
 pointer.

imho the mailbox protocol shouldn't be allowed from untrusted content,
because it eventually may allow modification of mailboxes.

can someone give an example of a valid mailbox url that modifies mailboxes?

Comment 1

[georgi - hopefully not receiving bugspam]

Attachment: local folder

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm.  So "mailbox" is listed as "DenyProtocol" in CheckLoadURI.  Which means that it should require chrome permissions to load it...

How do I use the attachment to reproduce this bug?  I suspect that a proper fix for bug 327078 (not the one I have there now, but the one I'm working on) should fix this, but I'd need to reproduce to verify it.

Comment 3

[georgi - hopefully not receiving bugspam]

(In reply to comment #2)
> How do I use the attachment to reproduce this bug?  I suspect that a proper fix
> for bug 327078 (not the one I have there now, but the one I'm working on)
> should fix this, but I'd need to reproduce to verify it.
> 

copy the attachment in
Mail/Local Folders/foldername

then access Local Folders/foldername and open/preview the only message in it

Comment 4

[georgi - hopefully not receiving bugspam]

(In reply to comment #2)
> How do I use the attachment to reproduce this bug?  I suspect that a proper fix
> for bug 327078 (not the one I have there now, but the one I'm working on)
> should fix this, but I'd need to reproduce to verify it.
> 

or try emailing yourself html message containing

<iframe src="mailbox://[e-mail]/?create>f00kb1l">

with a mailer that doesn't crash at composing.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Hmm...  So the problem is that we're loading mailbox:// from another mailbox:// URI, and secman allows that load.  Should the "same protocol" check not apply for some protocols?

Comment 6

[georgi - hopefully not receiving bugspam]

previewing
<iframe src="mailbox:////etc/passwd">

strace -e open -p PID
open("//etc/passwd", O_RDONLY|O_LARGEFILE)

Comment 7

[georgi - hopefully not receiving bugspam]

(In reply to comment #5)
> Hmm...  So the problem is that we're loading mailbox:// from another mailbox://
> URI, and secman allows that load.  Should the "same protocol" check not apply
> for some protocols?
> 

if the account is imap, and the message body has imap protocol, this may be not nice. iirc imap protocol allows embedding of actions

bz: a quick look at the stack shows problem with NULL, but more interesting is if the crash may be avoided and action done via the mailbox uri

Comment 8

[David :Bienvenu]

when we do a partial download of a pop3 message, the url the user clicks on to download the rest of the message is a mailbox url. It is a link we cons up, and the user does have to click on it, but we don't want to break that behaviour.

I don't know so much if it's the imap protocol that allows embedding of protocol in messages - but when we generate the html to display an imap message, we generate and run imap urls to fetch inline parts like images. Similarly for inline images in mailbox messages. 

Maybe this is all distinct from the case of the mail message itself containing an embedded url - seems like we should be able to distinguish the cases, and prevent urls in the original mail message from running.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> seems like we should be able to distinguish the cases

I'm not sure I see how.  CheckLoadURI doesn't really have a good idea of "what am I doing with this URI?"

Perhaps the answer is that in cases when we know the URI is safe to load (eg the URI is completely independent of all user data and can never be affected by anyone but us) we should simply skip the CheckLoadURI check.  And then make CheckLoadURI not allow any of these protocols, ever.

Comment 10

[Boris Zbarsky [:bzbarsky]]

So fwiw, the crash is indeed a null-pointer deref:

#5  0xb20d3270 in nsMailboxUrl::GetMsgHdrForKey (this=0xb2287410, msgKey=4294967295, 
    aMsgHdr=0xbfffde00) at ../../../../mozilla/mailnews/local/src/nsMailboxUrl.cpp:287
287         NS_NewFileSpecWithSpec(*m_filePath, getter_AddRefs(dbFileSpec));
(gdb) p m_filePath
$9 = (class nsFileSpec *) 0x0

This is being called from:

#9  0xb20f1d21 in nsMailboxService::NewChannel (this=0xb395db38, aURI=0xb2287414, 
    _retval=0xbfffe0cc)
    at ../../../../mozilla/mailnews/local/src/nsMailboxService.cpp:570

Sounds like this function needs to deal better with bogus mailbox: URIs...

Comment 11

[David :Bienvenu]

yes, I'll fix that...

Comment 12

[David :Bienvenu]

Attachment: fix for crash

safe fix...

Comment 13

[David :Bienvenu]

Attachment: fix for crash

safe fix...

Comment 14

[David :Bienvenu]

Attachment: fix for crash

Comment 15

[Scott MacGregor]

Comment on attachment 213079 [details] [diff] [review]
fix for crash

null ptr check, approving for 1.8.0.2 as well.

Comment 16

[David :Bienvenu]

I've checked the null pointer check into the trunk, the 1.8.1 and 1.8.0 branches -  not sure if I should leave this open for the larger issue of making sure mailbox/imap urls in mail messages are harmless but am leaving open for now. In particular, how does this play with the forward inline bug that's so popular right now?

Comment 17

[Daniel Veditz [:dveditz]]

Spun the URL issues off into bug 330070, the crash in this bug is fixed on trunk, 1.8 and 1.8.0 branches

Comment 18

[Marcia Knous [:marcia]]

verified on the 1.8.0.2 branch using version 1.5.0.2 (20060308). I used both variations in Comment 3 and Comment 4. No crashes observed. Adding sw term.

Comment 19

[Bob Clary [:bc] (inactive)]

(In reply to comment #18)
also verified fixed on windows xp for tb 1.5.0.2/20060308