Re: [PATCH] mac80211/wpa: use constant time memory comparison for MACs

[Johannes Berg <johannes@sipsolutions.net>]

On Sun, 2017-06-18 at 23:31 +0300, Emmanuel Grumbach wrote:
> On Sun, Jun 18, 2017 at 10:18 PM, Jason A. Donenfeld <Jason@zx2c4.com
> > wrote:
> > Otherwise, we enable all sorts of forgeries via timing attack.
> 
> crypto_memneq's description says:
[...]
> > ---
> > Here's the backport for 3.18.

Yeah, not sure what happened here, but ...

> >  #include "ieee80211_i.h"
> >  #include "michael.h"
> > @@ -150,7 +151,7 @@ ieee80211_rx_h_michael_mic_verify(struct
> > ieee80211_rx_data *rx)
> >         data_len = skb->len - hdrlen - MICHAEL_MIC_LEN;
> >         key = &rx->key-
> > >conf.key[NL80211_TKIP_DATA_OFFSET_RX_MIC_KEY];
> >         michael_mic(key, hdr, data, data_len, mic);
> > -       if (memcmp(mic, data + data_len, MICHAEL_MIC_LEN) != 0)
> > +       if (crypto_memneq(mic, data + data_len, MICHAEL_MIC_LEN) !=
> > 0)
> >                 goto mic_fail;

This is obviously wrong and not like that in the original,

> >         /* remove Michael MIC from payload */
> > @@ -520,7 +521,7 @@ ieee80211_crypto_ccmp_decrypt(struct
> > ieee80211_rx_data *rx)
> > 
> >         queue = rx->security_idx;
> > 
> > -       if (memcmp(pn, key->u.ccmp.rx_pn[queue],
> > IEEE80211_CCMP_PN_LEN) <= 0) {
> > +       if (crypto_memneq(pn, key->u.ccmp.rx_pn[queue],
> > IEEE80211_CCMP_PN_LEN) <= 0) {
> >                 key->u.ccmp.replays++;
> >                 return RX_DROP_UNUSABLE;
> >         }

this isn't in the original at all, and clearly shouldn't be here,

> > @@ -771,7 +772,7 @@ ieee80211_crypto_aes_cmac_decrypt(struct
> > ieee80211_rx_data *rx)
> >                 bip_aad(skb, aad);
> >                 ieee80211_aes_cmac(key->u.aes_cmac.tfm, aad,
> >                                    skb->data + 24, skb->len - 24,
> > mic);
> > -               if (memcmp(mic, mmie->mic, sizeof(mmie->mic)) != 0)
> > {
> > +               if (crypto_memneq(mic, mmie->mic, sizeof(mmie-
> > >mic)) != 0) {

and this is just as wrong as the first one.

johannes