DES_set_key(): return values as DES_set_key_checked() but always set

[t8m]

This avoids using accidentally uninitialized key schedule in applications that use DES_set_key() not expecting it to check the key which is the default on OpenSSL <= 1.1.1

Fixes #16859

[solardiz]

A couple of issues here:

1. The checks inside DES_check_key_parity and DES_is_weak_key are timing-unsafe, and this adds even further timing-unsafe behavior in DES_set_key (that's OK'ish if we accept calling those functions from here at all while keeping their implementations as-is).
2. Applications may really not expect a non-zero return from DES_set_key. Just because an application checks for that doesn't mean it handles this right (would reach never-tested code).

So my preference remains to revert the previous change instead. However, failing that, this PR looks good to me in that its code matches its intent.

[t8m]

You have to understand that reverting the change is not an option. We do not want to reintroduce a global variable in OpenSSL-3.x that was removed. And we also cannot make the function to mean DES_set_key_unchecked() unconditionally because we do not want to break the compatibility with the openssl-3.0.0 release.

This is the only compatible option that mitigates the problem with applications ignoring the return value. I do not think timing safety is really an issue here with this kind of legacy API call.

[solardiz]

OK, if you say so, but I think 3.0.0's behavior is a CVE-worthy vulnerability or two (making most DES-using applications proceed with an unset key and exposing the really bad timing leaks inside DES_check_key_parity and DES_is_weak_key) and should ideally be fixed as such. Maintaining compatibility with a regression seen in one release feels wrong to me, even if the change involving the regression was documented.

So I recommend to revert the change and explicitly document DES_set_key_checked, DES_check_key_parity, and DES_is_weak_key as timing-unsafe. (As it's probably not worth the effort to try and make these legacy interfaces timing-safe. BTW, that would probably also make them slower.)

The whole of DES (as implemented) is cache-timing-unsafe, though, but that's not exactly as bad as branching on key bits or bytes, and is not unique to DES. Maybe this should be mentioned, too, but I think is more of an expected behavior (for those who know how DES is typically implemented).

[solardiz]

That said, I think it's good to merge this PR first, and discuss and maybe revert the original change (and this PR along with it) later. This PR mostly corrects the issue, and is thus a major improvement to what's currently committed.

[paulidale]

I'm not convinced that the timing attack possibilities are all that bad here.

If you use a weak key, the timing changes and an attacker could find this out and potentially which weak key you are using. The weak part is damming here not the timing attack.

The parity check tells an attacker that the key has even parity and possibly the first byte that does. In what way does this give an attacker an advantage?

[t8m]

OTC: agreed to merge this. Timing of the weak key check can be resolved in a separate PR.

[solardiz]

I'm not convinced that the timing attack possibilities are all that bad here.

Those added with this PR are not that bad compared to what we already had. However, those inside (both) called functions (thus, introduced to common usage in 3.0.0) are per-byte (one of these depends on memcmp implementation, though), so if exploited they can reduce the key space to search substantially.

The weak part is damming here not the timing attack.

That's not so obvious. With just a handful of weak keys in existence, an attacker could have tested them all against ciphertext quickly (as long as there's a way to detect correct decryption) even if the keys were not in any way special. So them also being weak generally does not make a difference.

[paulidale]

Merged to master and 3.0.