Scope Bug still existing?

I have the bad feeling, that this bug was never fixed.

Also this
oauthjs/node-oauth2-server#629 (comment)

@Uzlopak

This is definitely something that has to be covered in tests as well. @Uzlopak what is the second comment actually saying?

I think the issue is still existing. Did not tested though... But only by reading the code it seems trivial to hack such an oauth2-server-implemenation.

In generateAccessToken of the model you get the scope passed. As an implementor you expect that the scope is validated and stripped from invalid or not allowed scopes.

This bug will not occur if you check the access token as simple identifier to look up in the database on every call or dont use scopes. But in cases like JWT and stateless resource server you have a big security issue as you could login with "admin"-scope and till the refreshtoken is used to create a new access token you have the admin scope.

So instead of making a Promise all, first you have to sanitize the scope and then you could run the following generateAccessToken, generateRefreshToken in parallel.

Reported it to snyk

I wrote here what I wrote to snyk
oauthjs/node-oauth2-server#720

Awesome, but not much hope in them doing anything. In the meantime we can address the fix here.

@Uzlopak

I thought that the RFC implementation is NOT for JWTs, this only handles returning access tokens. No where in the RFC does it mention how to handle JWTs, in which case, how are you using JWTs @Uzlopak ?

RFC 6750 makes it mandatory to use specific characters, which base64 uses, for the Bearer Token.
https://datatracker.ietf.org/doc/html/rfc6750#section-2.1

So they always had in mind, that you could use the Bearer Token for transporting information.

https://www.oauth.com/oauth2-servers/access-tokens/self-encoded-access-tokens/

And this specific "JSON Web Token (JWT) Profile for OAuth 2.0 Client Authentication and Authorization Grants"
https://datatracker.ietf.org/doc/html/rfc7523
https://datatracker.ietf.org/doc/html/draft-ietf-oauth-access-token-jwt

Also this:
https://oauth.net/2/jwt/

This seems to be high priority to me. Am I correct, that this is related to #71 ?

@jankapunkt

@jankapunkt

Well #71 would be a breaking change as we would modify the model. There could be implementations, were it the implementer wants to have partial valid scopes without throwing errors... for what reason...

This issue can be patched in the saveToken functions of ClientCredentialsGrant, PasswordGrant and AuthorizationCodeGrant by running the validateScope of the model separately and use the validated scope in the following code.

E.g.

node-oauth2-server/lib/grant-types/authorization-code-grant-type.js

Line 177 in 485147f

    
           AuthorizationCodeGrantType.prototype.saveToken = function(user, client, authorizationCode, scope) {

 AuthorizationCodeGrantType.prototype.saveToken = function(user, client, authorizationCode, requestedScope) {
  const scope = this.validateScope(user, client, requestedScope);
  var fns = [
    this.generateAccessToken(client, user, scope),
    this.generateRefreshToken(client, user, scope),
    this.getAccessTokenExpiresAt(),
    this.getRefreshTokenExpiresAt()
  ];

  return Promise.all(fns)
    .bind(this)
    .spread(function(accessToken, refreshToken, accessTokenExpiresAt, refreshTokenExpiresAt) {
      var token = {
        accessToken: accessToken,
        authorizationCode: authorizationCode,
        accessTokenExpiresAt: accessTokenExpiresAt,
        refreshToken: refreshToken,
        refreshTokenExpiresAt: refreshTokenExpiresAt,
        scope: scope
      };

      return promisify(this.model.saveToken, 3).call(this.model, token, client, user);
    });
};

Actually sonarqube was kind of hinting to this bug as it was classifying the "reuse" of the scope variable as code smell. Thats why I renamed the parameter from "scope" to "requestedScope".

@Uzlopak

@Uzlopak can we approach this issue from a different perspective? I first like to create a few tests (which could also be part of the new security test suite, see #65) before implementing the fixes. Can you help me out with a few ones? Maybe creating a new branch fix-scope-bug and add one or two?

I am currently too far in my implementation and tbh I have some deadlines to meet. Last week I printed out RFC 6749 and I implement currently an OAuth2-Authorization Server based on fastify. Everytime I implement something according to spec, i mark the corresponding part in the RFC as done. I use oauth2-server just as inspiration on how to implement it and thats why I am finding those issues, when the code does not fit the corresponding code in oauth2-server. I try to write for everything a unit test. So I am half way through and have 100% test coverage. So the chance for a fuck up shrinks...

But still I would actually prefer if we would first write an InMemoryModel. This is what I wrote for my code (but not used it for unit tests, yet). It would make sense to implement this according your needs and then write some unit tests. You could initialize the model with addUser and addClient in the mocha beforeEach-hook and run clear on the afterEach-hook.

But like I said: I did not use it yet. So maybe it does not work as expected.

import { IClient } from "../interfaces/IClient";
import { IUser } from "../interfaces/IUser";
import generateRandomToken from "../utils/generateRandomToken";
import { IToken } from "../interfaces/IToken";
import { IAuthorizationCode, IModel } from "../interfaces";

const authorizationCodeMap: Map<string, IAuthorizationCode> = new Map();
const clientMap: Map<string, IClient> = new Map();
const accessTokenMap: Map<string, IToken> = new Map();
const refreshTokenMap: Map<string, IToken> = new Map();
const userMap: Map<string, IUser> = new Map();

if (process.env.NODE_ENV === "production") {
	console.error("This model is not designed to be used in production. Terminating for security.");
	process.exit(1);
}

export const InMemoryModel: IModel = {
	getAccessToken: async (accessToken) => {
		return accessTokenMap.get(accessToken)!;
	},
	getAuthorizationCode: async (authorizationCode) => {
		return authorizationCodeMap.get(authorizationCode);
	},
	getClient: async (clientId, clientSecret) => {
		const client = clientMap.get(clientId);
		if (!client) {
			return;
		}
		if (client.secret && client.secret === clientSecret) {
			return client;
		}
		if (client) {
			return client;
		}
		return;
	},
	revokeAuthorizationCode: async (code) => {
		return authorizationCodeMap.delete(code.authorizationCode);
	},
	saveAuthorizationCode: async (code, client, user) => {
		authorizationCodeMap.set(code.authorizationCode, code);

		if (code.expiresAt) {
			setTimeout(function () { authorizationCodeMap.delete(code.authorizationCode!); }, (code.expiresAt.getTime() - Date.now()));
		}
		return code;
	},
	revokeRefreshToken: async (refreshToken) => {
		return refreshTokenMap.delete(refreshToken.refreshToken!);
	},
	generateAccessToken: async (client, user, scope) => {
		return generateRandomToken(32);
	},
	generateRefreshToken: async (client, user, scope) => {
		return generateRandomToken(32);
	},
	generateAuthorizationCode: async (client, user, scope) => {
		return generateRandomToken(32);
	},
	getRefreshToken: async (refreshToken) => {
		return refreshTokenMap.get(refreshToken);
	},
	getUser: async (username, password) => {
		const user = userMap.get(username);
		return (
			(user && user.password === password) 
			? user
			: undefined
		);
	},
	saveToken: async (token, client, user) => {
		const tokenData = {
			accessToken          : token.accessToken,
			accessTokenExpiresAt : token.accessTokenExpiresAt,
			refreshToken         : token.refreshToken,
			refreshTokenExpiresAt: token.refreshTokenExpiresAt,
			scope                : token.scope,
			client               : client,
			user                 : user,
		};

		if (token.accessToken) {
			accessTokenMap.set(token.accessToken, tokenData);
			setTimeout(function () { accessTokenMap.delete(token.accessToken); }, (token.accessTokenExpiresAt.getTime() - Date.now()));
		}
		if (token.refreshToken) {
			refreshTokenMap.set(token.refreshToken, token);
			setTimeout(function () { refreshTokenMap.delete(token.refreshToken!); }, (token.refreshTokenExpiresAt!.getTime() - Date.now()));
		}

		return tokenData;
	},
};

export function addClient(client: IClient) {
	clientMap.set(client.id, client);
}

export function addUser(user: IUser) {
	userMap.set(user.id, user);
}

export function clear() {
	userMap.clear();
	clientMap.clear();
	authorizationCodeMap.clear();
	accessTokenMap.clear();
	refreshTokenMap.clear();
}

export default InMemoryModel;

@Uzlopak

@Uzlopak - can you please check on #103 if this fixes the issue?

Fixed in #229 for 4.x & #228 for development

Navigation Menu

Repository navigation

Scope Bug still existing? #64

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Scope Bug still existing? #64

Description

Activity

Uzlopak commented on Nov 13, 2021

jankapunkt commented on Nov 13, 2021

Uzlopak commented on Nov 16, 2021

Uzlopak commented on Nov 16, 2021

dannyreg commented on Nov 16, 2021

dannyreg commented on Nov 16, 2021

Uzlopak commented on Nov 16, 2021

jankapunkt commented on Nov 17, 2021

Uzlopak commented on Nov 17, 2021

jankapunkt commented on Nov 17, 2021

Uzlopak commented on Nov 17, 2021

jankapunkt commented on Dec 10, 2021

jorenvandeweyer commented on Aug 26, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions