crypto: refuse stub RSA/ECC signing unless EOS_ALLOW_STUB_CRYPTO - #84
crypto: refuse stub RSA/ECC signing unless EOS_ALLOW_STUB_CRYPTO#84JoaoMorais03 wants to merge 1 commit into
Conversation
Verify already fail-closed; sign still minted fake signatures in production. Register the existing test_crypto_failclosed suite so that cannot regress. Signed-off-by: João Morais <118842104+JoaoMorais03@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The test CMakeLists currently defines duplicate targets (breaking test configuration) and the RSA signing stub still risks unsafe length underflow/overflow in EOS_ALLOW_STUB_CRYPTO builds.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens the stub RSA/ECC implementation in eos_crypto by making sign fail-closed (matching existing verify behavior) unless EOS_ALLOW_STUB_CRYPTO is explicitly enabled, and registers a regression test intended to prevent “fake signature” behavior from silently returning success in production builds.
Changes:
- Add
EOS_STUB_CRYPTO_REFUSE(...)guards toeos_rsa_sign_sha256()andeos_ecc_sign()so stub signing refuses to run withoutEOS_ALLOW_STUB_CRYPTO. - Register
test_crypto_failclosedin the test CMake list to exercise the fail-closed production path.
File summaries
| File | Description |
|---|---|
tests/CMakeLists.txt |
Registers test_crypto_failclosed (but currently contains duplicate AES/SHA512 targets that will prevent CMake from configuring tests). |
services/crypto/src/rsa_ecc_sha512.c |
Makes RSA/ECC signing match verify’s fail-closed behavior; small stub-safety/cleanup issues remain. |
Review details
Suppressed comments (1)
services/crypto/src/rsa_ecc_sha512.c:58
eos_rsa_sign_sha256()computeslenfromkey_bitsand then useslen - 35/len - 33later. If a caller sets an unexpectedly smallkey_bitsvalue (or an oversized one), this can underflow/overflow and cause out-of-bounds writes tosigin EOS_ALLOW_STUB_CRYPTO builds.
EOS_STUB_CRYPTO_REFUSE("eos_rsa_sign_sha256");
/* Stub: produce a deterministic fake signature for testing.
* Replace with real PKCS#1 v1.5 or PSS signing. */
size_t len = (size_t)(key->key_bits / 8);
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| add_executable(test_crypto_sha512 test_crypto_sha512.c) | ||
| target_link_libraries(test_crypto_sha512 PRIVATE eos_crypto) | ||
| add_test(NAME test_crypto_sha512 COMMAND test_crypto_sha512) | ||
|
|
||
| # --- test_crypto_failclosed: unimplemented RSA/ECC stubs refuse to run --- |
| EOS_STUB_CRYPTO_REFUSE("eos_ecc_sign"); | ||
| /* Stub: produce a fake 64-byte signature (r||s) */ | ||
| size_t len = 64; | ||
| if (hash_len < 32) len = 64; |
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Verified — this closes a real asymmetry
Checked origin/master before reviewing. The guards are on verify only:
50:int eos_rsa_sign_sha256(...) ← unguarded
67:int eos_rsa_verify_sha256(...)
69: EOS_STUB_CRYPTO_REFUSE("eos_rsa_verify_sha256")
80:int eos_ecc_sign(...) ← unguarded
92:int eos_ecc_verify(...)
97: EOS_STUB_CRYPTO_REFUSE("eos_ecc_verify")
So a build that correctly refuses to verify with stub crypto would still happily sign with it. Your reasoning for why that matters is the right one: a stub that mints a PKCS#1-looking blob or a fake r||s lets a signing pipeline appear to work end to end. The artifacts it produces are not signed by anything, and the failure surfaces at some later point — a real verifier rejecting them, or worse, a stub verifier accepting them — a long way from the code that caused it.
Making sign and verify symmetric is the correct fix, and reusing EOS_ALLOW_STUB_CRYPTO rather than inventing a second opt-in keeps one switch for one concept.
The test is the durable part
test_crypto_failclosed deliberately does not define EOS_ALLOW_STUB_CRYPTO, so it exercises the production path rather than the test path. That is the distinction that makes it a guard: the existing crypto tests all opt in, so none of them would notice the refusal being removed.
The note to replace it with known-answer tests against published vectors when real verification lands is the right instruction to leave behind — a fail-closed test becomes actively misleading once the function works.
One merge-order note
This conflicts with #82, which repairs master. Both touch tests/CMakeLists.txt: your new test_crypto_failclosed registration sits inside the duplicated test_crypto_aes / test_crypto_sha512 block that #82 removes — the duplicate is why master currently fails to configure at all.
The resolution is to keep your test_crypto_failclosed lines and drop the duplicated aes/sha512 ones, which are already registered earlier in the file. I did exactly that locally and it is clean.
Verification
Merged onto origin/master + #82, resolved as above: configure OK, 0 build errors, ctest 29/29. Not verified against master alone, because master does not currently configure.
ed25519_verify() decoded the public key and never checked which subgroup it
was in. Ed25519 has eight low-order points, and for any of them every term
of the verification equation collapses regardless of the message, so a
signature of all zeros verifies against arbitrary content.
This was not hypothetical. eos_pkg.c:94 ships
static const uint8_t eos_pkg_public_key[32] = {0};
and the all-zero encoding is one of the eight. Measured against the built
library, before this change:
all-zero key: 2 of 256 arbitrary signatures ACCEPTED
Package installation authenticated nothing. The SHA-256 check two lines
above is no help — it compares a hash of the payload against a field in the
same attacker-supplied file, which detects corruption, not origin. See #98.
The check multiplies the key by L and requires the identity, rather than
comparing against a table of the eight encodings. Both are correct; this one
cannot be wrong in a way that silently rejects valid keys because a constant
was transcribed incorrectly.
Two things this took to get right, both found by testing rather than
reasoning:
The subgroup test alone is insufficient. The identity has order 1, which
divides L, so [L]identity = identity and it passes. The first version of
this check still accepted an identity key. It is now rejected explicitly,
which is why the helper has two parts.
The regression test's first version passed against the unfixed code. The
all-zero key is order 4, so with S = 0 the equation reduces to [k](-A) and
whether that lands on R depends on k mod 4 — on the message. The forgery
succeeds for roughly one message in four, and "untrusted firmware" happened
to be one of the three that do not verify. It looked like a regression test
and was not one. It now sweeps messages verified to be accepted by the
unfixed implementation.
unfixed test_all_zero_key_is_rejected [FAIL] at line 112
fixed 5/5 tests passed
The suite covers all 64 combinations of the eight low-order encodings as key
and as R, plus RFC 8032 vector 1 — without which a check that rejected every
key would also pass.
ctest 28/29 -> 29/29 passed, 0 build errors
This does not make package signing work. A correct verifier with an all-zero
trust anchor now rejects everything instead of accepting everything, which is
the safer failure and still not a functioning signature check. #98 covers
supplying a real key, and whether eos_pkg should refuse to run at all while
its anchor is unset — the shape #84 uses for stub RSA/ECC.
Refs #98
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contribution after reviewing the project: RSA/ECC verify already fail-closed unless
EOS_ALLOW_STUB_CRYPTOis defined, but sign still minted fake signatures in production builds.Issue
services/crypto/src/rsa_ecc_sha512.cdocuments that the RSA/ECC routines are stubs, not PKCS#1/ECDSA. After the earlier verify-side fix,eos_rsa_verify_sha256/eos_ecc_verifyrefuse to run without the opt-in.eos_rsa_sign_sha256still wrote a PKCS#1-looking blob ending in the hash and returned 0;eos_ecc_signstill wrote a fake 64-byte r||s and returned 0. A signing pipeline could look like it worked while producing forgeries.tests/test_crypto_failclosed.calready asserts that signing must refuse, and is listed among unregistered tests. It was not intests/CMakeLists.txt.Approach
Call
EOS_STUB_CRYPTO_REFUSEfrom both sign functions after the existing NULL checks, matching verify. Registertest_crypto_failclosedagainsteos_cryptowithoutEOS_ALLOW_STUB_CRYPTOso CI covers the production path. Existingtest_crypto_rsa/test_crypto_ecconly(void)the sign/verify status, so they still pass.Testing
The fail-closed suite checks:
I did not run CTest here (no local checkout).
test_crypto_rsa.c/test_crypto_ecc.cwere read to confirm they do not assert sign success.Limitations
Does not implement real RSA/ECDSA. Tests that want the old stub behavior must define
EOS_ALLOW_STUB_CRYPTO(none of the currently registered tests do). Master currently has duplicatetest_crypto_aes/test_crypto_sha512CMake targets, so a full test configure still fails until that is fixed independently; this PR only addstest_crypto_failclosed.