157644 - Malicious pop3 server can write to arbitrary memory

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Mitchell Stoltz (not reading bugmail)]

Submitter email address:       [e-mail]
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

[Mitchell Stoltz (not reading bugmail)]

It's unlikely that a user would be connecting to an untrusted POP3 server
(unless there's someone working at an ISP or as a sever admin somewhere who's
out for mischief and can modify the POP server), but nonetheless this should be
fixed ASAP.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This should be a 1.2 blocker.

Comment 3

[Mitchell Stoltz (not reading bugmail)]

I suppose this could be exploited if a malicious site offered free email
accounts with its modified server. mscott, could you take a look and set a
target milestone?

Comment 4

[scottputterman]

reassigning to naving.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This could easily be exploited by someone on the same local network as someone
using a POP server. This is common in university environments.

Comment 6

[Navin Gupta]

One way to fix the CALLOC issue is to realloc as we go i.e when we get the list
entries we should try to increase block size by 1. I think I have already
addressed the other issue in bug 144228

Comment 7

[Navin Gupta]

Attachment: proposed fix

The fix as I said earlier is to increase size of msgInfo by 1 entry at a time.
This would prevent heap overflow.

Comment 8

[Navin Gupta]

Comment on attachment 103389 [details] [diff] [review]
proposed fix

missed one case.

Comment 9

[Navin Gupta]

Attachment: proposed fix

This also takes care if we run out of memory if we are in middle of allocation
entries.

Comment 10

[Navin Gupta]

cc cavin for review.

Comment 11

[Cavin Song]

Comment on attachment 103390 [details] [diff] [review]
proposed fix

r=cavin.

Comment 12

[Scott MacGregor]

Comment on attachment 103390 [details] [diff] [review]
proposed fix

sr=mscott

Comment 13

[Navin Gupta]

Attachment: Better patch

Realloc only when we have really large number of msgs to dowload. I have kept a
threshold of 50,000 msgs. I think this is more than sufficient.

Comment 14

[Scott MacGregor]

Comment on attachment 103622 [details] [diff] [review]
Better patch

sr=mscott

Comment 15

[Cavin Song]

Comment on attachment 103622 [details] [diff] [review]
Better patch

r=cavin.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 103622 [details] [diff] [review]
Better patch

a=roc+moz

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Thanks very much!

Comment 18

[Navin Gupta]

fixed

Comment 19

[David :Bienvenu]

This came up in the pop3 security review, I didn't understand the bug or the
fix, so I looked into it a bit more.

It seems to me the trivial fix would be something like this:

// check for server returning number of messages that will cause the calculation
of the size of the block for msg_info to
// overflow a 32 bit int, in turn causing us to allocate a block of memory much
smaller than we think we're allocating, and
// potentially allowing the server to make us overwrite memory outside our heap
block.

if (m_pop3ConData->number_of_messages > (0xFFFFF000 / sizeof(Pop3MsgInfo))
    return MK_OUT_OF_MEMORY; // or some sort of error code...


The fix as attached would still cause us to overflow the heap block size
eventually, except that on most machines, a memory allocation would probably
fail long before we overflowed the block size.

there are two other cases that more or less boil down to the same problem:

1. the server reports an int that's larger than MAX_INT, so atoi returns a
number less than that specified by the server.
2. The server says there are XX messages, but then gives XXXX messages.

I think these are more or less the same cases, since they boil down to the
server giving us more messages than we expect. It looks to me like the existing
code handles this since it always checks the msg_num we've been given is less
than the number we're expecting. If the existing code doesn't handle this
problem, then we still have a problem because the server could tell us there's
one header and return 49,999 headers, and overwrite the heap block. So, I think
we're left with the case that I described as a potential problem.

Navin, from the security meeting, I'm prety sure you have a different idea of
what the bug is here, but I didn't understand what it was.

Comment 20

[Navin Gupta]

As per the original bug report, I don't think it is an integer overflow problem.
He talks about how the "entire memory may be referenced as a relative offset to
allocated location". So, when you make a memory copy of the UIDL received you
could overwrite the fuction pointers on the heap but he admits that it is
difficult to do that because of the random order in which things are put on heap.

So the fix was to never allocate such a huge block. Allocate at the max, space
for 50,000 messages and then increase block size as we need more. I think this
upper limit of 50,000 would prevent over-writing contents of the heap. It
certainly would prevent an overflow of heap, because we try to increase block
size by 1 after this limit. 

This bug is certainly not about allocating less space and then using the space
which was not allocated, that would just lead to a crash. 

More importantly, as per the security meeting, we can trust mail servers. 

Comment 21

[David :Bienvenu]

I talked to Mitch, and he *did* think the bug was about allocating less space
then required, and overwriting contents of the heap. You seem to be saying that
allocating a huge hunk of memory and writing to it can cause security exploits,
and I don't believe that's true.

Comment 22

[(not reading, please use [e-mail] instead)]

> This bug is certainly not about allocating less space and then using the space
> which was not allocated, that would just lead to a crash. 

that doesn't mean a crash.

I can allocate one byte on the heap, and then just write off the end of it and 
stomp on other things on the heap.

that stomping may lead to a crash, or the actually writing might cause an 
access violation if I try to write to something I don't own.

Comment 23

[(not reading, please use [e-mail] instead)]

how often do we get into the REALLOC code?

PR_REALLOC(m_pop3ConData->msg_info, sizeof(Pop3MsgInfo) * (msg_num + 1));

if frequently, this is going to be perf hit. (And I'd recommend rethinking 
growing by 1.)

realloc() will allocate and copy the buffer to the new location (if the memory 
manager can't just grow without moving.)

Comment 24

[(not reading, please use [e-mail] instead)]

the good news is we won't hit this code unless we're told we have more than 
50,000 messages.

and if that's the case, you have bigger problems.

this will prevent a malicious server from killing us.

If someone things it's reasonable for someone to have more than 50,000 messages 
in their pop inbox, we should reconsider this fix to not grow by 1.

Comment 25

[(not reading, please use [e-mail] instead)]

> More importantly, as per the security meeting, we can trust mail servers. 

whoa.  we never decided that.

in fact, we decided that with all the "free" accounts out there, we *can't* 
trust servers.

I could easily set up an evil mail server, and say:  "free accounts!", and then 
whamo.

Comment 26

[Ben Bucksch (:BenB)]

This has been published on bugtraq by the reporter.

Comment 27

[Ben Bucksch (:BenB)]

Giving original reporter ([e-mail] = [e-mail]) access to the bug.

Comment 28

[bmartin]

Verified that the patch has landed as of 2/11/03.

The code now ensures that initially the lesser of (number of msgs) or 50000
(klargenumberofmessages) is allocated.  Should more than 50000 be needed, the
memory block is reallocated by one each time.

Should memory fail to be reallocated, an out of memory error is returned. 
Otherwise the msg struct is zeroed and the buffer is freed before returning.

Charles Rosendahl
[e-mail]

- if anyone has created a pop server program emulation to return the data in
question, we would be more than happy to try it.  Also, if anyone has a mail
account with greater than 50000 messages, please give this a try!

Comment 29

[(not reading, please use [e-mail] instead)]

we can use servterm http://www.snapfiles.com/get/servterm.html to emulate the
evil pop server.

Comment 30

[(not reading, please use [e-mail] instead)]

this was fixed another way (see bug #229374) and so this fix will be backed out.
 see bug #245066