dtls1_process_out_of_seq_message() has two bugs:
- Only one handshake message fragment per sequence number is saved.
However, there may be multiple fragments with identical sequence
numbers. All of them should be buffered.
- Fragments with zero length are not saved. This is incorrect behavior
because there may well be handshake messages with zero length like, for
example, ServerHelloDone.
This patch fixes these two bugs.
The implementation of pqueue had to be changed to accept duplicates,
i.e. multiple items with identical priority values. I came to the
conclusion that this is ok for all use cases of this pqueue implementation.
-Daniel
Index: crypto/pqueue/pqueue.c
===================================================================
RCS file: /v/openssl/cvs/openssl/crypto/pqueue/pqueue.c,v
retrieving revision 1.7
diff -u -8 -p -r1.7 pqueue.c
--- crypto/pqueue/pqueue.c 16 May 2009 16:18:19 -0000 1.7
+++ crypto/pqueue/pqueue.c 5 Apr 2010 19:53:56 -0000
@@ -132,18 +132,16 @@ pqueue_insert(pqueue_s *pq, pitem *item)
if (curr == NULL)
pq->items = item;
else
curr->next = item;
return item;
}
- else if (cmp == 0) /* duplicates not allowed */
- return NULL;
}
item->next = NULL;
curr->next = item;
return item;
}
Index: ssl/d1_both.c
===================================================================
RCS file: /v/openssl/cvs/openssl/ssl/d1_both.c,v
retrieving revision 1.28
diff -u -8 -p -r1.28 d1_both.c
--- ssl/d1_both.c 24 Mar 2010 23:17:15 -0000 1.28
+++ ssl/d1_both.c 5 Apr 2010 19:53:58 -0000
@@ -573,52 +573,53 @@ dtls1_process_out_of_seq_message(SSL *s,
if ((msg_hdr->frag_off+frag_len) > msg_hdr->msg_len)
goto err;
/* Try to find item in queue, to prevent duplicate entries */
memset(seq64be,0,sizeof(seq64be));
seq64be[6] = (unsigned char) (msg_hdr->seq>>8);
seq64be[7] = (unsigned char) msg_hdr->seq;
- item = pqueue_find(s->d1->buffered_messages, seq64be);
/* Discard the message if sequence number was already there, is
- * too far in the future, already in the queue or if we received
+ * too far in the future or if we received
* a FINISHED before the SERVER_HELLO, which then must be a stale
* retransmit.
*/
if (msg_hdr->seq <= s->d1->handshake_read_seq ||
- msg_hdr->seq > s->d1->handshake_read_seq + 10 || item != NULL ||
+ msg_hdr->seq > s->d1->handshake_read_seq + 10 ||
(s->d1->handshake_read_seq == 0 && msg_hdr->type == SSL3_MT_FINISHED))
{
unsigned char devnull [256];
while (frag_len)
{
i = s->method->ssl_read_bytes(s,SSL3_RT_HANDSHAKE,
devnull,
frag_len>sizeof(devnull)?sizeof(devnull):frag_len,0);
if (i<=0) goto err;
frag_len -= i;
}
}
-
- if (frag_len)
+ else
{
frag = dtls1_hm_fragment_new(frag_len);
if ( frag == NULL)
goto err;
memcpy(&(frag->msg_header), msg_hdr, sizeof(*msg_hdr));
- /* read the body of the fragment (header has already been read */
- i = s->method->ssl_read_bytes(s,SSL3_RT_HANDSHAKE,
- frag->fragment,frag_len,0);
- if (i<=0 || (unsigned long)i!=frag_len)
- goto err;
+ if (frag_len)
+ {
+ /* read the body of the fragment (header has already been read */
+ i = s->method->ssl_read_bytes(s,SSL3_RT_HANDSHAKE,
+ frag->fragment,frag_len,0);
+ if (i<=0 || (unsigned long)i!=frag_len)
+ goto err;
+ }
memset(seq64be,0,sizeof(seq64be));
seq64be[6] = (unsigned char)(msg_hdr->seq>>8);
seq64be[7] = (unsigned char)(msg_hdr->seq);
item = pitem_new(seq64be, frag);
if ( item == NULL)
goto err;
