fix: extension critical definition to default false

[JonathanWilbur]

If the critical field is missing from an X.509v3 extension (which it should be if critical is FALSE), the value of critical in the X509_EXTENSION struct will be -1, which evaluates to truthy! I know there is a function for getting the criticality of this field properly, but I made the mistake of not using it. This PR could fix this problem for other developers.

This PR simply makes a missing critical component set the critical field in the X509_EXTENSION to 0 instead of -1.

For reference, the definition of Extension from ITU Recommendation X.509 (2019) is:

Extension ::= SEQUENCE {
  extnId     EXTENSION.&id({ExtensionSet}),
  critical   BOOLEAN DEFAULT FALSE,
  extnValue  OCTET STRING
    (CONTAINING EXTENSION.&ExtnType({ExtensionSet}{@extnId})
       ENCODED BY der),
  ... }

[davidben]

If the aim is to change the in-memory representation of the critical field, should X509_EXTENSION_set_critical also be updated? Otherwise code still must handle -1 correctly.

Although, to handle -1 correctly, you can just use the X509_EXTENSION_get_critical function, which handles this. (The struct is opaque, so consumers are already required to use the accessor.)

[tmshort]

@davidben, i think the issue is that the field has a default, and it's not being set to that default.

[davidben]

@davidben, i think the issue is that the field has a default, and it's not being set to that default.

I don't think that's quite right. OpenSSL's ASN1_BOOLEAN representation is that 0 means FALSE, -1 means omitted OPTIONAL BOOLEAN, and 0xff (or other positive values in case of BER) means TRUE. ASN1_FBOOLEAN says that 0 means FALSE (which is omitted at encoding time), and 0xff (or other positive values) mans TRUE. You can see this behavior here:

openssl/crypto/asn1/tasn_enc.c

Line 573 in 7f4cc3b

case V_ASN1_BOOLEAN:

openssl/crypto/asn1/tasn_new.c

Line 293 in 7f4cc3b

*(ASN1_BOOLEAN *)pval = it->size;

(When tasn_dec.c sees an omitted value, it uses the tasn_new.c default. The ASN1_BOOLEAN variants smuggle their defaults into it->size.)

If you're trying to represent a DEFAULT FALSE boolean, while ASN1_FBOOLEAN is more natural, an optional ASN1_BOOLEAN works too: at read time, you map -1 to FALSE, and that, at write time, you set -1 instead 0 for FALSE. That is exactly what the getters and setters do right now. (Recall that the struct is opaque, so those are the only ways to get at those fields.)

openssl/crypto/x509/x509_v3.c

Line 234 in 7f4cc3b

int X509_EXTENSION_get_critical(const X509_EXTENSION *ex)

openssl/crypto/x509/x509_v3.c

Line 200 in 7f4cc3b

int X509_EXTENSION_set_critical(X509_EXTENSION *ex, int crit)

Switching to ASN1_FBOOLEAN is fine and probably more natural, but then you should fix X509_EXTENSION_set_critical accordingly. Also keep in mind that this will give a caller-visible behavior change: because OpenSSL does not implement a DER parser (contrary to the standards), it accepts explicitly-encoded FALSE values. Under the current optional ASN1_BOOLEAN, explicit vs default FALSE are distinguished in memory (0 vs -1). Then you go to re-encode the object, this distinguish in preserved.

After this PR, the two cases are not distinguished in memory. They're both represented as zero. So if you parse an extension with explicit FALSE and then re-encode it, it will be fixed to the correct omitted FALSE. Not unreasonable behavior, but it is a behavior change. Something to keep in mind. (Also keep in mind, when testing this, that X509 objects have a cached encoding.)

[JonathanWilbur]

It sounded like you were on to something @davidben , and I may still need to change those getters and setters, but as far as the encoding goes, it does not look like it ever encoded it incorrectly. Here is a decoding that I produced from a self-signed certificate:

    0:d=0  hl=4 l=1342 cons: SEQUENCE          
    4:d=1  hl=4 l= 806 cons: SEQUENCE          
    8:d=2  hl=2 l=   3 cons: cont [ 0 ]        
   10:d=3  hl=2 l=   1 prim: INTEGER           :02
   13:d=2  hl=2 l=  20 prim: INTEGER           :144C332625A907D85951B684889F508F261F5846
   35:d=2  hl=2 l=  13 cons: SEQUENCE          
   37:d=3  hl=2 l=   9 prim: OBJECT            :sha256WithRSAEncryption
   48:d=3  hl=2 l=   0 prim: NULL              
   50:d=2  hl=2 l=  22 cons: SEQUENCE          
   52:d=3  hl=2 l=  20 cons: SET               
   54:d=4  hl=2 l=  18 cons: SEQUENCE          
   56:d=5  hl=2 l=   3 prim: OBJECT            :commonName
   61:d=5  hl=2 l=  11 prim: UTF8STRING        :example.com
   74:d=2  hl=2 l=  30 cons: SEQUENCE          
   76:d=3  hl=2 l=  13 prim: UTCTIME           :230621020108Z
   91:d=3  hl=2 l=  13 prim: UTCTIME           :330618020108Z
  106:d=2  hl=2 l=  22 cons: SEQUENCE          
  108:d=3  hl=2 l=  20 cons: SET               
  110:d=4  hl=2 l=  18 cons: SEQUENCE          
  112:d=5  hl=2 l=   3 prim: OBJECT            :commonName
  117:d=5  hl=2 l=  11 prim: UTF8STRING        :example.com
  130:d=2  hl=4 l= 546 cons: SEQUENCE          
  134:d=3  hl=2 l=  13 cons: SEQUENCE          
  136:d=4  hl=2 l=   9 prim: OBJECT            :rsaEncryption
  147:d=4  hl=2 l=   0 prim: NULL              
  149:d=3  hl=4 l= 527 prim: BIT STRING        
  680:d=2  hl=3 l= 131 cons: cont [ 3 ]        
  683:d=3  hl=3 l= 128 cons: SEQUENCE          
  686:d=4  hl=2 l=  29 cons: SEQUENCE          
  688:d=5  hl=2 l=   3 prim: OBJECT            :X509v3 Subject Key Identifier
  693:d=5  hl=2 l=  22 prim: OCTET STRING      [HEX DUMP]:04141CBA87B39497406BC8B2903B27577A7291BADF63
  717:d=4  hl=2 l=  31 cons: SEQUENCE          
  719:d=5  hl=2 l=   3 prim: OBJECT            :X509v3 Authority Key Identifier
  724:d=5  hl=2 l=  24 prim: OCTET STRING      [HEX DUMP]:301680141CBA87B39497406BC8B2903B27577A7291BADF63
  750:d=4  hl=2 l=  15 cons: SEQUENCE          
  752:d=5  hl=2 l=   3 prim: OBJECT            :X509v3 Basic Constraints
  757:d=5  hl=2 l=   1 prim: BOOLEAN           :255
  760:d=5  hl=2 l=   5 prim: OCTET STRING      [HEX DUMP]:30030101FF
  767:d=4  hl=2 l=  45 cons: SEQUENCE          
  769:d=5  hl=2 l=   3 prim: OBJECT            :X509v3 Subject Alternative Name
  774:d=5  hl=2 l=  38 prim: OCTET STRING      [HEX DUMP]:3024820B6578616D706C652E636F6D820F7777772E6578616D706C652E6E657487040A000001
  814:d=1  hl=2 l=  13 cons: SEQUENCE          
  816:d=2  hl=2 l=   9 prim: OBJECT            :sha256WithRSAEncryption
  827:d=2  hl=2 l=   0 prim: NULL              
  829:d=1  hl=4 l= 513 prim: BIT STRING        

You can see that only the basicConstraints extension has a critical field encoded.

For thoroughness here were my steps for reproduction, in case you can find something wrong I did here:

1. Change over to the fix_extension_critical branch: git checkout fix_extension_critical
2. Run make.
3. Create a self-signed certificate: openssl req -x509 -newkey rsa:4096 -sha256 -days 3650 -nodes \ -keyout example.com.key -out example.com.crt -subj "/CN=example.com" \ -addext "subjectAltName=DNS:example.com,DNS:www.example.net,IP:10.0.0.1"
4. Inspect the certificate using the X.509 display: ./apps/openssl x509 -in ./example.com.crt -noout -text
5. Inspect the certificate using the ASN.1 display: ./apps/openssl asn1parse -in ./example.com.crt

[JonathanWilbur]

The ASN.1 code is a real nightmare to navigate, honestly, so I could not confirm this, but I would expect ASN1_FBOOLEAN to work properly according to DER encoding, since it is used in other places throughout OpenSSL.

[JonathanWilbur]

The getter definitely does not need to change to accommodate this:

openssl/crypto/x509/x509_v3.c

Lines 234 to 241 in 7f4cc3b

	int X509_EXTENSION_get_critical(const X509_EXTENSION *ex)
	{
	if (ex == NULL)
	return 0;
	if (ex->critical > 0)
	return 1;
	return 0;
	}

The setter might or might not. I just changed the -1 to 0 and got the same result, so it doesn't seem like the setter mattered either:

openssl/crypto/x509/x509_v3.c

Lines 200 to 206 in 7f4cc3b

	int X509_EXTENSION_set_critical(X509_EXTENSION *ex, int crit)
	{
	if (ex == NULL)
	return 0;
	ex->critical = (crit) ? 0xFF : -1;
	return 1;
	}

(Out of fairness, maybe the code I used with that command bypasses the setter. I am not sure.)

Dropping my approve until @davidben's comments are addressed.

Dismissing review based on @davidben's comments

[davidben]

Agreed that you can leave the getter alone, just the setter. Though ideally -1 will be impossible once you've fixed the setter, so it's moot whether you do anything with the getter.

Regarding encoding, I believe this change will indeed encode things correctly. That's not what I was saying. I was saying the PR change the behavior of this code:

#include <openssl/x509.h>
#include <stdio.h>

// SEQUENCE {
//   OBJECT_IDENTIFIER { 1.2.3.4 }
//   BOOLEAN { FALSE }
//   OCTET_STRING {}
// }
static const unsigned char kExt[] = {0x30, 0x0a, 0x06, 0x03, 0x2a, 0x03,
                                     0x04, 0x01, 0x01, 0x00, 0x04, 0x00};

#define CHECK(x)                          \
  do {                                    \
    if (!(x)) {                           \
      fprintf(stderr, "%s failed\n", #x); \
      abort();                            \
    }                                     \
  } while (0)

int main() {
  CHECK(OPENSSL_init_crypto(0, NULL));
  const unsigned char *inp = kExt;
  X509_EXTENSION *ext = d2i_X509_EXTENSION(NULL, &inp, sizeof(kExt));
  CHECK(ext);
  unsigned char *enc = NULL;
  int len = i2d_X509_EXTENSION(ext, &enc);
  CHECK(len >= 0);
  for (int i = 0; i < len; i++) {
    printf("%02x", enc[i]);
  }
  printf("\n");
  return 0;
}

Before, the output was:

$ ./a.out  | der2ascii -hex
SEQUENCE {
  OBJECT_IDENTIFIER { 1.2.3.4 }
  BOOLEAN { FALSE }
  OCTET_STRING {}
}

After, the output is:

$ ./a.out  | der2ascii -hex
SEQUENCE {
  OBJECT_IDENTIFIER { 1.2.3.4 }
  OCTET_STRING {}
}

To be clear, I don't personally think the behavior change is wrong, per se. OpenSSL already doesn't round-trip encodings in general (though it does cache TBSCertificate encodings), and the new encoding is correct for DER. (Though really the parser should just reject kExt, but OpenSSL isn't standards-compliant here.) I only point this out so the project can be aware of the behavior changes caused by the PRs it accepts. :-)

[openssl-machine]

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

[openssl-machine]

This PR is in a state where it requires action by @openssl/otc but the last update was 61 days ago

[JonathanWilbur]

Is there something else I need to do to get this PR accepted?

[davidben]

I don't think this PR is ready to merge. The setter still hasn't been updated as discussed above. While it works to leave it alone, it rather defeats the point of this CL because it means -1 is still a possible state for ex->critical and so all the code accessing that field must continue to account for it.

[t8m]

Yeah, along with it, the existing places which read the value directly could be updated to avoid the > 0 comparison.

[JonathanWilbur]

@tmshort @paulidale @davidben Have your concerns been addressed?

[JonathanWilbur]

@tmshort I believe this is awaiting your review.

[openssl-machine]

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

[openssl-machine]

This PR is in a state where it requires action by @openssl/committers but the last update was 61 days ago

[openssl-machine]

This PR is in a state where it requires action by @openssl/committers but the last update was 92 days ago

[openssl-machine]

This PR is in a state where it requires action by @openssl/committers but the last update was 123 days ago

[t8m]

ping @openssl/committers for second review

[tom-cosgrove-arm]

Is X509_EXTENSION a private structure? If not, it seems that this is a change to the API semantics, in which case is it appropriate to backport to earlier branches?

[t8m]

It is a private structure. It is declared in x509_local.h.

[t8m]

On the other hand this can be seen as refactoring as it should have no public API impact. So I would be OK applying this to the master branch only.

[tom-cosgrove-arm]

LGTM, as only going to master now

[openssl-machine]

This pull request is ready to merge

[t8m]

Squashed and merged to the master branch, finally! Thank you for your contribution.