Refactor #14
Merged
Refactor #14
Conversation
This file contains 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
Member
mchack-work
commented
Apr 16, 2024
mchack-work
commented
- Fix TOCTOU
- Introduce explicit state machine.
- Exit very early on suspicious behaviour.
- Remove PH commands, add firmware hash commands.
CDI is byte readable (but word writable) so we can use the address of CDI directly as input when generating the key pair. This way we don't have to keep a local copy on the stack at all. Note that QEMU will warn for writes of the wrong size since crypto_ed25519_key_pair() calls crypto_wipe() on the seed after using it.
Check message size on on an APP_CMD_SET_SIZE before setting it. Previously we set message_size *before* the check and later used that same variable, even if it was wrong, on any incoming APP_CMD_SIGN_DATA data blocks.
Add check target running at least clang-tidy.
dehanj
approved these changes
Apr 16, 2024
Introduce an explicit state machine controlling the protocol handling. States: - STATE_STARTED: Allows some info gathering commands, name/version, get public key, set size of message, or load a fix-sized pre-hashed message in a single command. - STATE_LOADING: Allows loading a message (up to 4 kByte) to sign. - STATE_SIGNING: Allows asking for the signature, which triggers the signing operation, and return of the signature. - STATE_FAILED: parse error or state failure. Never returns. We make sure to often move to STATE_FAILED on anything suspicious which then halts the CPU until powercycled. For instance we only allow firmware probes in STATE_STARTED and entering STATE_FAILED on any probes in another state and any parse errors or unexpected lengths in any states. See docs/implementation-notes.md for more details. One might argue that we should sign and return the signature unconditionally when the last block of the message has been received, but we'll have to wait to do that change since that changes the application protocol. This would break any client apps depending on the signer without warning. Other refactoring that was done simultaneously with the above work include: - Use memory writing functions with destination size whenever possible. - Less use of magic numbers. - Clean up code and simplify names of for instance protocol constants. This work was done in tandem to updates in tkey-libs and needs modern tkey-libs. App version is now: 3.
Instead of supporting pre-hash Ed25519 signatures, we replace these commands with an ability to get the device app to do a SHA512 hash over specified sizes of the firmware, the exact same commands that the verisigner app in tkey-verification does. This way we don't have to maintain two almost identical device apps. Since these commands just happened to have the exact same constants as the command and response verisigner used for firmware hashing we override them with this implementation. Pre-hash signing has never been officially supported in any released tkeysign Go package and a sampling of users showed no one else seems to use it either. Furthermore, Ed25519ph seems to be frowned upon by cryptographers. We prefer not to support it. We realize that since we support very small messages to be signed and have low bandwidth people will probably ask for a signature of a digest anyway, but it seems important to keep the signer pretty simple, and there are ways to mitigate an exploit of a hash collision. We leave that up to the caller.
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.