Fix man page and OSCP script: tls_serial_{n} is decimal

Commit 7d5e26cbb53 fixed extracting serial but did not change the format, which always has been decimal. This patch fixes the manpage and OSCP.sh script to conform with the implementation.

---
 contrib/OCSP_check/OCSP_check.sh | 2 +-
 doc/openvpn.8                    | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

index 847be45..7e9e1e8 100644
@@ -97,7 +97,7 @@ if [ $check_depth -eq -1 ] || [ $cur_depth -eq $check_depth ]; then
                     "$nonce" \
                     -CAfile "$verify" \
                     -url "$ocsp_url" \
-                    -serial "0x${serial}" 2>/dev/null)
+                    -serial "${serial}" 2>/dev/null)
 
     if [ $? -eq 0 ]; then
       # check that it's good

index 3a58317..6dc3f87 100644
@@ -6080,9 +6080,9 @@ where
 is the verification level.  Only set for TLS connections.  Set prior
 to execution of
 .B \-\-tls-verify
-script. This is in the form of a hex string like "37AB46E0", which is
-suitable for doing serial-based OCSP queries (with OpenSSL, you have
-to prepend "0x" to the string). If something goes wrong while reading
+script. This is in the form of a decimal string like "933971680", which is
+suitable for doing serial-based OCSP queries (with OpenSSL, do not
+prepend "0x" to the string) If something goes wrong while reading
 the value from the certificate it will be an empty string, so your
 code should check that.
 See the contrib/OCSP_check/OCSP_check.sh script for an example.

1.8.5.2 (Apple Git-48)

Hi,

NACK, because glancing over the code tells me PolarSSL builds do use hex strings. So this fixes the inconsistency between implementation and manpage for OpenSSL builds, but adds inconsistency for PolarSSL builds.

I see three options here:
1 - Leave the difference, explain in the manpage. 2 - Change PolarSSL builds to use dec representation 3 - Change OpenSSL builds to use hax representation

I tend towards "just update the manpage". Changing PolarSSL builds to dec needs changes in PolarSSL, or manually parsing the serial in OpenVPN. Changing OpenSSL builds to hex is trivial but breaks backward compatibility. Any other opinions?

-Steffan

I agree with Steffan on this. I think it'd be better to just explain the difference in the man page and let it be.

-----
Eric F Crist

Hi,

I tend toward this one - user visible behaviour shouldn't change (unless unavoidable) depending on SSL library used.

So for me this boils down to "how many users are relying on the current behaviour, which is not what the docs say it should be"?

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

I've posted a script to verify CRLs
http://www.nikhef.nl/~janjust/vpn/OpenVPN2-Cookbook/verify-crls.sh
which relies on the XX:XX:XX format ... I could change that, of course, but I am not sure how many people are actually using that script. Printing the serial in hex format would make it more difficult to do a CRL check, however, as the openssl tools by default use the XX:XX:XX format.

JM2CW,

JJK

Hi,

If I try 'openssl crl -noout -text -in some.crl', the serial output reads as XX:XX:XX, but is in hex, not decimal form. That makes me believe this script was broken by the hex -> decimal change (and thus probably isn't used very much in combination with openvpn 2.2 or 2.3). So this, too, votes for 'make OpenSSL builds use hex again'.

-Steffan

I think the problem here is that historically, going back as far as 2005 (commit 6fbf66fad3367b24fd6743bcd50254902fd9c8d5), tls_serial_n has always given the serial number as decimal, and the documentation never indicated that the serial number was hex.

Commit 7ae5fb20d7dc52641ef853b896dffc0f283d16d2 in early 2011 added support for large serial numbers using BN_bn2dec, but the format was still decimal.

Now it looks like there was an alternative tls_serial_n implementation committed in 2010 (7d5e26cbb53e2700c966e6b6e815f0c824da8956) that changed the implementation to hex, but the code was later reverted, however unfortunately the comment in openvpn.8 that tls_serial_n is hex remained from that commit until now.

So a lot of code out there is assuming n is decimal, including some of our own code at OpenVPN Tech.

I would vote for preserving decimal behavior and updating the docs to reflect this.

It's unfortunate that PolarSSL is using hex, and I'm wondering how disruptive it would be to the PolarSSL user base if we change this to decimal.

I would tend to agree that the behavior should be consistent, regardless of underlying SSL library.

James

If we keep the current behavior (PolarSSL serial numbers are hex while OpenSSL are decimal) then we should at least mark the serial number when it's hex, so client software can distinguish it.

This very simple patch does that.

James

From a8f0d219d4edd1e95520cc40d27a0cd79cace2c2 Mon Sep 17 00:00:00 2001
From: James Yonan <[email protected]...>
Date: Wed, 16 Apr 2014 21:29:42 -0600
Subject: [PATCH] tls_serial_{n} value should be distinguishable as hex or
decimal.

To accomplish this, prepend 'x' before hex serial numbers, so they can be distinguished from decimal serial numbers. For example:

tls_serial_1 = "x4D:9B:7C:94"

is equivalent to:

tls_serial_1 = "1302035604"

Currently, only PolarSSL generates hex serial numbers while OpenSSL returns decimal serial numbers.

RFC 5280, published in 2008, decrees that serial numbers can be up to 20 bytes long, hence it is necessary to support SSL libraries that return the serial number as a hex string.

---
 src/openvpn/ssl_verify_polarssl.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

index 1b2990c..4367eb1 100644
@@ -127,11 +127,12 @@ char *
 backend_x509_get_serial (x509_crt *cert, struct gc_arena *gc)
 {
   char *buf = NULL;
-  size_t len = cert->serial.len * 3 + 1;
+  size_t len = cert->serial.len * 3;
 
-  buf = gc_malloc(len, true, gc);
+  buf = gc_malloc(len+1, true, gc);
 
-  if(x509_serial_gets(buf, len-1, &cert->serial) < 0)
+  buf[0] = 'x';
+  if(x509_serial_gets(buf+1, len, &cert->serial) < 0)
     buf = NULL;
 
   return buf;

1.8.5.3

Hi,

As discussed yesterday on the IRC meeting, NAK on this - Steffan promised to send a patch that will change PolarSSL to provide decimal serial numbers as well (*nudge*), so we're fully consistent.

Emphasizing this again, don't use "%d" on serials :-)

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Your patch has been applied to the master and release/2.3 branches.

I've counted the result from yesterday's IRC discussion plus the mail thread as "ACK from James and me", and Steffan agreed as well (canceling his initial NAK).

commit 959d60789b6f0bd74296600f58f626cfa9738f78 (master) commit d160a62852408121ce8fc023a89f47c7920c16e8 (release/2.3)

Author: Arne Schwabe
Date:   Fri Mar 28 11:07:01 2014 +0100

     Fix man page and OSCP script: tls_serial_{n} is decimal

     Acked-by: James Yonan <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/8409
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering