Scope Bug still existing?

[Uzlopak]

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

oauthjs/node-oauth2-server#550

[Uzlopak]

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

[jankapunkt]

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

[Uzlopak]

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.

[Uzlopak]

Reported it to snyk

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

[dannyreg]

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

[dannyreg]

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 ?

[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/

[jankapunkt]

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

[Uzlopak]

@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".

[jankapunkt]

@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?