revocation

(moving to -devel as this is obviously pertains there more than -users)

Ok, so I'm looking into the sources of openssl. ASN1_INTEGER is a typedef for "struct asn1_string_st"; and the comments in the code for that structure are quite explicit:

-------------------------------------------------
/* This is the base type that holds just about everything :-) */ typedef struct asn1_string_st

    {
    int length;
    int type;
    unsigned char *data;
    /* The value of the following field depends on the type being
     * held.  It is mostly being used for BIT_STRING so if the
     * input data has a non-zero 'unused bits' value, it will be
     * handled correctly */
    long flags;
    } ASN1_STRING;

-------------------------------------------------

So an ASN1_INTEGER in OpenSSL can really contain arbitrarily long values (many other types are typedef'd to "struct asn1_string_st").

More to the point, when one runs "openssl x509 -text", it turns out that it explicitly checks for the serial number's length. If it's up to 4 bytes, it's printed like

        Serial Number: 927650371 (0x374ad243)

if instead it's longer, it's printed in the form

        Serial Number:
            05:40:69:bf:0f:c2:4f:f4:b0:05:ee:95:3f:18:71:17

(crypto/asn1/t_x509.c, line 138 and following), which confirms my initial guess. So in fact it's just a different output format, and the serial number is just an (almost) arbitrarily large number. Why would a CA choose such a serial number? I don't know, it's their business but we should be able to handle it.

Sorry for the - somewhat - OT introduction, but I felt it was necessary to clarify the puzzling output I was seeing earlier. Now let's turn to OpenVPN.

ssl.c, line 789:

-------------------------------------------------
/* export serial number as environmental variable */
const int serial = (int) ASN1_INTEGER_get (X509_get_serialNumber (ctx-

-------------------------------------------------

Here is ASN1_INTEGER_get (from openssl's crypto/asn1/a_int.c, line 376):

-------------------------------------------------
long ASN1_INTEGER_get(ASN1_INTEGER *a)

    {
    int neg=0,i;
    long r=0;

    if (a == NULL) return(0L);
    i=a->type;
    if (i == V_ASN1_NEG_INTEGER)
        neg=1;
    else if (i != V_ASN1_INTEGER)
        return -1;

    if (a->length > (int)sizeof(long))
        {
        /* hmm... a bit ugly */
        return(0xffffffffL);
        }
    if (a->data == NULL)
        return 0;

    for (i=0; i<a->length; i++)
        {
        r<<=8;
        r|=(unsigned char)a->data[i];
        }
    if (neg) r= -r;
    return(r);
    }

-------------------------------------------------

and that explains why we get -1 with big serial numbers on 32 bit.

Now, it seems that the "right" fix for OpenVPN would be to use a memory BIO object as explained in the docs. That way, the serial number is "printed" to the BIO object and can be read back into an array of characters. In that form, it could be exported into the environment using setenv_str() instead of setenv_int() (environment variables are all strings anyway). Also, due to the way i2a_ASN1_INTEGER() prints stuff, it will be a long hexadecimal string, which integrates nicely with OCSP, so it is ready to use when doing OCSP queries (by doing '-serial "0x${tls_serial_0}"' etc.)

So here's a patch that does exactly that; it's quite straightforward in fact. I have tested it on two Gentoo boxes and it seems to work fine, although of course more testing never hurts; here's a dump of the serial numbers in a full certificate chain:

054069bf0fc24ff4b005ee953f187117
0851F959814145CABDE124E232C9D20E
428740A5
374AD243

I produced that with a simple tls-verify script that dumps $tls_serial_{0,1,2,3}.

Well, enough blathering (sorry!); here's the patch. It's also attached separately in the likely case that line wrapping mess up the inline version.

Description: correctly manage serial numbers of any size (as long as OpenSSL can manage them) to be exported as environment variables for children scripts.

@@ -788,9 +788,25 @@ verify_callback (int preverify_ok, X509_
 
   /* export serial number as environmental variable */
   {
-    const int serial = (int) ASN1_INTEGER_get (X509_get_serialNumber (ctx-

+    BIO *bio = NULL;
+    char serial[1024] = {0};
+    int n;
+
+    if ((bio = BIO_new (BIO_s_mem ())) == NULL) {
+      msg (M_FATAL, "CALLBACK: Cannot create BIO");
+    }
+    /* "prints" the serial number onto the BIO */
+    i2a_ASN1_INTEGER(bio, X509_get_serialNumber (ctx->current_cert));
+    n = BIO_read (bio, serial, sizeof (serial)-1);
+    if (n < 0) {

+ serial[0] = '\x0';
+ }
+ else {
+ serial[n] = 0;
+ }
+
openvpn_snprintf (envname, sizeof(envname), "tls_serial_%d", ctx-

- setenv_int (opt->es, envname, serial);
+ setenv_str (opt->es, envname, serial);
}

/* export current untrusted IP */

D.

@@ -788,9 +788,25 @@ verify_callback (int preverify_ok, X509_
 
   /* export serial number as environmental variable */
   {
-    const int serial = (int) ASN1_INTEGER_get (X509_get_serialNumber (ctx->current_cert));
+    BIO *bio = NULL;
+    char serial[1024] = {0};
+    int n;
+
+    if ((bio = BIO_new (BIO_s_mem ())) == NULL) {
+      msg (M_FATAL, "CALLBACK: Cannot create BIO");
+    }
+    /* "prints" the serial number onto the BIO */
+    i2a_ASN1_INTEGER(bio, X509_get_serialNumber (ctx->current_cert));
+    n = BIO_read (bio, serial, sizeof (serial)-1);
+    if (n < 0) {
+      serial[0] = '\x0';
+    }
+    else {
+      serial[n] = 0;
+    }
+    
     openvpn_snprintf (envname, sizeof(envname), "tls_serial_%d", ctx->error_depth);
-    setenv_int (opt->es, envname, serial);
+    setenv_str (opt->es, envname, serial);
   }
 
   /* export current untrusted IP */

Sorry, too quick! I posted an incomplete version of the patch. The attached one should be better.

The only doubt I have is about error handling; in this case, if the allocation of the BIO fails, an error message is logged and nothing is done. Is this the right thing to do?

D.

@@ -788,9 +788,27 @@ verify_callback (int preverify_ok, X509_
 
   /* export serial number as environmental variable */
   {
-    const int serial = (int) ASN1_INTEGER_get (X509_get_serialNumber (ctx->current_cert));
+    BIO *bio = NULL;
+    char serial[1024] = {0};
+    int n;
+
+    if ((bio = BIO_new (BIO_s_mem ())) == NULL) {
+      msg (M_FATAL, "CALLBACK: Cannot create BIO");
+    }
+    else {
+      /* "prints" the serial number onto the BIO */
+      i2a_ASN1_INTEGER(bio, X509_get_serialNumber (ctx->current_cert));
+      n = BIO_read (bio, serial, sizeof (serial)-1);
+      if (n < 0) {
+        serial[0] = '\x0';
+      }
+      else {
+        serial[n] = 0;
+      }
+    
     openvpn_snprintf (envname, sizeof(envname), "tls_serial_%d", ctx->error_depth);
-    setenv_int (opt->es, envname, serial);
+      setenv_str (opt->es, envname, serial);
+    }
   }
 
   /* export current untrusted IP */

I don't know if a FATAL error is such a good thing - not being able to set an env var does not warrant a server crash , I'd say. Also, you're not freeing the BIO as far as I can tell.

apart from that: nice patch !

cheers,

JJK

Ok, thanks. It's now a warning (with a clearer message as well).

Right, well spotted! The variables are in a block which makes them local, but of course the memory pointed to by bio must be freed. Thanks for the heads-up.

Please find attached the revised patch.

D.

@@ -788,9 +788,28 @@ verify_callback (int preverify_ok, X509_
 
   /* export serial number as environmental variable */
   {
-    const int serial = (int) ASN1_INTEGER_get (X509_get_serialNumber (ctx->current_cert));
+    BIO *bio = NULL;
+    char serial[1024] = {0};
+    int n;
+
+    if ((bio = BIO_new (BIO_s_mem ())) == NULL) {
+      msg (M_WARN, "CALLBACK: Cannot create BIO (for tls_serial_%d)", ctx->error_depth);
+    }
+    else {
+      /* "prints" the serial number onto the BIO */
+      i2a_ASN1_INTEGER(bio, X509_get_serialNumber (ctx->current_cert));
+      n = BIO_read (bio, serial, sizeof (serial)-1);
+      if (n < 0) {
+        serial[0] = '\x0';
+      }
+      else {
+        serial[n] = 0;
+      }
+    
     openvpn_snprintf (envname, sizeof(envname), "tls_serial_%d", ctx->error_depth);
-    setenv_int (opt->es, envname, serial);
+      setenv_str (opt->es, envname, serial);
+      BIO_free(bio);
+    }
   }
 
   /* export current untrusted IP */

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Davide!

Thanks a lot! This is looking pretty well, I have a few comments, though.

Right. Even if the variable is short-lived, and even anticipating future needs...I suppose 100 bytes will do for now.

Ok, will do that. But anyway, since it's meant to be a string, I thought that initializing it to '\0' would mean "empty string".

Neither do I, but I thought that "better safe then sorry". And btw, I saw that other OpenVPN code is /already/ doing the same thing (in fact, I wrote my code modeled after that): see pkcs11.c, from line 935.

Unfortunately, both 0 and negative numbers are all possible serial numbers (although they are now forbidden and not found in practice, but the openssl routines DO handle them). The RFC also says

"CAs MUST force the serialNumber to be a non-negative integer.
...
Note: Non-conforming CAs may issue certificates with serial numbers that are negative or zero. Certificate users SHOULD be prepared to gracefully handle such certificates."

So I maintain that an empty string is the right thing to do. It will then be up to whatever code uses the environment variable to check that it's not empty (or negative, etc.).

See above. Since the other code fragment doesn't do that, neither did I. But right, I will add a check and a warning if it returns a value < 0, which will result in an empty string put into the environment variable.

Let me know what you think, and I'll send a revised patch (yes, with summary, Signed-off-by and all that; I'm learning, slowly).
Thanks for reviewing!

D.

A minor nitpick, but...

[...]

The initialization will, in fact, set all bytes of "serial" to zero. Quoting the C standard (well, C89 draft is the closest I have):

"If there are fewer initializers in a list than there are members of an aggregate, the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration."

Which means filling with zeroes:

"If an object that has static storage duration is not initialized explicitly, it is initialized implicitly as if every member that has arithmetic type were assigned 0 and [--]"

I.e. you cannot "partially" initialize an object: the array elements not mentioned in the initializer are implicitly initialized to zero. You only get indeterminate values by omitting the initializer completely. I guess it's concievable some compiler could get this wrong, though...

Heikki Kallasjoki
[email protected]...

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Yes, that is kind of right. The '\0' do give the needed NULL termination which tells most string functions when to stop reading further. But if you do printf("%s", serial+1) you'll "escape" that single NULL termination. If that memory region is not cleared, you're going to get a lot of garbage out.

On modern systems, this is most often the case.

That is as my main concern. Most GCC compilers nowadays, do give 0'ed values on statically allocated variables, at least on Linux kernel. IIRC, those local variables are allocated on the stack, which gets the memory allocation via the kernel - so it's also up to the kernel if it returns a zero'd memory buffer. But this code might be built on less "clever" systems as well.

And the OpenVPN code uses CLEAR() and even memset() a lot of places throughout the code. This is merely to align with the coding "standard" in OpenVPN, and to follow more a defensive coding style.

kind regards,

David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkvRkvQACgkQDC186MBRfrrU/QCcCk/JxpcAxtb//ZGD1vSGNEwi H/AAn0tMN5Ov3kydEDStoif7p6WKBqCT
=ONp1
-----END PGP SIGNATURE-----

In order to avoid a chosen-prefix collision that works among other things by predicting the serial number of certificates generated by the CA.

http://www.win.tue.nl/hashclash/rogue-ca/
https://documents.epfl.ch/users/l/le/lenstra/public/papers/lat.pdf

//Peter

Ah, that is interesting. I haven't seen an MD5 certificate in a long time, but I guess being safe doesn't hurt, as sooner or later SHA might become vulnerable as well (and people should start using the newer, longer SHA-256,etc. hashes anyway - not just for certificates).

Thanks!

D.