Discrepancy between doc and implementation in `KeyMaterial::concatenate`

[Quant-TheodoreFelix]

Title

Discrepancy between documentation and implementation in KeyMaterial::concatenate

Description

I have noticed a contradiction between the rustdoc comments and the actual implementation of the KeyMaterial::concatenate method in crypto/core-interface/src/key_material.rs.

Documentation states- The documentation explicitly states that concatenating a full entropy key with a low entropy key should downgrade the result to a low entropy key:

"The resulting [KeyType] and security strength will be the lesser of the two keys."
...
"Concatenating a full entropy key with a low entropy key will result in a low entropy key."

(Source: crypto/core-interface/src/key_material.rs lines 156-164)

Implementation does- However, the implementation uses max() instead of min():

self.key_type = max(&self.key_type, &other.key_type()).clone();
self.security_strength = max(&self.security_strength, &other.security_strength()).clone();

(Source: crypto/core-interface/src/key_material.rs lines 569-570)

The Conflict- According to the PartialOrd implementation for KeyType, BytesLowEntropy is less than BytesFullEntropy (BytesLowEntropy < BytesFullEntropy).

Therefore, calling max() on these two types will result in BytesFullEntropy, effectively "upgrading" the security level of the low entropy key, which contradicts the "safe usage" design philosophy described in the documentation.

Fix- The implementation should likely be changed to use min() to adhere to the conservative security model described in the docs.

// Proposed change
use bouncycastle_utils::min;

// ... inside concatenate ...
self.key_type = min(&self.key_type, &other.key_type()).clone();
self.security_strength = min(&self.security_strength, &other.security_strength()).clone();

I hope this issue serves a good purpose...

[ounsworth]

Thank you so much for this report!
I am pleased that people are looking at bc-rust in this much detail! ❤️

Now that I am thinking about it; I am not sure exactly what the correct behaviour should be.

There is an interaction here between the KeyType and the SecurityStrength. I'm just thinking out loud here: if you concatenate these two values, what would you expect to get?

KeyMaterial = {key_len: 32, KeyType: BytesFullEntropy, SecurityStrength: _128bit}
KeyMaterial = {key_len: 32, KeyType: Zeroized, SecurityStrength: none}

You will get a KeyMaterial where the first 32 bytes are strong and the last 32 bytes are all zeroes. What is the correct KeyType and SecurityStrength? Maybe {KeyType: BytesLowEntropy, SecurityStrength: none}. In which case, you are correct that this should be min(), not max().

I will certainly think about this and fix either the code or the docs for the next alpha release. Thank you so much!

I would love to hear how you find bc-rust in general. Feel free to email me at mike@bouncycastle.org.

[Quant-TheodoreFelix]

Wow! That is undoubtedly an excellent insight. 🧐

For instance, considering a scenario where 32 bytes of full-entropy data with 128-bit security are concatenated with a 32-byte zero array, from a purely information-theoretic perspective, the inherent entropy does not vanish. Consequently, the total entropy of the combined data is maintained at 128 bits, as denoted by $H(X, Y) \ge H(X)$. I believe you have accurately pinpointed the exact intersection of these cryptographic principles!

However, from the perspective of cryptographic engineering- which strives for supreme stability and absolute security- I argue that the narrative changes entirely. Most cryptographic primitives expect a uniform distribution across the entirety of the input key array. If 64 bytes of data containing predictably fixed zeros were to be injected directly as a 512-bit block cipher key or seed, it would expose a critical vulnerability regardless of the aggregate entropy.

Ultimately, we must consider that the core primary purpose of this wrapper object is to prevent inadvertent developer misuse (misuse) via a robust type system. When high-quality and low-quality keys are commingled, the system must forcibly downgrade the security strength and key type based on the weakest link. Therefore, I concluded that it is fundamentally sound to restrict the state from a conservative standpoint by utilizing min() instead of max().

To be more specific, my understanding is that the primary design objective of the KeyMaterial wrapper object is to prevent vulnerable key material from being subjected to inappropriate cryptographic operations. If max() were used and the concatenation of high-entropy and low-entropy data was consequently upgraded (upgrade) to BytesFullEntropy, wouldn't this equate to the type system providing developers with a false guarantee regarding the quality of that key? In security engineering, a 'false positive'—reporting a system as secure when it is actually vulnerable—leads directly to a fatal vulnerability (vulnerability). Consequently, in states where absolute security cannot be guaranteed, conservatively applying min() to downgrade (downgrade) the system's evaluated state is a fundamental principle of defensive API design. This concept can be defined as the 'prevention of false positives (false positive) from a defensive programming perspective'.

[ounsworth]

Yup! I believe we have the same opinion here: it should be min(), not max().

One small caveat though: if you concatenate a key that is not Zeroized with a key that is Zeroized, I think the result should be BytesLowEntropy, not Zeroized. So it's not a straight min, but something like:

if (key1 == Zeroized && key2 != Zeroized) || (key1 != Zeroized && key2 == Zeroized) { type = BytesLowEntropy }

and for the SecurityStrength, I think it's just straight min with no exceptions, but I'll have to think this through carefully, and add some unit tests that seem to give the right behaviour.

Overall, the fact that you are leaving these wonderful detailed comments tells me that you think the concept of the KeyMaterial wrapper is good, yes?

[Quant-TheodoreFelix]

I completely agree with your assessment!

Treating the combination of a Zeroized key and a valid key as BytesLowEntropy is a very precise way to model entropy pollution, rather than incorrectly marking the entire state as completely destroyed. Applying a strict min() for SecurityStrength without exceptions also perfectly aligns with conservative security principles.

To answer your question- YES, I think the KeyMaterial wrapper concept is absolutely brilliant. In cryptographic engineering, preventing accidental misuse is just as important as the mathematical algorithms themselves.

Tracking the provenance, entropy level, and security strength of key material throughout its lifecycle using Rust's robust type system is an exemplary defensive design. It perfectly resonates with my core engineering philosophy of "absolute security". I look forward to the unit tests!