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

[D. Hugh Redelmeier]

-----BEGIN PGP SIGNED MESSAGE-----


| From: Ken Bantoft <ken at xelerance.com>

| 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

My current opinion is that there are two bugs.  But I must admit that
my impression is based on second-hand information.

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

Of course this has nothing to do with the kernal (except perhaps my
attitude).

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

This valgrind report is about the second bug.  But the patch fixes a
different bug (what I call "the first bug").

The patch from 7 to 8 is clearly a fix: the sprinf/snprintf format
contributes 8 characters, not 7, to the resulting string.  It would be
nice if the relationship between the length calculation and the format
were made more clear.

The use of snprintf means that a mistake will have less confusing
consequences: a string truncation rather than a buffer overflow.  But
either way is a problem.

By way of confirmation, the patch eliminates a different report from
valgrind (an invalid write).

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

This is the line of code implicated in the quoted valgrind report.
This is the second bug.  I don't mean to say the bug is in this line
but that this line fetches something from memory that should not be
fetched, probably because len is wrong.  Investigation is not
complete.

Hugh Redelmeier
hugh at mimosa.com  voice: +1 416 482-8253


-----BEGIN PGP SIGNATURE-----
Version: 2.6.3ia
Charset: noconv

iQCVAwUBQISXDMFAuQPManGZAQEfXwQAi2pJtJFz9XjEy8oceALnLk1lCTccD9Fh
fWazsEIgbM4RztP0thdGQ7RRvzH+mQsrKtbcb5TJuV5pssRUDXWRT+wPHUCdzvZJ
cjVo2HWwXSHwvxD/pe/k4udLB6PWz4oQJSlRMeTPV1Bju3qOpVtcRZ7QPLycXV11
UeTUKBREGkI=
=sT1F
-----END PGP SIGNATURE-----