api7 / api7/lua-resty-saml

Values from the network are logged unescaped, so log lines can be forged

Open
#47 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Perl
Stars
2
Forks
3
Avg merge
2d 20h
Merged PRs (30d)
3

Description

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:

  • lua/resty/saml.lua:443 and the two comparisons after it log issuer, name_id and session_index from a logout message
  • the unexpected issuer line added in #41 logs the refused issuer
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.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in lua/resty/saml.lua at lines 295 and 443, then inspect the two comparisons and the unexpected-issuer log from #41. Trace the RelayState logging path first and identify every network-derived value reaching these messages. Add coverage for the RelayState case and verify that all listed log sites no longer emit unescaped control characters.

Written by the indexing model from the issue text.

Assessment

Tech stack
lua
Domain
security
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.