Input-validation bypass vulnerability

[xiaofen9]

We found that the input validation in routing-controllers can be bypassed. With this vulnerability, attackers can launch SQL Injection, XSS attacks by injecting malicious inputs.

routing-controllers use class-validator to validate user-input. However, an attacker can corrupt a critical internal attribute used by class-validator (i.e., constructor) by injecting an additional attribute to the user-input. The corruption can be done because routing-controller uses the class-transformer to convert user-input to the validation class instance, and the conversion will also overwrite the previous internal attribute if it exists in the user-input.

Proof of Concept:
Before corruption

After corruption

This issue goes all the way down to the underlying lib (class-validator) used by routing-controller, and we have reported this issue to this lib. However, just to be safe, my suggestion is that routing-controller should also filter proto attribute before invoking class-validator since it is an internal attribute used by class-validator and should never appear in user-input.

[xiaofen9]

Thanks for the quick response.
The following link points to the actual location of where the vulnerable code is:

routing-controllers/src/ActionParameterHandler.ts

Line 147 in aae917b

value = await this.validateValue(value, param);

I've consulted the class-validator contributors and their suggestion is to use forbidUnknownValues option when invoking validator. I think this is good enough to fix this bug.

typestack/class-validator#438 (comment)

EDIT a slightly better (?) monkeypatch that eliminates cause and not effect.

export default function fixValidation() {
    const original = JSON.parse
    JSON.parse = function (obj, reviver) {
        return original.call(this, obj, (key, value) => {
            if (key === '__proto__') {
                return undefined
            }
            if (reviver) {
                return reviver(key, value)
            }
            return value
        })
    }
}

Monkey-patch I use that somehow works, but breaks with complex cases

// @ts-nocheck
/* eslint-disable */
import { ActionParameterHandler } from 'routing-controllers/ActionParameterHandler'
import { BadRequestError } from 'routing-controllers'
import { ValidationExecutor } from 'class-validator/validation/ValidationExecutor'


export default function fixValidation() {
    // top-level guys
    let validateValue = ActionParameterHandler.prototype.validateValue
    ActionParameterHandler.prototype.validateValue = function (value, paramMetadata) {
        if (typeof value === 'object' && !(value instanceof paramMetadata.targetType)) {
            throw new BadRequestError('Malformed request')
        }

        return validateValue.call(this, value, paramMetadata)
    }

    // nested guys
    const execute = ValidationExecutor.prototype.execute
    ValidationExecutor.prototype.execute = function (object, targetSchema, validationErrors) {
        if (!this.validatorOptions) {
            this.validatorOptions = {}
        }
        this.validatorOptions.forbidUnknownValues = true

        return execute.call(this, object, targetSchema, validationErrors)
    }
}

[jotamorais]

Watching typestack/class-validator#438 (comment)

[jkiyo]

Is that still an issue on the latest version available?

I'm running a simple test, I'm missing something?
https://gist.github.com/jkiyo/735c7f363d469d777be0d5767e9c9042

Versions:

    "class-transformer": "0.3.1",
    "class-validator": "0.12.2",
    "routing-controllers": "^0.9.0",

Edit:
It looks like class-transformer has this issue fixed on 0.3.1 by cleaning pollution from objects.
routing-controllers@^0.9.0 already requires the fixed version of class-transformer.

[attilaorosz]

Is this still relevant?