Sometimes Security Decorators are just decoration

Earlier this year, I identified multiple security vulnerabilities that are related to the (insecure) usage of Security Decorators. I thought these examples were worth sharing more broadly, so here they are.

Introduction

Put simply, Decorators allow developers to add metadata to an object, method or property, so that other code can act on it. Think of it as hashtags for code. An easily understandable example is the usage of decorators to generate the OpenAPI / Swagger documentation of an API.

1@ApiTags('users')
2@Controller('users')
3export class UsersController {
4    @ApiOperation({ summary: 'List all users' })
5    @ApiResponse({ status: 200, type: [UserResponseDto], description: 'OK' })
6    @Get()
7    findAll(): PaginatedUsers {
8        return { data: [], total: 0 };
9  }

Do not confuse decorators with the decorator design pattern. Decorators are a language feature from TypeScript / JavaScript. Similar features exist in other languages, for example Java (Annotations) or .NET and PHP (Attributes).

When it comes to security, decorators mainly serve two purposes:

• Authentication / Authorization (for example to restrict access to a controller or method)
• Input validation (by adding type or pattern information)

When performing a code review, auditors often blindly trust these decorators and don’t question how the validation is actually implemented in the underlying framework or if they are checked at all. Depending on the used library and application configuration, this can cause some serious issues. Here are three examples, two for NestJS (TypeScript), one for Spring (Java):

Example: class-validator

The class-validator library provides Decorator based validation routines that are used to validate object properties. The following code snippet is a stripped down version from their readme.md:

 1export class Post {
 2  @Length(10, 20)
 3  title: string;
 4
 5  @Contains('hello')
 6  text: string;
 7
 8  @IsInt()
 9  @Min(0)
10  @Max(10)
11  rating: number;
12
13  @IsEmail()
14  email: string;
15}

By default, class-validator does not recurse into nested objects. When it goes through the metadata of a class, a property holding another object is treated as a black-box, unless the @validateNested() decorator is used. From the project readme:

If your object contains nested objects and you want the validator to perform their validation too, then you need to use the @ValidateNested() decorator.

Here a minimal example:

 1import { IsEmail, IsString, Length, validate } from 'class-validator';
 2
 3// Define the class address, using validators for the different properties
 4class Address {
 5  @IsString() @Length(1, 100) street: string;
 6  @IsString() country: string;
 7}
 8
 9class CreateUserDto {
10  @IsEmail() email: string;
11
12  // Holding an Address instance
13  // The validators in the Address class are not processed due to
14  // missing @ValidateNested()
15  address: Address;
16}

The missing @ValidateNested() decorator turns the validators in the Address class into dead code. For example, the following payload survives without issues (using a NoSQL injection as an example here):

1{
2  "email": "doesntmater@mogwailabs.de",
3  "address": { "street": 12345, "country": { "$ne": null }, "isAdmin": true }
4}

This is due to multiple reasons (at least what I assume):

Type information is erased when the TypeScript code gets compiled into JavaScript. At runtime, the class-validator therefore cannot determine what to validate against, without the developer providing this information in another way. If the actual class is unknown at runtime, the validator would silently do nothing. This is worse than requiring an explicit decorator.

The other reason is in the general design of the validator. class-validator is based on the idea that a developer adds decorators to enforce validation. A property without any decorator is therefore not validated. Auto-evaluating properties would break this design model.

So, here a secure version of the previous example. It uses the class-transformer library to pass the type information and enforces validation of the address through @ValidateNested(). Please note that class-validator provides additional features (such as blocking unknown properties), which are not implemented here.

 1import { IsEmail, IsString, Length, validate } from 'class-validator';
 2
 3// Define the class address, using validators for the different properties
 4class Address {
 5  @IsString() @Length(1, 100) street: string;
 6  @IsString() country: string;
 7}
 8
 9class CreateUserDto {
10  @IsEmail() email: string;
11
12  @ValidateNested()
13  @Type(() => Address)
14  address: Address;
15}

Example: Validation order in nest-keycloak-connect

Within NestJS you can implement basic authorization using a @Roles decorator and a custom guard that runs as middleware. In the reference implementation, the decorator can be applied on the class or on methods within the class:

 1@Roles('user')          // class-level default
 2@Controller('users')
 3export class UsersController {
 4  @Get()
 5  findAll() {}           // requires a 'user' role (from the class decorator)
 6
 7  @Roles('admin', 'editor') // handler-level decorators
 8  @Post()
 9  create() {}            
10}

If both (class- and handler-level decorators) are used, we need to determine which one should actually be applied. NestJS provides two strategies:

• getAllAndMerge - the roles defined on the class and handler level are combined, so the user would need to satisfy all roles.
• getAllAndOverwrite - the handler-level decorator replaces the class-level one. This is good for a “class default and handler exception” scenario as in the previous example, where only users with the roles “admin” or “editor” should be able to create new users.

These strategies are implemented within two reflector methods. In the case of getAllAndOverwrite, the order in which the objects are passed to this method is important and not super intuitive.

From the NestJS documentation:

If your intent is to specify ‘user’ as the default role and override it selectively for certain methods, use the getAllAndOverride() method. It returns the first defined value, checking the targets in the order you pass them:

Here the role.guard.ts implementation from the official NestJS example.

 1@Injectable()
 2export class RolesGuard implements CanActivate {
 3  constructor(private reflector: Reflector) {}
 4
 5  canActivate(context: ExecutionContext): boolean {
 6    const requiredRoles = this.reflector.getAllAndOverride<Role[]>(ROLES_KEY, [
 7      context.getHandler(),
 8      context.getClass(),
 9    ]);
10    if (!requiredRoles) {
11      return true;
12    }
13    const { user } = context.switchToHttp().getRequest();
14    return requiredRoles.some((role) => user.roles?.includes(role));
15  }
16}

We recently audited an application that uses the NestJS library nest-keycloak-connect. As the name indicates, this library can be used to integrate Keycloak’s authorization service into a NestJS-based application. As part of the integration, nest-keycloak-connect provides its own role guard.

Within the 1.0 branch you can find the following code. Notice the order of the parameters that are passed within the array:

57} else if (roleMerge == RoleMerge.OVERRIDE) {
58  const roleMetaData =
59    this.reflector.getAllAndOverride<RoleDecoratorOptionsInterface>(
60      META_ROLES,
61      [context.getClass(), context.getHandler()],
62    );

This role guard changes the order, thus the role assigned to the class overwrites the role that is assigned to the handler. So in our previous example, all users would be able to create new users, not just administrators / editors!

I assume that the author of the library simply got the order wrong during the role-guard development. The order has been changed in the master branch, but (due to compatibility reasons) it remains in the v1 release.

Example: Validation of @Secured Annotation in Spring Security

The previous two examples showed issues in NestJS, however the underlying problem can also be found in other languages / frameworks. For example, Spring Security provides different annotations that allow fine grained permission checks on the method level.

One of the most basic annotations is the @Secured annotation, which can be used to restrict method access to basic roles. Here examples from the official documentation:

1@Secured({ "ROLE_USER" })
2public void create(Contact contact);
3
4@Secured({ "ROLE_USER", "ROLE_ADMIN" })
5public void update(Contact contact);
6
7@Secured({ "ROLE_ADMIN" })
8public void delete(Contact contact);

To enforce these roles you normally use @EnableMethodSecurity (the successor of @EnableGlobalMethodSecurity). @EnableMethodSecurity wires up several authorization interceptors, but only for the annotation families that are explicitly enabled. Each configuration flag activates a different set of annotations:

So if your code just contains something like the following example snippet, the authorization interceptor for the @Secured annotation is not activated and thus the roles are not enforced.

@Configuration
@EnableMethodSecurity(prePostEnabled = true)

Here the correct version, which loads the interceptor to enforce @Secured annotations.

@Configuration
@EnableMethodSecurity(prePostEnabled = true, secured = true)

Can my LLM find this?

Of course it can, if you ask for it. For example consider the prompt:

Provide me an overview of the existing controllers / handlers and the necessary access permissions to use them.

Based on our experience, this will normally not reveal such issues as the LLM steps through your code itself and does not inspect the actual interceptors / middleware that are responsible for enforcing the permission. Depending on your setup, the actual code (from the library) might also not be there. If the LLM does not have this knowledge pre-trained, or is not able to download the library in use, it is possible that issues similar to the described examples will be missed.

If you ask for an in depth audit of the permission system, those things normally will get discovered.

Conclusion

As I pointed out at the beginning of the post, decorators only add metadata to a class or a method, not actual code itself. When auditing code, don’t blindly assume that they work as intended. Take the time to audit and understand the code that actually uses this metadata.

Thanks to Hannes Köttner on Unsplash for the title picture.