fix: bind the assertion to the request this SP issued

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()*

lua/resty/saml.lua

@@ -158,13 +158,13 @@ local AUTHN_REQUEST = [[
 </samlp:AuthnRequest>
 ]]
 
-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,15 +549,31 @@ 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
         ngx.log(ngx.ERR, "could not read the destination of the response from IdP: ",
             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()
 

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},

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);

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;