158776 - Possible memory corruption in pop3 protocol code.

Status: VERIFIED FIXED

(MailNews Core :: Networking: POP, defect)

Description

[Navin Gupta]

Issue details:
http://lxr.mozilla.org/mozilla/source/mailnews/local/src/nsPop3Protocol.cpp#1280

  PR_CALLOC(sizeof(Pop3MsgInfo) * 
m_pop3ConData->number_of_messages);

If the server claims there are 0x100000000 / 
(sizeof(Pop3MsgInfo)) messages, the minimum 
chunk size is allocated, and the entire 
memory may be referenced as a relative offset 
to the allocated location.

It is possible to overwrite function pointers 
on the heap with arbitary strings from the 
UIDL. (Possible, but it doesn't seem trivial 
to predict the offset to overwrite, because 
of the 'random' order in which items are put 
on the heap, due to threading.)

Exploitation would require the user connects 
to a maliciously modified POP3 server. 

(There are also a few crashes on badly formed 
input that are probably not exploitable, such 
as multiple instances like:

http://lxr.mozilla.org/mozilla/source/mailnews/local/src/nsPop3Protocol.cpp#1739
If there is no space, it dies with a null 
dereference.)

Comment 1

[scottputterman]

setting security flag until Mitch says we don't need to worry about that.

Comment 2

[Navin Gupta]

As mentioned in this report I don't think overwriting function pointers on heap
is easily exploitable (using uidl) because of the random order in which things
are allocated on the heap. I found this on google search

"Unlike many UNIX platforms, the heap that is used by malloc(), calloc() and
realloc() is not guaranteed to be one contiguous piece of memory. Thus it is
invalid to assume that all memory between two pointers returned by these
functions is accessible, and it is invalid to compare pointers returned by these
functions to determine the total size of the heap."

Mitch, what do you think ?

>http://lxr.mozilla.org/mozilla/source/mailnews/local/src/nsPop3Protocol.cpp#1739
>If there is no space, it dies with a null 
>dereference.)

I have fixes for nsCRT::strtok problem. 

Comment 3

[Navin Gupta]

Attachment: proposed fix for strtok part

The fix is to check token for null.

Comment 4

[Navin Gupta]

Cavin, David, Can I get reviews ? thx. 

Comment 5

[Navin Gupta]

Attachment: uw diff for strtok part of the fix

Comment 6

[David :Bienvenu]

Comment on attachment 92441 [details] [diff] [review]
uw diff for strtok part of the fix

looks OK, except what happens here to cur_msg_size if num is null?  Does it
matter?

+	   if (num)
+		  m_pop3ConData->cur_msg_size = atol(num);

Comment 7

[Navin Gupta]

m_pop3ConData->cur_msg_size is initialized to -1 when we get there. 

Comment 8

[David :Bienvenu]

Comment on attachment 92441 [details] [diff] [review]
uw diff for strtok part of the fix

OK, I assume you're mean that's OK and nothing bad will happen because of it -
sr=bienvenu

Comment 9

[David :Bienvenu]

Comment on attachment 92441 [details] [diff] [review]
uw diff for strtok part of the fix

OK, I assume you mean that's OK and nothing bad will happen because of it -
sr=bienvenu

Comment 10

[Cavin Song]

Comment on attachment 92441 [details] [diff] [review]
uw diff for strtok part of the fix

r=cavin.

Comment 11

[Navin Gupta]

yeah, it is ok because it will be taken care of in the next few lines. What do
you think about the calloc issue. See comment 0 and comment #2. 

Comment 12

[David :Bienvenu]

I'm not sure I understand the calloc issues. Sure, you can cause the client to
allocate a big chunk of memory with 0's in it. And then you can get some text
strings into that block of memory with the UIDL. But that's not sufficient to
cause a problem, is it? Doesn't that have to happen in conjunction with some
other security exploit to actually cause execution to switch into the block of
memory?

Comment 13

[scottputterman]

We don't need to get this fixed for Mach V, but it would be good to have a fix
baking on the trunk for Buffy.

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 92441 [details] [diff] [review]
uw diff for strtok part of the fix

a=asa (on behalf of drivers) for checkin to 1.1

Comment 15

[Navin Gupta]

fixed on trunk. 

Comment 16

[Sheela Ravindran]

changing qa contact ->stephend

Comment 17

[[e-mail] (gone - use [e-mail] instead)]

Anybody object if I verify this via source code, as opposed to at the API-level?

I really don't have a way of testing this (C++ knowledge required).

Comment 18

[Navin Gupta]

yes, lxr verification is ok because this code has been tested for more than a
month now. 

Comment 19

[[e-mail] (gone - use [e-mail] instead)]

verified fixed via lxr, thanks!

Comment 20

[Daniel Veditz [:dveditz]]

long fixed and no need to be confidential