diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index fbe5814..3d222d5 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -158,13 +158,13 @@ local AUTHN_REQUEST = [[ ]] -local function authn_request(opts) +local function authn_request(opts, request_id) return interp(AUTHN_REQUEST, { acs_url = sp_acs_url(opts), destination = opts.idp_uri, issue_instant = os.date("!%Y-%m-%dT%TZ"), issuer = opts.sp_issuer, - uuid = generate_saml_id(), + uuid = request_id, auth_protocol_binding_method = opts.auth_protocol_binding_method, }) end @@ -214,13 +214,17 @@ local function login(self, opts) local state = uuid.generate_v4() local request_uri = ngx.var.request_uri + -- kept so the callback can tell the answer to this request from the answer + -- to some other one + local request_id = generate_saml_id() sess:set("saml_state", state) + sess:set("saml_request_id", request_id) sess:set("request_uri", request_uri) sess:save() local query_str, err = create_redirect(self.sign_key, { - SAMLRequest = authn_request(opts), + SAMLRequest = authn_request(opts, request_id), SigAlg = RSA_SHA_512_HREF, RelayState = state, }) @@ -332,8 +336,11 @@ end -- The assertion may be presented to whoever the Recipient names, for as long as -- the confirmation data allows. Several confirmations can be offered and any one -- of them being satisfiable is enough. -local function confirmation_ok(confirmation, acs_url, now, skew) - if confirmation.recipient and confirmation.recipient ~= acs_url then +local function confirmation_ok(confirmation, expected, now, skew) + if confirmation.recipient and confirmation.recipient ~= expected.acs_url then + return false + end + if confirmation.in_response_to and confirmation.in_response_to ~= expected.request_id then return false end return (time_bounds_ok(confirmation.not_before, confirmation.not_on_or_after, now, skew)) @@ -342,7 +349,7 @@ end -- Every top-level assertion the verified signature left in the document is one -- the readers draw identity from, so every one of them has to hold up. -local function assertions_acceptable(opts, assertions, acs_url, now) +local function assertions_acceptable(opts, assertions, expected, now) local skew = opts.clock_skew or DEFAULT_CLOCK_SKEW local accepted = opts.sp_audiences or { opts.sp_issuer } @@ -373,7 +380,7 @@ local function assertions_acceptable(opts, assertions, acs_url, now) if #confirmations > 0 then local satisfiable = false for _, confirmation in ipairs(confirmations) do - if confirmation_ok(confirmation, acs_url, now, skew) then + if confirmation_ok(confirmation, expected, now, skew) then satisfiable = true break end @@ -425,10 +432,21 @@ local function login_callback(self, opts) ngx.exit(ngx.HTTP_UNAUTHORIZED) end - local acs_url = sp_acs_url(opts) + local expected = { + acs_url = sp_acs_url(opts), + request_id = sess:get("saml_request_id"), + } + + -- the Response is often left unsigned, so this only catches a stray answer; + -- the binding that holds is the one inside the signed assertion below + local in_response_to = saml.doc_in_response_to(doc) + if in_response_to and in_response_to ~= expected.request_id then + ngx.log(ngx.ERR, "response from IdP answers request ", in_response_to) + ngx.exit(ngx.HTTP_UNAUTHORIZED) + end local destination = saml.doc_destination(doc) - if destination and destination ~= acs_url then + if destination and destination ~= expected.acs_url then ngx.log(ngx.ERR, "response from IdP is addressed to ", destination) ngx.exit(ngx.HTTP_UNAUTHORIZED) end @@ -439,7 +457,7 @@ local function login_callback(self, opts) ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR) end - local acceptable, reason = assertions_acceptable(opts, assertions, acs_url, ngx.time()) + local acceptable, reason = assertions_acceptable(opts, assertions, expected, ngx.time()) if not acceptable then ngx.log(ngx.ERR, "response from IdP rejected: ", reason) ngx.exit(ngx.HTTP_UNAUTHORIZED) @@ -478,6 +496,7 @@ local function login_callback(self, opts) -- clear temporary authentication state no longer needed after successful login sess:set("saml_state", nil) + sess:set("saml_request_id", nil) sess:set("request_uri", nil) sess:save() diff --git a/src/lua_saml.c b/src/lua_saml.c index 7412122..4ba67fe 100644 --- a/src/lua_saml.c +++ b/src/lua_saml.c @@ -519,6 +519,34 @@ static int doc_attrs(lua_State* L) { } +/*** +Get the InResponseTo attribute of the root message +@function doc_in_response_to +@tparam xmlDoc* doc +@treturn ?string in_response_to +*/ +static int doc_in_response_to(lua_State* L) { + lua_settop(L, 1); + xmlDoc* doc = doc_check(L, 1); + lua_pop(L, 1); + + xmlNode* root = xmlDocGetRootElement(doc); + if (root == NULL) { + lua_pushnil(L); + return 1; + } + + xmlChar* in_response_to = xmlGetNoNsProp(root, (const xmlChar*)"InResponseTo"); + if (in_response_to == NULL) { + lua_pushnil(L); + } else { + lua_pushstring(L, (char*)in_response_to); + xmlFree(in_response_to); + } + return 1; +} + + /*** Get the Destination attribute of the root message @function doc_destination @@ -1294,6 +1322,7 @@ static const struct luaL_Reg saml_funcs[] = { {"doc_attrs", doc_attrs}, {"doc_assertions", doc_assertions}, {"doc_destination", doc_destination}, + {"doc_in_response_to", doc_in_response_to}, {"key_read_memory", key_read_memory}, {"key_read_file", key_read_file}, diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index b9445ad..e9df3c7 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -157,9 +157,10 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== spec = spec or {} local data = "" if spec.data ~= false then - data = string.format('', + data = string.format('', attr("Recipient", spec.recipient), attr("NotBefore", spec.not_before), - attr("NotOnOrAfter", spec.not_on_or_after)) + attr("NotOnOrAfter", spec.not_on_or_after), + attr("InResponseTo", spec.in_response_to)) end return string.format('%s', spec.method or BEARER, data) @@ -176,17 +177,31 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== spec.confirmations or "", spec.conditions or "") end - function response(body, destination) + function response(body, destination, in_response_to) return string.format('%s' .. '%s', - attr("Destination", destination), IDP, SUCCESS, body) + attr("Destination", destination), attr("InResponseTo", in_response_to), + IDP, SUCCESS, body) end -- only the assertion is signed, the shape an IdP sends by default - function saml_response(spec, destination) - return response(sign_doc(assertion(spec)), destination) + function saml_response(spec, destination, in_response_to) + return response(sign_doc(assertion(spec)), destination, in_response_to) + end + + -- the ID of the AuthnRequest the SP just issued, read back out of the + -- redirect it sent the browser + function authn_request_id(location) + local args = {} + for k, v in location:gmatch("([^?&=]+)=([^&]*)") do + args[k] = ngx.unescape_uri(v) + end + local cert = assert(saml.key_read_memory(CERT_PEM, saml.KeyDataFormatCertPem)) + local doc = assert(saml.binding_redirect_parse("SAMLRequest", args, + function(_) return cert end)) + return saml.doc_id(doc) end function callback_headers(name, cookie, extra) @@ -212,6 +227,12 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== if type(cookie) == "table" then cookie = cookie[1] end local state = res.headers["Location"]:match("RelayState=([^&]+)") + -- a response that has to name the request gets built once the SP + -- has issued one + if type(xml) == "function" then + xml = xml(authn_request_id(res.headers["Location"])) + end + res, err = httpc:request_uri(base .. "/acs", { method = "POST", body = "SAMLResponse=" .. ngx.escape_uri(saml.base64_encode(xml)) .. @@ -265,7 +286,6 @@ __DATA__ 302 / - === TEST 2: an expired assertion is refused however it is replayed --- config location /t { @@ -281,7 +301,6 @@ __DATA__ is not valid on or after - === TEST 3: an assertion whose window has not opened is refused --- config location /t { @@ -297,7 +316,6 @@ is not valid on or after is not valid before - === TEST 4: the clock skew allowance covers a small difference with the IdP --- config location /t { @@ -314,7 +332,6 @@ is not valid before is not valid on or after - === TEST 5: an assertion restricted to another SP is refused --- config location /t { @@ -331,7 +348,6 @@ is not valid on or after is restricted to https://other.example.com - === TEST 6: an assertion restricted to this SP is accepted --- config location /t { @@ -345,7 +361,6 @@ is restricted to https://other.example.com 302 / - === TEST 7: sp_audiences names the audience the IdP was configured with --- config location /t { @@ -364,7 +379,6 @@ is restricted to https://other.example.com is restricted to https://sp.example.com/metadata - === TEST 8: each AudienceRestriction narrows the audience on its own --- config location /t { @@ -383,7 +397,6 @@ is restricted to https://sp.example.com/metadata is restricted to https://other.example.com - === TEST 9: a confirmation addressed to another endpoint is refused --- config location /t { @@ -399,7 +412,6 @@ is restricted to https://other.example.com offers no subject confirmation this SP can satisfy - === TEST 10: a confirmation addressed here and still open is accepted --- config location /t { @@ -413,7 +425,6 @@ offers no subject confirmation this SP can satisfy 302 / - === TEST 11: a confirmation that has run out is refused --- config location /t { @@ -429,7 +440,6 @@ offers no subject confirmation this SP can satisfy offers no subject confirmation this SP can satisfy - === TEST 12: one satisfiable confirmation among several is enough --- config location /t { @@ -444,7 +454,6 @@ offers no subject confirmation this SP can satisfy 302 / - === TEST 13: a condition this SP cannot satisfy leaves the assertion indeterminate --- config location /t { @@ -475,7 +484,6 @@ offers no subject confirmation this SP can satisfy qr/carries a condition this SP cannot satisfy: Condition/] - === TEST 14: a response addressed to another endpoint is refused --- config location /t { @@ -491,7 +499,6 @@ qr/carries a condition this SP cannot satisfy: Condition/] response from IdP is addressed to http://evil.example.com/acs - === TEST 15: an assertion carrying no constraints is still accepted --- config location /t { @@ -503,7 +510,6 @@ response from IdP is addressed to http://evil.example.com/acs 302 / - === TEST 16: the constraints are reported per assertion, not pooled --- config location /t { @@ -531,7 +537,6 @@ a2 conditions=false expires=nil audiences=0 confirmations=1 destination: nil - === TEST 17: a UTC timestamp is read as UTC whatever the machine's timezone is --- config location /t { @@ -551,7 +556,6 @@ env TZ=XXX-14; 302 / - === TEST 18: a configured ACS URL settles what the endpoint checks compare against --- config location /t { @@ -581,7 +585,6 @@ env TZ=XXX-14; offers no subject confirmation this SP can satisfy - === TEST 19: an audience with no text leaves the rest of its restriction readable --- config location /t { @@ -595,3 +598,48 @@ offers no subject confirmation this SP can satisfy } --- response_body 302 / + + +=== TEST 20: a response answering another request is refused +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("plain", saml_response({}, nil, "ID_some-other-request"))) + } + } +--- response_body +401 nil +--- error_log +response from IdP answers request ID_some-other-request + + +=== TEST 21: a confirmation answering another request is refused +--- config + location /t { + content_by_lua_block { + -- inside the signature, so this is the binding an attacker replaying + -- a captured assertion cannot rewrite + ngx.say(login_with("plain", saml_response({ + confirmations = confirmation({ recipient = ACS, in_response_to = "ID_some-other-request" }), + }))) + } + } +--- response_body +401 nil +--- error_log +offers no subject confirmation this SP can satisfy + + +=== TEST 22: a response answering this SP's own request is accepted +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("plain", function(request_id) + return saml_response({ + confirmations = confirmation({ recipient = ACS, in_response_to = request_id }), + }, ACS, request_id) + end)) + } + } +--- response_body +302 /