diff --git a/README.md b/README.md index 297ffb0..d3e5bc0 100644 --- a/README.md +++ b/README.md @@ -84,6 +84,36 @@ local saml = resty_saml.new(opts) | `sp_audiences` | array of strings | `{ sp_issuer }` | Audiences this SP answers to. An assertion carrying an `AudienceRestriction` has to name one of them; an assertion carrying none is unrestricted. | | `clock_skew` | number | `60` | Seconds of clock difference tolerated against the IdP when weighing `NotBefore` and `NotOnOrAfter`. | +#### Binding a response to the request + +An ID is minted for every `AuthnRequest` this SP sends and kept on the session as +`saml_request_id`. On the way back, a `SubjectConfirmationData` naming a different +request refuses the login, so an assertion captured from one login cannot be +presented in another. `Response/@InResponseTo` is weighed the same way, +though what it is worth depends on what the IdP signed: an IdP signing the whole +`Response` covers it, while one signing only the assertion, the common shape, leaves +it outside the signature, where the party replaying an assertion deletes it. Rely on +the copy inside the assertion, and read this one as catching a misdirected answer. + +That guarantee is worth what the IdP sends. Profile 4.1.4.2 asks an IdP answering an +`AuthnRequest` to name it, and every mainstream IdP does, but an IdP that leaves +`InResponseTo` out, or sends no `SubjectConfirmation` at all, keeps working and gets +no binding. Refusing it would trade a working login for protection against another +party's misconfiguration, and no attacker can produce the shape: the value sits +inside the signature, so it cannot be stripped from a captured assertion. + +One note for upgrading. A session minted before this SP kept the ID has nothing for +the assertion to name, so the login is started again rather than refused. The window +lasts as long as an `AuthnRequest` is outstanding across the upgrade. + +#### Seeding the worker + +Request IDs and `RelayState` both come from `resty.jit-uuid`, which is seeded when +this module is first loaded, from the clock and the process ID. Loading `resty.saml` +from `init_by_lua` therefore seeds once in the master, and every worker forked after +it inherits that same sequence. Require this module from `init_worker_by_lua`, or +call `uuid.seed()` there yourself. + #### saml:authenticate() **syntax:** *data = saml:authenticate()* diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index df68e6a..a7f92d8 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 @@ -189,7 +189,10 @@ local function logout_request(opts, name_id, session_index) }) end -local function login(self, opts) +-- request_uri is where to return once the IdP answers. It defaults to what is +-- being asked for, and is passed in when a login is restarted from somewhere +-- else, where ngx.var.request_uri is that somewhere else. +local function login(self, opts, request_uri) local sess = session.start(self.session_config) local authenticated = sess:get("authenticated") @@ -213,14 +216,18 @@ local function login(self, opts) end local state = uuid.generate_v4() - local request_uri = ngx.var.request_uri + request_uri = request_uri or 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, }) @@ -366,13 +373,22 @@ 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) +local function confirmation_ok(confirmation, expected, now, skew) -- Recipient is the only thing a confirmation says about where the assertion -- may be presented, so it has to be there. An absent one, an empty -- SubjectConfirmationData, and one carrying nothing but conditions that -- happen to hold all say the same nothing, and any of them would otherwise -- answer in place of a sibling that binds the assertion somewhere else. - if confirmation.recipient ~= acs_url then + if confirmation.recipient ~= expected.acs_url then + return false + end + -- Checked when the IdP names a request, and not demanded. Naming one is + -- what profile 4.1.4.2 asks of an IdP answering an AuthnRequest, so an IdP + -- that leaves it out is already out of spec, and refusing that trades a + -- working login for protection against somebody else's misconfiguration. + -- Nothing an attacker sends produces the shape: the value sits inside the + -- signature, so it cannot be stripped from a captured assertion. + 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)) @@ -381,7 +397,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 } @@ -412,7 +428,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 @@ -496,6 +512,16 @@ local function login_callback(self, opts) local request_uri = sess:get("request_uri") + -- A session minted before this SP kept the ID of the request it issued has + -- nothing for the assertion to name, and refusing dead-ends a login that is + -- genuinely this user's. Starting over gets a request that is remembered, + -- and cannot repeat, since the session it mints carries one. + local request_id = sess:get("saml_request_id") + if not request_id then + ngx.log(ngx.WARN, "session carries no request id, starting the login again") + return login(self, opts, request_uri) + end + local method = ngx.req.get_method() local doc, args, err if method == "POST" then @@ -523,7 +549,23 @@ 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 = 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, in_response_to_err = saml.doc_in_response_to(doc) + if in_response_to_err then + ngx.log(ngx.ERR, "could not read what the response from IdP answers: ", + in_response_to_err) + ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR) + end + if in_response_to and in_response_to ~= expected.request_id then + ngx.log(ngx.ERR, "response from IdP answers request ", loggable(in_response_to)) + ngx.exit(ngx.HTTP_UNAUTHORIZED) + end local destination, destination_err = saml.doc_destination(doc) if destination_err then @@ -531,7 +573,7 @@ local function login_callback(self, opts) destination_err) ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR) end - 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 ", loggable(destination)) ngx.exit(ngx.HTTP_UNAUTHORIZED) end @@ -542,7 +584,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: ", loggable(reason)) ngx.exit(ngx.HTTP_UNAUTHORIZED) @@ -591,6 +633,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 5286321..05f56fb 100644 --- a/src/lua_saml.c +++ b/src/lua_saml.c @@ -548,6 +548,36 @@ 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 +@treturn ?string error +*/ +static int doc_in_response_to(lua_State* L) { + lua_settop(L, 1); + xmlDoc* doc = doc_check(L, 1); + lua_pop(L, 1); + + xmlChar* in_response_to; + if (saml_doc_in_response_to(doc, &in_response_to) < 0) { + lua_pushnil(L); + lua_pushstring(L, "could not read InResponseTo"); + return 2; + } + + if (in_response_to == NULL) { + lua_pushnil(L); + } else { + lua_pushstring(L, (char*)in_response_to); + xmlFree(in_response_to); + } + lua_pushnil(L); + return 2; +} + + /*** Get the Destination attribute of the root message @function doc_destination @@ -1328,6 +1358,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/src/saml.h b/src/saml.h index f324fb0..ac54afc 100644 --- a/src/saml.h +++ b/src/saml.h @@ -115,6 +115,7 @@ int saml_doc_attrs(xmlDoc* doc, saml_attr_t** attrs, size_t* attrs_len); void saml_attrs_free(saml_attr_t* attrs, size_t attrs_len); int saml_doc_assertions(xmlDoc* doc, saml_assertion_t** assertions, size_t* assertions_len); int saml_doc_destination(xmlDoc* doc, xmlChar** destination); +int saml_doc_in_response_to(xmlDoc* doc, xmlChar** in_response_to); void saml_assertions_free(saml_assertion_t* assertions, size_t assertions_len); xmlSecTransformCtx* saml_sign_binary(xmlSecKey* key, xmlSecTransformId transform_id, unsigned char* data, size_t data_len); diff --git a/src/xml.c b/src/xml.c index 15f975b..6858757 100644 --- a/src/xml.c +++ b/src/xml.c @@ -591,6 +591,19 @@ int saml_doc_destination(xmlDoc* doc, xmlChar** destination) { } +// What the root message answers, or NULL when it answers nothing. Reported +// separately from a read that failed, for the same reason as Destination. +int saml_doc_in_response_to(xmlDoc* doc, xmlChar** in_response_to) { + *in_response_to = NULL; + + xmlNode* root = xmlDocGetRootElement(doc); + if (root == NULL) { + return 0; + } + return read_attr(root, "InResponseTo", in_response_to); +} + + void saml_assertions_free(saml_assertion_t* assertions, size_t assertions_len) { for (size_t i = 0; i < assertions_len; i++) { saml_assertion_t* a = assertions + i; diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index 4d48cae..d730b32 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -14,6 +14,11 @@ add_block_preprocessor(sub { if ((!defined $block->error_log) && (!defined $block->no_error_log)) { $block->set_value("no_error_log", "[error]"); + } elsif (!defined $block->no_error_log) { + # a block naming the error it expects gets no other assertion about the + # log, so a block that also drives a success asserts nothing about that + # half. This is the part of it that can be said generically. + $block->set_value("no_error_log", "[crit]\n[alert]\n[emerg]"); } if (!defined $block->request) { @@ -157,9 +162,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) @@ -189,17 +195,31 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== authn_statement(spec.session_expires)) 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) @@ -225,6 +245,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)) .. @@ -235,6 +261,26 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== return res.status .. " " .. tostring(res.headers["Location"]) end + -- hand a response to a session the old code would have left behind + function login_with_legacy(name, xml) + local httpc = require("resty.http").new() + local base = "http://127.0.0.1:1984" + + local res = assert(httpc:request_uri(base .. "/legacy", { + headers = { ["X-Test-SP"] = name }, + })) + local cookie = res.headers["Set-Cookie"] + if type(cookie) == "table" then cookie = cookie[1] end + + res = assert(httpc:request_uri(base .. "/acs", { + method = "POST", + body = "SAMLResponse=" .. ngx.escape_uri(saml.base64_encode(xml)) .. + "&RelayState=legacy-state", + headers = callback_headers(name, cookie), + })) + return res.status .. " " .. (res.headers["Location"] or ""):match("^[^?]*") + end + -- log in, then ask the app again carrying the session the callback -- handed out: a live session answers 200, an expired one starts over function session_after_login(name, xml) @@ -276,6 +322,19 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== server { listen 1984; + # a login in progress with no record of the request that started it, + # the shape a session minted before the binding existed has + location /legacy { + content_by_lua_block { + local name = ngx.var.http_x_test_sp or "plain" + local sess = require("resty.session").start(sp(name).session_config) + sess:set("saml_state", "legacy-state") + sess:set("request_uri", "/") + sess:save() + ngx.exit(200) + } + } + location / { access_by_lua_block { sp(ngx.var.http_x_test_sp or "plain"):authenticate() @@ -848,3 +907,82 @@ qr/offers no subject confirmation this SP can satisfy/] --- error_log eval [qr/parse post from IdP: document carries a document type declaration/, qr/offers no subject confirmation this SP can satisfy/] + + +=== TEST 27: 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 28: 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 29: 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 / + + +=== TEST 30: a confirmation naming no request keeps working +--- config + location /t { + content_by_lua_block { + -- naming one is what the profile asks of an IdP answering a + -- request, so an IdP that leaves it out is already out of spec. + -- The binding is worth what that IdP sends and no more. + ngx.say(login_with("plain", saml_response({ + confirmations = confirmation({ recipient = ACS }), + }))) + } + } +--- response_body +302 / + + +=== TEST 31: a session minted before the binding starts the login again +--- config + location /t { + content_by_lua_block { + -- nothing to compare the assertion against, and the login is + -- genuinely this user's, so send them back to the IdP for one + -- that is remembered + ngx.say(login_with_legacy("plain", saml_response({ + confirmations = confirmation({ recipient = ACS, in_response_to = "ID_earlier" }), + }))) + } + } +--- response_body +302 http://127.0.0.1:1984/idp +--- error_log +session carries no request id, starting the login again