I recently came across an issue using DTLS with a non-blocking transport in which SSL_read would return bad data when packets were received out of sequence. I hit this issue when due to an unrelated bug in 0.9.8a's DTLS windowing logic (which looks fixed in HEAD). In 0.9.8a any DTLS packets with a sequence number less than the higher received sequence number will be considered outside the window. The problem occurs for any packet that is ignored -- I hit it on out-of-sequence data, but spoofed data might be able to generate the same effect.
The bug happens when the first SSL_read hits the logic on line d1_pkt:595, an attempt to read another packet. While s->packet_length is set to 0, rr->length remains set to the value in the discarded record, and the non-blocking bio (in my case a bio_pair) returns WANT_READ.
On the next read, the problem occurs on line d1_pkt.c:701. Because rr->length is still set to the value from the previous packet, dtls1_get_record will not be invoked for the new packet. Which ultimately results in bogus data being returned from SSL_read.
The attached patch fixes this by setting rr->length to 0 whenever a record is ignored and the code tries to read another record.
--- openssl-0.9.8a/ssl/d1_pkt.c 2005-06-05 20:32:30.000000000 -0400
+++ openssl-0.9.8a-patched/ssl/d1_pkt.c 2006-02-25 05:58:12.000000000 -0500
@@ -573,6 +573,7 @@
if ( n != i)
{
s->packet_length = 0;
+ rr->length = 0;
goto again;
}
@@ -584,9 +585,10 @@
/* match epochs. NULL means the packet is dropped on the floor */
bitmap = dtls1_get_bitmap(s, rr, &is_next_epoch);
if ( bitmap == NULL)
- {
- s->packet_length = 0; /* dump this record */
- goto again; /* get another record */
+ {
+ s->packet_length = 0; /* dump this record */
+ rr->length = 0;
+ goto again; /* get another record */
}
/* check whether this is a repeat, or aged record */
@@ -608,6 +610,7 @@
dtls1_record_bitmap_update(s, bitmap);
dtls1_buffer_record(s, &(s->d1->unprocessed_rcds), rr->seq_num);
s->packet_length = 0;
+ rr->length = 0;
goto again;
}
