Raised while reviewing #41, which is where the escaping question came up, but the exposure is older than that PR and wider than the line it was raised on.
What
Values that arrive from the network are logged unescaped, so a newline in one forges log lines. The easiest one needs no key at all:
-- lua/resty/saml.lua:295
ngx.log(ngx.ERR, "state different: args.state=", state, ", state=", saml_state)
state is args.RelayState, straight off the query string on an unauthenticated endpoint. Nothing is verified before it is logged, so anybody who can reach the ACS URL can write whatever they like into the error log, repeatedly.
The others take signed content, so they need a certificate the deployment trusts, which makes them narrower:
Why it matters
Forged entries in an error log undermine whatever reads it: an operator scanning by eye, or anything that parses the file into an alerting or audit pipeline. A convincing [error] line naming a different client address is cheap to write.
Suggested fix
Escape the value at every site rather than one of them, since the point of the exercise is that no untrusted string reaches the log verbatim. Something small and local, replacing control characters with an escape, applied uniformly. A test for the RelayState line is the one that matters most, being the one an attacker can reach without a key.
Raised while reviewing #41, which is where the escaping question came up, but the exposure is older than that PR and wider than the line it was raised on.
What
Values that arrive from the network are logged unescaped, so a newline in one forges log lines. The easiest one needs no key at all:
stateisargs.RelayState, straight off the query string on an unauthenticated endpoint. Nothing is verified before it is logged, so anybody who can reach the ACS URL can write whatever they like into the error log, repeatedly.The others take signed content, so they need a certificate the deployment trusts, which makes them narrower:
lua/resty/saml.lua:443and the two comparisons after it logissuer,name_idandsession_indexfrom a logout messageunexpected issuerline added in fix: read the issuer from signed content, and let it be pinned #41 logs the refused issuerWhy it matters
Forged entries in an error log undermine whatever reads it: an operator scanning by eye, or anything that parses the file into an alerting or audit pipeline. A convincing
[error]line naming a different client address is cheap to write.Suggested fix
Escape the value at every site rather than one of them, since the point of the exercise is that no untrusted string reaches the log verbatim. Something small and local, replacing control characters with an escape, applied uniformly. A test for the
RelayStateline is the one that matters most, being the one an attacker can reach without a key.