Skip to content

fix: prevent HMACKey secret from leaking via Debug#126

Open
Paraworker wants to merge 1 commit intojedisct1:masterfrom
Paraworker:hamc-key-debug
Open

fix: prevent HMACKey secret from leaking via Debug#126
Paraworker wants to merge 1 commit intojedisct1:masterfrom
Paraworker:hamc-key-debug

Conversation

@Paraworker
Copy link
Copy Markdown

HMACKey previously derived Debug, which would print the raw key bytes.

This PR implements Debug manually without exposing key data.

@jedisct1
Copy link
Copy Markdown
Owner

jedisct1 commented Apr 9, 2026

Swival Review

This review was automatically generated by Swival using exclusively open-source models running locally.

Finding

High severity. This pull request is incomplete, because it changes the Debug behavior only for HMAC keys and not for the other secret key types in the crate.

In src/algorithms/hmac.rs:16, the PR removes #[derive(Debug, Clone)] from HMACKey and adds a custom Debug implementation that hides the secret bytes. This part looks sensible by itself.

The problem is that the same protection is not applied to the other private or symmetric key types. Several secret-bearing structs still derive Debug, for example RSAKeyPair and its signing wrappers in src/algorithms/rsa.rs:79, src/algorithms/rsa.rs:258, src/algorithms/rsa.rs:420, src/algorithms/rsa.rs:582, src/algorithms/rsa.rs:744, src/algorithms/rsa.rs:898, and src/algorithms/rsa.rs:1060. The same issue exists for ECDSA key pairs in src/algorithms/es256.rs:18, src/algorithms/es384.rs:18, and src/algorithms/es256k.rs:17.

There is also still derived Debug on the HMAC wrapper types in src/algorithms/hmac.rs:303, src/algorithms/hmac.rs:431, src/algorithms/hmac.rs:530, and src/algorithms/hmac.rs:631, which keep a HMACKey inside.

What makes this more clear is that the project already has the redacted pattern for some other secret key types. You can see manual Debug implementations in src/algorithms/jwe/aes_kw.rs:21, src/algorithms/jwe/rsa_oaep.rs:120, src/algorithms/jwe/ecdh_es.rs:209, and src/algorithms/jwe/ecdh_es.rs:525.

So the codebase already goes in that direction, but this PR applies it only to one specific key family.

Because of this, I do not think the patch is ready to merge in the current form.

Either the redacted Debug policy should be applied consistently to all private and symmetric key types, or the change should not be introduced only for HMAC. Right now the project state becomes uneven.

Assumption

I assume the intent of the PR is to avoid accidental leakage of secret material through Debug. If that is the goal, the implementation needs to be extended across the whole set of secret-bearing key types.

Short conclusion

I found one important issue. The change is correct in isolation, but incomplete at project level. It addresses HMACKey in src/algorithms/hmac.rs only, while comparable key types still keep derived Debug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants