documentation: default settings for class-validator 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: https://github.com/advisories/GHSA-fj58-h2fr-3pp2.

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

[fhp]

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.

Nothing wrong with well-communicated breaking changes. On the other hand, audit scanners that are flagging critical issues, that are known for about two years, easy to fix, but still not patched, are rather annoying.

And as you have not yet reached version v1.0.0, version v0.14.0 is considered breaking (Semver spec). So no need to use/(waste?) the v1.0.0 release to fix this.

[horrendo]

Is it possible for class-transformer to remove 'harmful' attributes without the sledgehammer forbidUnknownValues? We have used class-validator extensively in our projects and in most cases decorate expected payloads completely. However in some cases we have a 'send us anything else and we'll just store it, and you can retrieve it as required' approach. Enabling forbidUnknownValues would be a breaking change for us.

[fhp]

@horrendo You might be confusing forbidUnknownValues with forbidNonWhitelisted. However the docs are not very clear about this, and I did not check the code.

[cagrisungur]

I'm using this on nestJS with DTO's. Is this enough to adding this main.ts to avoid from this vulnerability? app.useGlobalPipes(new ValidationPipe({ forbidUnknownValues: true }));

[horrendo]

@horrendo You might be confusing forbidUnknownValues with forbidNonWhitelisted. However the docs are not very clear about this, and I did not check the code.

Thanks @fhp , that's very helpful. You're right, the docs aren't very clear. From what I can tell, setting validation options to:

        const options: ValidatorOptions = {
            whitelist: true,
            forbidNonWhitelisted: false,
            forbidUnknownValues: true
        };

Will allow additional attributes to a class with at least one validation decorator but will throw an error on a completely undecorated class. So my question is, will this approach definitely avoid the potential vulnerability?

[cagrisungur]

Is there a something more to do using class-validator with DTO and Swagger?. I've added new ValidationPipe({ transform: true, forbidUnknownValues: true, forbidNonWhitelisted: true, }),

to my mainTS but i still can send unwanted props.

[horrendo]

Is there a something more to do using class-validator with DTO and Swagger?.

Did you set whitelist: true in the options?

[cagrisungur]

Is there a something more to do using class-validator with DTO and Swagger?.

Did you set whitelist: true in the options?

app.useGlobalPipes( new ValidationPipe({ forbidUnknownValues: true, forbidNonWhitelisted: true, whitelist: true, }), );

Yeah my main ts like this. But still the same.

[AnandChowdhary]

Yes, if you're using NestJS DTOs, this is sufficient and in most cases you don't need to do much else:

  app.useGlobalPipes(
    // Globally validate DTOs
    new ValidationPipe({
      // Strip data of properties without decorators
      whitelist: true,

      // Throw an error if non-whitelisted values are provided
      forbidNonWhitelisted: true,

      // Throw an error if unknown values are provided
      forbidUnknownValues: true,
    })
  );

[cagrisungur]

Yes, if you're using NestJS DTOs, this is sufficient and in most cases you don't need to do much else:

  app.useGlobalPipes(
    // Globally validate DTOs
    new ValidationPipe({
      // Strip data of properties without decorators
      whitelist: true,

      // Throw an error if non-whitelisted values are provided
      forbidNonWhitelisted: true,

      // Throw an error if unknown values are provided
      forbidUnknownValues: true,
    })
  );

Exactly what i did. But it is not throwing me to error. Is it normal?

[iwt-gregorpoloczek]

• When using class-transformer 's plainToClass, __proto__ seems not to be overwritten, and consecutive validation works just fine, without any issues. We tested it with class-transformer 0.4.0.
• Since NestJS uses class-transformer internally, there might not be an issue at all for NestJS based API.