Dear all,
I agree with Daniel that reading a record from multiple UDP packets
is a bug. I need some time to figure out if the proposed fix is the
right one.
Robin is on holiday for two weeks.
Best regards
Michael
On Jul 8, 2009, at 10:15 PM, Daniel Mentz wrote:
> 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++);
>
______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-dev@openssl.org
Automated List Manager majord...@openssl.org
