[1.1.1][EC][ASN1] Detect missing OID when serializing EC parameters and keys by romen · Pull Request #12312 · openssl/openssl

romen · 2020-06-29T02:02:13Z

This PR is for 1.1.1: it is identical to #12313 but for the commit with the make update results.

The following built-in curves do not have an assigned OID:

Oakley-EC2N-3

Oakley-EC2N-4

In general we shouldn't assume that an OID is always available.

This commit detects such cases, raises an error and returns appropriate
return values so that the condition can be detected and correctly
handled by the callers, when serializing EC parameters or EC keys with
the default ec_param_enc:named_curve.

Fixes #12306

TODO

approval for [EC][ASN1] Detect missing OID when serializing EC parameters and keys #12313 (master counterpart of this PR)

…nd keys The following built-in curves do not have an assigned OID: - Oakley-EC2N-3 - Oakley-EC2N-4 In general we shouldn't assume that an OID is always available. This commit detects such cases, raises an error and returns appropriate return values so that the condition can be detected and correctly handled by the callers, when serializing EC parameters or EC keys with the default `ec_param_enc:named_curve`. Fixes openssl#12306

…eters and keys make update

romen · 2020-06-29T02:37:22Z

Travis failures are expected: #12305, #12312 and #12307 depend on each other.

Check #12307 (based on top of the other 2) for (hopefully) green Travis.

levitte · 2020-06-29T03:48:58Z

crypto/pem/pem_lib.c

    }

-    if ((dsize = i2d(x, NULL)) < 0) {
+    if ((dsize = i2d(x, NULL)) <= 0) {


Have you checked through git history to find if there was reason not to err on zero?

@levitte do you have an answer in mind?

As far as I can see this has been checking only for < 0 for 22 years, since 58964a492275ca9a59a0cd9c8155cb2491b4b909 , and before that the return was unchecked.

Functions like i2d_ECPKParameters() return 0 on error conditions.

openssl/crypto/ec/ec_asn1.c

Lines 953 to 968 in c437fc2

int i2d_ECPKParameters(const EC_GROUP *a, unsigned char **out)

{

int ret = 0;

ECPKPARAMETERS *tmp = EC_GROUP_get_ecpkparameters(a, NULL);

if (tmp == NULL) {

ECerr(EC_F_I2D_ECPKPARAMETERS, EC_R_GROUP2PKPARAMETERS_FAILURE);

return 0;

}

if ((ret = i2d_ECPKPARAMETERS(tmp, out)) == 0) {

ECerr(EC_F_I2D_ECPKPARAMETERS, EC_R_I2D_ECPKPARAMETERS_FAILURE);

ECPKPARAMETERS_free(tmp);

return 0;

}

ECPKPARAMETERS_free(tmp);

return ret;

}

The documentation is a little ambiguous, as the relevant manpage says:

i2d_TYPE() encodes the structure pointed to by a into DER format. If ppout is not NULL, it writes the DER encoded data to the buffer at *ppout, and increments it to point after the data just written. If the return value is negative an error occurred, otherwise it returns the length of the encoded data.

i2d_TYPE() returns the number of bytes successfully encoded or a negative value if an error occurs.

It's another instance of "fix the behavior" vs "fix the documentation" (one might consider "no bytes have been successfully written" as an error condition as well).

The difference between changing this line and not changing this line is how many times i2d() is called from PEM_ASN1_write_bio():

openssl/crypto/pem/pem_lib.c

Lines 335 to 348 in c437fc2

if ((dsize = i2d(x, NULL)) < 0) {

PEMerr(PEM_F_PEM_ASN1_WRITE_BIO, ERR_R_ASN1_LIB);

dsize = 0;

goto err;

}

/* dsize + 8 bytes are needed */

/* actually it needs the cipher block size extra... */

data = OPENSSL_malloc((unsigned int)dsize + 20);

if (data == NULL) {

PEMerr(PEM_F_PEM_ASN1_WRITE_BIO, ERR_R_MALLOC_FAILURE);

goto err;

}

p = data;

i = i2d(x, &p);

L335 determines the required buffer size, on a returned 0 we then allocate something that has length 0 +20 to have room for encryption needs

L348 actually tries to encode, which also returns 0

the lines afterwards try then to either encrypt 0 bytes of data or to write 0 bytes into a BIO

To me it seems clearly a bug to consider "0 bytes are required to encode this thing" as a non-error on L335.

We could fix i2d_ECPKParameters() to return negative value on those errors, instead of 0, but that is a public function and it has been behaving that way for some time.

We could keep this as it is, and as far as I can tell the consequence would be limited to raising the missing OID error twice, on each i2d() call.

; # if we patch this line to treat 0 as an error, i2d() is called only once to determine the required buffer size for allocation ; util/shlib_wrap.sh apps/openssl genpkey -genparam -algorithm EC -pkeyopt 'ec_paramgen_curve:Oakley-EC2N-4' Error writing key 140388092275648:error:101060A7:elliptic curve routines:EC_GROUP_get_ecpkparameters:missing OID:crypto/ec/ec_asn1.c:554: 140388092275648:error:100BF078:elliptic curve routines:i2d_ECPKParameters:group2pkparameters failure:crypto/ec/ec_asn1.c:965: 140388092275648:error:0906900D:PEM routines:PEM_ASN1_write_bio:ASN1 lib:crypto/pem/pem_lib.c:336: ; # without patching L335, the errors from the EC stack are raised twice, and the encoding is halted when PEM_write_bio fails trying to encode 0 bytes ; util/shlib_wrap.sh apps/openssl genpkey -genparam -algorithm EC -pkeyopt 'ec_paramgen_curve:Oakley-EC2N-4' -----BEGIN EC PARAMETERS----- -----END EC PARAMETERS----- Error writing key 139739847752640:error:101060A7:elliptic curve routines:EC_GROUP_get_ecpkparameters:missing OID:crypto/ec/ec_asn1.c:554: 139739847752640:error:100BF078:elliptic curve routines:i2d_ECPKParameters:group2pkparameters failure:crypto/ec/ec_asn1.c:965: 139739847752640:error:101060A7:elliptic curve routines:EC_GROUP_get_ecpkparameters:missing OID:crypto/ec/ec_asn1.c:554: 139739847752640:error:100BF078:elliptic curve routines:i2d_ECPKParameters:group2pkparameters failure:crypto/ec/ec_asn1.c:965: 139739847752640:error:09072007:PEM routines:PEM_write_bio:BUF lib:crypto/pem/pem_lib.c:658:

My analysis is that, even for non EC stuff, it should be safe to error out already on L335 if i2d() returns 0: anyway a failure will be triggered further down the line anyway, when trying to encrypt or encode a 0 bytes buffer.

ping @levitte

openssl-machine · 2020-07-03T18:00:14Z

This pull request is ready to merge

The following built-in curves do not have an assigned OID: - Oakley-EC2N-3 - Oakley-EC2N-4 In general we shouldn't assume that an OID is always available. This commit detects such cases, raises an error and returns appropriate return values so that the condition can be detected and correctly handled by the callers, when serializing EC parameters or EC keys with the default `ec_param_enc:named_curve`. Fixes openssl#12306 Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#12312)

romen · 2020-07-07T09:02:00Z

Merged to 1.1.1 as:

2797fea [EC][ASN1] Detect missing OID when serializing EC parameters and keys

Thanks!

romen added 2 commits June 29, 2020 00:53

fixup! [1.1.1][EC][ASN1] Detect missing OID when serializing EC param…

342e858

…eters and keys make update

romen added branch: 1.1.1 Applies to OpenSSL_1_1_1-stable branch (EOL) approval: otc review pending triaged: bug The issue/pr is/fixes a bug labels Jun 29, 2020

romen self-assigned this Jun 29, 2020

This was referenced Jun 29, 2020

[EC][ASN1] Detect missing OID when serializing EC parameters and keys #12313

Closed

[1.1.1][test][15-test_genec] Improve EC tests with genpkey #12308

Closed

romen mentioned this pull request Jun 29, 2020

[apps/genpkey] exit status should not be 0 on output errors #12305

Closed

levitte reviewed Jun 29, 2020

View reviewed changes

t8m approved these changes Jul 2, 2020

View reviewed changes

t8m added approval: done This pull request has the required number of approvals and removed approval: otc review pending labels Jul 2, 2020

openssl-machine removed the approval: done This pull request has the required number of approvals label Jul 3, 2020

openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Jul 3, 2020

romen closed this Jul 7, 2020

romen mentioned this pull request Jul 7, 2020

genpkey -genparam fails for some EC curves #12306

Closed

romen mentioned this pull request Jul 16, 2020

[1.1.1] Fix d2i_ECPKParameters_fp and i2d_ECPKParameters_fp macroes #12457

Closed

1 task

123e443 pushed a commit to 123e443/bulid-repp that referenced this pull request Apr 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

[1.1.1][EC][ASN1] Detect missing OID when serializing EC parameters and keys#12312

[1.1.1][EC][ASN1] Detect missing OID when serializing EC parameters and keys#12312
romen wants to merge 2 commits intoopenssl:OpenSSL_1_1_1-stablefrom
romen:issues/1.1.1/ec_asn1_handle_no_OID

romen commented Jun 29, 2020 •

edited

Loading

Uh oh!

romen commented Jun 29, 2020

Uh oh!

levitte Jun 29, 2020

Uh oh!

romen Jun 29, 2020 •

edited

Loading

Uh oh!

romen Jun 29, 2020

Uh oh!

romen Jun 29, 2020

Uh oh!

romen Jul 2, 2020

Uh oh!

openssl-machine commented Jul 3, 2020

Uh oh!

romen commented Jul 7, 2020

Uh oh!

	int i2d_ECPKParameters(const EC_GROUP a, unsigned char *out)
	{
	int ret = 0;
	ECPKPARAMETERS *tmp = EC_GROUP_get_ecpkparameters(a, NULL);
	if (tmp == NULL) {
	ECerr(EC_F_I2D_ECPKPARAMETERS, EC_R_GROUP2PKPARAMETERS_FAILURE);
	return 0;
	}
	if ((ret = i2d_ECPKPARAMETERS(tmp, out)) == 0) {
	ECerr(EC_F_I2D_ECPKPARAMETERS, EC_R_I2D_ECPKPARAMETERS_FAILURE);
	ECPKPARAMETERS_free(tmp);
	return 0;
	}
	ECPKPARAMETERS_free(tmp);
	return ret;
	}

	if ((dsize = i2d(x, NULL)) < 0) {
	PEMerr(PEM_F_PEM_ASN1_WRITE_BIO, ERR_R_ASN1_LIB);
	dsize = 0;
	goto err;
	}
	/* dsize + 8 bytes are needed */
	/* actually it needs the cipher block size extra... */
	data = OPENSSL_malloc((unsigned int)dsize + 20);
	if (data == NULL) {
	PEMerr(PEM_F_PEM_ASN1_WRITE_BIO, ERR_R_MALLOC_FAILURE);
	goto err;
	}
	p = data;
	i = i2d(x, &p);

Uh oh!

Comments

Conversation

romen commented Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

romen commented Jun 29, 2020

Uh oh!

levitte Jun 29, 2020

Choose a reason for hiding this comment

Uh oh!

romen Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romen Jun 29, 2020

Choose a reason for hiding this comment

Uh oh!

romen Jun 29, 2020

Choose a reason for hiding this comment

Uh oh!

romen Jul 2, 2020

Choose a reason for hiding this comment

Uh oh!

openssl-machine commented Jul 3, 2020

Uh oh!

romen commented Jul 7, 2020

Uh oh!

romen commented Jun 29, 2020 •

edited

Loading

romen Jun 29, 2020 •

edited

Loading