|
|
|
#1752: DTLS drops incoming packets when they are reordered.
|
|
|
|
X
Ticket metadata
|
|
|
X
The Basics
|
|
| Id: |
1752 |
| Status: |
resolved |
| Left: |
0 min |
| Priority: |
0/0 |
| Queue: |
OpenSSL-Bugs |
|
|
X
People
|
|
| Owner: |
jaenicke
<lutz@lutz-jaenicke.de>
|
| Requestors: |
dwmw2@infradead.org
|
| Cc: |
|
| AdminCc: |
|
|
|
|
X
Links
|
|
| Depends on: |
|
| Depended on by: |
|
| Parents: |
|
| Children: |
|
| Refers to: |
|
| Referred to by: |
|
|
|
X
More about David Woodhouse
|
|
Comments about this user:
No comment entered about this user
This user's 10 highest priority tickets:
Groups this user belongs to:
|
|
|
|
X
History
|
Display mode:[Brief headers] [Full headers]
|
| # |
|
Mon Oct 06 09:24:05 2008 |
dwmw2@infradead.org - Ticket created
|
|
|
|
If incoming data packets are received out of order, they seem to get
dropped by my test case using blocking I/O.
Worse, my real application using non-blocking I/O seems to just receive
garbage when the out-of-order packet arrives.
Even the blocking behaviour seems inappropriate for DTLS. Reordering
within reason should be fine -- it's a datagram service, after all.
It seems to occur because dtls1_record_replay_check() is rejecting the
packets. This, in turn, is because s->d1->next_bitmap.length is never
being set. This patch to dtls1_new() seems to fix it:
--- ssl/d1_lib.c~ 2008-10-02 06:43:47.000000000 +0100
+++ ssl/d1_lib.c 2008-10-05 21:31:38.000000000 +0100
@@ -106,6 +106,7 @@ int dtls1_new(SSL *s)
pq_64bit_init(&(d1->bitmap.map));
pq_64bit_init(&(d1->bitmap.max_seq_num));
+ d1->next_bitmap.length = d1->bitmap.length;
pq_64bit_init(&(d1->next_bitmap.map));
pq_64bit_init(&(d1->next_bitmap.max_seq_num));
That doesn't solve the question of why non-blocking I/O was returning
crap for the offending out-of-order packets, instead of just returning
an error with SSL_ERROR_WANT_READ as might be expected. But at least
it'll make that bug offend me less.
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
|
| # |
|
Mon Oct 06 09:25:14 2008 |
dwmw2@infradead.org - Correspondence added
|
|
|
|
(Was waiting for the RT to autoreply with a number before I followed up,
but it doesn't seem to have arrived after half an hour, so I'll send
anyway. Hopefully the References: header will associate this with the
previous mail anyway...)
On Sun, 2008-10-05 at 21:38 +0100, David Woodhouse wrote:
> That doesn't solve the question of why non-blocking I/O was returning
> crap for the offending out-of-order packets, instead of just returning
> an error with SSL_ERROR_WANT_READ as might be expected. But at least
> it'll make that bug offend me less.
This seems to fix the garbage packets.
--- ssl/d1_pkt.c~ 2008-10-02 06:43:47.000000000 +0100
+++ ssl/d1_pkt.c 2008-10-05 21:44:54.000000000 +0100
@@ -597,6 +597,7 @@ again:
/* check whether this is a repeat, or aged record */
if ( ! dtls1_record_replay_check(s, bitmap, &(rr->seq_num)))
{
+ rr->length = 0;
s->packet_length=0; /* dump this record */
goto again; /* get another record */
}
--
David Woodhouse Open Source Technology Centre
David.Woodhouse@intel.com Intel Corporation
|
| # |
|
Tue Oct 07 10:57:04 2008 |
openssl-dev@openssl.org - Ticket 1759: Ticket created
|
|
|
|
This patch to the 0.9.8 branch fixes two bugs with misordered incoming
packets in DTLS, which are reported as RT #1752.
Firstly, the bitmap we use for replay protection was ending up with zero
length, so a _single_ pair of packets getting switched around would
cause one of them to be 'dropped'.
Secondly, it wasn't even _dropping_ the offending packets, in the
non-blocking case. It was just returning garbage instead.
--- ssl/d1_lib.c~ 2008-10-02 06:43:47.000000000 +0100
+++ ssl/d1_lib.c 2008-10-05 21:31:38.000000000 +0100
@@ -106,6 +106,7 @@ int dtls1_new(SSL *s)
pq_64bit_init(&(d1->bitmap.map));
pq_64bit_init(&(d1->bitmap.max_seq_num));
+ d1->next_bitmap.length = d1->bitmap.length;
pq_64bit_init(&(d1->next_bitmap.map));
pq_64bit_init(&(d1->next_bitmap.max_seq_num));
--- ssl/d1_pkt.c~ 2008-10-02 06:43:47.000000000 +0100
+++ ssl/d1_pkt.c 2008-10-05 21:44:54.000000000 +0100
@@ -597,6 +597,7 @@ again:
/* check whether this is a repeat, or aged record */
if ( ! dtls1_record_replay_check(s, bitmap, &(rr->seq_num)))
{
+ rr->length = 0;
s->packet_length=0; /* dump this record */
goto again; /* get another record */
}
--
dwmw2
______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-dev@openssl.org
Automated List Manager majordomo@openssl.org
|
| # |
|
Tue Oct 07 16:09:57 2008 |
jaenicke - Ticket 1759: Requestor openssl-dev@openssl.org deleted
|
|
|
|
|
| # |
|
Tue Oct 07 16:10:06 2008 |
jaenicke - Ticket 1759: Requestor dwmw2@infradead.org added
|
|
|
|
|
| # |
|
Tue Oct 07 16:11:30 2008 |
jaenicke - Ticket 1759: Merged into ticket #1752
|
|
|
|
|
| # |
|
Tue Oct 07 16:11:30 2008 |
jaenicke - Merged into ticket #1752
|
|
|
|
|
| # |
|
Fri Oct 10 12:51:13 2008 |
jaenicke - Correspondence added
|
|
|
|
> [openssl-dev@openssl.org - Tue Oct 07 10:57:04 2008]:
>
> This patch to the 0.9.8 branch fixes two bugs with misordered incoming
> packets in DTLS, which are reported as RT #1752.
Could you comment on the 0.9.9-dev branch as well?
The patch to d1_pkt.c applies fine. The "length" object is gone from the
code so it should not be needed any longer.
Best regards,
Lutz
>
> Firstly, the bitmap we use for replay protection was ending up with zero
> length, so a _single_ pair of packets getting switched around would
> cause one of them to be 'dropped'.
>
> Secondly, it wasn't even _dropping_ the offending packets, in the
> non-blocking case. It was just returning garbage instead.
>
> --- ssl/d1_lib.c~ 2008-10-02 06:43:47.000000000 +0100
> +++ ssl/d1_lib.c 2008-10-05 21:31:38.000000000 +0100
> @@ -106,6 +106,7 @@ int dtls1_new(SSL *s)
> pq_64bit_init(&(d1->bitmap.map));
> pq_64bit_init(&(d1->bitmap.max_seq_num));
>
> + d1->next_bitmap.length = d1->bitmap.length;
> pq_64bit_init(&(d1->next_bitmap.map));
> pq_64bit_init(&(d1->next_bitmap.max_seq_num));
>
> --- ssl/d1_pkt.c~ 2008-10-02 06:43:47.000000000 +0100
> +++ ssl/d1_pkt.c 2008-10-05 21:44:54.000000000 +0100
> @@ -597,6 +597,7 @@ again:
> /* check whether this is a repeat, or aged record */
> if ( ! dtls1_record_replay_check(s, bitmap, &(rr->seq_num)))
> {
> + rr->length = 0;
> s->packet_length=0; /* dump this record */
> goto again; /* get another record */
> }
>
>
>
|
| # |
|
Fri Oct 10 12:51:13 2008 |
RT_System - Status changed from 'new' to 'open'
|
|
|
|
|
| # |
|
Mon Oct 13 08:46:28 2008 |
jaenicke - Taken
|
|
|
|
|
| # |
|
Mon Oct 13 08:47:48 2008 |
jaenicke - Correspondence added
|
|
|
|
From answer only sent to mailing list:
Yeah, it looks right. I haven't yet got it working with my test case,
because I need to use DTLS1_BAD_VER and there are other parts missing
from HEAD for that, on top of my patch in #1751 -- but I agree with your
assessment that it shouldn't be needed any longer.
Thanks, fix applied.
Best regards,
Lutz
|
| # |
|
Mon Oct 13 08:47:49 2008 |
jaenicke - Status changed from 'open' to 'resolved'
|
|
|
|
|
|
|
|
Time to display: 0.842759
|
|
