Last Comment Bug 157644 - Malicious pop3 server can write to arbitrary memory
: Malicious pop3 server can write to arbitrary memory
Status: VERIFIED FIXED
[sg:blocker]
:
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Navin Gupta
: bsharma
Mentors:
Depends on:
Blocks: 229374 245066
  Show dependency treegraph
 
Reported: 2002-07-15 18:44 PDT by Mitchell Stoltz (not reading bugmail)
Modified: 2004-05-29 17:27 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed fix (1.29 KB, patch)
2002-10-18 16:21 PDT, Navin Gupta
no flags Details | Diff | Review
proposed fix (1.43 KB, patch)
2002-10-18 16:37 PDT, Navin Gupta
cavin: review+
mscott: superreview+
Details | Diff | Review
Better patch (1.76 KB, patch)
2002-10-21 15:09 PDT, Navin Gupta
cavin: review+
mscott: superreview+
roc: approval+
Details | Diff | Review

Description Mitchell Stoltz (not reading bugmail) 2002-07-15 18:44:36 PDT
Submitter email address:       zen-parse@gmx.net
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) 2002-07-15 18:46:37 PDT
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) (Exited; email my personal email if necessary) 2002-10-02 09:42:29 PDT
This should be a 1.2 blocker.
Comment 3 Mitchell Stoltz (not reading bugmail) 2002-10-17 17:40:35 PDT
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 2002-10-17 17:43:55 PDT
reassigning to naving.
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2002-10-18 11:15:01 PDT
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 2002-10-18 12:23:04 PDT
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 2002-10-18 16:21:34 PDT
Created attachment 103389 [details] [diff] [review]
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 2002-10-18 16:27:02 PDT
Comment on attachment 103389 [details] [diff] [review]
proposed fix

missed one case.
Comment 9 Navin Gupta 2002-10-18 16:37:28 PDT
Created attachment 103390 [details] [diff] [review]
proposed fix

This also takes care if we run out of memory if we are in middle of allocation
entries.
Comment 10 Navin Gupta 2002-10-18 17:12:52 PDT
cc cavin for review.
Comment 11 Cavin Song 2002-10-18 17:24:22 PDT
Comment on attachment 103390 [details] [diff] [review]
proposed fix

r=cavin.
Comment 12 Scott MacGregor 2002-10-18 18:54:58 PDT
Comment on attachment 103390 [details] [diff] [review]
proposed fix

sr=mscott
Comment 13 Navin Gupta 2002-10-21 15:09:38 PDT
Created attachment 103622 [details] [diff] [review]
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 2002-10-21 15:16:21 PDT
Comment on attachment 103622 [details] [diff] [review]
Better patch

sr=mscott
Comment 15 Cavin Song 2002-10-21 15:54:12 PDT
Comment on attachment 103622 [details] [diff] [review]
Better patch

r=cavin.
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2002-10-21 16:14:56 PDT
Comment on attachment 103622 [details] [diff] [review]
Better patch

a=roc+moz
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2002-10-21 16:15:57 PDT
Thanks very much!
Comment 18 Navin Gupta 2002-10-21 16:43:57 PDT
fixed
Comment 19 David :Bienvenu 2002-11-11 10:25:55 PST
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 2002-11-11 11:50:05 PST
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 2002-11-11 11:56:44 PST
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 seth@sspitzer.org instead) 2002-11-19 19:25:05 PST
> 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 seth@sspitzer.org instead) 2002-11-19 19:32:42 PST
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 seth@sspitzer.org instead) 2002-11-19 19:36:41 PST
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 seth@sspitzer.org instead) 2002-11-19 19:41:32 PST
> 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) 2002-12-04 01:53:39 PST
This has been published on bugtraq by the reporter.
Comment 27 Ben Bucksch (:BenB) 2002-12-04 12:14:27 PST
Giving original reporter (zen-parse@gmx.net = neuro@es.co.nz) access to the bug.
Comment 28 bmartin 2003-02-11 14:42:14 PST
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
carosendahl@netscape.com

- 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 seth@sspitzer.org instead) 2004-05-29 17:03:55 PDT
we can use servterm http://www.snapfiles.com/get/servterm.html to emulate the
evil pop server.
Comment 30 (not reading, please use seth@sspitzer.org instead) 2004-05-29 17:27:50 PDT
this was fixed another way (see bug #229374) and so this fix will be backed out.
 see bug #245066

Note You need to log in before you can comment on or make changes to this bug.