From: jsing <jsing@openbsd.org>
Date: Sat, 19 Jun 2021 17:21:39 +0000 (+0000)
Subject: Correctly handle epoch wrapping in dtls1_get_bitmap().
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=bdaf1583af1de60bbad2f5dfc8aa17126cb23563;p=openbsd

Correctly handle epoch wrapping in dtls1_get_bitmap().

Due to a type bug that has been present in DTLS since the code was first
committed in 2005, dtls1_get_bitmap() fails to handle next epoch correctly
when the epoch is currently 0xffff (and wraps to zero).

For various reasons unknown, the epoch field in the SSL3_RECORD_INTERNAL
(formerly SSL3_RECORD) was added as unsigned long (even though the value
is an unsigned 16 bit value on the wire, hence cannot exceed 0xffff),
however was added to other code as unsigned short.

Due to integer promotion, the r_epoch value is incremented by one to
become 0x10000, before being cast to an unsigned long and compared to
the value pulled from the DTLS record header (which is zero). Strangely
0x10000 != 0, meaning that we drop the DTLS record, instead of queueing
it for the next epoch.

Fix this issue by using more appropriate types and pulling up the
calculation of the next epoch value for improved readability.

ok inoguchi@ tb@
---

diff --git a/lib/libssl/d1_pkt.c b/lib/libssl/d1_pkt.c
index 11735f0d2c7..17be6dd182d 100644
--- a/lib/libssl/d1_pkt.c
+++ b/lib/libssl/d1_pkt.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: d1_pkt.c,v 1.98 2021/06/15 19:09:03 jsing Exp $ */
+/* $OpenBSD: d1_pkt.c,v 1.99 2021/06/19 17:21:39 jsing Exp $ */
 /*
  * DTLS implementation written by Nagendra Modadugu
  * (nagendra@cs.stanford.edu) for the OpenSSL project 2005.
@@ -1191,6 +1191,7 @@ dtls1_dispatch_alert(SSL *s)
 static DTLS1_BITMAP *
 dtls1_get_bitmap(SSL *s, SSL3_RECORD_INTERNAL *rr, unsigned int *is_next_epoch)
 {
+	uint16_t next_epoch = D1I(s)->r_epoch + 1;
 
 	*is_next_epoch = 0;
 
@@ -1199,7 +1200,7 @@ dtls1_get_bitmap(SSL *s, SSL3_RECORD_INTERNAL *rr, unsigned int *is_next_epoch)
 		return &D1I(s)->bitmap;
 
 	/* Only HM and ALERT messages can be from the next epoch */
-	else if (rr->epoch == (unsigned long)(D1I(s)->r_epoch + 1) &&
+	else if (rr->epoch == next_epoch &&
 		(rr->type == SSL3_RT_HANDSHAKE || rr->type == SSL3_RT_ALERT)) {
 		*is_next_epoch = 1;
 		return &D1I(s)->next_bitmap;
diff --git a/lib/libssl/ssl_locl.h b/lib/libssl/ssl_locl.h
index 18509438aed..d171ef0984a 100644
--- a/lib/libssl/ssl_locl.h
+++ b/lib/libssl/ssl_locl.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_locl.h,v 1.349 2021/06/19 16:52:47 jsing Exp $ */
+/* $OpenBSD: ssl_locl.h,v 1.350 2021/06/19 17:21:40 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -886,7 +886,7 @@ typedef struct ssl3_record_internal_st {
 	unsigned int off;       /* read/write offset into 'buf' */
 	unsigned char *data;    /* pointer to the record data */
 	unsigned char *input;   /* where the decode bytes are */
-	unsigned long epoch;    /* epoch number, needed by DTLS1 */
+	uint16_t epoch;		/* epoch number, needed by DTLS1 */
 	unsigned char seq_num[8]; /* sequence number, needed by DTLS1 */
 } SSL3_RECORD_INTERNAL;