Closed Bug 356238 Opened 18 years ago Closed 18 years ago

Avoid null, dups and bad indexes in jsxml.c

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(Keywords: verified1.8.0.9, verified1.8.1.1, Whiteboard: [sg:critical])

Attachments

(4 files, 1 obsolete file)

[ This is a spin-off of security-sensitive bug 354145 comment 71. ]

With the bug 354145 fixed xml_child_helper may set rval to JSVAL_NULL, but xml_child does not check for this leading to potential null pointer access.
I am renaming the bug to reflect its generic nature for extra fixes after landing bug 354145.

  
Summary: xml_child should check for null rval set by xml_child_helper → Avoid null, dups and bad indexes in jsxml.c
Replace method in jsxml.c contains an assert that checks that the passed index is not exceeding the length. But this is a wrong assert as Replace is called directly from  xml_replace so it can contain an arbitrary index. This assert is harmless since the code resets the index in any case later. The following one-liner shows the problem in a debug build:


js> <xml/>.replace(1);
Assertion failure: i <= n, at jsxml.c:3834
Trace/breakpoint trap (core dumped)
Here is a test case that show that Insert can create duplicates: 

var xml = <xml><child><a/><b/></child></xml>;
var child = xml.child[0];

try {
  child.insertChildAfter(null, xml.child);
  throw "insertChildAfter succeded when it should throw an exception";
} catch (e if e instanceof Error) {
}

if (child.a[0] === child.a[1])
  throw "Duplicate detected!"


Currently it throws "Duplicate detected!" since Insert first reserve the room for list elements to insert and then checks for loops there.
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
Attached patch Fix v1Splinter Review
Attachment #241938 - Flags: review?(brendan)
Attachment #241938 - Flags: approval1.8.1.1?
Comment on attachment 241938 [details] [diff] [review]
Fix v1

Looks good, some questions:

* Does Rhino have this bug?
* Is this a spec bug?
* Why undefined instead of null from XML.prototype.child?  Spec doesn't help here, so suspect a spec bug in part.

/be
(In reply to comment #5)
> (From update of attachment 241938 [details] [diff] [review] [edit])
> Looks good, some questions:
> 
> * Does Rhino have this bug?

No. In Rhino E4X implementation is effectively a bridge from XMLBeans Apache library and JS. Thus all the unwrapping of object into corresponding native classes is done very early or not done at all. That is, in Rhino in many places the code just returns when it see NON-xml object. 

> * Is this a spec bug?

[[Insert]] and [[Replace]] definitions, 9.1.1.11 and 9.1.1.12 do not require to detect cycles. In particular [[Insert]] states:

1. If x.[[Class]] ∈ {"text", "comment", "processing-instruction", "attribute"}, return
2. Let i = ToUint32(P)
3. If (ToString(i) is not equal to P), throw a TypeError exception
4. Let n = 1
5. If Type(V) is XMLList, let n = V.[[Length]]
6. If n == 0, Return
7. For j = x.[[Length]]-1 downto i, rename property ToString(j) of x to ToString(j + n)
8. Let x.[[Length]] = x.[[Length]] + n
9. If Type(V) is XMLList
     a. For j = 0 to V.[[Length-1]]
          i. V[j].[[Parent]] = x
         ii. x[i + j] = V[j]
              NOTE: the E4X data model does not enforce the constraint:
      ∀ x ∈ XML : x.[[InScopeNamespaces]] ⊇ x.[[Parent]].[[InScopeNamespaces]]. 
              However, implementations may at this point add namespaces from
              V[j].[[InScopeNamespaces]] to x or any ancestors of x. Likewise,
              implementations may at this point add namespaces from x to V[j]
              or any descendents of V[j].

10. Else
     a. Call the [[Replace]] method of x with arguments i and V
11. Return

> * Why undefined instead of null from XML.prototype.child?  Spec doesn't help
> here, so suspect a spec bug in part.

Because JSVAL_VOID is already used by xml_child_helper to indicate that child does not exist and I wanted a minimal patch. Note that rval in xml_child_helper represent local jsval in xml_child which is never exposed to scripts. 
(In reply to comment #6)
> > * Is this a spec bug?
> 
> [[Insert]] and [[Replace]] definitions, 9.1.1.11 and 9.1.1.12 do not require to
> detect cycles.

Actually we added cycle checking to Edition 2 of ECMA-357 (what went to ISO), IIRC.  I'll see if I can find the .pdf.

Ok, patch looks good, thanks for answers.  Stamping in a minute.

/be
Comment on attachment 241938 [details] [diff] [review]
Fix v1

r=me, thanks again.

/be
Attachment #241938 - Flags: review?(brendan) → review+
Comment on attachment 241938 [details] [diff] [review]
Fix v1

Should this follow the patch for bug 354145 into 1.8.0.x?

/be
I committed the patch from comment 4 to the trunk:

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.132; previous revision: 3.131
done
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment on attachment 241938 [details] [diff] [review]
Fix v1

The patch applies to 1.8.0 branch as is.
Attachment #241938 - Flags: approval1.8.0.9?
Flags: in-testsuite+
fix the initial value of the actual variable.
Attachment #242052 - Attachment is obsolete: true
verified fixed 1.9 20061012 windows/linux
Status: RESOLVED → VERIFIED
This is not a cleanup or null-pointer-only bug. The problem is that Insert in jsxml.c uses XMLArrayInsert. That in turn calls XMLArraySetCapacity which calls realloc without clearing the extra allocated tail. Then insert in case of tail insert simply increases the array length so junk elements become a part of the array. Since prior this bug fix a script can prevent overwriting them, this allows  for the script to reinterpret as JSXML* a memory that realloc brought, like in the following example:

 @callisto~/m/1.8.1/mozilla/js/src> cat ~/m/files/y2.js 
var xml = <xml><child><a/></child></xml>;
var child = xml.child[0];

try {
  child.insertChildBefore(null, xml.child);
  throw "insertChildBefore succeded when it should throw an exception";
} catch (e if e instanceof Error) {
}

var list = child.*;
var text = uneval(list[1]);
if (text !== "undefined")
  throw "child got unecxpected second element: "+text;
@callisto~/m/1.8.1/mozilla/js/src> ./Linux_All_DBG.OBJ/js ~/m/files/y2.js 
Assertion failure: (uint32)2 < JS_MIN(((obj)->map)->freeslot, ((obj)->map)->nslots), at jsapi.c:2358
Trace/breakpoint trap (core dumped)
@callisto~/m/1.8.1/mozilla/js/src>

On the trunk with the patch fixed the inserted elements are always cleared.
(In reply to comment #16)
> On the trunk with the patch fixed the inserted elements are always cleared.

I meant "with the patch landed". That is, the committed fix solved this problem as well.
additional Igor test from comment 16.
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9+
Comment on attachment 241938 [details] [diff] [review]
Fix v1

a=mconnor on behalf of drivers for 1.8.0.9 and 1.8.1.1 checkin
Attachment #241938 - Flags: approval1.8.1.1?
Attachment #241938 - Flags: approval1.8.1.1+
Attachment #241938 - Flags: approval1.8.0.9?
Attachment #241938 - Flags: approval1.8.0.9+
I committed the patch from comment 4 to MOZILLA_1_8_BRANCH:

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.50.2.53; previous revision: 3.50.2.52
done
Keywords: fixed1.8.1.1
I committed the patch from comment 4 to MOZILLA_1_8_0_BRANCH:

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.50.2.15.2.24; previous revision: 3.50.2.15.2.23
done
Keywords: fixed1.8.0.9
verified fixed 1.8.0.9, 1.8.1.1 20061108 windows/linux/mac*
Whiteboard: [sg:moderate] (mem snooping?)
> Status Whiteboard: [sg:moderate] (memsnooping?)

See coment 16 where the example crashes due to a reinterpretation of the garbge that fills the tail of realloced memory as JSXML.  
Thanks, missed that comment (was triaging a bunch of bugs taking best guess)
Whiteboard: [sg:moderate] (mem snooping?) → [sg:critical]
Group: security
/cvsroot/mozilla/js/tests/e4x/Regress/regress-356238-01.js,v  <--  regress-356238-01.js

/cvsroot/mozilla/js/tests/e4x/Regress/regress-356238-02.js,v  <--  regress-356238-02.js

/cvsroot/mozilla/js/tests/e4x/Regress/regress-356238-03.js,v  <--  regress-356238-03.js

modified to remove conditional catch clause to remove SpiderMonkey only extension.
bug 375801 changed uneval behavior.

/cvsroot/mozilla/js/tests/e4x/Regress/regress-356238-03.js,v  <--  regress-356238-03.js
new revision: 1.3; previous revision: 1.2
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: