Scope Bug still existing? #64
Comments
|
Also this |
|
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 |
|
Awesome, but not much hope in them doing anything. In the meantime we can address the fix here. |
|
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. 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" Also this: |
jankapunkt
added
security ❗
|
This seems to be high priority to me. Am I correct, that this is related to #71 ? |
|
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. 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". |
|
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; |
I have the bad feeling, that this bug was never fixed.
oauthjs/node-oauth2-server#550
The text was updated successfully, but these errors were encountered: