[Openswan dev] Re: PATCH: X.509 Off by one bug in crl processing (aka crl.pem crash bug)

[Andreas Steffen]

Hi Ken,

I checked my original code and I still think that it is correct.

I ran the following practical example:

   #define CRL_PATH   "/etc/ipsec.d/crls"  sizeof(CRL_PATH) = 18

the terminating null character is included by sizeof()

   filename = "cert.crl"                   strlen(filename) =  8

the terminating null character is not included by strlen()

   crl_uri.len = 7 + sizeof(CRL_PATH) + strlen(filename) = 33

crl_uri.len is the length of the composite string without the
terminating null character.

   allocate_bytes(crl_uri.len + 1) i.e. 34 bytes are allocated

the additional allocated byte is for the terminating null character
which will not be used by the crl fetching routine because
the ASN.1 coded crlDistributionPoints extracted from X.509 certificates
are *not* null terminated.

   sprintf(crl_uri.ptr, "file://%s/%s", CRL_PATH, filename);

results in the crlDistributionPoint

   "file:///etc/ipsec.d/crls/cert.crl"

with

    strlen(crl_uri.ptr) = 33

so the 34 allocated bytes are sufficient to store the
crlDistributionPoint *plus* the unused null character at the end.

Of course it is a safer approach to restrict the sprintf operation to the
maximum available memory. Thus the modification should be

   snprintf(crl_uri.ptr, crl_uri.len+1, "file://%s/%s", CRL_PATH, filename);

taking into account that an additional byte was allocated. Thus crl_uri.len
should not be increased by one and the following patch results:

@@ -336,7 +336,8 @@
                     crl_uri.ptr = alloc_bytes(crl_uri.len + 1, "crl uri");

                     /* build CRL file URI */
-                   sprintf(crl_uri.ptr, "file://%s/%s", CRL_PATH, filename);
+                   snprintf(crl_uri.ptr, crl_uri.len + 1, "file://%s/%s"
+                       , CRL_PATH, filename);

                     insert_crl(blob, crl_uri);
                 }

In openswan-2 the header file certs.h defines

   #define CRL_PATH	  plutopaths.crls.path

I don't know if sizeof(CRL_PATH) will still give the correct result
because the content of plutopaths.crls.path is assigned dynamically in log.c
during runtime. Probably a safer approach would be

-		    crl_uri.len = 7 + sizeof(CRL_PATH) + strlen(filename);
+		    crl_uri.len = 8 + strlen(CRL_PATH) + strlen(filename);

Regards

Andreas

Bantoft wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> 
> Hi Andreas,
> 
> We (myself + dhr + mcr) tracked down the 'crl.pem' bug to an off-by-one 
> error in x509.c, which is still present in Strongswan 2.0.2
> 
> Attached is a patch, and we switched to snprintf too, as dhr's been 
> snprintf happy for dealing with /proc stuff, as it's all been changed by 
> 2.4.25 kernels and higher.
> 
> This is against 1.4.8, but Strongswan 2.0.2 has the same code.
> 
> Note that in running pluto through valgrind, I also uncovered a read 
> error buried in the id.c code:
> 
> ==22686== Invalid read of size 1
> ==22686==    at 0x4207A995: strnlen (in /lib/tls/libc-2.3.2.so)
> ==22686==    by 0x420478C7: _IO_vfprintf_internal (in /lib/tls/libc-2.3.2.so)
> ==22686==    by 0x4206A363: _IO_vsnprintf (in /lib/tls/libc-2.3.2.so)
> ==22686==    by 0x4204F513: snprintf (in /lib/tls/libc-2.3.2.so)
> ==22686==    by 0x805D4D9: idtoa (id.c:325)
> ==22686==    by 0x805CD9C: calc_myid_str (id.c:82)
> ==22686==    by 0x805D008: set_myFQDN (id.c:142)
> ==22686==    by 0x805CD65: init_id (id.c:73)
> ==22686==  Address 0x75080362 is 0 bytes after a block of size 22 alloc'd
> ==22686==    at 0x7501D858: malloc (vg_replace_malloc.c:105)
> ==22686==    by 0x809676C: clone_bytes (alloc.c:179)
> ==22686==    by 0x805CFEC: set_myFQDN (id.c:140)
> 
> 
> The actual line that valgrind catches is
> 
> 
>     case ID_FQDN:
>         n = snprintf(dst, dstlen, "@%.*s", (int)id->name.len, id->name.ptr);
>         break;
> 
> However since variations of that snprintf() are above/below, it's really 
> elsewhere.
> 
> Ken
> 
> 
> Index: x509.c
> ===================================================================
> RCS file: /xelerance/master/openswan-2/programs/pluto/x509.c,v
> retrieving revision 1.9
> retrieving revision 1.10
> diff -u -d -r1.9 -r1.10
> - --- x509.c	21 Mar 2004 05:00:18 -0000	1.9
> +++ x509.c	20 Apr 2004 00:27:29 -0000	1.10
> @@ -1767,12 +1767,11 @@
>  		if (load_coded_file(filename, NULL, "crl", &blob, &pgp))
>  		{
>  		    chunk_t crl_uri;
> - -		    crl_uri.len = 7 + sizeof(CRL_PATH) + strlen(filename);
> +		    crl_uri.len = 8 + sizeof(CRL_PATH) + strlen(filename);
>  		    crl_uri.ptr = alloc_bytes(crl_uri.len + 1, "crl uri");
>  
>  		    /* build CRL file URI */
> - -		    sprintf(crl_uri.ptr, "file://%s/%s", CRL_PATH, filename);
> - -
> +		    snprintf(crl_uri.ptr, crl_uri.len, "file://%s/%s", CRL_PATH, filename);
>  		    insert_crl(blob, crl_uri);
>  		}
>  		free(filelist[n]);
> 
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.2.1 (GNU/Linux)
> 
> iD8DBQFAhHc4PiOgilmwgkgRAu0tAKCJfVsW6p/Q3Lrt2apgeqIOh4mB6wCeOpRF
> va8GQ9ABm0Z0CgYJiY2Tk5Q=
> =LNxZ
> -----END PGP SIGNATURE-----

=======================================================================
Andreas Steffen                   e-mail: andreas.steffen at strongsec.com
strongSec GmbH                    home:   http://www.strongsec.com
Alter Zürichweg 20                phone:  +41 1 730 80 64
CH-8952 Schlieren (Switzerland)   fax:    +41 1 730 80 65
==========================================[strong internet security]===