Regression: Unable to set EC_KEY private_key to NULL starting in 1.1.1h

[dpirotte]

OpenSSL 1.1.1h introduces a behavior change wherein one can no longer set an EC_KEY's private_key to NULL.

This behavior changes in 6a01f6f. Based on the original PR (#11127), it appears that this is hardening backported from 3.0.0 that unintentionally introduced a regression.

From 6a01f6f#diff-38a47bf175cde97de2df18870a9ab300366f191a848bdea84def068fe0fde69fR484-R486:

+    tmp_key = BN_dup(priv_key);
+    if (tmp_key == NULL)
+        return 0;

EC_KEY_set_private_key returns early and never sets key->priv_key to NULL.

[romen]

So, before the patch in #11127, EC_KEY_set_private_key(some_eck, NULL); returned an error but also modified the inner field, which at the time looked like a bug.

Documentation said and says nothing about being allowed to pass NULL and what is the expected behavior.

So, questions for OTC:

• what do we think is the expected behavior for 1.1.1? which one is the bug, original behavior or the one after hardening of EC/BN hardening backports from #10631 #11127?
• depending on the answer above, what do we want for 3.0.0?
• finally what about master?

[romen]

Formally my hold is to let OTC evaluate if the original report describes a regression or not.
I am not objecting to fixing it if we deem that it is a regression, but this statement needs more consideration:

OpenSSL 1.1.1h introduces a behavior change wherein one can no longer set an EC_KEY's private_key to NULL.

As far as the public API is defined, passing NULL returns an error code, so one can say that the API before 1.1.1h did not allow the semantic of passing NULL to unset the private key.
In this perspective, letting EC_KEY_set_private_key(some_eck, NULL) unset the private key is a new feature.

On the other hand, if the caller did not check the return value and relied on undocumented inner behavior, one can say this is a regression.

[t-j-h]

All those functions explicitly handle being passed NULL as an argument and set the corresponding value to NULL - that is their intent. It didn't happen by accident.

BN_dup(NULL) returns NULL and that is not an error - as that is what it should do.

Changing long term explicit behaviour like this is one place where things get broken unnecessarily and we should not be backfitting this short of change into a stable release - that was a mistake to backfit it IMHO entirely independent of what you think about what the behaviour should be.

EC_KEY_set_group, EC_KEY_set_private_key, EC_KEY_set_public_key all operate the same way and changing one API out of a set of APIs to behave inconsistently was clearly the wrong choice to be making (for backfit or in any other context).

Someone wanting to make a behaviour change like that should be explicitly flagging that as a separate PR and discussing the context and the scope - not including it in with other changes.

[romen]

I don't disagree with anything you said @t-j-h !

But the previous code was already considering the BN_dup() returning NULL as an error, and returning an error to the caller:

openssl/crypto/ec/ec_key.c

Lines 428 to 429 in b5acbf9

	key->priv_key = BN_dup(priv_key);
	return (key->priv_key == NULL) ? 0 : 1;

[mattcaswell]

So, questions for OTC:

OTC: We should revert to the old behaviour on all branches, where passing NULL sets the internal key to NULL and we get a failure result code back

[romen]

Thanks @dpirotte for reporting this and @robertohueso for the fix.

The fix has been merged:

• Fix EC_KEY_set_private_key() NULL private key behavior in master #18942 to master as:
  • b304b3e Fix EC_KEY_set_private_key() priv_key regression
  • d93f154 Add test for EC_KEY_set_private_key()
  • fae06b5 Fix EC_KEY_set_private_key() NULL priv_key docs
• Fix EC_KEY_set_private_key() NULL private key behavior in 3.0 #18941 to openssl-3.0 as:
  • f76cf4d Fix EC_KEY_set_private_key() priv_key regression
  • 66627cd Add test for EC_KEY_set_private_key()
  • c682dfd Fix EC_KEY_set_private_key() NULL priv_key docs
• Fix EC_KEY_set_private_key() NULL private key behavior in 1.1.1 #18874 to OpenSSL_1_1_1-stable as:
  • 143d7d4 Fix EC_KEY_set_private_key() priv_key regression
  • 1c2f52b Add test for EC_KEY_set_private_key()
  • 4648839 Fix EC_KEY_set_private_key() NULL priv_key docs