fix: three consent-access defects left over after #2882 - #87
Merged
Conversation
…ID header findPsuByPsuId resolved the header to a user without asking whether that user may still act. The header is not a credential -- under Berlin Group the caller is the TPP and the PSU arrives as a header value -- so this path never authenticates the PSU, and AfterApiAuth.checkUserIsDeletedOrLocked, which every authenticated request passes through, never runs on it. What the resolved user is then used for is the problem. startConsentAuthorisation takes it as the PSU, assertBerlinGroupConsentAccountsHeld checks it against the consent's accounts and passes -- the accounts really are theirs -- createChallengesC2 mints an SCA challenge for them and an OTP goes out to that person, and the PUT twin binds the consent to them, after which it grants access to their accounts. A lock is the ASPSP's decision that this user may not authenticate; nothing on this path consulted it, and nothing downstream would have caught it. Refused with the same Empty an unresolvable PSU-ID gives, so resolvePsuIdHeader answers UserNotFoundByProviderAndUsername at 401 -- the standard's PSU_CREDENTIALS_INVALID -- either way. A distinct "that user is locked" would tell a TPP the username exists, which is the oracle the consent reads were unified to close. The real reason is logged. Same predicates as the canonical guard rather than a second opinion about what usable means. Both scenarios are mutation-checked: with the filter disabled the unit one resolves a locked user, and the endpoint one starts the authorisation (201 where 401 is expected) -- which is the OTP actually being sent, and the reason the refusal has to land before the challenge is minted rather than after.
Four of the five reads were unified to a bare ConsentNotFound so that a caller cannot tell "there is no such consent" from "that one is not yours". getConsentScaStatus kept spelling the id back: the replacement matched the default-status-code spelling of unboxFullOrFail and this site already passed 403 explicitly, so the pattern skipped it. Same status either way, different message -- the oracle survived at one endpoint. The scenario added here holds all five to the same answer at once, which is what would have caught the miss. It compares the status, the tppMessages code and the tppMessages text rather than the whole body. The Berlin Group error envelope carries a `path` field holding the request path, so it always contains whichever id the caller put in the URL -- they already knew it, and asserting on it would make the scenario fail for a reason that is not a leak.
…the doubt assertBerlinGroupConsentAccountsHeld collapsed the JWT read into `.getOrElse(Nil)`, so "this consent's JWT cannot be parsed" became "this consent names no accounts". The second is a legitimate state -- the availableAccounts shape, whose views are materialised later against the PSU's own holdings -- and the guard is right to pass on it. The first is not, and the guard passed on it too: with no account list to object to, a PSU who holds none of the consent's accounts was admitted, an OTP went out to them, and the PUT twin would bind the consent to them. Reaching it takes a structurally invalid JWT, which getSignedPayloadAsJson answers with a Failure. A consent can carry one: createConsent writes the consent row before computing and storing the JWT, so a consent whose JWT generation failed persists with none. A well-formed JWT carrying some other payload throws out of the extract instead -- Box.map does not catch -- and already failed closed with a 500; tryo brings that to the same clean refusal. Both are now the same answer, because they are the same fact: we cannot establish what this consent covers, so we do not let anyone authorise it. The guard had no test of its own. It has three now: the PSU who holds the named account may start the authorisation, one who does not may not, and an unreadable JWT refuses rather than passing vacuously -- the last of which fails with "201 equaled 201" without this change, the 201 being the OTP going out. Not included: updateViewsOfBerlinGroupConsentJWT rewrites a consent's JWT after the fact and has no callers, and it was the one thing that could have turned this into real data access -- bind under an unreadable JWT, then have a readable one written. That is moot now the guard refuses outright, so removing 60 lines of possibly-intended code no longer buys anything here; its fate is worth deciding on its own.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The three items deliberately excluded from OpenBankProject#2882, analysed before fixing. One commit each; each written test-first.
The analysis changed the ranking I had given. F5 — the Berlin Group accounts-held check failing open — was described as the most dangerous of the three. Tracing it shows it is not exploitable today: the same unreadable JWT that bypasses the check also stops the consent granting anything, because the read path cannot reach
applyConsentRules(consent: ConsentJWT, …)without a parsed JWT. The only thing that would make it live is a path that repairs or re-issues the JWT after binding —updateViewsOfBerlinGroupConsentJWTis exactly that, and it has no callers. So the hole is sealed by an accident of what is wired up, not by design. It is fixed here as a latent defect, last.The live one turned out to be the PSU-ID header.
1. A deleted or locked PSU could be named in
PSU-IDfindPsuByPsuIdresolved the header to a user without asking whether that user may still act. The header is not a credential — under Berlin Group the caller is the TPP and the PSU arrives as a header value — so this path never authenticates the PSU, andAfterApiAuth.checkUserIsDeletedOrLocked, which every authenticated request passes through, never runs on it.What the resolved user is then used for is the problem:
resolveBerlinGroupPsuaccepts it as the PSU.assertBerlinGroupConsentAccountsHeldchecks it against the consent's accounts — and passes, because the accounts really are theirs.createChallengesC2mints an SCA challenge and an OTP goes out of band to that person.A lock is the ASPSP's decision that this user may not authenticate. Nothing on this path consulted it, and nothing downstream would have caught it.
Refused with the same
Emptyan unresolvable PSU-ID gives, soresolvePsuIdHeaderanswersUserNotFoundByProviderAndUsernameat 401 — the standard'sPSU_CREDENTIALS_INVALID— either way. A distinct "that user is locked" would tell a TPP the username exists, which is the oracle OpenBankProject#2882 closed on the consent reads. The real reason is logged. Same predicates as the canonical guard rather than a second opinion about what "usable" means.Mutation-checked in both scenarios. With the filter disabled the unit one resolves a locked user, and the endpoint one starts the authorisation — 201 where 401 is expected. That 201 is the OTP actually being sent, and it is why the refusal has to land before the challenge is minted rather than after.
Verification
code.api.berlin.group.v1_3+code.api.UKOpenBanking+code.api.v5_1_0+DirectLoginTest— 826 tests, 0 failures.git_commitchecked against branch HEAD):run_probes.py109/0,uk_direction.py9/0,uk_view_endpoint_matrix.py12/12,stale_revoke_failure.py17/0,bg_legacy_payment.py15/15,uk_token_path.pyanduk_challenge_binding.pyOK.(The remaining two — the consent-id echo
#2882missed atgetConsentScaStatus, and the accounts-held fail-open — follow on this branch.)