Remove AES weak key test

[AaronFeickert]

A test for alleged AES weak keys was introduced in #465 based on TCG documentation. But as noted in a comment to #525, no rationale or further citation for the claim was given in that documentation or was otherwise known when the test was added.

Unless there is a known rationale for the weak key claim, it seems unnecessary to retain the test at all.

This PR removes it for AES.

Closes #525.

[tarcieri]

Why do you consider the TCG citation inadequate? And why don't you consider such keys weak?

As I said on #525, if you have an AES key with an upper half of all zeros, one of two things has happened:

• a 1:18446744073709551616 event has occurred
• your RNG is outputting at least 64-bits of zeros

If it's an AES-128 key, that means you have at most a key with 64-bits entropy, which is well within the realm of a brute force attack.

But beyond that, it speaks to a critical failure of the RNG where the keys it's outputting are not uniformly random (or, if they are, a 1:18446744073709551616 event has occurred)

[AaronFeickert]

The TCG claim doesn't include any reasoning or further citation about why the particular bit pattern results in a weak key.

You could include an almost infinite number of tests to try and detect low entropy, but that does not appear to be what the weak-key test is intended to capture.

If there is a cryptographic reason why the claim is correct, it should be included. I have not been able to track down any detail on it elsewhere, but would like to know if it exists.

[tarcieri]

The TCG claim doesn't include any reasoning or further citation about why the particular bit pattern results in a weak key.

The reasoning is easy for anyone with a cryptographic background to spell out. I spelled it out for you. I don't think the absence of a stated rationale in the standards document alone is sufficient reason to remove the test.

You could include an almost infinite number of tests to try and detect low entropy, but that does not appear to be what the weak-key test is intended to capture.

That seems like a slippery slope argument. This seems like a reasonable enough test to me. If you can find a better test in a different standards document elsewhere, we can switch to that instead. "I can think of a different test for the same property" is not a sufficient reason to remove the test.

If there is a cryptographic reason why the claim is correct, it should be included. I have not been able to track down any detail on it elsewhere, but would like to know if it exists.

As I already said, at least for AES-128 such keys lack sufficient entropy to be secure and are vulnerable to a brute force attack.

[AaronFeickert]

I don't want this discussion to go too far into an unhelpful back and forth, but I do want to ensure there's clarity on two things:

• Is the weak-key test intended to capture keys that are weak for reasons inherent to the algorithm (as in DES), or also keys that are generated using improper entropy or a faulty generator?
• Is it sufficient to rely on the linked document's lack of citation on the particular bit pattern weakness?

I disagree with the idea that the test should cover the property of "poor entropy" at all, and should only focus on weak keys inherent to the algorithm in question (here, AES).

[tarcieri]

Is the weak-key test intended to capture keys that are weak for reasons inherent to the algorithm (as in DES), or also keys that are generated using improper entropy or a faulty generator?

This is a reasonable enough question and the KeyInit::weak_key_test method is perhaps a bit underdocumented in that regard.

I don't see the harm in the API covering both of those cases, especially if the test for the latter is coming from a standards document like the TCG, even without a stated rationale so long as we understand the rationale.

The compliance processes I'm familiar with tend to focus on the former and completely ignore the latter, however I am not an expert on those and it would be good to hear from people who are what they expect out of a weak key test.

Is it sufficient to rely on the linked document's lack of citation on the particular bit pattern weakness?

I don't have a problem with it because it is easy enough to conclude that such a test will, with a vanishingly small chance of false positives, reliably detect a class of keys which lack sufficient entropy to be secure which are vulnerable to practical brute force attacks.

I asked you earlier but I don't think you've fundamentally disagreed that such keys are weak. Do you? And if you don't, what specifically do you find problematic about the test from a technical perspective, beyond your already stated argument that the standards document does not spell out its rationale?

[newpavlov]

@tarcieri
A big problem with your position is that according to it we would need to add similar tests to all other block cipher implementations and I don't think we want to do it.

My expectation for the weak block cipher test is that it rejects keys which cause a significant degradation of cipher security bigger than reduction of key entropy. I should've looked into rationale behind the TCG check, but I don't think it satisfies the criteria.

I think it may be worth to consider full removal of the weak key test from the trait API and replace it with an inherent test function in the des crate, similarly to how we do not define traits for sha1-checked.

[AaronFeickert]

I agree that any key sampled using poor entropy or a faulty generator should not be used, absolutely. But I also think there two important points:

• When discussing weak keys, what usually seems to be meant is keys that structurally lead to unexpected or undesirable behavior due to the algorithms with which they are used, not keys that are poorly sampled. I think it's an important distinction that should at the very least be well documented. When someone asks "does DES have weak keys?" the answer is yes for structural reasons. When someone asks "does AES have weak keys?" the answer is (unless there's work I am unaware of) no by this definition, except if you read the TCG document.
• The library should not give a false sense of security regarding poor entropy testing. It's of course not possible to definitively identify a faulty generator. You could write a thousand tests that heuristically try to catch this, but I would not want users to think that passing this test gives more certainty than the user should otherwise expect.

Further, unless I am missing something, the TCG document for AES is not consistent with any other AES-related standard I've seen with respect to key sampling. I absolutely get that the keys in question should never occur using proper sampling, and that perhaps there's an argument to be made that going with the most conservative approach has benefit. However, I would be wary of adopting a claim that seems very much to stand out (as this does) without being apparently corroborated anywhere else, and without any deeper analysis. I think it would set a risky example for the library, which is (IMHO) very much a gold standard for cryptographic implementations.

[tarcieri]

The problem with your position is that according to it we would need to add similar tests to all other block cipher implementations and I don't think we want to do it.

Honestly I think it's a reasonable enough default implementation, but I digress. What you're making is also a slippery slope argument, and while the algorithm is general enough and I agree it could work for all block ciphers, TCG stipulated it specifically for AES.

To give a motivating example, here is a recent vulnerability in a (contributed) feature in one of our crates:

GHSA-w3g8-fp6j-wvqw

In this example, a weak k was being generated because the code confused bits and bytes.

I think it would be good if someone made a similar mistake in symmetric key generation and failed to fill the entire key with uniform randomness that the weak key test could catch such a bug.

[tarcieri]

When discussing weak keys, what usually seems to be meant is keys that structurally lead to unexpected or undesirable behavior due to the algorithms with which they are used, not keys that are poorly sampled. I think it's an important distinction that should at the very least be well documented.

While there's an argument to be made that it's more semantically correct when discussing the specific properties of a block cipher, in terms of practical outcomes, applying a baseline check that the key has sufficient entropy could catch bugs without harmful consequences.

I would be curious though if any compliance process had something specific to say on classes of keys the tests should specifically allow, e.g. "FIPS says the weak key test needs to detect structurally weak DES keys but MUST allow all-zero AES keys"

[AaronFeickert]

In case it's helpful to step back a bit, from a standards (and related documentation) perspective, there seems to be at the very least an inconsistency between TCG and, say, FIPS 197 that perhaps warrants discussion about how to decide on an implementation approach.

Specifically, TCG says don't use the bit pattern. FIPS says little about key generation, aside from noting (in section 6.2) that if a key is generated according to a cited NIST publication, then there are no further restrictions on the use of the key. Notably, the NIST publication includes nothing about the bit pattern.

It could be that other AES-related standards say other things (I'd love to know if this is the case) that are similarly inconsistent, and which could warrant similar discussion about how to assess the differences and decide what goes into the implementation.

[AaronFeickert]

And I apologize if this sounds petty, but I think it's really on TCG to justify their claim about why the bit pattern is weak (for structural reasons, not entropy), not on the rest of the ecosystem to justify why it isn't.

[AaronFeickert]

I would be curious though if any compliance process had something specific to say on classes of keys the tests should specifically allow, e.g. "FIPS says the weak key test needs to detect structurally weak DES keys but MUST allow all-zero AES keys"

I think this is intriguing, and I wish the standards were more clear on it.

[newpavlov]

I asked about it in /r/crypto, maybe someone will know more about rationale behind the AES check.

I think it's really on TCG to justify their claim about why the bit pattern is weak

I agree.

[newpavlov]

I think it would be good if someone made a similar mistake in symmetric key generation and failed to fill the entire key with uniform randomness that the weak key test could catch such a bug.

It's debatable whether it's responsibility of a block cipher implementation to catch such bugs. Do not forget that the weak key test is optional, so it's unlikely to help with a sloppy user.

I think we should define what we mean by "weak key test". I suggested the definition above and IIUC the AES check does not satisfy it, dunno why you call argument based on lack of a DES-like cryptographic vulnerability "a slippery slope". What would be your definition? "Just in case" is a bad criteria IMO.

[tarcieri]

The real question here seems to be whether or not the weak key test should cover insufficient entropy in addition to classes of structurally flawed keys in the context of algorithm-specific weaknesses. A key with insufficient entropy is, fundamentally, weak. It is weak to brute force attacks. It is a different kind of weakness from a structurally flawed key used in conjunction with a flawed block cipher, but IMO it is still weak nonetheless.

My argument is "if it did, it could help catch bugs". The counterargument seems to be "that's not this test's job".

I think the practical outcome of removing this check is not an improvement. Bugs that it could've caught won't be. The only way it's an "improvement" is the function of the weak key test will be more semantically correct for certain definitions of weakness, while still allowing what can reasonably be considered weak keys under other definitions.

[AaronFeickert]

This may not be a particular concern for the context of this discussion, but if there is at least no documentation change, I worry that this check will result in an unnecessary "cargo culting" elsewhere, due to poor understanding of what the check is intended to convey. My anecdote is but a single perspective, but I came across this issue in the first place while looking into particular low-level AES operations for a new library. I chose to use this implementation as a secondary check for fuzz testing, and was confused when I saw the test and assumed there was an AES cryptographic weakness that I was unaware of.

On net, I think weak-key testing should be defined here to account for structurally-weak keys. If there's a desire to do optional heuristic low-entropy checks, that seems better suited for a broader and well-documented design that could apply to all ciphers identically, giving better clarity.

[tarcieri]

a broader and well-documented design that could apply to all ciphers identically, giving better clarity.

FWIW, I think such a test broadly applies not just to all symmetric ciphers, but to practically all classes of keys.

Such a check, applied to the standard byte-oriented serialization of SM2 scalars, would've caught the issue with k having insufficient entropy (though it wouldn't have if SM2 used a little endian serialization, suggesting a better check would be if either half is all-zero)

[newpavlov]

I suggest to do the following:

1. Remove the weak key testing from the traits API. AFAIK in our implemented ciphers only DES has such cryptographic vulnerability, so I think the situation is similar to sha1-checked.
2. Add a free-standing function for weak key testing to des.
3. Introduce a new utils crate with better statistical tests than the criteria with unclear rationale from the TPM spec and link to it from the Key(Iv)Init trait docs.

I had to deal with a similar problem during certification process and we used chi-squared tests and checked that number of bits set to 1 falls into a certain range. We also could add a criteria that none of 64 bit chunks is fully equal to zeros or ones.

[tarcieri]

That seems like a reasonable approach to me

[AaronFeickert]

I'll update this PR (with appropriate change to title and description) for the second point.

Is it confirmed that we want to remove the trait support altogether?

[tarcieri]

That’s fine by me

[AaronFeickert]

Got it. This would be breaking for KeyInit: specifically, it would remove weak_key_test and also new_checked.