245066 - [internal] nsPop3Protocol.cpp, back out fix for bug #157644, since david has fixed the problem another way (bug #229374)

Status: RESOLVED FIXED

(MailNews Core :: Networking: POP, defect)

Description

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

[internal] nsPop3Protocol.cpp, back out fix for bug #157644, since david has
fixed the problem another way (bug #229374)

nsPop3Protocol.cpp

Comment 1

[David :Bienvenu]

Attachment: proposed fix

back out previous fix.

Comment 2

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

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

r/a=sspitzer

Comment 3

[Daniel Veditz [:dveditz]]

Temporarily marking security sensitive, nominating blocking, and pasting in the
full research into this problem we received.

--------------------------------------------------------------
Security Audit of Mozilla's POP3 client protocol (August 2004) 
by Gael Delalleau  <[e-mail]>
--------------------------------------------------------------


Overview
--------
This report presents the results of a security audit of a part
of the Mozilla 1.7.2 C++ source code tree.

This security audit was done during my free time as another attempt
to promote and participate to the Mozilla Security Bug Bounty
Program (http://www.mozilla.org/security/bug-bounty.html).

Several bugs (with the same origin) were found, leading to memory
corruption by writing outside the bounds of a buffer allocated on the
heap. They could potentially be exploited to run arbitrary code on the
system of the user, by a rogue POP3 server, or by any attacker being
able to hijack the TCP connexion to the POP3 server (this includes
computers on the local network).


Source code file audited
------------------------
mozilla/mailnews/local/src/nsPop3Protocol.cpp


Technical Details
-----------------
Bad days for nsPop3Protocol.cpp... The size of the msg_info buffer is
not tracked correctly in this code, which causes various bugs.

* Line 859 in nsPop3Protocol::FreeMsgInfo(), called from the class
  destructor, the cleanup loop goes iterates from 0 to
  m_pop3ConData->number_of_messages. However the msg_info buffer can
  have a smaller length: a rogue POP3 server could make msg_info have
  an arbitrary lower length ranging from kLargeNumberOfMessages to
  m_pop3ConData->number_of_messages.

Proof:
- msg_info is allocated here:
1793     m_pop3ConData->msg_info = (Pop3MsgInfo *)
1794       PR_CALLOC(sizeof(Pop3MsgInfo) *
1795       (m_pop3ConData->number_of_messages < kLargeNumberOfMessages ?
m_pop3ConData->number_of_messages : kLargeNumberOfMessages));

- msg_info can grow here:
1858       if (msg_num >= kLargeNumberOfMessages && msg_num <
m_pop3ConData->number_of_messages)
1859       {
1860         m_pop3ConData->msg_info = (Pop3MsgInfo *)   //allocate space for
next entry
1861           PR_REALLOC(m_pop3ConData->msg_info, sizeof(Pop3MsgInfo) *
(msg_num + 1));


Thus, in FreeMsgInfo, the loop will step out from the msg_info
buffer... It means we do calls to free() on pointers stored on the
heap after the msg_info buffer (line 859), then we set those pointers
to NULL (line 860).

851 nsPop3Protocol::FreeMsgInfo()
852 {
853   int i;
854   if (m_pop3ConData->msg_info) 
855   {
856     for (i=0 ; i<m_pop3ConData->number_of_messages ; i++) 
857     {
858       if (m_pop3ConData->msg_info[i].uidl)
859         PR_Free(m_pop3ConData->msg_info[i].uidl);
860       m_pop3ConData->msg_info[i].uidl = nsnull;
861     }
862     PR_Free(m_pop3ConData->msg_info);
863     m_pop3ConData->msg_info = nsnull;
864   }
865 }

Consequences:
- DoS (crash of the client)
- arbitrary code execution might be possible, depending on the OS and
the heap layout (not investigated, but think "double free" etc.).


* In nsPop3Protocol::GetXtndXlstMsgid() a rogue POP3 server can
  provide a msg_num higher than the maximum index of msg_info, because
  there is no check on the real size of msg_info. Thus we can
  overwrite "arbitrary" addresses in memory with a pointer to uidl
  (line 2192).

2179     msg_num = atol(token);
2180     if(msg_num <= m_pop3ConData->number_of_messages && msg_num > 0) 
2181     {
2183       char *uidl = nsCRT::strtok(newStr, " ", &newStr);/* not really a uidl
but a unique token -km */
2185       if (!uidl)
2190         uidl = "";     
2192       m_pop3ConData->msg_info[msg_num-1].uidl = PL_strdup(uidl);

Consequences:
 By overwriting (function) pointers on the heap or in other writable
 memory areas within reach, it is likely we can achieve arbitrary code
 execution.


* There is the same problem in nsPop3Protocol::GetUidlList (see
  description of the bug in nsPop3Protocol::GetXtndXlstMsgid).


* in nsPop3Protocol::GetList at line 1856, we can overwrite any
  address within reach with 4 arbitrary bytes. This is because the
  growth of the msg_info buffer happens too late, at line 1860.
  (Moreover, the msg_info buffer can grow and then shrink at a later
  time, thus loosing data which was previously stored.)

1850     msg_num = atol(token);
1851     
1852     if(msg_num <= m_pop3ConData->number_of_messages && msg_num > 0)
1853     {
1854       token = nsCRT::strtok(newStr, " ", &newStr);

1855       if (token)
1856         m_pop3ConData->msg_info[msg_num-1].size = atol(token);
1857       
1858       if (msg_num >= kLargeNumberOfMessages && msg_num <
m_pop3ConData->number_of_messages)
1859       {
1860         m_pop3ConData->msg_info = (Pop3MsgInfo *)   //allocate space for
next entry
1861           PR_REALLOC(m_pop3ConData->msg_info, sizeof(Pop3MsgInfo) *
(msg_num + 1));

Consequences:
 By overwriting data, control structures or function pointers on the
 heap or in other writable memory areas within reach, it is likely we
 can achieve arbitrary code execution.


Example of a bogus POP3 session
-------------------------------
+OK
AUTH
-ERR
CAPA
-ERR
USER ******
+OK
PASS ******
+OK
STAT
+OK 262670336 77777
LIST
+OK
1 77
2 77
262670336 77777
<< Mozilla crashes here >>


Conclusion
----------
I reckon these bugs were introduced by an improper fix of the first
POP3 security issue reported in 2002 by zen-parse. Another fix was
applied earlier this year but even though the initial issue has been
patched, it seems the "new" issues were missed.


- Cheers -

Gael Delalleau

Comment 4

[Daniel Veditz [:dveditz]]

This patch fixes the first DOS (at the cost of going back to potentially
allocating an obscene amount of memory, a DOS in its own right).

The serious issue of using the server-supplied msg_num as an index (multiple
places) was fixed on the trunk in bug 226669. That fix needs to land on the 1.7
branch. According to the bug it has been landed on the Aviary branch.

Comment 5

[David :Bienvenu]

fixed all three places

Comment 6

[David :Bienvenu]

adding Christian...

Comment 7

[Daniel Veditz [:dveditz]]

fix landed on 1.7.2 branch