Ticket #1270 (closed defect: fixed)

Opened 1 year ago

Last modified 3 months ago

Beacon Interval may be zero; it is used as a divisor

Reported by: md.ahmad@airtightnetworks.net Assigned to:
Priority: critical Milestone: version 0.9.5
Component: madwifi: driver Version: trunk
Keywords: Cc:
Patch is attached: 0

Description

In the madwifi/ath component if_ath.c handles the beacon configuration related initialization task both for clients and aps in the function ath_beacon_config(). The function uses macro "howmany" which performs divide operation. The macro is used without ensuring that the argument(denominator 'intval') could be zero. The divide by zero condition can be triggered externally using a malformed packet.

This bug has been discovered as focussed analysis of publicly available madwifi driver with a motivation to make it most robust and safest driver to use in the public domain.

Attachments

patch-ticket-1270.diff (3.6 kB) - added by md.ahmad@airtightnetworks.net on 04/18/07 14:52:48.
The patch is created for the branch madwifi-hal-0.9.30.10-r2257
madwifi-divzero.diff (4.5 kB) - added by mentor on 04/20/07 22:02:28.
Proposed Fix
beacon_interval_range-fix.patch (4.8 kB) - added by mrenzmann on 05/14/07 12:44:25.
Third approach, extending mentor's patch with suggestions from scottr

Change History

(follow-up: ↓ 2 ) 04/18/07 10:29:06 changed by kelmo

Did your analysis also lead to a patch to fix the found problem?

(in reply to: ↑ 1 ; follow-up: ↓ 3 ) 04/18/07 13:11:58 changed by md.ahmad@airtightnetworks.net

Replying to kelmo:

Did your analysis also lead to a patch to fix the found problem?

Hi Kelmo! Nice to see your immediate response. I am running the driver wih my fix and it is working. I can upload the patch for the same. Can you tell me how madwifi developmet team is going to test that ? Shall I also assume that the problem reported in this ticket as been verified by the team?

Thanks Sohail AirTight? Networks Inc.

(in reply to: ↑ 2 ) 04/18/07 13:23:23 changed by kelmo

Replying to md.ahmad@airtightnetworks.net:

Hi Kelmo! Nice to see your immediate response. I am running the driver wih my fix and it is working. I can upload the patch for the same.

Please do. Don't forget to sign it off.

Can you tell me how madwifi developmet team is going to test that ?

By reading, understanding and applying/using your patch.

Shall I also assume that the problem reported in this ticket as been verified by the team?

Not AFAIK. Thats what this ticket is for ;-)

Thanks, Kel.

04/18/07 14:52:48 changed by md.ahmad@airtightnetworks.net

  • attachment patch-ticket-1270.diff added.

The patch is created for the branch madwifi-hal-0.9.30.10-r2257

04/18/07 14:58:28 changed by anonymous

The attach patch tries to fix the possibility of divide by zero condition in the driver. The patch is submitted for the branch madwifi-hal-0.9.30.10-r2257 Signed-off-by: Md Sohail Ahmad <md.ahmad@airtightnetworks.net>, AirTight? Networks Inc.

(follow-up: ↓ 6 ) 04/18/07 23:25:48 changed by mentor

  • summary changed from Possibility of "divide by zero" condition in the driver code ignored, to Beacon Interval may be zero; it is used as a divisor.

I'd prefer it if this was fixed at the input stage (i.e. verify beacon length when we process the packet). The 802.11 specifications are annoyingly silent on what we should do if the beacon interval is zero, especially as this is clearly an invalid value.

(in reply to: ↑ 5 ; follow-ups: ↓ 7 ↓ 8 ) 04/19/07 05:49:10 changed by anonymous

Replying to mentor:

I'd prefer it if this was fixed at the input stage (i.e. verify beacon length when we process the packet). The 802.11 specifications are annoyingly silent on what we should do if the beacon interval is zero, especially as this is clearly an invalid value.

es it is he beacon interval field that can be zero and hence it was creating the condiion of divide by zero. I hope the impact would be clear now as it can crash a client which is trying to associate with AP emitting beacon with beacon interval zero. By verify Beacon length, you mean beacon frame length of beacon inerval. If beacon interval than i would say yes the problem could have solved that time only. Can you tell me if the patch submitted is ok ? With your point i can assume that the condition can be moved from ath_beacon_config function o a place where beacon parsing is happening.

(in reply to: ↑ 6 ) 04/19/07 05:53:01 changed by md.ahmad@airtightnetworks.net

Replying to anonymous:

Replying to mentor:

I'd prefer it if this was fixed at the input stage (i.e. verify beacon length when we process the packet). The 802.11 specifications are annoyingly silent on what we should do if the beacon interval is zero, especially as this is clearly an invalid value.

yes it is the beacon interval field that can be zero and hence it was creating the condiion of divide by zero. I hope the impact would be clear now as it can crash a client which is trying to associate with AP emitting beacon with beacon interval zero. By verifying Beacon length, you mean beacon frame length of beacon interval. If beacon interval than i would say yes the problem could have solved that time only. Can you tell me if the patch submitted is ok ? With your point i can assume that the condition introduced in the patch to solve the problem can be moved from ath_beacon_config function to a place where beacon parsing is happening.

(in reply to: ↑ 6 ; follow-up: ↓ 9 ) 04/19/07 16:39:09 changed by mentor

Replying to anonymous: That should be verify the Beacon Interval when we process the packets; I.e. Probe Responses, Beacons and Association Requests, Responses in net80211/ieee80211_input.c.

(in reply to: ↑ 8 ) 04/19/07 16:46:42 changed by md.ahmad@airtightnetworks.net

Replying to mentor:

Replying to anonymous: That should be verify the Beacon Interval when we process the packets; I.e. Probe Responses, Beacons and Association Requests, Responses in net80211/ieee80211_input.c.

How soon madwifi dev team is going to release working patch that would solve the problem ? I found huge impact on both Linux and windows based clients which are using madwifi based drivers? Thanks Sohail

04/20/07 22:02:28 changed by mentor

  • attachment madwifi-divzero.diff added.

Proposed Fix

04/20/07 22:04:28 changed by mentor

Here is how I propose to fix the bug; however, I'm not sure what range of values is valid. The spec. says that a full uint16 is valid, but clearly this is not true, so I'm not sure where to put the values.

05/14/07 12:44:25 changed by mrenzmann

  • attachment beacon_interval_range-fix.patch added.

Third approach, extending mentor's patch with suggestions from scottr

05/14/07 12:50:51 changed by mrenzmann

  • milestone set to version 0.9.4.

Attached is a combination of mentor's patch and some suggestions from scottr. Scott has checked which value range is accepted in FreeBSD, we now follow that (means: IEEE80211_BINTVAL_MAX is increased to 1000). In addition, we now make sure that beacon intervals seen during scans are in an acceptable range.

It seems this patch now is pretty complete. Please review and immediately rise any objections you might have.

05/18/07 06:59:24 changed by mrenzmann

  • status changed from new to closed.
  • resolution set to fixed.

Committed in r2348.

05/23/07 11:46:08 changed by mrenzmann

This is also fixed in security release v0.9.3.1 with a combination of changesets r2272 and r2348.

02/11/08 06:17:17 changed by mrenzmann

  • milestone changed from version 0.9.4 to version 0.9.5.

Add/Change #1270 (Beacon Interval may be zero; it is used as a divisor)