ssl3_read_n() was conceived to read blocks of data from a byte oriented
stream. This can be easily explained by an example: You call
ssl3_read_n() with the a parameter like "Read 50 bytes of data". As
opposed to the read() function provided by the OS, ssl3_read_n() makes
sure you really get 50 bytes of data and not less. ssl3_read_n() calls
read() - or I should say BIO_read() - multiple times if that is
necessary to get this amount of data.
Compare this with the read() function provided by the OS. If you want to
read 50 bytes then read() *might* return a chunk of data that is shorter
than 50 bytes. On the other hand ssl3_read_n() calls read() multiple
times if necessary to make sure you really get the number of bytes you
requested.
The key point is that ssl3_read_n() *concatenates* data from successive
read() calls which is fine for a byte oriented stream like in the TCP
case. But in the UDP case this is very bad because each read() call
returns one datagram and each datagram needs to be treated i.e. parsed
separately.
This patch aims to make sure that ssl3_read_n() *never* concatenates two
different datagrams. Also, dtls1_get_record() had to be changed slightly
to make it cope with datagrams that are shorter than the minimum length
required by the DTLS Record Header.
I'm asking the OpenSSL developers to double check this patch because
ssl3_read_n() is also used by the TLS code and not only by the DTLS part.
The bug that this patch tries to fix can be used by an off path attacker
to hamper the ongoing connection. He could use for example
hping3 --udp --baseport 49468 --destport 4433 -d 1 -c 1 localhost
to send an UDP datagram of size 1 to one DTLS peer. This peer buffers
this packet and concatenates it with the next packet it receives. The
peer then tries to parse this concatenated piece of data which fails.
An alternative to this patch would be IMHO to come up with a replacement
for ssl3_read_n() that is used in the DTLS case only.
-Daniel
--- s3_pkt.c 20 Apr 2009 11:33:11 -0000 1.74
+++ s3_pkt.c 8 Jul 2009 18:24:59 -0000
@@ -172,18 +172,21 @@
}
}
s->packet = rb->buf + rb->offset;
s->packet_length = 0;
/* ... now we can act as if 'extend' was set */
}
/* extend reads should not span multiple packets for DTLS */
- if ( (SSL_version(s) == DTLS1_VERSION || SSL_version(s) == DTLS1_BAD_VER)
- && extend)
+
+ /* reads should *never* span multiple packets for DTLS because
+ * the underlying transport protocol is message oriented as opposed
+ * to byte oriented as in the TLS case. */
+ if ( (SSL_version(s) == DTLS1_VERSION || SSL_version(s) == DTLS1_BAD_VER))
{
if ( left > 0 && n > left)
n = left;
}
/* if there is enough in the buffer from a previous read, take some */
if (left >= n)
{
@@ -239,16 +242,24 @@
{
rb->left = left;
if (s->mode & SSL_MODE_RELEASE_BUFFERS)
if (len+left == 0)
ssl3_release_read_buffer(s);
return(i);
}
left+=i;
+ /* reads should *never* span multiple packets for DTLS because
+ * the underlying transport protocol is message oriented as opposed
+ * to byte oriented as in the TLS case. */
+ if ( (SSL_version(s) == DTLS1_VERSION || SSL_version(s) == DTLS1_BAD_VER))
+ {
+ if (n > left)
+ n = left; /* makes the while condition false */
+ }
}
/* done reading, now the book-keeping */
rb->offset += n;
rb->left = left - n;
s->packet_length += n;
s->rwstate=SSL_NOTHING;
return(n);
--- d1_pkt.c 4 Jul 2009 12:04:06 -0000 1.36
+++ d1_pkt.c 8 Jul 2009 18:25:13 -0000
@@ -556,17 +556,19 @@
/* check if we have the header */
if ( (s->rstate != SSL_ST_READ_BODY) ||
(s->packet_length < DTLS1_RT_HEADER_LENGTH))
{
n=ssl3_read_n(s, DTLS1_RT_HEADER_LENGTH, s->s3->rbuf.len, 0);
/* read timeout is handled by dtls1_read_bytes */
if (n <= 0) return(n); /* error or non-blocking */
- OPENSSL_assert(s->packet_length == DTLS1_RT_HEADER_LENGTH);
+ /* this packet contained a partial record, dump it */
+ if(s->packet_length != DTLS1_RT_HEADER_LENGTH)
+ goto again;
s->rstate=SSL_ST_READ_BODY;
p=s->packet;
/* Pull apart the header into the DTLS1_RECORD */
rr->type= *(p++);
ssl_major= *(p++);
