[PATCH v3 1/3] mpdu: expose mmpdu_header_len

[PATCH v3 1/3] mpdu: expose mmpdu_header_len

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 856 bytes --]

---
 src/mpdu.c | 2 +-
 src/mpdu.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/mpdu.c b/src/mpdu.c
index eb141c75..844e61d5 100644
--- a/src/mpdu.c
+++ b/src/mpdu.c
@@ -623,7 +623,7 @@ const struct mmpdu_header *mpdu_validate(const uint8_t *frame, int len)
 	}
 }
 
-static size_t mmpdu_header_len(const struct mmpdu_header *mmpdu)
+size_t mmpdu_header_len(const struct mmpdu_header *mmpdu)
 {
 	return mmpdu->fc.order == 0 ? 24 : 28;
 }
diff --git a/src/mpdu.h b/src/mpdu.h
index 4cf08d00..f3f7e4cd 100644
--- a/src/mpdu.h
+++ b/src/mpdu.h
@@ -408,3 +408,4 @@ struct mmpdu_deauthentication {
 
 const struct mmpdu_header *mpdu_validate(const uint8_t *frame, int len);
 const void *mmpdu_body(const struct mmpdu_header *mpdu);
+size_t mmpdu_header_len(const struct mmpdu_header *mmpdu);
-- 
2.17.1

[PATCH v3 2/3] sae: fix potential integer overflow

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 2893 bytes --]

If an authentication frame of length <= 5 is sent sae will overflow an
integer. The original cause of this was due to incorrectly using the
sizeof(struct mmpdu_header). The header can be either 24 or 28 bytes
depending on fc.order. sizeof does not account for this so 28 is always
the calculated length.

This, in addition to hostapd not including a group number when rejecting,
cause this erroneous length calculation to be worked around as seen in
the removed comment. The comment is still valid (and described again
in another location) but the actual check for len == 4 is not correct.

To fix this we now rely on mpdu_validate to check that the authentication
frame is valid, and then subtract the actual header length using
mmpdu_header_len rather than sizeof. Doing this lets us also remove the
length check since it was validated previously.
---
 src/sae.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

-v3:
 * Removed length check as mpdu_validate does this for us
 * Use mmpdu_header_len instead of auth - hdr calculation

diff --git a/src/sae.c b/src/sae.c
index 8f9425f1..a6b2c582 100644
--- a/src/sae.c
+++ b/src/sae.c
@@ -689,6 +689,9 @@ static int sae_verify_nothing(struct sae_sm *sm, uint16_t transaction,
 	if (status != 0)
 		return -EBADMSG;
 
+	if (len < 2)
+		return -EBADMSG;
+
 	/* reject with unsupported group */
 	if (l_get_le16(frame) != sm->group) {
 		sae_reject_authentication(sm,
@@ -909,6 +912,9 @@ static int sae_verify_confirmed(struct sae_sm *sm, uint16_t trans,
 	if (sm->sync > SAE_SYNC_MAX)
 		return -EBADMSG;
 
+	if (len < 2)
+		return -EBADMSG;
+
 	/* frame shall be silently discarded */
 	if (l_get_le16(frame) != sm->group)
 		return 0;
@@ -944,6 +950,9 @@ static bool sae_verify_accepted(struct sae_sm *sm, uint16_t trans,
 	if (sm->sync > SAE_SYNC_MAX)
 		return false;
 
+	if (len < 2)
+		return false;
+
 	sc = l_get_le16(frame);
 
 	/*
@@ -1021,24 +1030,7 @@ static int sae_rx_authenticate(struct auth_proto *ap,
 		goto reject;
 	}
 
-	len -= sizeof(struct mmpdu_header);
-
-	if (len < 4) {
-		l_error("bad packet length");
-		goto reject;
-	}
-
-	/*
-	 * TODO: Hostapd seems to not include the group number when rejecting
-	 * with an unsupported group, which violates the spec. This means our
-	 * len == 4, but we can still recover this connection by renegotiating
-	 * a new group. Because of this we need to special case this status
-	 * code, as well as add the check in the verify function to allow for
-	 * this missing group number.
-	 */
-	if (len == 4 && L_LE16_TO_CPU(auth->status) !=
-				MMPDU_STATUS_CODE_UNSUPP_FINITE_CYCLIC_GROUP)
-		goto reject;
+	len -= mmpdu_header_len(hdr);
 
 	ret = sae_verify_packet(sm, L_LE16_TO_CPU(auth->transaction_sequence),
 					L_LE16_TO_CPU(auth->status),
-- 
2.17.1

[PATCH v3 3/3] sae: fix inproper return value in sae_verify_accepted

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 2172 bytes --]

This function was returning a boolean and the expected return was
a signed integer. Since this function actually returned false in
all cases the check for a success (0) return always worked.

The comment about the 'standard code path' was removed as this is
no longer valid.
---
 src/sae.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/src/sae.c b/src/sae.c
index a6b2c582..327277eb 100644
--- a/src/sae.c
+++ b/src/sae.c
@@ -935,7 +935,7 @@ static int sae_verify_confirmed(struct sae_sm *sm, uint16_t trans,
 /*
  * 802.11-2016 - 12.4.8.6.6 Protocol instance behavior - Accepted state
  */
-static bool sae_verify_accepted(struct sae_sm *sm, uint16_t trans,
+static int sae_verify_accepted(struct sae_sm *sm, uint16_t trans,
 					uint16_t status, const uint8_t *frame,
 					size_t len)
 {
@@ -944,14 +944,14 @@ static bool sae_verify_accepted(struct sae_sm *sm, uint16_t trans,
 	/* spec does not specify what to do here, so print and discard */
 	if (trans != SAE_STATE_CONFIRMED) {
 		l_error("received transaction %u in accepted state", trans);
-		return false;
+		return -EBADMSG;
 	}
 
 	if (sm->sync > SAE_SYNC_MAX)
-		return false;
+		return -EBADMSG;
 
 	if (len < 2)
-		return false;
+		return -EBADMSG;
 
 	sc = l_get_le16(frame);
 
@@ -961,14 +961,14 @@ static bool sae_verify_accepted(struct sae_sm *sm, uint16_t trans,
 	 * silently discarded.
 	 */
 	if (sc <= sm->rc || sc == 0xffff)
-		return false;
+		return -EBADMSG;
 
 	/*
 	 * If the verification fails, the received frame shall be silently
 	 * discarded.
 	 */
 	if (!sae_verify_confirm(sm, frame))
-		return false;
+		return -EBADMSG;
 
 	/*
 	 * If the verification succeeds, the Rc variable shall be set to the
@@ -981,11 +981,7 @@ static bool sae_verify_accepted(struct sae_sm *sm, uint16_t trans,
 
 	sae_send_confirm(sm);
 
-	/*
-	 * Since the confirmed needed special processing because of accepted
-	 * state we don't want the standard code path to execute.
-	 */
-	return false;
+	return 0;
 }
 
 static int sae_verify_packet(struct sae_sm *sm, uint16_t trans,
-- 
2.17.1

Re: [PATCH v3 1/3] mpdu: expose mmpdu_header_len

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 211 bytes --]

Hi James,

On 10/21/19 4:01 PM, James Prestwood wrote:
> ---
>   src/mpdu.c | 2 +-
>   src/mpdu.h | 1 +
>   2 files changed, 2 insertions(+), 1 deletion(-)
> 

All applied, thanks.

Regards,
-Denis