[PATCH v8 0/1] staging: rtl8723bs: fix missing frame length checks in OnAuth()

[PATCH v8 0/1] staging: rtl8723bs: fix missing frame length checks in OnAuth()

[Alexandru Hossu]

v8, addressing the sashiko review comments on v7.

The Challenge Text IE checks from v7 patch 1 and the OnAuthClient
frame length checks from v7 patch 2 are already in your tree:

  9d6d63cf3c37 ("staging: rtl8723bs: fix heap overflow in OnAuthClient shared key path")
  6ad4395aab1e ("staging: rtl8723bs: fix missing frame length checks in OnAuthClient")

This single patch adds the remaining fix: frame length guards for the
AP-mode handler OnAuth().

What changed in v8 (compared to v7 patch 2/2):

  - The len < WLAN_HDR_A3_LEN guard uses return _FAIL instead of goto
    auth_fail.  The auth_fail block calls memcpy(pstat->hwaddr, sa, ...)
    to build a rejection frame.  At that point sa has not been
    initialised, so goto auth_fail would copy stack garbage into the
    rejection frame's destination address.  Since we cannot build a
    meaningful rejection without knowing the sender, return _FAIL is
    the correct choice.

  - A new guard is added before the iv[3] key-index read inside the
    GetPrivacy() branch.  When the Privacy bit is set, the code computes
    iv = pframe + WLAN_HDR_A3_LEN and immediately reads iv[3] without
    verifying that len >= WLAN_HDR_A3_LEN + 4.  This was a new OOB read
    path not present in any earlier version of the patch.

  - The len < WLAN_HDR_A3_LEN + offset + 4 check now sets
    status = WLAN_STATUS_UNSPECIFIED_FAILURE before goto auth_fail so
    that the rejection frame carries a defined status code rather than
    whatever happened to be on the stack.

Alexandru Hossu (1):
  staging: rtl8723bs: fix missing frame length checks in OnAuth()

 drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
--
2.53.0

[PATCH v8 1/1] staging: rtl8723bs: fix missing frame length checks in OnAuth()

[Alexandru Hossu]

Three out-of-bounds read paths caused by missing frame length guards in
the AP-mode authentication handler OnAuth():

1. OnAuth() reads GetAddr2Ptr(pframe) without verifying the frame is at
   least WLAN_HDR_A3_LEN bytes long.

   The first operation on pframe after the AP-state guard is
   GetAddr2Ptr(pframe), which reads 6 bytes at offset 10..15 (Addr2).
   If the received frame is shorter than WLAN_HDR_A3_LEN (24 bytes),
   this reads past the end of the frame buffer.  Add:
     if (len < WLAN_HDR_A3_LEN) return _FAIL;

   Using return _FAIL rather than goto auth_fail: the auth_fail block
   calls memcpy(pstat->hwaddr, sa, ETH_ALEN) to build a rejection
   frame, but sa has not been set at this point.

2. OnAuth() reads iv[3] inside the GetPrivacy() branch without checking
   the frame is long enough to contain the IV.

   When the Privacy bit is set, the code sets iv = pframe + WLAN_HDR_A3_LEN
   and immediately reads iv[3] to extract the key index.  If the frame is
   shorter than WLAN_HDR_A3_LEN + 4 bytes, this reads past the frame
   buffer.  Add:
     if (len < WLAN_HDR_A3_LEN + 4) return _FAIL;

   Using return _FAIL: pstat has not been looked up yet and status is
   uninitialised, so goto auth_fail would send a malformed rejection frame.

3. OnAuth() reads the algorithm and sequence fields at pframe +
   WLAN_HDR_A3_LEN + offset + {0,2} without verifying those offsets
   are within the frame.

   offset is 0 for open-system and 4 for WEP-encapsulated frames.  The
   reads at offset+0 and offset+2 are both 2-byte, so the last byte
   accessed is at WLAN_HDR_A3_LEN + offset + 3.  Add:
     if (len < WLAN_HDR_A3_LEN + offset + 4) goto auth_fail;

   At this point sa is valid and pstat is NULL, so goto auth_fail is safe.
   WLAN_STATUS_UNSPECIFIED_FAILURE is set first to avoid sending a garbage
   rejection status code.

Fixes: 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
Cc: stable@vger.kernel.org
Signed-off-by: Alexandru Hossu <hossu.alexandru@gmail.com>
---
Changes in v8:
  - Change if (len < WLAN_HDR_A3_LEN) from goto auth_fail to return _FAIL:
    sa is uninitialised at that point and auth_fail dereferences it via
    memcpy(pstat->hwaddr, sa, ETH_ALEN) to build the rejection frame
    (sashiko review of v7).
  - Add if (len < WLAN_HDR_A3_LEN + 4) return _FAIL; before iv[3] access
    in the GetPrivacy() branch: the missing length guard was a new OOB
    read path not caught in any earlier version (sashiko review of v7).
  - Set status = WLAN_STATUS_UNSPECIFIED_FAILURE before the
    if (len < WLAN_HDR_A3_LEN + offset + 4) goto auth_fail check to avoid
    sending an uninitialised status code in the rejection frame.

Changes in v7:
  - Add frame length checks for OnAuth(): guard before GetAddr2Ptr
    (len < WLAN_HDR_A3_LEN) and guard before algorithm/seq reads
    (len < WLAN_HDR_A3_LEN + offset + 4).
  - Correct commit message: rtw_get_ie() uses signed int limit and
    returns NULL when limit < 2, so the prior unsigned-underflow claim
    was incorrect (sashiko review of v6).

Changes in v6:
  - Add frame length checks for OnAuthClient(): guard before get_da()
    and guard before seq/status reads.

 drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index 68ce422305ed..c3f2d4e5f6a7 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -687,6 +687,9 @@ unsigned int OnAuth(struct adapter *padapter, union recv_frame *precv_frame)
 	if ((pmlmeinfo->state&0x03) != WIFI_FW_AP_STATE)
 		return _FAIL;
 
+	if (len < WLAN_HDR_A3_LEN)
+		return _FAIL;
+
 	sa = GetAddr2Ptr(pframe);
 
 	auth_mode = psecuritypriv->dot11AuthAlgrthm;
@@ -698,6 +701,9 @@ unsigned int OnAuth(struct adapter *padapter, union recv_frame *precv_frame)
 		prxattrib->hdrlen = WLAN_HDR_A3_LEN;
 		prxattrib->encrypt = _WEP40_;
 
+		if (len < WLAN_HDR_A3_LEN + 4)
+			return _FAIL;
+
 		iv = pframe+prxattrib->hdrlen;
 		prxattrib->key_index = ((iv[3]>>6)&0x3);
 
@@ -709,6 +715,11 @@ unsigned int OnAuth(struct adapter *padapter, union recv_frame *precv_frame)
 		offset = 4;
 	}
 
+	if (len < WLAN_HDR_A3_LEN + offset + 4) {
+		status = WLAN_STATUS_UNSPECIFIED_FAILURE;
+		goto auth_fail;
+	}
+
 	algorithm = le16_to_cpu(*(__le16 *)((SIZE_PTR)pframe + WLAN_HDR_A3_LEN + offset));
 	seq	= le16_to_cpu(*(__le16 *)((SIZE_PTR)pframe + WLAN_HDR_A3_LEN + offset + 2));
 
--
2.53.0