fix: default settings allows arbitrary bypass vulnerability

[xiaofen9]

With this vulnerability, an attacker can bypass any security checks enforced by class-validator.

When class-validator is used to validate user-input, the attributes in the user-input object will be transformed into the validation class instance.
However, the transforming procedure will overwrite the internal attribute of validation class instance (e.g., constructor attribute) if the attacker injects an attribute with the same name into user-input. Once this internal attribute being overwritten, class-validator will be bypassed.

PoC

import {validate, validateOrReject, Contains, IsInt, Length, IsEmail, IsFQDN, IsDate, Min, Max} from "class-validator";
import {plainToClass} from "class-transformer";

class Post {

    @Length(10, 20)
    title: string;

    @Contains("hello")
    text: string;

    @IsInt()
    @Min(0)
    @Max(10)
    rating: number;

    @IsEmail()
    email: string;

    @IsFQDN()
    site: string;

    @IsDate()
    createDate: Date;

}

let userJson = JSON.parse('{"title":1233, "__proto__":{}}');  // a malformed input
let users = plainToClass(Post, userJson);

validate(users).then(errors => { // errors is an array of validation errors
    if (errors.length > 0) {
        console.log("validation failed. errors: ", errors);
    } else {
        console.log("validation succeed");
    }
});

Our suggestion is that class-validator should check the integrity of the constructor: if it is being corrupted, the validation should automatically fail.

[vlapo]

Transformation plain json object into class instance is not class-validator responsibility. I think this should be handled on class-transformer side.

[xiaofen9]

Thanks for the comment. we will report this to class-transformer since the transformer and validator are usually used together, which leads to vulnerable logics.

Even if class-transformer adds a patch for this issue, I think we might still want to add a simple check to the class-validator because we cannot guarantee that developers will only use class-transformer to transform those plain JSON object. For example, it is found that other methods (e.g., Object.assign() [1]) are used to transform user-input before validation. They will also invalidate the class-validator once attackers inject the same payload. From the perspective of these transformers, they just correctly transform all attributes.

If we can check whether the constructor of the validation class instance is empty or not, this bug will be fixed.

[1] https://github.com/MichalLytek/type-graphql/blob/44e12eefc97ff51cfa795502bb91ff8fcc9804b5/src/helpers/types.ts#L104

[vlapo]

I think for this purpose exists option forbidUnknownValues. It will return validation error in case class-validator dont know target class of validated object.

I know the name of option is missleading and also we do not have proper documentation.

Example:
https://stackblitz.com/edit/class-validator-boilerplate-gh518?file=index.ts

[xiaofen9]

Thanks for the comment. I think we should at least mention this option somewhere in readme right?

[vlapo]

Of course we need doc improvement in this area.

[xiaofen9]

Yes... because class-validator will be vulnerable to the mentioned attack in its default settings. We should let developers know this undocumented forbidUnknownValues option and use it when handling user-inputs or enforcing it as a default option when handling user-input...

[jmtelljohann]

Is there a reason why forbidUnknownValues shouldn't be default? It seems like an issue that class-validator has a security vulnerability in it's default state.

[NicoleG25]

@vlapo was this issue ever addressed?
Please note that CVE-2019-18413 was assigned.

[lalit0024]

@vlapo @xiaofen9 What's going on with this issue? Can you please provide an update.
Due to assigned CVE-2019-18413, corporate repo like nexus and other scanning tools prohibiting the library use being flagged as vulnerable and creating lots of issues for developers.

[NoNameProvided]

Is there a reason why forbidUnknownValues shouldn't be default? It seems like an issue that class-validator has a security vulnerability in it's default state.

Historical reason, it is a breaking change.

[NoNameProvided]

Note to self: handle in class-transformer as well before closing this issue.

[donnd-t]

Does "forbidUnknownValue: true" prevent this issue entirely or only mitigate it?

I want to start using class-validator in my project but it is concerning that such an important security issue has remained open since October last year.

I will have trouble convincing my security department that everything is ok when this package is reported as "High Risk" in their reports. An actual fix to the package would be very much preferred.

[vinu-vrize]

@NoNameProvided when would this be resolved, any particular timeline planned for it.

[NoNameProvided]

Does "forbidUnknownValue: true" prevent this issue entirely or only mitigate it?

@donnd-t It prevents this issue. The forbidUnknownValue instructs the class-validator to throw when it encounters any value which it doesn't know. The attack method demonstrated in the example would exactly do this, reset the prototype of the class so class-validator would not recognize it, however when the above-mentioned setting is enabled it will throw an error.

[NoNameProvided]

when would this be resolved, any particular timeline planned for it.

I am kind of reluctant pushing out breaking changes one by one, my hope is to collect them and make one release where all the quirks are disabled by default, but there is so much stuff to do.

[mkmita]

Hey, just wondering if we have any plan to fix this in the future?

[eylonmalin]

@NoNameProvided I don't really understand why not set the default value of forbidUnknownValue to true by default. If it's a breaking change you can release a major release.
Running static security scans is a standard in the industry, and this vulnerability prevent using class-validator library.

[es-lynn]

Any update on this? Same issue; can't use this library because it's being flagged by Nexus

[AnandChowdhary]

Lots of people are going to come here now that GitHub has opened this as a critical severity: GHSA-fj58-h2fr-3pp2.

What will a PR to fix this look like? Should we set forbidUnknownValues: true by default?

[mikeguta]

Pull request #1403 is now ready for review: changes the default value for forbidUnknownValues to true.

I'd appreciate it if maintainers could take a look at it in the next few days.

[mikeguta]

Hi @NoNameProvided
Are you or any other maintainers available for reviews of PR #1403 ?

[katrotz]

The class-transformer + class-validator setup is not vulnerable to this issue.
The Object.assign + class-validator is vulnerable, but only because Object.assign's shortcomings.
I've created a couple of tests to demo these different scenarios here

[capc0]

It might be a better approach to let forbidUnknownValues remain false by default and create a new option forbidConstructorOverwrite (true by default) that strips the __proto__ part out of the JSON before passing it to the remaining validation logic?

[mlapibp]

Any update on this issue ?
The option "forbidConstructorOverwrite" proposed by @capc0 might be a nice approach

[debragail]

I'd like to fix this it's showing up in my tests as critical: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-18413

[fullstackzach]

Any updates or ETA on a fix for this? Ditto on appearing as a critical vuln in our project.

[NoNameProvided]

Related discussion: #1422.

The provided POC doesn't result in a bypass since at least 2021 October 22. (So for almost a year)

[NoNameProvided]

The proposed fix of changing forbidUnknownValues to true by default has landed in #1798. I am closing this for now.

The progress of making the advisories marked as fixed can be tracked here: #1422

[NoNameProvided]

This feature/fix has been released in 0.14.0.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.