From 0cbf07919051586f20ed7351781ec21999c70475 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Mon, 17 Aug 2026 23:07:33 +0545 Subject: [PATCH 1/5] fix: read the issuer from the assertion the signature covers saml_doc_issuer returned the first Issuer under the document root, which for a Response is the Response's own. SAML lets the IdP sign the assertion rather than the whole response, and that Issuer then sits outside the signature: an attacker holding one signed assertion can rewrite it and the signature still verifies, so the value stored on the session was never attested. Read it from the assertion instead, the element the identity itself comes from and the one every other accessor already reads. Messages that carry no assertion are signed whole, so they keep reading their own Issuer. A Response left with no assertion after verification now yields no issuer rather than an unverified one. --- src/sig.c | 8 ------ src/xml.c | 47 ++++++++++++++++++++++++++++------ t/signed-response.t | 61 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 16 deletions(-) diff --git a/src/sig.c b/src/sig.c index 1d8e33d..08a6d04 100644 --- a/src/sig.c +++ b/src/sig.c @@ -339,14 +339,6 @@ static int signature_covers(xmlDoc* doc, xmlNode* sig, xmlNode* node) { } -static int is_saml_assertion(xmlNode* node) { - return node->type == XML_ELEMENT_NODE && - xmlStrEqual(node->name, (const xmlChar*)"Assertion") == 1 && - node->ns != NULL && - xmlStrEqual(node->ns->href, (const xmlChar*)SAML_XMLNS_ASSERTION) == 1; -} - - // Identity is read from /samlp:Response/saml:Assertion, i.e. only from an // assertion that is a direct child of the verified root message. saml_verify_doc // checks one Signature but not that it covers the assertion a reader will pick, diff --git a/src/xml.c b/src/xml.c index bbc1bfb..65162b1 100644 --- a/src/xml.c +++ b/src/xml.c @@ -25,20 +25,51 @@ static xmlXPathObject* eval_xpath(xmlDoc* doc, xmlXPathCompExpr* xpath) { } +static int is_saml_assertion(xmlNode* node) { + return node->type == XML_ELEMENT_NODE && + xmlStrEqual(node->name, (const xmlChar*)"Assertion") == 1 && + node->ns != NULL && + xmlStrEqual(node->ns->href, (const xmlChar*)SAML_XMLNS_ASSERTION) == 1; +} + + +// The text of node's own Issuer child, or NULL. Issuer is in the assertion +// namespace wherever it appears, so a look-alike in another one is not it. +static xmlChar* issuer_of(xmlDoc* doc, xmlNode* node) { + for (xmlNode* child = node->children; child != NULL; child = child->next) { + if (child->type == XML_ELEMENT_NODE && + xmlStrEqual(child->name, (const xmlChar*)"Issuer") == 1 && + child->ns != NULL && + xmlStrEqual(child->ns->href, (const xmlChar*)SAML_XMLNS_ASSERTION) == 1) { + return xmlNodeListGetString(doc, child->children, 1); + } + } + return NULL; +} + + +// A Response's issuer is read from its assertion, the element the identity +// itself comes from. The Response's own Issuer sits outside an assertion-level +// signature and can be rewritten without breaking it, while every top-level +// assertion still in the document is one the signature covers. Other messages +// carry no assertion and are signed whole, so their own Issuer is the one to +// read. xmlChar* saml_doc_issuer(xmlDoc* doc) { - xmlNode* node = xmlDocGetRootElement(doc); - if (node == NULL) { + xmlNode* root = xmlDocGetRootElement(doc); + if (root == NULL) { return NULL; } - node = node->children; - while (node != NULL) { - if (xmlStrEqual(node->name, (xmlChar*)"Issuer") == 1) { - return xmlNodeListGetString(doc, node->children, 1); + if (xmlStrEqual(root->name, (const xmlChar*)"Response") == 1) { + for (xmlNode* child = root->children; child != NULL; child = child->next) { + if (is_saml_assertion(child)) { + return issuer_of(doc, child); + } } - node = node->next; + return NULL; } - return NULL; + + return issuer_of(doc, root); } diff --git a/t/signed-response.t b/t/signed-response.t index c4df4c9..a2fd92d 100644 --- a/t/signed-response.t +++ b/t/signed-response.t @@ -496,3 +496,64 @@ name_id: first@example.com, dept: eng, role: ops } --- response_body name_id: signed@example.com + + + +=== TEST 18: the issuer comes from the assertion, not the unsigned Response +--- config + location /t { + content_by_lua_block { + local key, mngr, transform = saml_ctx() + -- the signature covers the assertion only, so the Response around + -- it, its own Issuer included, is the attacker's to write + local signed = sign(key, transform, assertion("a1", "signed@example.com")) + local outer = '' .. + 'https://attacker.example.com' .. + '' .. + signed .. '' + local doc, err = submit(mngr, outer) + if err then ngx.say("err: ", err) else ngx.say("issuer: ", tostring(saml.doc_issuer(doc))) end + } + } +--- response_body +issuer: https://idp.example.com + + + +=== TEST 19: a whole-response signature reads the same issuer +--- config + location /t { + content_by_lua_block { + local key, mngr, transform = saml_ctx() + local resp = response(SUCCESS, "resp-1", assertion("a1", "signed@example.com")) + local doc, err = submit(mngr, sign(key, transform, resp)) + if err then ngx.say("err: ", err) else ngx.say("issuer: ", tostring(saml.doc_issuer(doc))) end + } + } +--- response_body +issuer: https://idp.example.com + + + +=== TEST 20: a message carrying no assertion reads its own issuer +--- config + location /t { + content_by_lua_block { + local key, mngr, transform = saml_ctx() + local logout = 'https://idp.example.com' .. + '' .. + '' + local doc, err = submit(mngr, sign(key, transform, logout)) + if err then + ngx.say("err: ", err) + else + ngx.say(saml.doc_root_name(doc), " issuer: ", tostring(saml.doc_issuer(doc))) + end + } + } +--- response_body +LogoutResponse issuer: https://idp.example.com From 92c511cedb31303b3281f8f0195547130180f928 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Mon, 17 Aug 2026 23:07:33 +0545 Subject: [PATCH 2/5] feat: pin the issuers accepted on a login response A valid signature says the response came from the configured idp_cert. It does not say which IdP that key speaks for, which matters when one key signs for several issuers, or when the certificate is a shared or intermediate issued one. idp_issuers names the issuers a deployment expects and the login callback rejects anything else; leaving it unset keeps current behaviour. --- README.md | 1 + lua/resty/saml.lua | 21 ++++ t/login-callback.t | 244 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 266 insertions(+) create mode 100644 t/login-callback.t diff --git a/README.md b/README.md index 04a7ef6..3866358 100644 --- a/README.md +++ b/README.md @@ -73,6 +73,7 @@ local saml = resty_saml.new(opts) | `sp_issuer` | string | None | SP name to access IdP. | | `idp_uri` | string | None | URI of IdP. | | `idp_cert` | string | None | IdP Certificate, used to verify saml response. | +| `idp_issuers` | array of strings | None | Issuers accepted on a login response. Unset accepts any issuer the `idp_cert` signs for. | | `login_callback_uri` | string | None | redirect uri used to callback the SP from IdP after login. | | `logout_uri` | string | None | logout uri to trigger logout. | | `logout_callback_uri` | string | None | redirect uri used to callback the SP from IdP after logout. | diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 8ab7985..8673e39 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -258,6 +258,22 @@ local function parse_iso8601_utc_time(str) return os.time{year=year, month=month, day=day, hour=hour, min=min, sec=sec} end +-- A valid signature says the message came from the configured key. It does not +-- say which IdP that key speaks for, so pin the issuer when the caller names +-- the ones it expects. No list keeps the previous behaviour; a list nothing +-- matches, an empty one included, admits nobody. +local function issuer_allowed(allowed, issuer) + if allowed == nil then + return true + end + for _, expected in ipairs(allowed) do + if expected == issuer then + return true + end + end + return false +end + local function login_callback(self, opts) local sess = session.start(self.session_config) @@ -301,6 +317,11 @@ local function login_callback(self, opts) local name_id = saml.doc_name_id(doc) local session_index = saml.doc_session_index(doc) + if not issuer_allowed(opts.idp_issuers, issuer) then + ngx.log(ngx.ERR, "unexpected issuer in response from IdP: ", tostring(issuer)) + ngx.exit(ngx.HTTP_UNAUTHORIZED) + end + -- a success response the signature leaves without a readable assertion -- carries no identity, so there is nobody to authenticate as if not name_id then diff --git a/t/login-callback.t b/t/login-callback.t new file mode 100644 index 0000000..4c71f9c --- /dev/null +++ b/t/login-callback.t @@ -0,0 +1,244 @@ +use Test::Nginx::Socket::Lua; + +log_level('info'); +no_long_string(); +repeat_each(1); +no_shuffle(); +plan 'no_plan'; + +my $pwd = `pwd`; +chomp $pwd; + +add_block_preprocessor(sub { + my ($block) = @_; + + if ((!defined $block->error_log) && (!defined $block->no_error_log)) { + $block->set_value("no_error_log", "[error]"); + } + + if (!defined $block->request) { + $block->set_value("request", "GET /t"); + } + + my $main_config = $block->main_config // <<_EOC_; + env SAML_DATA_DIR=./; +_EOC_ + + $block->set_value("main_config", $main_config); + + my $http_config = $block->http_config // <<_EOC_; + lua_package_path '$pwd/lua/?.lua;$pwd/deps/share/lua/5.1/?.lua;$pwd/t/?.lua;;'; + lua_package_cpath '$pwd/?.so;$pwd/deps/lib/lua/5.1/?.so;;'; + + init_by_lua_block { + saml = require "saml" + local err = saml.init({ debug = true, data_dir = os.getenv("SAML_DATA_DIR") }) + if err then assert(nil, err) end + + SUCCESS = "urn:oasis:names:tc:SAML:2.0:status:Success" + IDP = "https://idp.example.com" + + KEY_PEM = [[-----BEGIN PRIVATE KEY----- +MIIEvwIBADANBgkqhkiG9w0BAQEFAASCBKkwggSlAgEAAoIBAQDYYOJFazEru+eF +1bGFzH8xuC2clcWjnpIvXf5Jrseg7gfMh0nMM83OddLWB2Er+RWmVj361qaQR35p +JHGm3hFw20b2S+zBPxA6LCrHJ7vD/kOKEiDKxU3Ls5QK9+fTHFXIbpDtGAuISmmc +eWNaTZPIMdxPlpKYIyNJIUc2RxSREjsGlsrWWEtsroMjxpaHNNupadRUmkHXvZsC +EAsi3penjfZxG6v9R22tBwJxgj/ceXZwtTQJ7tuNtthv+kWP6/Q9owHW3uGL8Bin +46GRqAfHSGC64No+NwETF5iuephkIggtbvrlazTdPwu8Ddl8l4I1QfYmNxKPxnzJ +7pDwvBeRAgMBAAECggEAFkMTjKZcav48cg/cIaK6VGx5XuKm8LBcJHz0cHLHzbYn +vcKOlHChBFSpgkVEmWBZeqFlY5Upkm8Uoa8y9ULkQvsAiE8j9vbszbtlFFPxdNcI +bmBymMIngKWDfgRnCNiht8suZIJkj1tulb+EehJAuehtXQ/mGbqFwxymJb627jzk +MJ5bDsaVeBNu4gBQAp0USzreMO3AN9YxXmcJapZ5Bdc8avQzhzWRxNNJxtp6Uw56 +cviuDxg7OJCaEHhUBFiDVu4O2HmrS/XdYUAwFcRO1hY/JfcaJ3DOHOl6y5eoRHwC +kMb8DhT/qECJ9rWc+APdUqiY1ag0Kq9BcRxkEGlcMQKBgQD32hzAPpuwW9Z0M9qd +x70PPkrJD8jgIprC92DHpHfztiZ2ctH3WxupH7UtZfI8tSVzh7WhWPPtrQ01ZcFh +ZPsFN74c7pWtW+JSm0pvDCQQG5qX9eJLna8GeI6f3hpM+u8pXr6p2ZQJGnjlGZfc +VNfJhvqCVH7hiG9fdAavsH1dKQKBgQDffeUD7x8I3ARbiZqDgANA9HqJi1ffhqFZ +xTWKLtr8NCPS8X+DvFrUDlGhBoDY7IGZhDhmBcb8/v7Kke3GT0/mff8GFsj9TUqh +fgzDxj5I/9HEjBKgpAG1J4B87QYZueLriMfX5Ff2wmCeqCwF4ftfjZVU9izyIa7B +hKYubQBMKQKBgQDslAk1h41cfYzqRkS6rllMH42K9cIsD1viFfcPGXJV8twr29WH +YjO470clGlZqlA43hKZeaGYNzEz7VzGLIbRpepfBTgsY+sfBSfF2pgQWTAL4Yf+r +ZcwXRSP+fSZlrHB08LbVsZWYSuhy5kcKTQHcnzanCLhD1tNYLYvkT3aaYQKBgQDK +c3nMuYUMenn8DceJTaIk6hJCnJZqZsOs1UdtuIooona9NITFag+BPsNVMdXwKzYv +QaXxTVR3g+p8x/pzhQ8lBYfKFUPWqXhsmAmqIt/zMsHr4NNS756YYoMzJ2c6ULgt +ksctW60PW/84WbEfVxll8pSO1T3bzQVISghbz+PQGQKBgQCEptD2bKHhF8RzRyfC +QXydnF7O6GEK3au3OKPb6BsLwJpTP2Wc1feTcg/lzCS5eUhNMxPv+4Ua7SLiF4li +vnI8SyPV2nGlsjna9maSkBq01YrLEMsPPSqw01Nf4W5jtUgk+jbZt9K3SrvTGzpJ +/2lpqvTIUUQTrTJNL6GZUBY1/Q== +-----END PRIVATE KEY-----]] + + CERT_PEM = [[-----BEGIN CERTIFICATE----- +MIIDFTCCAf2gAwIBAgIUC9GZCQFhxDfguRhTjIcG/LxOZMQwDQYJKoZIhvcNAQEL +BQAwGjEYMBYGA1UEAwwPaWRwLmV4YW1wbGUuY29tMB4XDTI2MDgxMDExMDkwNFoX +DTM2MDgwNzExMDkwNFowGjEYMBYGA1UEAwwPaWRwLmV4YW1wbGUuY29tMIIBIjAN +BgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA2GDiRWsxK7vnhdWxhcx/MbgtnJXF +o56SL13+Sa7HoO4HzIdJzDPNznXS1gdhK/kVplY9+tamkEd+aSRxpt4RcNtG9kvs +wT8QOiwqxye7w/5DihIgysVNy7OUCvfn0xxVyG6Q7RgLiEppnHljWk2TyDHcT5aS +mCMjSSFHNkcUkRI7BpbK1lhLbK6DI8aWhzTbqWnUVJpB172bAhALIt6Xp432cRur +/UdtrQcCcYI/3Hl2cLU0Ce7bjbbYb/pFj+v0PaMB1t7hi/AYp+OhkagHx0hguuDa +PjcBExeYrnqYZCIILW765Ws03T8LvA3ZfJeCNUH2JjcSj8Z8ye6Q8LwXkQIDAQAB +o1MwUTAdBgNVHQ4EFgQUlbLjSTfPYYltgF5anYLJxHTRS/owHwYDVR0jBBgwFoAU +lbLjSTfPYYltgF5anYLJxHTRS/owDwYDVR0TAQH/BAUwAwEB/zANBgkqhkiG9w0B +AQsFAAOCAQEAjCv57yzpZMReoVJaZor6NGd5kcf8DfI2LLWJ4MGXzq/6kZLYy+Op +M1CxHA2wnxFmqcVmEra0zi2H2PkbM9p3oPK3upPdrL/ke2dIChP1yokaQoW9f2bY +K2INu9LIVuSD8hOUHDXPiH4Smt91V0GfrFHcxysfm97Y+TC+84grwcFE3JiRgfF+ +WYG9w8xaCTTorUKUGum8/5beRd8qNCxVnh4Ke5vaRaUj28MbqLSQp1dvm0cqe+4d +kna+UpbWKQOQ8uAAtFIH+bX2uh8NbCBfATfwEMYzAffGKkmRkkoQHNv0Uf5uIduu +GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== +-----END CERTIFICATE-----]] + + -- one SP per allow-list under test, picked by request header + ALLOW_LISTS = { + none = nil, + exact = { IDP }, + other = { "https://other.example.com" }, + } + SPS = {} + + function sp(name) + if SPS[name] == nil then + SPS[name] = require("resty.saml").new({ + sp_issuer = "sp", + idp_uri = "http://127.0.0.1:1984/idp", + login_callback_uri = "/acs", + logout_uri = "/logout", + logout_callback_uri = "/sls", + logout_redirect_uri = "/logout_ok", + sp_cert = CERT_PEM, + sp_private_key = KEY_PEM, + idp_cert = CERT_PEM, + secret = "very-secret-key-that-is-32-byte!", + idp_issuers = ALLOW_LISTS[name], + }) + end + return SPS[name] + end + + -- an assertion signed by the IdP key, so the wrapping Response below is + -- outside the signature exactly as it is in the wild + function signed_assertion(issuer, name_id) + local key = assert(saml.key_read_memory(KEY_PEM, saml.KeyDataFormatPem)) + saml.key_add_cert_memory(key, CERT_PEM, saml.KeyDataFormatCertPem) + local transform = saml.find_transform_by_href( + "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256") + local xml = string.format('' .. + '%s' .. + '%s', + issuer, name_id) + local out = assert(saml.sign_xml(key, transform, xml, + { id_attr = "ID", insert_after = { saml.XMLNS_ASSERTION, "Issuer" } })) + return (out:gsub("<%?xml.-%?>%s*", "")) + end + + function saml_response(response_issuer, assertion_issuer, name_id) + return string.format('%s' .. + '%s', + response_issuer, SUCCESS, signed_assertion(assertion_issuer, name_id)) + end + + -- start a login, then hand the crafted response back to the callback + -- with the session and RelayState that login handed out + function login_with(name, xml) + local httpc = require("resty.http").new() + local base = "http://127.0.0.1:1984" + local headers = { ["X-Test-SP"] = name } + + local res, err = httpc:request_uri(base .. "/", { headers = headers }) + if not res then return "login request: " .. err end + local cookie = res.headers["Set-Cookie"] + if type(cookie) == "table" then cookie = cookie[1] end + local state = res.headers["Location"]:match("RelayState=([^&]+)") + + res, err = httpc:request_uri(base .. "/acs", { + method = "POST", + body = "SAMLResponse=" .. ngx.escape_uri(saml.base64_encode(xml)) .. + "&RelayState=" .. state, + headers = { + ["X-Test-SP"] = name, + ["Cookie"] = cookie:match("^[^;]+"), + ["Content-Type"] = "application/x-www-form-urlencoded", + }, + }) + if not res then return "callback request: " .. err end + return res.status .. " " .. tostring(res.headers["Location"]) + end + } + + server { + listen 1984; + + location / { + access_by_lua_block { + sp(ngx.var.http_x_test_sp or "none"):authenticate() + } + + content_by_lua_block { + ngx.exit(200) + } + } + } +_EOC_ + + $block->set_value("http_config", $http_config); +}); + +run_tests(); + +__DATA__ + +=== TEST 1: no allow-list accepts the issuer the IdP key signs for +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("none", saml_response(IDP, IDP, "signed@example.com"))) + } + } +--- response_body +302 / + + + +=== TEST 2: an allow-listed issuer is accepted +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("exact", saml_response(IDP, IDP, "signed@example.com"))) + } + } +--- response_body +302 / + + + +=== TEST 3: an issuer outside the allow-list is rejected +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("exact", saml_response("https://elsewhere.example.com", + "https://elsewhere.example.com", "signed@example.com"))) + } + } +--- response_body +401 nil +--- error_log +unexpected issuer in response from IdP: https://elsewhere.example.com + + + +=== TEST 4: an allow-listed Issuer on the unsigned Response does not admit a foreign assertion +--- config + location /t { + content_by_lua_block { + -- the assertion is signed by the same key but issued by another + -- IdP, and the Response around it claims the allow-listed one + ngx.say(login_with("exact", saml_response(IDP, + "https://other.example.com", "attacker@example.com"))) + } + } +--- response_body +401 nil +--- error_log +unexpected issuer in response from IdP: https://other.example.com From 041bb56407320d3f7f07f73b420e427113f174b4 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Tue, 18 Aug 2026 16:21:50 +0545 Subject: [PATCH 3/5] fix: check the issuer of every assertion the readers consume A response signed as a whole may carry several assertions, and the readers do not confine themselves to one: doc_attrs collects from all of them and doc_name_id takes the first carrying a subject. Matching only the issuer doc_issuer returns therefore let an allow-listed first assertion carry a second one from an issuer nobody approved. doc_issuers lists the issuer of every top-level assertion, and the login callback requires all of them to be allow-listed. --- README.md | 2 +- lua/resty/saml.lua | 26 +++++++++++++----- src/lua_saml.c | 30 +++++++++++++++++++++ src/saml.h | 2 ++ src/xml.c | 66 +++++++++++++++++++++++++++++++++++++++++++++ t/login-callback.t | 62 +++++++++++++++++++++++++++++++++++------- t/signed-response.t | 21 +++++++++++++++ 7 files changed, 191 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 3866358..b594f87 100644 --- a/README.md +++ b/README.md @@ -73,7 +73,7 @@ local saml = resty_saml.new(opts) | `sp_issuer` | string | None | SP name to access IdP. | | `idp_uri` | string | None | URI of IdP. | | `idp_cert` | string | None | IdP Certificate, used to verify saml response. | -| `idp_issuers` | array of strings | None | Issuers accepted on a login response. Unset accepts any issuer the `idp_cert` signs for. | +| `idp_issuers` | array of strings | None | Issuers accepted on a login response; every assertion it carries has to name one. Unset accepts any issuer the `idp_cert` signs for. | | `login_callback_uri` | string | None | redirect uri used to callback the SP from IdP after login. | | `logout_uri` | string | None | logout uri to trigger logout. | | `logout_callback_uri` | string | None | redirect uri used to callback the SP from IdP after logout. | diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 8673e39..4d636c7 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -262,16 +262,27 @@ end -- say which IdP that key speaks for, so pin the issuer when the caller names -- the ones it expects. No list keeps the previous behaviour; a list nothing -- matches, an empty one included, admits nobody. -local function issuer_allowed(allowed, issuer) +-- +-- Every assertion is weighed, not just the one the issuer is taken from: a +-- response may legitimately carry several, and attributes are read from all of +-- them. Returns the offending issuer alongside a refusal. +local function issuers_allowed(allowed, issuers) if allowed == nil then return true end - for _, expected in ipairs(allowed) do - if expected == issuer then - return true + for _, issuer in ipairs(issuers or {}) do + local ok = false + for _, expected in ipairs(allowed) do + if expected == issuer then + ok = true + break + end + end + if not ok then + return false, issuer end end - return false + return true end local function login_callback(self, opts) @@ -317,8 +328,9 @@ local function login_callback(self, opts) local name_id = saml.doc_name_id(doc) local session_index = saml.doc_session_index(doc) - if not issuer_allowed(opts.idp_issuers, issuer) then - ngx.log(ngx.ERR, "unexpected issuer in response from IdP: ", tostring(issuer)) + local allowed, unexpected = issuers_allowed(opts.idp_issuers, saml.doc_issuers(doc)) + if not allowed then + ngx.log(ngx.ERR, "unexpected issuer in response from IdP: ", tostring(unexpected)) ngx.exit(ngx.HTTP_UNAUTHORIZED) end diff --git a/src/lua_saml.c b/src/lua_saml.c index 80baa9a..0d7adde 100644 --- a/src/lua_saml.c +++ b/src/lua_saml.c @@ -407,6 +407,35 @@ static int doc_issuer(lua_State* L) { } +/*** +Get the issuer of every assertion whose content the document's readers consume +@function doc_issuers +@tparam xmlDoc* doc +@treturn table issuers +*/ +static int doc_issuers(lua_State* L) { + lua_settop(L, 1); + xmlDoc* doc = doc_check(L, 1); + lua_pop(L, 1); + + xmlChar** issuers; + size_t issuers_len; + if (saml_doc_issuers(doc, &issuers, &issuers_len) < 0) { + lua_pushnil(L); + return 1; + } + + lua_newtable(L); + for (size_t i = 0; i < issuers_len; i++) { + lua_pushinteger(L, i + 1); + lua_pushstring(L, (char*)issuers[i]); + lua_settable(L, -3); + } + saml_issuers_free(issuers, issuers_len); + return 1; +} + + /*** Get the value of the StatusCode[Value] attribute in the document @function doc_status_code @@ -1160,6 +1189,7 @@ static const struct luaL_Reg saml_funcs[] = { {"doc_root_name", doc_root_name}, {"doc_id", doc_id}, {"doc_issuer", doc_issuer}, + {"doc_issuers", doc_issuers}, {"doc_name_id", doc_name_id}, {"doc_status_code", doc_status_code}, {"doc_session_index", doc_session_index}, diff --git a/src/saml.h b/src/saml.h index 7df4bfd..d582023 100644 --- a/src/saml.h +++ b/src/saml.h @@ -78,6 +78,8 @@ void saml_shutdown(); int saml_doc_validate(xmlDoc* doc); xmlChar* saml_doc_issuer(xmlDoc* doc); +int saml_doc_issuers(xmlDoc* doc, xmlChar*** issuers, size_t* issuers_len); +void saml_issuers_free(xmlChar** issuers, size_t issuers_len); xmlChar* saml_doc_name_id(xmlDoc* doc); xmlChar* saml_doc_status_code(xmlDoc* doc); xmlChar* saml_doc_session_index(xmlDoc* doc); diff --git a/src/xml.c b/src/xml.c index 65162b1..bc48190 100644 --- a/src/xml.c +++ b/src/xml.c @@ -73,6 +73,72 @@ xmlChar* saml_doc_issuer(xmlDoc* doc) { } +// Every issuer the message attributes content to: one per top-level assertion +// of a Response, or its own for a message that carries none. A caller matching +// the issuer against a policy has to weigh all of them, because doc_attrs reads +// every top-level assertion and doc_name_id the first one carrying a subject. +// An assertion with no Issuer is invalid SAML; it is listed as an empty string, +// which no configured issuer matches. +int saml_doc_issuers(xmlDoc* doc, xmlChar*** issuers, size_t* issuers_len) { + *issuers = NULL; + *issuers_len = 0; + + xmlNode* root = xmlDocGetRootElement(doc); + if (root == NULL) { + return 0; + } + + if (xmlStrEqual(root->name, (const xmlChar*)"Response") != 1) { + xmlChar* issuer = issuer_of(doc, root); + if (issuer == NULL) { + return 0; + } + *issuers = malloc(sizeof(xmlChar*)); + if (*issuers == NULL) { + xmlFree(issuer); + return -1; + } + (*issuers)[0] = issuer; + *issuers_len = 1; + return 0; + } + + size_t count = 0; + for (xmlNode* child = root->children; child != NULL; child = child->next) { + if (is_saml_assertion(child)) { + count++; + } + } + if (count == 0) { + return 0; + } + + *issuers = malloc(count * sizeof(xmlChar*)); + if (*issuers == NULL) { + return -1; + } + + size_t i = 0; + for (xmlNode* child = root->children; child != NULL && i < count; child = child->next) { + if (!is_saml_assertion(child)) { + continue; + } + xmlChar* issuer = issuer_of(doc, child); + (*issuers)[i++] = issuer == NULL ? xmlStrdup((const xmlChar*)"") : issuer; + } + *issuers_len = i; + return 0; +} + + +void saml_issuers_free(xmlChar** issuers, size_t issuers_len) { + for (size_t i = 0; i < issuers_len; i++) { + xmlFree(issuers[i]); + } + free(issuers); +} + + xmlChar* saml_doc_name_id(xmlDoc* doc) { xmlNode* node = xmlDocGetRootElement(doc); if (node == NULL) { diff --git a/t/login-callback.t b/t/login-callback.t index 4c71f9c..a17f144 100644 --- a/t/login-callback.t +++ b/t/login-callback.t @@ -92,6 +92,7 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== none = nil, exact = { IDP }, other = { "https://other.example.com" }, + both = { IDP, "https://other.example.com" }, } SPS = {} @@ -114,29 +115,36 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== return SPS[name] end - -- an assertion signed by the IdP key, so the wrapping Response below is - -- outside the signature exactly as it is in the wild - function signed_assertion(issuer, name_id) + function sign_doc(xml) local key = assert(saml.key_read_memory(KEY_PEM, saml.KeyDataFormatPem)) saml.key_add_cert_memory(key, CERT_PEM, saml.KeyDataFormatCertPem) local transform = saml.find_transform_by_href( "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256") - local xml = string.format('' .. - '%s' .. - '%s', - issuer, name_id) local out = assert(saml.sign_xml(key, transform, xml, { id_attr = "ID", insert_after = { saml.XMLNS_ASSERTION, "Issuer" } })) return (out:gsub("<%?xml.-%?>%s*", "")) end - function saml_response(response_issuer, assertion_issuer, name_id) + function assertion(issuer, id, name_id) + return string.format('' .. + '%s' .. + '%s', + id, issuer, name_id) + end + + function response(issuer, body) return string.format('%s' .. '%s', - response_issuer, SUCCESS, signed_assertion(assertion_issuer, name_id)) + issuer, SUCCESS, body) + end + + -- only the assertion is signed, so the Response around it, its own + -- Issuer included, is whatever the sender wants + function saml_response(response_issuer, assertion_issuer, name_id) + return response(response_issuer, sign_doc(assertion(assertion_issuer, "a1", name_id))) end -- start a login, then hand the crafted response back to the callback @@ -242,3 +250,37 @@ unexpected issuer in response from IdP: https://elsewhere.example.com 401 nil --- error_log unexpected issuer in response from IdP: https://other.example.com + + + +=== TEST 5: a second assertion the allow-list does not name is rejected +--- config + location /t { + content_by_lua_block { + -- the whole response is signed, so both assertions are covered and + -- both are read from, but only the first names an expected issuer + local xml = sign_doc(response(IDP, + assertion(IDP, "a1", "signed@example.com") .. + assertion("https://other.example.com", "a2", "other@example.com"))) + ngx.say(login_with("exact", xml)) + } + } +--- response_body +401 nil +--- error_log +unexpected issuer in response from IdP: https://other.example.com + + + +=== TEST 6: two assertions are accepted when the allow-list names both +--- config + location /t { + content_by_lua_block { + local xml = sign_doc(response(IDP, + assertion(IDP, "a1", "signed@example.com") .. + assertion("https://other.example.com", "a2", "other@example.com"))) + ngx.say(login_with("both", xml)) + } + } +--- response_body +302 / diff --git a/t/signed-response.t b/t/signed-response.t index a2fd92d..793f328 100644 --- a/t/signed-response.t +++ b/t/signed-response.t @@ -557,3 +557,24 @@ issuer: https://idp.example.com } --- response_body LogoutResponse issuer: https://idp.example.com + + + +=== TEST 21: every assertion the readers consume reports its issuer +--- config + location /t { + content_by_lua_block { + local key, mngr, transform = saml_ctx() + local a1 = assertion("a1", "first@example.com") + local a2 = (assertion("a2", "second@example.com") + :gsub("https://idp.example.com", "https://other.example.com")) + local doc, err = submit(mngr, sign(key, transform, response(SUCCESS, "resp-1", a1 .. a2))) + if err then + ngx.say("err: ", err) + else + ngx.say(table.concat(saml.doc_issuers(doc), ", ")) + end + } + } +--- response_body +https://idp.example.com, https://other.example.com From 7bbea1e28f4704d6952bf3c6fb46a08af4c6f091 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Tue, 18 Aug 2026 16:28:46 +0545 Subject: [PATCH 4/5] fix: fail closed when the issuers cannot be read A short or missing issuer list read as fewer assertions to vouch for than the document holds, and the callback let it through. saml_doc_issuers now reports an allocation failure instead of returning a partial list, and a configured allow-list refuses a response whose issuers come back empty or unreadable. --- lua/resty/saml.lua | 8 ++++++-- src/lua_saml.c | 2 +- src/xml.c | 28 +++++++++++++++++++--------- t/login-callback.t | 14 ++++++++++++++ 4 files changed, 40 insertions(+), 12 deletions(-) diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 4d636c7..9d019dc 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -265,12 +265,16 @@ end -- -- Every assertion is weighed, not just the one the issuer is taken from: a -- response may legitimately carry several, and attributes are read from all of --- them. Returns the offending issuer alongside a refusal. +-- them. A response whose issuers cannot be read vouches for nobody. Returns +-- what to name in the log alongside a refusal. local function issuers_allowed(allowed, issuers) if allowed == nil then return true end - for _, issuer in ipairs(issuers or {}) do + if type(issuers) ~= "table" or #issuers == 0 then + return false, "none readable" + end + for _, issuer in ipairs(issuers) do local ok = false for _, expected in ipairs(allowed) do if expected == issuer then diff --git a/src/lua_saml.c b/src/lua_saml.c index 0d7adde..fb314be 100644 --- a/src/lua_saml.c +++ b/src/lua_saml.c @@ -428,7 +428,7 @@ static int doc_issuers(lua_State* L) { lua_newtable(L); for (size_t i = 0; i < issuers_len; i++) { lua_pushinteger(L, i + 1); - lua_pushstring(L, (char*)issuers[i]); + lua_pushstring(L, issuers[i] == NULL ? "" : (char*)issuers[i]); lua_settable(L, -3); } saml_issuers_free(issuers, issuers_len); diff --git a/src/xml.c b/src/xml.c index bc48190..47f71f0 100644 --- a/src/xml.c +++ b/src/xml.c @@ -73,6 +73,14 @@ xmlChar* saml_doc_issuer(xmlDoc* doc) { } +void saml_issuers_free(xmlChar** issuers, size_t issuers_len) { + for (size_t i = 0; i < issuers_len; i++) { + xmlFree(issuers[i]); + } + free(issuers); +} + + // Every issuer the message attributes content to: one per top-level assertion // of a Response, or its own for a message that carries none. A caller matching // the issuer against a policy has to weigh all of them, because doc_attrs reads @@ -124,21 +132,23 @@ int saml_doc_issuers(xmlDoc* doc, xmlChar*** issuers, size_t* issuers_len) { continue; } xmlChar* issuer = issuer_of(doc, child); - (*issuers)[i++] = issuer == NULL ? xmlStrdup((const xmlChar*)"") : issuer; + if (issuer == NULL) { + issuer = xmlStrdup((const xmlChar*)""); + } + if (issuer == NULL) { + // a short list would read as fewer assertions to vouch for than the + // document holds, so report the failure rather than an incomplete answer + saml_issuers_free(*issuers, i); + *issuers = NULL; + return -1; + } + (*issuers)[i++] = issuer; } *issuers_len = i; return 0; } -void saml_issuers_free(xmlChar** issuers, size_t issuers_len) { - for (size_t i = 0; i < issuers_len; i++) { - xmlFree(issuers[i]); - } - free(issuers); -} - - xmlChar* saml_doc_name_id(xmlDoc* doc) { xmlNode* node = xmlDocGetRootElement(doc); if (node == NULL) { diff --git a/t/login-callback.t b/t/login-callback.t index a17f144..d2a590f 100644 --- a/t/login-callback.t +++ b/t/login-callback.t @@ -284,3 +284,17 @@ unexpected issuer in response from IdP: https://other.example.com } --- response_body 302 / + + + +=== TEST 7: a response with no readable issuer is rejected +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("exact", sign_doc(response(IDP, "")))) + } + } +--- response_body +401 nil +--- error_log +unexpected issuer in response from IdP: none readable From 39392632276c3d3f16da2c31736ea817844d635d Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 19 Aug 2026 19:48:53 +0545 Subject: [PATCH 5/5] fix: refuse a message the verified signature does not cover samlp:Extensions takes elements of any other namespace, so a LogoutRequest carrying an IdP-signed assertion there satisfies saml_verify_doc while the message around it stays the sender's to write. Nothing confined the readers in that case: the assertion is not a direct child, so the sweep a Response gets never reached it, and doc_name_id searched the whole document. Verification now requires the signature to cover the root of any message that carries no assertion to confine, and the logout readers take NameID and SessionIndex from the message itself rather than from wherever they appear first. This is what the issuer branch added here already assumed. Closes #36. An ArtifactResponse whose only signature sits on a nested assertion is refused outright now rather than read as empty (TEST 14). --- src/binding.c | 5 ++++- src/saml.h | 1 + src/sig.c | 26 +++++++++++++++------ src/xml.c | 53 +++++++++++++++++++++---------------------- t/signed-response.t | 55 ++++++++++++++++++++++++++++++++++++++++++++- 5 files changed, 103 insertions(+), 37 deletions(-) diff --git a/src/binding.c b/src/binding.c index 69f89a5..0f77e6e 100644 --- a/src/binding.c +++ b/src/binding.c @@ -43,6 +43,7 @@ static char* ERRORS[] = { "document does not validate against schema", "invalid signature algorithm", "signature does not match", + "signature does not cover the message", }; char* saml_binding_error_msg(saml_binding_status_t status) { @@ -303,7 +304,9 @@ saml_binding_status_t saml_binding_post_verify(xmlSecKeysMngr* mngr, xmlDoc* doc if (res < 0) { return SAML_XMLSEC_ERROR; } else if (res == 0) { - confine_identity_to_signature(doc); + if (!bind_identity_to_signature(doc)) { + return SAML_UNSIGNED_IDENTITY; + } return SAML_OK; } else { return SAML_INVALID_SIGNATURE; diff --git a/src/saml.h b/src/saml.h index d582023..d70aa3a 100644 --- a/src/saml.h +++ b/src/saml.h @@ -57,6 +57,7 @@ typedef enum { SAML_INVALID_DOC, SAML_INVALID_SIG_ALG, SAML_INVALID_SIGNATURE, + SAML_UNSIGNED_IDENTITY, } saml_binding_status_t; char* saml_binding_error_msg(saml_binding_status_t status); diff --git a/src/sig.c b/src/sig.c index 08a6d04..8800d76 100644 --- a/src/sig.c +++ b/src/sig.c @@ -339,20 +339,31 @@ static int signature_covers(xmlDoc* doc, xmlNode* sig, xmlNode* node) { } +// Leave the document with nothing a reader can read that the verified signature +// does not cover, and say whether that was possible at all. +// // Identity is read from /samlp:Response/saml:Assertion, i.e. only from an -// assertion that is a direct child of the verified root message. saml_verify_doc -// checks one Signature but not that it covers the assertion a reader will pick, -// so remove every top-level assertion that signature leaves out. A signature -// over the whole message covers all of them. The removed nodes are siblings, so +// assertion that is a direct child of the root message. saml_verify_doc checks +// one Signature but not that it covers the assertion a reader will pick, so +// remove every top-level assertion that signature leaves out. A signature over +// the whole message covers all of them. The removed nodes are siblings, so // freeing one never dangles another. -static void confine_identity_to_signature(xmlDoc* doc) { +// +// A message that carries no assertion has nothing to confine this way, and +// samlp:Extensions takes elements of any other namespace, so a signed assertion +// parked there satisfies saml_verify_doc while the message around it stays the +// sender's to write. Such a message is only trustworthy signed whole. +static int bind_identity_to_signature(xmlDoc* doc) { xmlNode* root = xmlDocGetRootElement(doc); if (root == NULL) { - return; + return 0; } xmlNode* sig = xmlSecFindNode(root, xmlSecNodeSignature, xmlSecDSigNs); if (sig != NULL && signature_covers(doc, sig, root)) { - return; + return 1; + } + if (xmlStrEqual(root->name, (const xmlChar*)"Response") != 1) { + return 0; } xmlNode* child = root->children; while (child != NULL) { @@ -363,4 +374,5 @@ static void confine_identity_to_signature(xmlDoc* doc) { } child = next; } + return 1; } diff --git a/src/xml.c b/src/xml.c index 47f71f0..1390e8f 100644 --- a/src/xml.c +++ b/src/xml.c @@ -33,27 +33,36 @@ static int is_saml_assertion(xmlNode* node) { } -// The text of node's own Issuer child, or NULL. Issuer is in the assertion -// namespace wherever it appears, so a look-alike in another one is not it. -static xmlChar* issuer_of(xmlDoc* doc, xmlNode* node) { +// The direct child of node named name in namespace ns, or NULL. Only direct +// children: an element the message itself declares is not the same as one a +// document-wide search happens to reach first. +static xmlNode* ns_child(xmlNode* node, const xmlChar* name, const char* ns) { for (xmlNode* child = node->children; child != NULL; child = child->next) { if (child->type == XML_ELEMENT_NODE && - xmlStrEqual(child->name, (const xmlChar*)"Issuer") == 1 && + xmlStrEqual(child->name, name) == 1 && child->ns != NULL && - xmlStrEqual(child->ns->href, (const xmlChar*)SAML_XMLNS_ASSERTION) == 1) { - return xmlNodeListGetString(doc, child->children, 1); + xmlStrEqual(child->ns->href, (const xmlChar*)ns) == 1) { + return child; } } return NULL; } +// The text of node's own Issuer child, or NULL. Issuer is in the assertion +// namespace wherever it appears, so a look-alike in another one is not it. +static xmlChar* issuer_of(xmlDoc* doc, xmlNode* node) { + xmlNode* issuer = ns_child(node, (const xmlChar*)"Issuer", SAML_XMLNS_ASSERTION); + return issuer == NULL ? NULL : xmlNodeListGetString(doc, issuer->children, 1); +} + + // A Response's issuer is read from its assertion, the element the identity // itself comes from. The Response's own Issuer sits outside an assertion-level // signature and can be rewritten without breaking it, while every top-level -// assertion still in the document is one the signature covers. Other messages -// carry no assertion and are signed whole, so their own Issuer is the one to -// read. +// assertion still in the document is one the signature covers. A message that +// carries no assertion is only accepted signed whole, so its own Issuer is the +// one to read. xmlChar* saml_doc_issuer(xmlDoc* doc) { xmlNode* root = xmlDocGetRootElement(doc); if (root == NULL) { @@ -82,7 +91,8 @@ void saml_issuers_free(xmlChar** issuers, size_t issuers_len) { // Every issuer the message attributes content to: one per top-level assertion -// of a Response, or its own for a message that carries none. A caller matching +// of a Response, or its own for a message that carries none and is therefore +// only accepted signed whole. A caller matching // the issuer against a policy has to weigh all of them, because doc_attrs reads // every top-level assertion and doc_name_id the first one carrying a subject. // An assertion with no Issuer is invalid SAML; it is listed as an empty string, @@ -156,7 +166,8 @@ xmlChar* saml_doc_name_id(xmlDoc* doc) { } if (xmlStrEqual(node->name, (xmlChar*)"LogoutRequest") == 1) { - node = xmlSecFindNode(node, (xmlChar*)"NameID", (xmlChar*)SAML_XMLNS_ASSERTION); + // the subject the request names, which the schema puts directly under it + node = ns_child(node, (const xmlChar*)"NameID", SAML_XMLNS_ASSERTION); if (node == NULL) { return NULL; } @@ -183,20 +194,6 @@ xmlChar* saml_doc_name_id(xmlDoc* doc) { } -// The direct child of node named name in the protocol namespace, or NULL. -static xmlNode* protocol_child(xmlNode* node, const xmlChar* name) { - for (xmlNode* child = node->children; child != NULL; child = child->next) { - if (child->type == XML_ELEMENT_NODE && - xmlStrEqual(child->name, name) == 1 && - child->ns != NULL && - xmlStrEqual(child->ns->href, (const xmlChar*)SAML_XMLNS_PROTOCOL) == 1) { - return child; - } - } - return NULL; -} - - xmlChar* saml_doc_status_code(xmlDoc* doc) { // Read the top-level message's status directly, not a document-wide match: // a nested Response (for example inside saml:Advice) can precede the root @@ -205,11 +202,11 @@ xmlChar* saml_doc_status_code(xmlDoc* doc) { if (root == NULL) { return NULL; } - xmlNode* status = protocol_child(root, (const xmlChar*)"Status"); + xmlNode* status = ns_child(root, (const xmlChar*)"Status", SAML_XMLNS_PROTOCOL); if (status == NULL) { return NULL; } - xmlNode* code = protocol_child(status, (const xmlChar*)"StatusCode"); + xmlNode* code = ns_child(status, (const xmlChar*)"StatusCode", SAML_XMLNS_PROTOCOL); if (code == NULL) { return NULL; } @@ -251,7 +248,7 @@ xmlChar* saml_doc_session_index(xmlDoc* doc) { } if (xmlStrEqual(node->name, (xmlChar*)"LogoutRequest") == 1) { - node = xmlSecFindNode(node, (xmlChar*)"SessionIndex", (xmlChar*)SAML_XMLNS_PROTOCOL); + node = ns_child(node, (const xmlChar*)"SessionIndex", SAML_XMLNS_PROTOCOL); if (node == NULL) { return NULL; } diff --git a/t/signed-response.t b/t/signed-response.t index 793f328..ee50524 100644 --- a/t/signed-response.t +++ b/t/signed-response.t @@ -436,7 +436,7 @@ LogoutResponse: urn:oasis:names:tc:SAML:2.0:status:Success } } --- response_body -root: ArtifactResponse, name_id: nil, role: nil +err: signature does not cover the message @@ -578,3 +578,56 @@ LogoutResponse issuer: https://idp.example.com } --- response_body https://idp.example.com, https://other.example.com + + + +=== TEST 22: a message with no assertion of its own must be signed whole +--- config + location /t { + content_by_lua_block { + local key, mngr, transform = saml_ctx() + -- the request carries no signature; the only one in the document + -- belongs to an assertion parked in Extensions, which samlp + -- accepts because it takes any other namespace + local stolen = sign(key, transform, assertion("stolen", "attacker@example.com")) + local logout = 'https://attacker.example.com' .. + '' .. stolen .. '' .. + 'victim@example.com' + local doc, err = submit(mngr, logout) + if err then + ngx.say("err: ", err) + else + ngx.say("issuer: ", tostring(saml.doc_issuer(doc)), + ", name_id: ", tostring(saml.doc_name_id(doc))) + end + } + } +--- response_body +err: signature does not cover the message + + + +=== TEST 23: a logout request names its own subject, not one parked in Extensions +--- config + location /t { + content_by_lua_block { + local key, mngr, transform = saml_ctx() + local logout = 'https://idp.example.com' .. + 'elsewhere@example.com' .. + 'victim@example.com' .. + 's-1' + local doc, err = submit(mngr, sign(key, transform, logout)) + if err then + ngx.say("err: ", err) + else + ngx.say("name_id: ", tostring(saml.doc_name_id(doc)), + ", session_index: ", tostring(saml.doc_session_index(doc))) + end + } + } +--- response_body +name_id: victim@example.com, session_index: s-1