Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

@romen
Copy link
Member

@romen romen commented Jun 29, 2020

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 #12313 (master counterpart of this PR)
romen added 2 commits Jun 28, 2020
…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 #12306
…eters and keys

make update
@romen
Copy link
Member Author

@romen romen commented Jun 29, 2020

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.

@@ -332,7 +332,7 @@ int PEM_ASN1_write_bio(i2d_of_void *i2d, const char *name, BIO *bp,
}
}

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

This comment has been minimized.

@levitte

levitte Jun 29, 2020
Member

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

This comment has been minimized.

@romen

romen Jun 29, 2020
Author Member

@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).

This comment has been minimized.

@romen

romen Jun 29, 2020
Author Member

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

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:

This comment has been minimized.

@romen

romen Jun 29, 2020
Author Member

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.

This comment has been minimized.

@romen

romen Jul 2, 2020
Author Member

ping @levitte

@t8m
t8m approved these changes Jul 2, 2020
@openssl-machine
Copy link

@openssl-machine openssl-machine commented Jul 3, 2020

This pull request is ready to merge

romen added a commit to romen/openssl that referenced this pull request Jul 4, 2020
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 <tmraz@fedoraproject.org>
(Merged from openssl#12312)
@romen
Copy link
Member Author

@romen romen commented Jul 7, 2020

Merged to 1.1.1 as:

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

Thanks!

@romen romen closed this Jul 7, 2020
romen added a commit to romen/openssl that referenced this pull request Jul 16, 2020
Some curves don't have an associated OID: for those we should not
default to `OPENSSL_EC_NAMED_CURVE` encoding of parameters and instead
set the ASN1 flag to `OPENSSL_EC_EXPLICIT_CURVE`.

This is a follow-up to openssl#12312
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants