Merge commit from fork

fix: multiple PKCE vulnerabilities addressed

Author: jankapunkt

lib/grant-types/authorization-code-grant-type.js

@@ -3,7 +3,7 @@
 /*
  * Module dependencies.
  */
-
+const crypto = require('crypto');
 const AbstractGrantType = require('./abstract-grant-type');
 const InvalidArgumentError = require('../errors/invalid-argument-error');
 const InvalidGrantError = require('../errors/invalid-grant-error');
@@ -39,6 +39,9 @@ class AuthorizationCodeGrantType extends AbstractGrantType {
     }
 
     super(options);
+
+    // xxx: plain PKCE is only allowed if explicitly enabled
+    this.enablePlainPKCE = options.enablePlainPKCE === true;
   }
 
   /**
@@ -60,6 +63,10 @@ class AuthorizationCodeGrantType extends AbstractGrantType {
 
     const code = await this.getAuthorizationCode(request, client);
     await this.revokeAuthorizationCode(code);
+    // xxx: PKCE verification is done after revoking the code,
+    // so that a failed verification attempt consumes the code and prevents
+    // online brute-force guessing.
+    await this.verifyPKCE(request, code);
     await this.validateRedirectUri(request, code);
 
     return this.saveToken(code.user, client, code.authorizationCode, code.scope);
@@ -111,26 +118,49 @@ class AuthorizationCodeGrantType extends AbstractGrantType {
       throw new InvalidGrantError('Invalid grant: `redirect_uri` is not a valid URI');
     }
 
-    // optional: PKCE code challenge
+    return code;
+  }
 
+  /**
+   * Verify PKCE code_verifier against the stored code_challenge.
+   *
+   * This is called from handle() AFTER the authorization code has been
+   * revoked, so that a failed verification attempt consumes the code
+   * and prevents online brute-force guessing.
+   *
+   * @param request {Request}
+   * @param code {AuthorizationCodeData}
+   * @see https://datatracker.ietf.org/doc/html/rfc7636#section-4.6
+   */
+
+  verifyPKCE(request, code) {
     if (code.codeChallenge) {
+      const method = this.getCodeChallengeMethod(code.codeChallengeMethod);
+
+      if (!this.enablePlainPKCE && method === 'plain') {
+        throw new InvalidRequestError('Invalid request: `code_challenge_method` "plain" is not allowed; use "S256"');
+      }
+
       if (!request.body.code_verifier) {
         throw new InvalidGrantError('Missing parameter: `code_verifier`');
       }
 
+      if (!pkce.codeChallengeMatchesABNF(request.body.code_verifier)) {
+        throw new InvalidRequestError('Invalid parameter: `code_verifier`');
+      }
+
       const hash = pkce.getHashForCodeChallenge({
-        method: code.codeChallengeMethod,
-        verifier: request.body.code_verifier
+        verifier: request.body.code_verifier,
+        method
       });
 
       if (!hash) {
-        // notice that we assume that codeChallengeMethod is already
-        // checked at an earlier stage when being read from
-        // request.body.code_challenge_method
-        throw new ServerError('Server error: `getAuthorizationCode()` did not return a valid `codeChallengeMethod` property');
+        throw new ServerError('Server error: no valid hash algorithm available to verify `code_verifier`');
       }
 
-      if (code.codeChallenge !== hash) {
+      // xxx: Use timingSafeEqual to prevent against timing attacks when comparing
+      // the hash of the code_verifier to the stored code_challenge.
+      if (!this.hashesAreEqual(hash, code.codeChallenge)) {
         throw new InvalidGrantError('Invalid grant: code verifier is invalid');
       }
     }
@@ -140,8 +170,24 @@ class AuthorizationCodeGrantType extends AbstractGrantType {
         throw new InvalidGrantError('Invalid grant: code verifier is invalid');
       }
     }
+  }
 
-    return code;
+  hashesAreEqual(trusted, untrusted) {
+    const trustedBuf = Buffer.isBuffer(trusted) ? trusted : Buffer.from(trusted);
+    const untrustedBuf = Buffer.isBuffer(untrusted) ? untrusted : Buffer.from(untrusted);
+    const equalLength = trustedBuf.byteLength === untrustedBuf.byteLength;
+    // if the buffers are the same length, compare them,
+    // otherwise only compare with the trusted buffer but return false anyway
+    return crypto.timingSafeEqual(trustedBuf, equalLength ? untrustedBuf : trustedBuf) && equalLength;
+  }
+
+  getCodeChallengeMethod(method) {
+    if (method) {
+      return method;
+    }
+    // Per RFC 7636 §4.6, the default code challenge method is "plain".
+    // However, plain PKCE is not secure, so we only allow it if explicitly enabled.
+    return this.enablePlainPKCE ? 'plain' : 'S256';
   }
 
   /**

lib/handlers/authorize-handler.js

@@ -62,6 +62,7 @@ class AuthorizeHandler {
     this.allowEmptyState = options.allowEmptyState;
     this.authenticateHandler = options.authenticateHandler || new AuthenticateHandler(options);
     this.authorizationCodeLifetime = options.authorizationCodeLifetime;
+    this.enablePlainPKCE = options.enablePlainPKCE === true;
     this.model = options.model;
   }
 
@@ -371,9 +372,14 @@ class AuthorizeHandler {
   }
 
   /**
-   * Get code challenge method from request or defaults to plain.
-   * https://www.rfc-editor.org/rfc/rfc7636#section-4.3
+   * Get code challenge method from request.
    *
+   * When `enablePlainPKCE` is false (the default), the "plain" method is
+   * rejected and the default (when no method is provided) is "S256".
+   * When `enablePlainPKCE` is true, "plain" is accepted and used as the
+   * default per RFC 7636 §4.3.
+   *
+   * @see https://www.rfc-editor.org/rfc/rfc7636#section-4.3
    * @throws {InvalidRequestError} if request contains unsupported code_challenge_method
    *  (see https://www.rfc-editor.org/rfc/rfc7636#section-4.4)
    */
@@ -384,7 +390,19 @@ class AuthorizeHandler {
       throw new InvalidRequestError(`Invalid request: transform algorithm '${algorithm}' not supported`);
     }
 
-    return algorithm || 'plain';
+    if (!this.enablePlainPKCE && algorithm === 'plain') {
+      throw new InvalidRequestError('Invalid request: `code_challenge_method` "plain" is not allowed; use "S256"');
+    }
+
+    // return the verified algorithm, if provided
+    if (algorithm) {
+      return algorithm;
+    }
+
+    // otherwise, return the default algorithm based on the value of `enablePlainPKCE`
+    // which enables extended hardening by default, while
+    // optionally enable legacy support for the "plain" method per RFC 7636 §4.3
+    return this.enablePlainPKCE ? 'plain' : 'S256';
   }
 }
 

lib/handlers/token-handler.js

@@ -61,6 +61,7 @@ class TokenHandler {
     this.allowExtendedTokenAttributes = options.allowExtendedTokenAttributes;
     this.requireClientAuthentication = options.requireClientAuthentication || {};
     this.alwaysIssueNewRefreshToken = options.alwaysIssueNewRefreshToken !== false;
+    this.enablePlainPKCE = options.enablePlainPKCE === true;
   }
 
   /**
@@ -229,7 +230,8 @@ class TokenHandler {
       accessTokenLifetime: accessTokenLifetime,
       model: this.model,
       refreshTokenLifetime: refreshTokenLifetime,
-      alwaysIssueNewRefreshToken: this.alwaysIssueNewRefreshToken
+      alwaysIssueNewRefreshToken: this.alwaysIssueNewRefreshToken,
+      enablePlainPKCE: this.enablePlainPKCE === true
     };
 
     return new Type(options).handle(request, client);

lib/pkce/pkce.js

@@ -5,7 +5,17 @@
  */
 const { base64URLEncode } = require('../utils/string-util');
 const { createHash } = require('../utils/crypto-util');
-const codeChallengeRegexp = /^([a-zA-Z0-9.\-_~]){43,128}$/;
+
+/**
+ * ABNF for "code_verifier" and "code_challenge" is as follows.
+ *
+ * code-verifier = 43*128unreserved
+ * unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~"
+ * ALPHA = %x41-5A / %x61-7A
+ * DIGIT = %x30-39
+ * @type {RegExp}
+ */
+const codeChallengeAndVerifierRegexp = /^([\u0041-\u005a\u0061-\u007A0-9.\-_~]){43,128}$/;
 
 /**
  * @module pkce
@@ -48,8 +58,7 @@ function getHashForCodeChallenge({ method, verifier }) {
  * @return {Boolean}
  */
 function codeChallengeMatchesABNF (codeChallenge) {
-  return typeof codeChallenge === 'string' &&
-        !!codeChallenge.match(codeChallengeRegexp);
+  return typeof codeChallenge === 'string' && codeChallengeAndVerifierRegexp.test(codeChallenge);
 }
 
 

lib/server.js

@@ -44,6 +44,7 @@ class OAuth2Server {
    * @param [options.requireClientAuthentication=object] {object|boolean} Require a client secret for grant types (names as keys). Defaults to `true` for all grant types.
    * @param [options.alwaysIssueNewRefreshToken=true] {boolean=} Always revoke the used refresh token and issue a new one for the `refresh_token` grant.
    * @param [options.extendedGrantTypes=object] {object} Additional supported grant types.
+   * @param [options.enablePlainPKCE=false] {boolean} Allow the use of the `plain` code challenge method for PKCE. This is not recommended for production environments.
    *
    * @throws {InvalidArgumentError} if the model is missing
    * @return {OAuth2Server} A new `OAuth2Server` instance.

package.json

@@ -1,7 +1,7 @@
 {
   "name": "@node-oauth/oauth2-server",
   "description": "Complete, framework-agnostic, compliant and well tested module for implementing an OAuth2 Server in node.js",
-  "version": "5.2.0",
+  "version": "5.3.0",
   "keywords": [
     "oauth",
     "oauth2"