Closed Bug 1358300 Opened 4 years ago Closed 4 years ago

Harmless (?) underflow in ArrayBufferObject::create()

Categories

(Core :: JavaScript Engine, enhancement)

52 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: q1, Assigned: sfink)

Details

(Keywords: csectype-intoverflow)

Attachments

(1 file)

ArrayBufferObject::create() (js\src\vm\ArrayBufferObject.cpp) can cause an integer underflow. The underflowed value is used only to select the "GC object kind", so the underflow appears harmless. But since I know little about the JS code, I don't know whether my conclusion is correct, so I have made this bug private.

The bug is in line 1069, which underflows if |nbytes| == 0:

1032: ArrayBufferObject*
1033: ArrayBufferObject::create(JSContext* cx, uint32_t nbytes, BufferContents contents,
1034:                           OwnsState ownsState /* = OwnsData */,
1035:                           HandleObject proto /* = nullptr */,
1036:                           NewObjectKind newKind /* = GenericObject */)
...
1051:     size_t reservedSlots = JSCLASS_RESERVED_SLOTS(&class_);
1052: 
1053:     size_t nslots = reservedSlots;
1054:     bool allocated = false;
1055:     if (contents) {
...
1065:     } else {
1066:         MOZ_ASSERT(ownsState == OwnsData);
1067:         size_t usableSlots = NativeObject::MAX_FIXED_SLOTS - reservedSlots;
1068:         if (nbytes <= usableSlots * sizeof(Value)) {
1069:             int newSlots = (nbytes - 1) / sizeof(Value) + 1;
1070:             MOZ_ASSERT(int(nbytes) <= newSlots * int(sizeof(Value)));
1071:             nslots = reservedSlots + newSlots;
1072:             contents = BufferContents::createPlain(nullptr);
1073:         } else {
1074:             contents = AllocateArrayBufferContents(cx, nbytes);
1075:             if (!contents)
1076:                 return nullptr;
1077:             allocated = true;
1078:         }
1079:     }
1080: 
1081:     MOZ_ASSERT(!(class_.flags & JSCLASS_HAS_PRIVATE));
1082:     gc::AllocKind allocKind = GetGCObjectKind(nslots);
1083: 
1084:     AutoSetNewObjectMetadata metadata(cx);
1085:     Rooted<ArrayBufferObject*> obj(cx,
1086:         NewObjectWithClassProto<ArrayBufferObject>(cx, proto, allocKind, newKind));

The POC for this bug is simple. Just create a script with the following line:

   var a = new ArrayBuffer(0);

Attach a debugger to FF, set a BP on create(), run the script, and watch line 1069 calculate |newSlots| == 0x20000000.
Group: core-security → javascript-core-security
Steve: is this a potential security problem or harmless as Ron is guessing?
Flags: needinfo?(sphink)
I believe Ron's analysis is correct. It does result in some minor space wastage; zero-length ArrayBuffers will now get the largest sort of object allocated for them (AllocKind::OBJECT16) and then not use any of that space (I guess that would be 128 bytes on 64-bit).

But any logic error like that in this code is scary. It's probably time to switch over to CheckedInt for everything here.

I don't think this bug needs to be s-s.
Flags: needinfo?(sphink)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Group: javascript-core-security
Attachment #8863007 - Flags: review?(jwalden+bmo) → review+
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10f258c2d818
Fix underflow resulting in wasted space for 0-byte ArrayBuffer, r=Waldo
https://hg.mozilla.org/mozilla-central/rev/10f258c2d818
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.