Bug#512995: [Rt2400-devel] Possible security flaw in ad-hoc probe request processing

[Ben Hutchings]

On Sun, 2009-01-25 at 08:43 -0800, Bryan Batten wrote:
> Ben Hutchings wrote:
> > Package: rt73 Severity: critical Tags: security, upstream
> > 
> > "Aviv" <spring...@gmail.com> wrote on Bugtraq:
> >> Some Ralinktech wireless cards drivers are suffer from integer 
> >> overflow. by sending malformed 802.11 Probe Request packet with
> >> no care about victim's MAC\BSS\SSID can cause to remote code
> >> execution in kernel mode.
> ...
> > pFrame->Octet is an array of signed char and MAX_LEN_OF_SSID
> > expands to a decimal literal which will have type int.  Therefore
> > unsigned values in the range [128, 255] will be treated as values
> > in the range [-128, -1] and will pass the test.
> ...
> Hi Ben,
> 
> Thanks for the info. Do you know if redefining the FRAME_802_11 
> structure in mlme.h so that the Octet member is UCHAR fixes the problem?

I think it probably would, but I'm a bit wary of doing that.

I reviewed sanity.c in the Debian package (CVS snapshot from 2008-06-23
but I don't believe the driver has changed much) and I found only one
more case of signed/unsigned confusion.  My proposed patch is:

--- rt73.orig/Module/sanity.c
+++ rt73/Module/sanity.c
@@ -447,7 +447,7 @@
 
     COPY_MAC_ADDR(pAddr2, pFrame->Hdr.Addr2);
 
-    if ((pFrame->Octet[0] != IE_SSID) || (pFrame->Octet[1] > MAX_LEN_OF_SSID))
+    if ((pFrame->Octet[0] != IE_SSID) || ((UCHAR)pFrame->Octet[1] > 
MAX_LEN_OF_SSID))
     {
         DBGPRINT(RT_DEBUG_TRACE, "PeerProbeReqSanity fail - wrong SSID 
IE(Type=%d,Len=%d)\n",pFrame->Octet[0],pFrame->Octet[1]);
         return FALSE;
@@ -649,8 +649,8 @@
                                        pCfParm->bValid = TRUE;
                                        pCfParm->CfpCount = pEid->Octet[0];
                                        pCfParm->CfpPeriod = pEid->Octet[1];
-                                       pCfParm->CfpMaxDuration = 
pEid->Octet[2] + 256 * pEid->Octet[3];
-                                       pCfParm->CfpDurRemaining = 
pEid->Octet[4] + 256 * pEid->Octet[5];
+                                       pCfParm->CfpMaxDuration = 
(UCHAR)pEid->Octet[2] + 256 * (UCHAR)pEid->Octet[3];
+                                       pCfParm->CfpDurRemaining = 
(UCHAR)pEid->Octet[4] + 256 * (UCHAR)pEid->Octet[5];
                                }
                                else
                                {
--- END ---

(The code for IE_QBSS_LOAD has a similar problem, but it's disabled by
#if 0.)

Ben.

signature.asc
Description: This is a digitally signed message part