Merge pull request #156 from bodewig/tighten-jwt-signature-validation

Tighten jwt signature validation

Author: zandbelt

ChangeLog

@@ -1,3 +1,14 @@
+04/27/2018
+- disabled support for "none" alg tokens introduced with 1.5.2 by default.
+  If you want to enable it, you will now have to explicitly set the accept_none_alg
+  option to true.
+- id tokens using a signature algorithm not announced by the discovery
+  endpoint are now rejected.
+- you can now specify which signing algorithm you expect a bearer token to
+  use in order to avoid being tricked into accepting a rogue token signed
+  with a symmetric key when expecting an asymmetric cypher.
+- added an option to reject tokens signed by an algorithm not supported by lua-resty-jwt
+
 04/19/2018
 - added support for passing bearer token as cookie
 - added support introspection interval

README.md

@@ -111,6 +111,16 @@ http {
              --token_endpoint_auth_method = ["client_secret_basic"|"client_secret_post"],
              --ssl_verify = "no"
 
+             --accept_none_alg = false
+             -- if your OpenID Connect Provider doesn't sign its id tokens
+             -- (uses the "none" signature algorithm) then set this to true.
+
+             --accept_unsupported_alg = true
+             -- if you want to reject tokens signed using an algorithm
+             -- not supported by lua-resty-jwt set this to false. If
+             -- you leave it unset or set it to true, the token signature will not be
+             -- verified when an unsupported algorithm is used.
+
              --renew_access_token_on_expiry = true
              -- whether this plugin shall try to silently renew the access token once it is expired if a refresh token is available.
              -- if it fails to renew the token, the user will be redirected to the authorization endpoint.
@@ -233,6 +243,24 @@ lAc5Csj0o5Q+oEhPUAVBIF07m4rd0OvAVPOCQ2NJhQSL1oWASbf+fg==
             -- contains "jwks_uri" entry; the jwks endpoint must provide either an "x5c" entry
             -- or both the "n" modulus and "e" exponent entries for RSA signature verification
             -- discovery = "https://accounts.google.com/.well-known/openid-configuration",
+
+             -- the signature algorithm that you expect has been used;
+             -- can be a single string or a table.
+             -- You should set this for security reasons in order to
+             -- avoid accepting a token claiming to be signed by HMAC
+             -- using a public RSA key.
+             --token_signing_alg_values_expected = { "RS256" }
+
+             -- if you want to accept unsigned tokens (using the
+             -- "none" signature algorithm) then set this to true.
+             --accept_none_alg = false
+
+             -- if you want to reject tokens signed using an algorithm
+             -- not supported by lua-resty-jwt set this to false. If
+             -- you leave it unset, the token signature will not be
+             -- verified at all.
+             --accept_unsupported_alg = true
+
           }
 
           -- call bearer_jwt_verify for OAuth 2.0 JWT validation

lib/resty/openidc.lua

@@ -451,7 +451,6 @@ local function openidc_load_jwt_none_alg(enc_hdr, enc_payload)
   local header = cjson_s.decode(openidc_base64_url_decode(enc_hdr))
   local payload = cjson_s.decode(openidc_base64_url_decode(enc_payload))
   if header and payload and header.alg == "none" then
-    ngx.log(ngx.DEBUG, "accept JWT with alg \"none\" and no signature from \"code\" flow")
     return {
       raw_header = enc_hdr,
       raw_payload = enc_payload,
@@ -742,13 +741,37 @@ local function uses_asymmetric_algorithm(jwt_header)
   return string.sub(jwt_header.alg, 1, 2) == "RS"
 end
 
+-- is the JWT signing algorithm one that has been expected?
+local function is_algorithm_expected(jwt_header, expected_algs)
+  if expected_algs == nil or not jwt_header or not jwt_header.alg then
+    return true
+  end
+  if type(expected_algs) == 'string' then
+    expected_algs = { expected_algs }
+  end
+  for _, alg in ipairs(expected_algs) do
+    if alg == jwt_header.alg then
+      return true
+    end
+  end
+  return false
+end
+
 -- parse a JWT and verify its signature (if present)
-local function openidc_load_jwt_and_verify_crypto(opts, jwt_string, asymmetric_secret, symmetric_secret, ...)
+local function openidc_load_jwt_and_verify_crypto(opts, jwt_string, asymmetric_secret,
+  symmetric_secret, expected_algs, ...)
   local jwt = require "resty.jwt"
   local enc_hdr, enc_payload, enc_sign = string.match(jwt_string, '^(.+)%.(.+)%.(.*)$')
   if enc_payload and (not enc_sign or enc_sign == "") then
     local jwt = openidc_load_jwt_none_alg(enc_hdr, enc_payload)
-    if jwt then return jwt end -- otherwise the JWT is invalid and load_jwt produces an error
+    if jwt then
+      if opts.accept_none_alg then
+        ngx.log(ngx.DEBUG, "accept JWT with alg \"none\" and no signature")
+        return jwt
+      else
+        return jwt, "token uses \"none\" alg but accept_none_alg is not enabled"
+      end
+    end -- otherwise the JWT is invalid and load_jwt produces an error
   end
 
   local jwt_obj = jwt:load_jwt(jwt_string, nil)
@@ -760,6 +783,11 @@ local function openidc_load_jwt_and_verify_crypto(opts, jwt_string, asymmetric_s
     return nil, reason
   end
 
+  if not is_algorithm_expected(jwt_obj.header, expected_algs) then
+    local alg = jwt_obj.header and jwt_obj.header.alg or "no algorithm at all"
+    return nil, "token is signed by unexpected algorithm \"" .. alg .. "\""
+  end
+
   local secret
   if is_algorithm_supported(jwt_obj.header) then
     if uses_asymmetric_algorithm(jwt_obj.header) then
@@ -853,13 +881,20 @@ local function openidc_authorization_response(opts, session)
   end
 
   local jwt_obj
-  jwt_obj, err = openidc_load_jwt_and_verify_crypto(opts, json.id_token, opts.secret, opts.client_secret)
+  jwt_obj, err = openidc_load_jwt_and_verify_crypto(opts, json.id_token, opts.secret, opts.client_secret,
+      opts.discovery.id_token_signing_alg_values_supported)
   if err then
+    local alg = (jwt_obj and jwt_obj.header and jwt_obj.header.alg) or ''
     local is_unsupported_signature_error = jwt_obj and not jwt_obj.verified and not is_algorithm_supported(jwt_obj.header)
     if is_unsupported_signature_error then
-      ngx.log(ngx.WARN, "ignored id_token signature as algorithm '" .. jwt_obj.header.alg .. "' is not supported")
+      if opts.accept_unsupported_alg == nil or opts.accept_unsupported_alg then
+        ngx.log(ngx.WARN, "ignored id_token signature as algorithm '" .. alg .. "' is not supported")
+      else
+        err = "token is signed using algorithm \"" .. alg .. "\" which is not supported by lua-resty-jwt"
+        ngx.log(ngx.ERR, err)
+        return nil, err, session.data.original_url, session
+      end
     else
-      local alg = (jwt_obj and jwt_obj.header and jwt_obj.header.alg) or ''
       ngx.log(ngx.ERR, "id_token '" .. alg .. "' signature verification failed")
       return nil, err, session.data.original_url, session
     end
@@ -1352,7 +1387,8 @@ function openidc.jwt_verify(access_token, opts, ...)
   local v = openidc_cache_get("introspection", access_token)
   if not v then
     local jwt_obj
-    jwt_obj, err = openidc_load_jwt_and_verify_crypto(opts, access_token, opts.secret, opts.secret, ...)
+    jwt_obj, err = openidc_load_jwt_and_verify_crypto(opts, access_token, opts.secret, opts.secret,
+        opts.token_signing_alg_values_expected, ...)
     if not err then
       json = jwt_obj.payload
       ngx.log(ngx.DEBUG, "jwt: ", cjson.encode(json))

tests/spec/bearer_token_verification_spec.lua

@@ -215,24 +215,48 @@ describe("when the access token doesn't contain the exp claim at all", function(
 end)
 
 describe("when using a JWT not signed but using the 'none' alg", function()
-  test_support.start_server({
-    verify_opts = {
-      discovery = {
-        jwks_uri = "http://127.0.0.1/jwk",
-      }
-    },
-    jwk = test_support.load("/spec/jwks_with_two_keys.json"),
-  })
-  teardown(test_support.stop_server)
-  local jwt = test_support.self_signed_jwt({
-      exp = os.time() + 3600,
-  })
-  local _, status = http.request({
-      url = "http://127.0.0.1/verify_bearer_token",
-      headers = { authorization = "Bearer " .. jwt }
-  })
-  it("the token is valid", function()
-    assert.are.equals(204, status)
+  describe("and we are willing to accept the none alg", function()
+    test_support.start_server({
+      verify_opts = {
+        discovery = {
+          jwks_uri = "http://127.0.0.1/jwk",
+        },
+        accept_none_alg = true,
+      },
+      jwk = test_support.load("/spec/jwks_with_two_keys.json"),
+    })
+    teardown(test_support.stop_server)
+    local jwt = test_support.self_signed_jwt({
+        exp = os.time() + 3600,
+    })
+    local _, status = http.request({
+        url = "http://127.0.0.1/verify_bearer_token",
+        headers = { authorization = "Bearer " .. jwt }
+    })
+    it("the token is valid", function()
+      assert.are.equals(204, status)
+    end)
+  end)
+  describe("and we are not willing to accept the none alg", function()
+    test_support.start_server({
+      verify_opts = {
+        discovery = {
+          jwks_uri = "http://127.0.0.1/jwk",
+        },
+      },
+      jwk = test_support.load("/spec/jwks_with_two_keys.json"),
+    })
+    teardown(test_support.stop_server)
+    local jwt = test_support.self_signed_jwt({
+        exp = os.time() + 3600,
+    })
+    local _, status = http.request({
+        url = "http://127.0.0.1/verify_bearer_token",
+        headers = { authorization = "Bearer " .. jwt }
+    })
+    it("the token is invalid", function()
+      assert.are.equals(401, status)
+    end)
   end)
 end)
 
@@ -535,3 +559,29 @@ describe("when the token is signed by an RSA key but jwks_uri is empty", functio
     assert.error_log_contains("Invalid token:.*jwks_uri is not present or not a string")
   end)
 end)
+
+describe("when expecting an RSA signature but token uses HMAC", function()
+  test_support.start_server({
+    verify_opts = {
+      secret = test_support.load("/spec/public_rsa_key.pem"),
+      token_signing_alg_values_expected = "RS256"
+    },
+    jwt_verify_secret = test_support.load("/spec/public_rsa_key.pem"),
+    token_header = {
+      alg = "HS256",
+    }
+  })
+  teardown(test_support.stop_server)
+  local jwt = test_support.trim(http.request("http://127.0.0.1/jwt"))
+  local _, status = http.request({
+    url = "http://127.0.0.1/verify_bearer_token",
+    headers = { authorization = "Bearer " .. jwt }
+  })
+  it("the response is invalid", function()
+    assert.are.equals(401, status)
+  end)
+  it("an error has been logged", function()
+    assert.error_log_contains("Invalid token:.*token is signed by unexpected algorithm \"HS256\"")
+  end)
+end)
+

tests/spec/id_token_validation_spec.lua

@@ -234,16 +234,64 @@ describe("when the id token signature uses a symmetric algorithm", function()
 end)
 
 describe("when the id claims to be signed by an unsupported algorithm", function()
-  test_support.start_server({
-    fake_id_token_signature = "true"
-  })
-  teardown(test_support.stop_server)
-  local _, status = test_support.login()
-  it("login succeeds", function()
-    assert.are.equals(302, status)
+  describe("and accept_unsupported_alg is not set", function()
+    test_support.start_server({
+      fake_id_token_signature = "true",
+      oidc_opts = {
+        discovery = {
+          id_token_signing_alg_values_supported = { "AB256" }
+        }
+      }
+    })
+    teardown(test_support.stop_server)
+    local _, status = test_support.login()
+    it("login succeeds", function()
+      assert.are.equals(302, status)
+    end)
+    it("an error is logged", function()
+      assert.error_log_contains("ignored id_token signature as algorithm 'AB256' is not supported")
+    end)
+  end)
+  describe("and accept_unsupported_alg is true", function()
+    test_support.start_server({
+      fake_id_token_signature = "true",
+      oidc_opts = {
+        discovery = {
+          id_token_signing_alg_values_supported = { "AB256" }
+        },
+        accept_unsupported_alg = true
+      }
+    })
+    teardown(test_support.stop_server)
+    local _, status = test_support.login()
+    it("login succeeds", function()
+      assert.are.equals(302, status)
+    end)
+    it("an error is logged", function()
+      assert.error_log_contains("ignored id_token signature as algorithm 'AB256' is not supported")
+    end)
   end)
-  it("an error is logged", function()
-    assert.error_log_contains("ignored id_token signature as algorithm 'AB256' is not supported")
+  describe("and accept_unsupported_alg is false", function()
+    test_support.start_server({
+      fake_id_token_signature = "true",
+      oidc_opts = {
+        discovery = {
+          id_token_signing_alg_values_supported = { "AB256" }
+        },
+        accept_unsupported_alg = false
+      }
+    })
+    teardown(test_support.stop_server)
+    local _, status = test_support.login()
+    it("login has failed", function()
+      assert.are.equals(401, status)
+    end)
+    it("an error message has been logged", function()
+      assert.error_log_contains("token is signed using algorithm \"AB256\" which is not supported by lua%-resty%-jwt")
+    end)
+    it("authenticate returns an error", function()
+      assert.error_log_contains("authenticate failed: token is signed using algorithm \"AB256\" which is not supported by lua%-resty%-jwt")
+    end)
   end)
 end)
 
@@ -264,3 +312,59 @@ describe("when the id token signature is invalid", function()
   end)
 end)
 
+describe("when the id token signature uses the 'none' alg", function()
+  describe("and we are not willing to accept the none alg", function()
+    test_support.start_server({
+      none_alg_id_token_signature = "true",
+    })
+    teardown(test_support.stop_server)
+    local _, status = test_support.login()
+    it("login has failed", function()
+      assert.are.equals(401, status)
+    end)
+    it("an error message has been logged", function()
+      assert.error_log_contains("id_token 'none' signature verification failed")
+    end)
+    it("authenticate returns an error", function()
+      assert.error_log_contains("authenticate failed: token uses \"none\" alg but accept_none_alg is not enabled")
+    end)
+  end)
+  describe("and we are willing to accept the none alg", function()
+    test_support.start_server({
+      none_alg_id_token_signature = "true",
+      oidc_opts = {
+        accept_none_alg = true,
+      }
+    })
+    teardown(test_support.stop_server)
+    local _, status = test_support.login()
+    it("login succeeds", function()
+      assert.are.equals(302, status)
+    end)
+    it("an message has been logged", function()
+      assert.error_log_contains("accept JWT with alg \"none\" and no signature")
+    end)
+  end)
+end)
+
+describe("when the id token is signed by an algorithm not announced by discovery endpoint", function()
+  test_support.start_server({
+    oidc_opts = {
+      discovery = {
+        id_token_signing_alg_values_supported = { "HS256" }
+      }
+    }
+  })
+  teardown(test_support.stop_server)
+  local _, status = test_support.login()
+  it("login has failed", function()
+    assert.are.equals(401, status)
+  end)
+  it("an error message has been logged", function()
+    assert.error_log_contains("token is signed by unexpected algorithm \"RS256\"")
+  end)
+  it("authenticate returns an error", function()
+    assert.error_log_contains("authenticate failed: token is signed by unexpected algorithm \"RS256\"")
+  end)
+end)
+