fix: the login form carries a request token too - #880
Conversation
`Csrf::check()` opened with `isLoggedIn()`, and `initialize()` minted a token only for a
session that was already authenticated. So every request made before signing in was
unprotected — the sign-in itself above all.
That is login CSRF, and it is reachable here with a bare cross-site form:
<form action="https://victim/index.php?r=login/login" method="POST">
<input type="hidden" name="user" value="attacker">
<input type="hidden" name="pass" value="...">
</form>
`analyzeEncrypted()` falls back to the raw request value when what it received is not RSA
ciphertext — deliberately, for scripted installs — so the attacker does not even need the
installation's public key. SameSite does not stand in the way: it governs whether an
existing cookie is *sent*, and this attack does not want the victim's cookie, it wants the
response to set a new one. The victim's browser ends up holding a session authenticated as
the attacker, with no visible logout, and everything they file afterwards — accounts,
passwords — goes into a vault the attacker can open.
Two changes, and the second is the one that matters:
- The token is minted for any session, not only an authenticated one. It has to exist
before the request that needs it, and the request that needs it most is the sign-in.
- A state-changing request whose session holds no token is now refused rather than passed.
Without that the check still means nothing before sign-in: a browser that has never
loaded a page of ours has no token, which is exactly the request to refuse.
Being signed in no longer takes part in the decision either way. The token is the check.
The unit tests pinned the old contract and are rewritten against the new one, including
that a plain read is still let through — the token cannot travel on a bare GET, there is
no header to put it in.
Three integration tests also had to change, and what they showed is worth recording: they
build their own SessionContext stub with isLoggedIn true and no getCSRF, so the old guard
skipped them on the `getCSRF() !== null` clause and they were never exercising CSRF at
all. They model a real session now, which is what the shared harness has always done.
|
Closing rather than merging: this breaks login, and the reason is worth recording. The vulnerability is real. What this branch missed is why the original code was written that way: Fixing it properly means changing the login page's session lifecycle — either having it |
Csrf::check()opened withisLoggedIn(), andinitialize()minted a token only for asession that was already authenticated. So every request made before signing in was
unprotected — the sign-in itself above all.
That is login CSRF, and it is reachable here with a bare cross-site form:
analyzeEncrypted()falls back to the raw request value when what it received is not RSAciphertext — deliberately, for scripted installs — so the attacker does not even need the
installation's public key. SameSite does not stand in the way: it governs whether an
existing cookie is sent, and this attack does not want the victim's cookie, it wants the
response to set a new one. The victim's browser ends up holding a session authenticated as
the attacker, with no visible logout, and everything they file afterwards — accounts,
passwords — goes into a vault the attacker can open.
Two changes, and the second is the one that matters:
before the request that needs it, and the request that needs it most is the sign-in.
Without that the check still means nothing before sign-in: a browser that has never
loaded a page of ours has no token, which is exactly the request to refuse.
Being signed in no longer takes part in the decision either way. The token is the check.
The unit tests pinned the old contract and are rewritten against the new one, including
that a plain read is still let through — the token cannot travel on a bare GET, there is
no header to put it in.
Three integration tests also had to change, and what they showed is worth recording: they
build their own SessionContext stub with isLoggedIn true and no getCSRF, so the old guard
skipped them on the
getCSRF() !== nullclause and they were never exercising CSRF atall. They model a real session now, which is what the shared harness has always done.