Last Comment Bug 236618 - Netscape SOAPParameter Constructor Integer Overflow Vulnerability
: Netscape SOAPParameter Constructor Integer Overflow Vulnerability
Status: RESOLVED FIXED
[sg:fix]
: fixed1.4.3
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: x86 Windows XP
: P1 normal (vote)
: mozilla1.7beta
Assigned To: Brendan Eich [:brendan]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2004-03-05 15:35 PST by iDefense Vendor Relations
Modified: 2004-08-03 15:25 PDT (History)
11 users (show)
brendan: blocking1.7b+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Screen shot of what it is supposed to do. (73.57 KB, image/png)
2004-03-08 12:19 PST, gregm
no flags Details
proposed fix (1.53 KB, patch)
2004-03-08 12:37 PST, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
the right fix (1.63 KB, patch)
2004-08-03 10:59 PDT, Brendan Eich [:brendan]
brendan: review+
brendan: superreview+
brendan: approval1.4.3+
Details | Diff | Splinter Review

Description iDefense Vendor Relations 2004-03-05 15:35:51 PST
User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; .NET CLR 1.0.3705; .NET CLR 1.1.4322)
Build Identifier: 

iDEFENSE Security Advisory xx.02.04:

I. BACKGROUND

Netscape SOAPParameter Constructor Integer Overflow Vulnerability 

II. DESCRIPTION

Improper input validation to the SOAPParameter object constructor in Netscape 
allows 

execution of arbitrary code.

The SOAPParameter object's constructor contains an integer overflow which 
allows 

contollable heap corruption.

A webpage can be constructed to leverage this into remote execution of 
arbitrary code.

III. ANALYSIS

Successful exploitation allows the remote attacker to execute abitrary code in 
the 

context of the user running the browser.

IV. DETECTION

Netscape version 7.0 and 7.1 have been confirmed to be vulnerable. Mozilla 1.6 
is also 

vulnerable to this issue. It is suspected that earlier versions of both 
browsers may 

also be vulnerable.

V. WORKAROUNDS

Disable Javascript in the browser.


VI. VENDOR RESPONSE


VII. CVE INFORMATION


VIII. DISCLOSURE TIMELINE



January 17, 2004      Exploit acquired by iDEFENSE.




IX. CREDIT

zen-parse (zen-parse at gmx.net) is credited with this discovery.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.
Comment 1 Darin Fisher 2004-03-05 16:36:43 PST
here is the code for nsSOAPParameter::nsSOAPParameter:

http://lxr.mozilla.org/seamonkey/source/extensions/webservices/soap/src/nsSOAPParameter.cpp#46

note: the function does nothing.  there is no code.

perhaps this has to do with the way one constructs a SOAPParameter object in
JavaScript?  nsSOAPParameter inherits from nsSOAPBlock which has a JS Initialize
method.  perhaps that is the culprit?
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2004-03-05 17:02:23 PST
Michael, could we get more info on this bug? It's not obvious what the input
should be to trigger the overflow, and it's not obvious from the code what the
overflow would be.
Comment 3 Doron Rosenberg (IBM) 2004-03-08 08:31:45 PST
var param = new SOAPParameter();
param.name = "translationmode";
param.value = "en_fr";

is how you set a SOAP parameter from JavaScript.
Comment 4 gregm 2004-03-08 08:50:26 PST
Try:

 var p=new Array(0x40000001);
 var q=new SOAPParameter(p);

Comment 5 Brendan Eich [:brendan] 2004-03-08 12:09:54 PST
gregm, what is that supposed to do?  new Array(<very large number here>) will
run for a long, long time, and probably thrash your system, due to suboptimal
code in jsarray.c that initializes every element from 0 to <very large number
here>-1 to undefined.

Assuming you have enough memory and wait long enough, then what happens?  No one
has pointed to native code that takes the length of that array and multiplies it
by 4 (e.g.), feeding the result to malloc.  Please give an lxr link to such code
if you know of it.  Thanks,

/be
Comment 6 gregm 2004-03-08 12:19:42 PST
Created attachment 143315 [details]
Screen shot of what it is supposed to do.

Screenshot of what the script does.
Comment 7 Brendan Eich [:brendan] 2004-03-08 12:24:01 PST
Argh.  Evil, pure and simple, from the 8th dimentions:

http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcconvert.cpp#1692

Thanks, idefense guys.

/be
Comment 8 Brendan Eich [:brendan] 2004-03-08 12:29:16 PST
Er, 8th dimension (need more caffeine still, not worthy of Team Banzai).

I'll take this, actually, to get it fixed ASAP.

/be
Comment 9 Brendan Eich [:brendan] 2004-03-08 12:37:55 PST
Created attachment 143316 [details] [diff] [review]
proposed fix

I grepped around in xpconnect/src for ' \* sizeof' and found nothing else
amiss, but other eyes should look too.

/be
Comment 10 Brendan Eich [:brendan] 2004-03-08 12:40:58 PST
Comment on attachment 143316 [details] [diff] [review]
proposed fix

dbradley, jband: feel free to r= too -- I'm just not sure how often you read
bugmail, and how much time you have for quick-turnaround fix reviewing.

/be
Comment 11 Johnny Stenback (:jst, jst@mozilla.com) 2004-03-08 12:42:25 PST
Comment on attachment 143316 [details] [diff] [review]
proposed fix

sr=jst
Comment 12 Brendan Eich [:brendan] 2004-03-08 12:50:29 PST
Comment on attachment 143316 [details] [diff] [review]
proposed fix

Fix collision.	Checking in now.

/be
Comment 13 Brendan Eich [:brendan] 2004-03-08 12:51:55 PST
Fixed.

/be
Comment 14 gregm 2004-03-08 13:21:01 PST
There could be some values that will pass the test but not allocate enough space.

for example:

0x55555556 * 4 

main()
{
 unsigned int size;
 unsigned int capacity=0x55555556;
 size=capacity*sizeof(int);
 printf("size=%u\ncapacity=%u\n",size,capacity);
 if(size < capacity)
 {
  printf("Failed to allocate.\n");
 } 
 else printf("It should've failed to allocated.\n");
 
}



$ ./sample.exe 
size=1431655768
capacity=1431655766
It should've failed to allocated.

Admittedly, this would only work if the end user has a REALLY large amount of
memory, but people have more memory in their machines all the time.

Changing the patch from this:

+        size_t size_ = capacity * sizeof(_t);                                \
+        if (size_ < capacity || nsnull == (array = nsMemory::Alloc(size_)))  \

into this:

+        size_t size_ = capacity * sizeof(_t);                                \
+        if (size_ !=  (capacity * sizeof(_t)) || nsnull == (array =
nsMemory::Alloc(size_)))  \

Comment 15 Brendan Eich [:brendan] 2004-03-08 14:04:29 PST
gregm: good point (I wrongly assumed capacity is a multiple of sizeof(_t)), but
your proposed patch won't work either, without 64-bit integer or floating point
being used to get the multiple result into a larger domain.

I'm fixing the fix to do this instead:

        if ((capacity != 0 && size_ < PR_ROUNDUP(capacity, sizeof(_t))) ||   \
            nsnull == (array = nsMemory::Alloc(size_)))                      \

/be
Comment 16 David Bradley 2004-03-08 15:12:19 PST
Was just going to say that, but you beat me to it.

Why can't you just do:

size_t const maxCapacity = UINT_MAX / sizeof(_t);
if (capacity < maxCapacity)
....

And replace UINT_MAX with whatever our max PRUint32 value is.
Comment 17 Brendan Eich [:brendan] 2004-03-08 15:38:51 PST
> being used to get the multiple result into a larger domain.

I meant "the *multiply* result" (brain still caffeinating today).

Note: PR_ROUNDUP(i, j) is j * ((i + j - 1) div j).

Claim:
  Let i, j, and M be integers where 0 < j <= i and 0 < i < M:
    (i * j) mod M < j * ((i + j - 1) div j)
      => i * j >= M.

Proof by contradiction:
  Let (i * j) mod M < j * ((i + j - 1) div j),
    but i * j < M.
  Then (i * j) mod M == i * j, so:
    i * j < j * ((i + j - 1) div j),
    i * j < i + j - 1.
  If j is 1, i < i => contradiction.
  If 1 < j <= i, let j be j[n] = j[n-1] + 1:
    i * (j[n-1] + 1) < i + j[n-1],
    i * j[n-1] < j[n-1],
      => contradiction.
  Therefore, there is no j in [1, i] such that i * j < i + j - 1, so
    (i * j) mod M < j * ((i + j - 1) div j)
      => i * j >= M.

Note that we can assume j <= i and i < M where M is 2^32, j is sizeof(t_), and i
is capacity, because we can ignore the case where i < j.  In that case, capacity
* sizeof(t_) << M, because sizeof(t_) << M.

Dbradley: we'd want something like

        if (capacity > ~(size_t)0 / sizeof(_t) ||                            \
            nsnull == (array = nsMemory::Alloc(capacity * sizeof(_t))))      \
        {                                                                    \
            if(pErr)                                                         \
                *pErr = NS_ERROR_OUT_OF_MEMORY;                              \
            goto failure;                                                    \
        }                                                                    \

This is cheaper than all the rounding and so on.  Thanks, I'm checking in (I
didn't do the followup checkin yet -- had to do the math first ;-).

I wanted to do the proof above to show what was going on with the initial fix
attempt (both the idea behind it, and the flaw in it).

/be
Comment 18 gregm 2004-03-16 13:21:07 PST
This is marked as RESOLVED FIXED, but the diff attachment wouldn't work as a fix. 
Is there a new diff?
Comment 19 Daniel Veditz [:dveditz] 2004-05-26 13:55:21 PDT
Brendan checked in the code seen at the end of comment 17, following "we'd want
something like"
Comment 20 Daniel Veditz [:dveditz] 2004-06-17 13:38:19 PDT
Adding Jon Granrose to CC list to help round up QA resources for verification
Comment 21 Christopher Aillon (sabbatical, not receiving bugmail) 2004-07-09 08:27:15 PDT
Comment on attachment 143316 [details] [diff] [review]
proposed fix

a=blizzard
Comment 22 Christopher Aillon (sabbatical, not receiving bugmail) 2004-07-09 08:31:28 PDT
Checked in to the 1.4 branch.
Comment 23 Daniel Veditz [:dveditz] 2004-07-22 02:35:40 PDT
Removing security-sensitive flag for bugs on the known-vulnerabilities list
Comment 24 Mark Cox 2004-08-03 00:44:52 PDT
Note: The Common Vulnerabilities and Exposures project (cve.mitre.org) has
assigned the name CAN-2004-0722 to this issue.
Comment 25 Brendan Eich [:brendan] 2004-08-03 10:57:06 PDT
caillon: you want this patch
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/js/src/xpconnect/src&command=DIFF_FRAMESET&root=/cvsroot&file=xpcconvert.cpp&rev1=1.85&rev2=1.86
not the one attached here that you patched the 1.4 branch with.  I'll obsolete
the bad patch.

/be
Comment 26 Brendan Eich [:brendan] 2004-08-03 10:59:52 PDT
Created attachment 155096 [details] [diff] [review]
the right fix
Comment 27 Brendan Eich [:brendan] 2004-08-03 11:00:35 PDT
Comment on attachment 155096 [details] [diff] [review]
the right fix

Restoring flags set on last patch.

/be
Comment 28 Christopher Aillon (sabbatical, not receiving bugmail) 2004-08-03 15:25:27 PDT
Brendan, thanks.  I overlooked that when I looked back at the cvs log the first
time.  Got it in for 1.4.3.

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