Re: iwn/athn/wpi: fix CCMP replay check with HW crypto
Le Fri, 15 May 2020 11:02:46 +0200,
Stefan Sperling a écrit :
> On Fri, May 08, 2020 at 10:53:30AM +0200, Stefan Sperling wrote:
> > So the diff below contains just the reordering fix for drivers using
> > hardware acceleration for WPA.
>
> Is there anybody who wants to OK this?
>
> To recap:
>
> Currently, CCMP replay checking is skipped for aggregated 11n frames
> on drivers which use CCMP hardware acceleration: athn(4) and iwn(4).
>
> This diff makes reply checking in that case possible without false
> positives, by updating the last-seen packet number after the
> aggregated subframes, which may be received out of order, have been
> put back into their intended order by ieee80211_input_ba(). The old
> code only works in the single-frame case for 11a/b/g modes where no
> such legitimate re-transmissions can occur.
>
> > diff refs/heads/master refs/heads/pn
> > blob - 9193ae3c7b5101a86b85b1b3ba48489d75f5150c
> > blob + 8bc189dc146562f7ab0af2f1690fa02676aecfcd
> > --- sys/dev/ic/ar5008.c
> > +++ sys/dev/ic/ar5008.c
> > @@ -794,9 +794,8 @@ ar5008_ccmp_decap(struct athn_softc *sc, struct
> > mbuf * struct ieee80211_frame *wh;
> > struct ieee80211_rx_ba *ba;
> > uint64_t pn, *prsc;
> > - u_int8_t *ivp, *mmie;
> > + u_int8_t *ivp;
> > uint8_t tid;
> > - uint16_t kid;
> > int hdrlen, hasqos;
> > uintptr_t entry;
> >
> > @@ -805,35 +804,8 @@ ar5008_ccmp_decap(struct athn_softc *sc,
> > struct mbuf * ivp = mtod(m, u_int8_t *) + hdrlen;
> >
> > /* find key for decryption */
> > - if (!IEEE80211_IS_MULTICAST(wh->i_addr1)) {
> > - k = >ni_pairwise_key;
> > - } else if ((wh->i_fc[0] & IEEE80211_FC0_TYPE_MASK) !=
> > - IEEE80211_FC0_TYPE_MGT) {
> > - /* retrieve group data key id from IV field */
> > - /* check that IV field is present */
> > - if (m->m_len < hdrlen + 4)
> > - return 1;
> > - kid = ivp[3] >> 6;
> > - k = >ic_nw_keys[kid];
> > - } else {
> > - /* retrieve integrity group key id from MMIE */
> > - if (m->m_len < sizeof(*wh) + IEEE80211_MMIE_LEN) {
> > - return 1;
> > - }
> > - /* it is assumed management frames are contiguous
> > */
> > - mmie = (u_int8_t *)wh + m->m_len -
> > IEEE80211_MMIE_LEN;
> > - /* check that MMIE is valid */
> > - if (mmie[0] != IEEE80211_ELEMID_MMIE || mmie[1] !=
> > 16) {
> > - return 1;
> > - }
> > - kid = LE_READ_2([2]);
> > - if (kid != 4 && kid != 5) {
> > - return 1;
> > - }
> > - k = >ic_nw_keys[kid];
> > - }
> > -
> > - if (k->k_cipher != IEEE80211_CIPHER_CCMP)
> > + k = ieee80211_get_rxkey(ic, m, ni);
> > + if (k == NULL || k->k_cipher != IEEE80211_CIPHER_CCMP)
> > return 1;
> >
> > /* Sanity checks to ensure this is really a key we
> > installed. */ @@ -853,7 +825,7 @@ ar5008_ccmp_decap(struct
> > athn_softc *sc, struct mbuf * hasqos = ieee80211_has_qos(wh);
> > tid = hasqos ? ieee80211_get_qos(wh) & IEEE80211_QOS_TID :
> > 0; ba = hasqos ? >ni_rx_ba[tid] : NULL;
> > - prsc = >k_rsc[0];
> > + prsc = >k_rsc[tid];
> >
> > /* Extract the 48-bit PN from the CCMP header. */
> > pn = (uint64_t)ivp[0] |
> > @@ -863,30 +835,12 @@ ar5008_ccmp_decap(struct athn_softc *sc,
> > struct mbuf * (uint64_t)ivp[6] << 32 |
> > (uint64_t)ivp[7] << 40;
> > if (pn <= *prsc) {
> > - if (hasqos && ba->ba_state == IEEE80211_BA_AGREED)
> > {
> > - /*
> > -* This is an A-MPDU subframe.
> > -* Such frames may be received out of
> > order due to
> > -* legitimate retransmissions of failed
> > subframes
> > -* in previous A-MPDUs. Duplicates will be
> > handled
> > -* in ieee80211_inputm() as part of A-MPDU
> > reordering.
> > -*
> > -* XXX TODO We can probably do better than
> > this! Store
> > -* re-ordered PN in BA agreement state and
> > check it?
> > -*/
> > - } else {
> > - ic->ic_stats.is_ccmp_replays++;
> > - return 1;
> > - }
> > + ic->ic_stats.is_ccmp_replays++;
> > + return 1;
> > }
> > - /* Update last seen packet number. */
> > - *prsc = pn;
> > + /* Last seen packet number is updated in
> > ieee80211_inputm(). */
> > - /* Clear Protected bit and strip IV. */
> > - wh->i_fc[1] &= ~IEEE80211_FC1_PROTECTED;
> > - memmove(mtod(m, caddr_t) + IEEE80211_CCMP_HDRLEN, wh,
> > hdrlen);
> > - m_adj(m, IEEE80211_CCMP_HDRLEN);
> > - /* Strip MIC. */
> > + /* Strip MIC. IV will be stripped by ieee80211_inputm(). */
> > m_adj(m, -IEEE80211_CCMP_MICLEN);
> > return 0;
> > }
> > blob - 6b6331d167a881a526b9d0a30f7f452882fee19a
Re: iwn/athn/wpi: fix CCMP replay check with HW crypto
On Fri, May 08, 2020 at 10:53:30AM +0200, Stefan Sperling wrote:
> So the diff below contains just the reordering fix for drivers using
> hardware acceleration for WPA.
Is there anybody who wants to OK this?
To recap:
Currently, CCMP replay checking is skipped for aggregated 11n frames on
drivers which use CCMP hardware acceleration: athn(4) and iwn(4).
This diff makes reply checking in that case possible without false positives,
by updating the last-seen packet number after the aggregated subframes, which
may be received out of order, have been put back into their intended order by
ieee80211_input_ba(). The old code only works in the single-frame case for
11a/b/g modes where no such legitimate re-transmissions can occur.
> diff refs/heads/master refs/heads/pn
> blob - 9193ae3c7b5101a86b85b1b3ba48489d75f5150c
> blob + 8bc189dc146562f7ab0af2f1690fa02676aecfcd
> --- sys/dev/ic/ar5008.c
> +++ sys/dev/ic/ar5008.c
> @@ -794,9 +794,8 @@ ar5008_ccmp_decap(struct athn_softc *sc, struct mbuf *
> struct ieee80211_frame *wh;
> struct ieee80211_rx_ba *ba;
> uint64_t pn, *prsc;
> - u_int8_t *ivp, *mmie;
> + u_int8_t *ivp;
> uint8_t tid;
> - uint16_t kid;
> int hdrlen, hasqos;
> uintptr_t entry;
>
> @@ -805,35 +804,8 @@ ar5008_ccmp_decap(struct athn_softc *sc, struct mbuf *
> ivp = mtod(m, u_int8_t *) + hdrlen;
>
> /* find key for decryption */
> - if (!IEEE80211_IS_MULTICAST(wh->i_addr1)) {
> - k = >ni_pairwise_key;
> - } else if ((wh->i_fc[0] & IEEE80211_FC0_TYPE_MASK) !=
> - IEEE80211_FC0_TYPE_MGT) {
> - /* retrieve group data key id from IV field */
> - /* check that IV field is present */
> - if (m->m_len < hdrlen + 4)
> - return 1;
> - kid = ivp[3] >> 6;
> - k = >ic_nw_keys[kid];
> - } else {
> - /* retrieve integrity group key id from MMIE */
> - if (m->m_len < sizeof(*wh) + IEEE80211_MMIE_LEN) {
> - return 1;
> - }
> - /* it is assumed management frames are contiguous */
> - mmie = (u_int8_t *)wh + m->m_len - IEEE80211_MMIE_LEN;
> - /* check that MMIE is valid */
> - if (mmie[0] != IEEE80211_ELEMID_MMIE || mmie[1] != 16) {
> - return 1;
> - }
> - kid = LE_READ_2([2]);
> - if (kid != 4 && kid != 5) {
> - return 1;
> - }
> - k = >ic_nw_keys[kid];
> - }
> -
> - if (k->k_cipher != IEEE80211_CIPHER_CCMP)
> + k = ieee80211_get_rxkey(ic, m, ni);
> + if (k == NULL || k->k_cipher != IEEE80211_CIPHER_CCMP)
> return 1;
>
> /* Sanity checks to ensure this is really a key we installed. */
> @@ -853,7 +825,7 @@ ar5008_ccmp_decap(struct athn_softc *sc, struct mbuf *
> hasqos = ieee80211_has_qos(wh);
> tid = hasqos ? ieee80211_get_qos(wh) & IEEE80211_QOS_TID : 0;
> ba = hasqos ? >ni_rx_ba[tid] : NULL;
> - prsc = >k_rsc[0];
> + prsc = >k_rsc[tid];
>
> /* Extract the 48-bit PN from the CCMP header. */
> pn = (uint64_t)ivp[0] |
> @@ -863,30 +835,12 @@ ar5008_ccmp_decap(struct athn_softc *sc, struct mbuf *
>(uint64_t)ivp[6] << 32 |
>(uint64_t)ivp[7] << 40;
> if (pn <= *prsc) {
> - if (hasqos && ba->ba_state == IEEE80211_BA_AGREED) {
> - /*
> - * This is an A-MPDU subframe.
> - * Such frames may be received out of order due to
> - * legitimate retransmissions of failed subframes
> - * in previous A-MPDUs. Duplicates will be handled
> - * in ieee80211_inputm() as part of A-MPDU reordering.
> - *
> - * XXX TODO We can probably do better than this! Store
> - * re-ordered PN in BA agreement state and check it?
> - */
> - } else {
> - ic->ic_stats.is_ccmp_replays++;
> - return 1;
> - }
> + ic->ic_stats.is_ccmp_replays++;
> + return 1;
> }
> - /* Update last seen packet number. */
> - *prsc = pn;
> + /* Last seen packet number is updated in ieee80211_inputm(). */
>
> - /* Clear Protected bit and strip IV. */
> - wh->i_fc[1] &= ~IEEE80211_FC1_PROTECTED;
> - memmove(mtod(m, caddr_t) + IEEE80211_CCMP_HDRLEN, wh, hdrlen);
> - m_adj(m, IEEE80211_CCMP_HDRLEN);
> - /* Strip MIC. */
> + /* Strip MIC. IV will be stripped by ieee80211_inputm(). */
> m_adj(m, -IEEE80211_CCMP_MICLEN);
> return 0;
> }
> blob - 6b6331d167a881a526b9d0a30f7f452882fee19a
> blob + 620cbd4c138516398a4683d83c1b6bf8aac57c82
> --- sys/dev/pci/if_iwn.c
> +++ sys/dev/pci/if_iwn.c
> @@ -1936,47 +1936,13 @@ iwn_ccmp_decap(struct
Re: iwn/athn/wpi: fix CCMP replay check with HW crypto
On Fri, May 01, 2020 at 06:05:47PM +0200, Stefan Sperling wrote:
> The following diff moves the Packet Number check out of affected drivers
> into ieee80211_inputm() so the check can be performed after frames have
> been re-ordered.
Here is a new version of this diff.
I realized I made a mistake in my reasoning here:
> - Fix a wrong assumption made about reply counters and TIDs. We only
>announce support for one replay counter per key, and never initialize
>counters other than k_rsc[0]. Some code was accessing k_rsc[tid] based
>on the TID value in the frame header. Based on the fact that counters
>at offsets other than zero are not initialized, and on my understanding
>of the 802.11-2012 spec, this is wrong. Multiple counters are only used
>if support for them is announced in an RSN capability field, and we do
>not announce this.
Here's my error: In fact we do announce whatever the peer has announced,
by echoing back the relevant setting in the peer's RSN capability field.
My misunderstanding came about due to a limited sample size of APs.
An AP which advertises support for multiple counters sees a matching
response from us and works fine with our current code.
>We also use TIDs only to correlate frames with an
>existing block ack agreement. The multi-replay counter case assumes
>instead that TIDs are used for purposes of real OoS, which we do not
>implement. Real QoS could lead to out-of-order transmissions based
>on the priority assigned to the data contained in a frame, so it makes
>sense to maintain one replay counter per priority with real QoS.
What's really happening in our implementation is that we maintain an
array of up to 16 replay counters per key. All counters start at zero
which is the lowest possible value since they are unsigned. No matter how
many counters a peer advertises, we support any configuration the peer wants,
up to the max number of counters allowed by the spec (16, not 8 as I wrote
in comments in my earlier diff). The peer then sends frames with particular
TIDs using the appropriate replay counter value, and we use our corresponding
per-TID replay counter if support for it was advertised by the peer.
For the rest we use the default replay counter (TID 0).
So the diff below contains just the reordering fix for drivers using
hardware acceleration for WPA.
diff refs/heads/master refs/heads/pn
blob - 9193ae3c7b5101a86b85b1b3ba48489d75f5150c
blob + 8bc189dc146562f7ab0af2f1690fa02676aecfcd
--- sys/dev/ic/ar5008.c
+++ sys/dev/ic/ar5008.c
@@ -794,9 +794,8 @@ ar5008_ccmp_decap(struct athn_softc *sc, struct mbuf *
struct ieee80211_frame *wh;
struct ieee80211_rx_ba *ba;
uint64_t pn, *prsc;
- u_int8_t *ivp, *mmie;
+ u_int8_t *ivp;
uint8_t tid;
- uint16_t kid;
int hdrlen, hasqos;
uintptr_t entry;
@@ -805,35 +804,8 @@ ar5008_ccmp_decap(struct athn_softc *sc, struct mbuf *
ivp = mtod(m, u_int8_t *) + hdrlen;
/* find key for decryption */
- if (!IEEE80211_IS_MULTICAST(wh->i_addr1)) {
- k = >ni_pairwise_key;
- } else if ((wh->i_fc[0] & IEEE80211_FC0_TYPE_MASK) !=
- IEEE80211_FC0_TYPE_MGT) {
- /* retrieve group data key id from IV field */
- /* check that IV field is present */
- if (m->m_len < hdrlen + 4)
- return 1;
- kid = ivp[3] >> 6;
- k = >ic_nw_keys[kid];
- } else {
- /* retrieve integrity group key id from MMIE */
- if (m->m_len < sizeof(*wh) + IEEE80211_MMIE_LEN) {
- return 1;
- }
- /* it is assumed management frames are contiguous */
- mmie = (u_int8_t *)wh + m->m_len - IEEE80211_MMIE_LEN;
- /* check that MMIE is valid */
- if (mmie[0] != IEEE80211_ELEMID_MMIE || mmie[1] != 16) {
- return 1;
- }
- kid = LE_READ_2([2]);
- if (kid != 4 && kid != 5) {
- return 1;
- }
- k = >ic_nw_keys[kid];
- }
-
- if (k->k_cipher != IEEE80211_CIPHER_CCMP)
+ k = ieee80211_get_rxkey(ic, m, ni);
+ if (k == NULL || k->k_cipher != IEEE80211_CIPHER_CCMP)
return 1;
/* Sanity checks to ensure this is really a key we installed. */
@@ -853,7 +825,7 @@ ar5008_ccmp_decap(struct athn_softc *sc, struct mbuf *
hasqos = ieee80211_has_qos(wh);
tid = hasqos ? ieee80211_get_qos(wh) & IEEE80211_QOS_TID : 0;
ba = hasqos ? >ni_rx_ba[tid] : NULL;
- prsc = >k_rsc[0];
+ prsc = >k_rsc[tid];
/* Extract the 48-bit PN from the CCMP header. */
pn = (uint64_t)ivp[0] |
@@ -863,30 +835,12 @@ ar5008_ccmp_decap(struct athn_softc *sc, struct mbuf *
(uint64_t)ivp[6] << 32 |
(uint64_t)ivp[7] << 40;
Re: iwn/athn/wpi: fix CCMP replay check with HW crypto
On Fri, May 01, 2020 at 08:06:05PM +, Kevin Chadwick wrote: > On 2020-05-01 16:05, Stefan Sperling wrote: > > The CCMP header contains a nonce, > > which is incremented by the transmitter whenever it encrypts a new frame. > > I assume there isn't opportunity to set the nonce to a 128 bit random one. It > would avoid a lot of risk with the likelihood of collisions being > probalistically guaranteed in all circumstances, rather than absolutely > guaranteed to avoid collisions, in some? No, this cannot be changed. (Unless you are willing to break interop with other 802.11 implementations and all the hardware/firmware our drivers support ;-)
Re: iwn/athn/wpi: fix CCMP replay check with HW crypto
On 2020-05-01 16:05, Stefan Sperling wrote: > The CCMP header contains a nonce, > which is incremented by the transmitter whenever it encrypts a new frame. I assume there isn't opportunity to set the nonce to a 128 bit random one. It would avoid a lot of risk with the likelihood of collisions being probalistically guaranteed in all circumstances, rather than absolutely guaranteed to avoid collisions, in some?
iwn/athn/wpi: fix CCMP replay check with HW crypto
This diff needs testing in particular on: athn(4), iwn(4), wpi(4)
I have tested on iwn(4) and athn(4) so far.
Testing with other drivers would be good, too, to ensure that no
regressions are introduced for the software crypto case.
Drivers which offload CCMP decryption to hardware contain a check for
replayed frames. This check is simple: The CCMP header contains a nonce,
which is incremented by the transmitter whenever it encrypts a new frame.
This nonce is known as the "Packet Number" in CCMP (WPA2) and as the
"Transmit Sequence Counter" in TKIP (WPA1). If an encrypted frame is
received which uses a nonce lower than the most recently received nonce,
then the frame can be assumed to be a replay and is discarded.
This is important to defend against replay attacks on WPA.
The way our drivers implement this check doesn't work with block ack.
This is because a driver-side check must assume that frames arrive in order.
With block ack, frames may arrive out of order, and they are put back into
the proper order in ieee80211_inputm() by checking their sequence number
(a separate number from the nonce discussed above).
The 802.11 standard says that the Packet Number should be checked *after*
re-ordering frames back into their proper sequence.
In our drivers, this check can only happen before re-ordering and hence
legitimate out-of-order frame transmissions would be discarded.
For this reason, we currently skip the replay check entirely if block ack
and hardware decryption are used together. Which makes things work but is
obviously not good for security.
This problem only affects hardware crypto because software decryption
happens after re-ordering. I would like to fix this problem before adding
CCMP hardware offload support to more drivers.
The following diff moves the Packet Number check out of affected drivers
into ieee80211_inputm() so the check can be performed after frames have
been re-ordered.
Drivers no longer update the expected replay counter value, but they can
still read its current value to discard replayed frames early.
This simplifies CCMP input handling in affected drivers significantly.
Drivers which only do WEP in hardware don't need any change.
While here:
- Implement ieee80211_get_rxkey(); a prototype already existed but now
we actually need an implementation of this to avoid code duplication
within net80211 and some drivers.
- Fix a wrong assumption made about reply counters and TIDs. We only
announce support for one replay counter per key, and never initialize
counters other than k_rsc[0]. Some code was accessing k_rsc[tid] based
on the TID value in the frame header. Based on the fact that counters
at offsets other than zero are not initialized, and on my understanding
of the 802.11-2012 spec, this is wrong. Multiple counters are only used
if support for them is announced in an RSN capability field, and we do
not announce this. We also use TIDs only to correlate frames with an
existing block ack agreement. The multi-replay counter case assumes
instead that TIDs are used for purposes of real OoS, which we do not
implement. Real QoS could lead to out-of-order transmissions based
on the priority assigned to the data contained in a frame, so it makes
sense to maintain one replay counter per priority with real QoS.
diff refs/heads/master refs/heads/pn
blob - 9193ae3c7b5101a86b85b1b3ba48489d75f5150c
blob + 5c17a7d065c622cdde56ccc81baa1859df93404f
--- sys/dev/ic/ar5008.c
+++ sys/dev/ic/ar5008.c
@@ -792,12 +792,9 @@ ar5008_ccmp_decap(struct athn_softc *sc, struct mbuf *
struct ieee80211com *ic = >sc_ic;
struct ieee80211_key *k;
struct ieee80211_frame *wh;
- struct ieee80211_rx_ba *ba;
uint64_t pn, *prsc;
- u_int8_t *ivp, *mmie;
- uint8_t tid;
- uint16_t kid;
- int hdrlen, hasqos;
+ u_int8_t *ivp;
+ int hdrlen;
uintptr_t entry;
wh = mtod(m, struct ieee80211_frame *);
@@ -805,35 +802,8 @@ ar5008_ccmp_decap(struct athn_softc *sc, struct mbuf *
ivp = mtod(m, u_int8_t *) + hdrlen;
/* find key for decryption */
- if (!IEEE80211_IS_MULTICAST(wh->i_addr1)) {
- k = >ni_pairwise_key;
- } else if ((wh->i_fc[0] & IEEE80211_FC0_TYPE_MASK) !=
- IEEE80211_FC0_TYPE_MGT) {
- /* retrieve group data key id from IV field */
- /* check that IV field is present */
- if (m->m_len < hdrlen + 4)
- return 1;
- kid = ivp[3] >> 6;
- k = >ic_nw_keys[kid];
- } else {
- /* retrieve integrity group key id from MMIE */
- if (m->m_len < sizeof(*wh) + IEEE80211_MMIE_LEN) {
- return 1;
- }
- /* it is assumed management frames are contiguous */
- mmie = (u_int8_t *)wh + m->m_len - IEEE80211_MMIE_LEN;
- /* check that MMIE is valid */
-
