1610_verify_integrity_of_high_value_payloads - #2257
Draft
HayimShaul wants to merge 1 commit into
Draft
Conversation
The store services persisted whatever they were handed. Nothing in their API stated whether the bytes had been cryptographically verified, and nothing re-checked them on the way out, so a wrong row, a truncated blob or a cross-transaction payload was indistinguishable from authentic evidence. Add a small `token/services/storage/integrity` package with typed sentinel errors and wire it into the paths where a stored payload becomes a claim someone acts on: - Token requests are refused at insert if the transaction id, the request bytes or the public parameters hash are empty. On retrieval, `ttxdb`/`auditdb` deserialise the stored payload and require its anchor to equal the transaction id it is filed under. `endorserdb` holds the bare actions format, which carries no anchor, so it gets a version and non-empty-actions check instead. - Public parameters are addressed by their hash, so `PublicParamsByHash` now recomputes that hash over what it is about to return and refuses a mismatch. These carry the issuer and auditor keys and the setup every action is validated against. - Empty identities are refused, and identity lookups compare the stored identity against the requested one. This matters because an empty identity's unique id is the constant "<empty>", so every empty identity collides on one row and one cache key. Applied to both the SQL and KVS backends. - Endorsement acknowledgements are refused at insert if the endorser or the signature is empty. Re-verifying the signature on retrieval is not possible today: the row holds only (endorser, sigma), not the per-party filtered payload that was signed. Signer registration additionally refuses an empty identity or a nil verifier. A stronger check — that a supplied verifier agrees with the identity it is registered for — is not expressible: `driver.Verifier` is only `Verify(message, sigma) error` and exposes no canonical key. The checks are unconditional. `nobypass_test.go` parses the AST of the affected packages and fails if anyone adds a bypass-shaped identifier, gives a check an options parameter or a second result, introduces a verification config key, or discards a check's result. Cross-backend expectations live in the shared `dbtest` spec so SQL and KVS are held to one contract; that spec caught a real gap in SQL `GetTokenInfo`, which had no empty-identity guard and never compared the stored identity. Two fuzz targets cover the request parsers and are wired into the nightly fuzz matrix. `docs/security/store_integrity_verification.md` records the posture per asset class, including the classes that are deliberately not verified and why. Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com>
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
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.
Fixes #1610
The store services persisted whatever they were handed. Nothing in their API said whether the
bytes had been cryptographically verified, and nothing re-checked them on the way out — so a
wrong row, a truncated blob or a cross-transaction payload was indistinguishable from authentic
evidence.
This adds
token/services/storage/integrity(typed sentinel errors, no options, no config) andwires it into the paths where stored bytes become a claim someone acts on:
empty public parameters hash. On retrieval,
ttxdb/auditdbdeserialise the payload andrequire its anchor to equal the transaction id it is filed under.
endorserdbholds thebare actions format, which has no anchor field, so it gets a version and non-empty-actions
check instead.
PublicParamsByHashnow recomputes the hash of what it is about toreturn and refuses a mismatch. Parameters are addressed by that hash and carry the issuer
and auditor keys plus the setup every action is validated against, which makes them the
highest-value payload in the store.
identity against the requested one. This is substantive rather than cosmetic: an empty
identity's unique id is the constant
"<empty>", so every empty identity collides on one rowand one cache key. Applied to both the SQL and the KVS backend.
The checks are unconditional, and
nobypass_test.gomakes that enforceable rather thanconventional: it parses the AST of the affected packages and fails if anyone adds a
bypass-shaped identifier, gives a check an options parameter or a second result, exposes mutable
state from the check package, introduces a verification config key, or discards a check's
result.
Cross-backend expectations live in the shared
dbtestspec so SQL and KVS are held to onecontract. That spec caught a real gap: SQL
GetTokenInfohad no empty-identity guard and nevercompared the stored identity against the requested one — fixed here. Two fuzz targets cover the
request parsers and are wired into the nightly fuzz matrix.
What this deliberately does not do
Written up per asset class in the new
docs/security/store_integrity_verification.md,including:
(endorser, sigma); the per-party filtered payload that was actually signed is not persisted,so there is nothing to verify against without a schema change.
driver.Verifieris onlyVerify(message, sigma) errorand exposes no canonical key, so thecomparison is not expressible against the current interface.
pp_hashis not a digest of the request bytes, so it is not what makes a retrieved requestcheckable — the anchor is.
implied.
Behavioural note
Requests are now refused at insert where they would previously have been stored, and retrieval
fails closed on a payload that does not bind to its transaction id. The audit in the issue thread
found no in-tree caller affected — every current producer already verifies before storing — so
this is defence-in-depth and an explicit contract, not a fix for an exploitable hole in a current
flow. Out-of-tree callers that store unverified data would see an explicit error naming the failed
check.
Verification
make checks— passes.make unit-tests— passes, excepttoken/services/identity/configTestTranslatePath, whichasserts the absolute checkout path contains the string
"panurus"and so fails for any clonenot named that. Pre-existing and unrelated.
make lint-auto-fix— clean for every file this branch touches. It still reports 17 reviveunhandled-errorfindings in files this branch does not touch (token/driver/wallet.go,token/services/benchmark/runner.go,token/services/network/common/rws/…,token/services/storage/db/sql/query/common/builder.go, and others). Those reproduce on a cleanworktree at
origin/main, somake lintdoes not currently pass onmaineither. Left alonehere to keep this change reviewable; happy to fix them in a separate PR.