Submitter email address: firstname.lastname@example.org 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.)
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.
This should be a 1.2 blocker.
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?
reassigning to 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.
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
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 on attachment 103389 [details] [diff] [review] proposed fix missed one case.
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.
cc cavin for review.
Comment on attachment 103390 [details] [diff] [review] proposed fix r=cavin.
Comment on attachment 103390 [details] [diff] [review] proposed fix sr=mscott
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 on attachment 103622 [details] [diff] [review] Better patch sr=mscott
Comment on attachment 103622 [details] [diff] [review] Better patch r=cavin.
Comment on attachment 103622 [details] [diff] [review] Better patch a=roc+moz
Thanks very much!
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.
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.
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.
This has been published on bugtraq by the reporter.
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 email@example.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!
14 years ago
we can use servterm http://www.snapfiles.com/get/servterm.html to emulate the evil pop server.
13 years ago