Malicious pop3 server can write to arbitrary memory

VERIFIED FIXED

Status

()

Core
Security
VERIFIED FIXED
15 years ago
13 years ago

People

(Reporter: Mitchell Stoltz (not reading bugmail), Assigned: Navin Gupta)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:blocker])

Attachments

(1 attachment, 2 obsolete attachments)

1.76 KB, patch
Cavin Song
: review+
Scott MacGregor
: superreview+
Details | Diff | Splinter Review
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.)
(Reporter)

Comment 1

15 years ago
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.
Keywords: nsbeta1
This should be a 1.2 blocker.
Whiteboard: [sg:blocker]
(Reporter)

Comment 3

15 years ago
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

15 years ago
reassigning to naving.
Assignee: mscott → naving
This could easily be exploited by someone on the same local network as someone
using a POP server. This is common in university environments.
(Assignee)

Comment 6

15 years ago
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

Status: NEW → ASSIGNED
(Assignee)

Comment 7

15 years ago
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.
(Assignee)

Comment 8

15 years ago
Comment on attachment 103389 [details] [diff] [review]
proposed fix

missed one case.
Attachment #103389 - Attachment is obsolete: true
(Assignee)

Comment 9

15 years ago
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.
(Assignee)

Comment 10

15 years ago
cc cavin for review.

Comment 11

15 years ago
Comment on attachment 103390 [details] [diff] [review]
proposed fix

r=cavin.
Attachment #103390 - Flags: review+

Comment 12

15 years ago
Comment on attachment 103390 [details] [diff] [review]
proposed fix

sr=mscott
Attachment #103390 - Flags: superreview+
(Assignee)

Comment 13

15 years ago
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.
Attachment #103390 - Attachment is obsolete: true

Comment 14

15 years ago
Comment on attachment 103622 [details] [diff] [review]
Better patch

sr=mscott
Attachment #103622 - Flags: superreview+

Comment 15

15 years ago
Comment on attachment 103622 [details] [diff] [review]
Better patch

r=cavin.
Attachment #103622 - Flags: review+
Comment on attachment 103622 [details] [diff] [review]
Better patch

a=roc+moz
Attachment #103622 - Flags: approval+
Thanks very much!
(Assignee)

Comment 18

15 years ago
fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 19

15 years ago
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.
(Assignee)

Comment 20

15 years ago
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

15 years ago
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.
> 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.
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.)
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.
> 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

15 years ago
This has been published on bugtraq by the reporter.

Comment 27

15 years ago
Giving original reporter (zen-parse@gmx.net = neuro@es.co.nz) access to the bug.

Comment 28

14 years ago
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!
Status: RESOLVED → VERIFIED
(Reporter)

Updated

14 years ago
Group: security
Blocks: 229374
we can use servterm http://www.snapfiles.com/get/servterm.html to emulate the
evil pop server.
Blocks: 245066
this was fixed another way (see bug #229374) and so this fix will be backed out.
 see bug #245066
You need to log in before you can comment on or make changes to this bug.