net80211: reject mixed plaintext/encrypted fragments
ClosedPublic
Actions

Authored by bz on Jun 6 2021, 10:36 PM.

Details

Reviewers

None

Group Reviewers

wireless

Commits

rG11572d7d7fb9: net80211: reject mixed plaintext/encrypted fragments

Summary

ieee80211_defrag() accepts fragmented 802.11 frames in a protected Wi-Fi
network even when some of the fragments are not encrypted.
Track whether the fragments are encrypted or not and only accept
successive ones if they match the state of the first fragment.

This relates to section 6.3 in the 2021 Usenix "FragAttacks" (Fragment
and Forge: Breaking Wi-Fi Through Frame Aggregation and Fragmentation)
paper.

Submitted by: Mathy Vanhoef (Mathy.Vanhoef kuleuven.be)
Security: CVE-2020-26147
PR: 256118

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 39753
Build 36642: arc lint + arc unit

Event Timeline

bz created this revision.Jun 6 2021, 10:36 PM

Herald added subscribers: melifaro, imp. · View Herald TranscriptJun 6 2021, 10:36 PM

bz requested review of this revision.Jun 6 2021, 10:36 PM

Harbormaster completed remote builds in B39753: Diff 90500.Jun 6 2021, 10:37 PM

See the PR for the original description/patch.

If no one has any (further) comments I'll commit these tomorrow morning (UTC).

From the PR,

I'm not sure if I'm using the best way to track whether a fragment was encrypted, but it gets the job done with a low number of code changes.

I'm not really a fan of tracking the enc state this way, but am not familiar enough with the rest of net80211 to have a better idea and it indeed is not too intrusive. I might add an XXX comment where IEEE80211_FC1_PROTECTED is set that mentions this.

But overall no objection to this change.

sys/net80211/ieee80211_input.c
174	`bool has_decrypted` perhaps?

emaste mentioned this in D30664: net80211: mitigation against A-MSDU design flaw.Sep 29 2021, 1:18 PM

In D30663#726964, @emaste wrote:

From the PR,

I'm not sure if I'm using the best way to track whether a fragment was encrypted, but it gets the job done with a low number of code changes.

I'm not really a fan of tracking the enc state this way, but am not familiar enough with the rest of net80211 to have a better idea and it indeed is not too intrusive. I might add an XXX comment where IEEE80211_FC1_PROTECTED is set that mentions this.

I am not either but adding an mtag for this and all the infrastructure to handle that seems to be overkill.
I was pondering (ab)using other mbuf fields we could at that layer (e.g., PH_loc) but then again that is not easily portable between mbuf implementations I assume.

I am open to alternate suggestions which are cleaner or if one of the two above should be used better?