Otherwise, we enable all sorts of forgeries via timing attack.
Signed-off-by: Jason A. Donenfeld <[email protected]>
Cc: Johannes Berg <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Johannes Berg <[email protected]>
---
Here's the backport for 3.18.
net/mac80211/wpa.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/net/mac80211/wpa.c b/net/mac80211/wpa.c
index 983527a4c1ab..49592c7e4199 100644
--- a/net/mac80211/wpa.c
+++ b/net/mac80211/wpa.c
@@ -16,6 +16,7 @@
#include <asm/unaligned.h>
#include <net/mac80211.h>
#include <crypto/aes.h>
+#include <crypto/algapi.h>
#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;
/* 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;
}
@@ -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) {
key->u.aes_cmac.icverrors++;
return RX_DROP_UNUSABLE;
}
--
2.13.1
On Mon, Jun 19, 2017 at 06:44:06PM +0200, Jason A. Donenfeld wrote:
> Otherwise, we enable all sorts of forgeries via timing attack.
>
> Signed-off-by: Jason A. Donenfeld <[email protected]>
> Cc: Johannes Berg <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Johannes Berg <[email protected]>
> ---
> This is for 3.18. Tested this, and it works as intended.
Thanks, now applied.
greg k-h
Otherwise, we enable all sorts of forgeries via timing attack.
Signed-off-by: Jason A. Donenfeld <[email protected]>
Cc: Johannes Berg <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Johannes Berg <[email protected]>
---
This is for 3.18. Tested this, and it works as intended.
net/mac80211/wpa.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/mac80211/wpa.c b/net/mac80211/wpa.c
index 983527a4c1ab..bf87de469c03 100644
--- a/net/mac80211/wpa.c
+++ b/net/mac80211/wpa.c
@@ -16,6 +16,7 @@
#include <asm/unaligned.h>
#include <net/mac80211.h>
#include <crypto/aes.h>
+#include <crypto/algapi.h>
#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))
goto mic_fail;
/* remove Michael MIC from payload */
@@ -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))) {
key->u.aes_cmac.icverrors++;
return RX_DROP_UNUSABLE;
}
--
2.13.1
On Sun, Jun 18, 2017 at 10:18 PM, Jason A. Donenfeld <[email protected]> wrote:
> Otherwise, we enable all sorts of forgeries via timing attack.
crypto_memneq's description says:
Returns 0 when data is equal, 1 otherwise.
Clearly this is not suitable here. You are allowing replay attacks...
For network drivers, this is worse than timing attacks. You still need
to explain how you can exploit timing attacks *on a remote system*. On
your local system, threads are impacted etc... Fine. On a remote
system (you are in Rx path here..) how do you exploit them?
>
> Signed-off-by: Jason A. Donenfeld <[email protected]>
> Cc: Johannes Berg <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Johannes Berg <[email protected]>
> ---
> Here's the backport for 3.18.
>
> net/mac80211/wpa.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/net/mac80211/wpa.c b/net/mac80211/wpa.c
> index 983527a4c1ab..49592c7e4199 100644
> --- a/net/mac80211/wpa.c
> +++ b/net/mac80211/wpa.c
> @@ -16,6 +16,7 @@
> #include <asm/unaligned.h>
> #include <net/mac80211.h>
> #include <crypto/aes.h>
> +#include <crypto/algapi.h>
>
> #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;
>
> /* 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;
> }
> @@ -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) {
> key->u.aes_cmac.icverrors++;
> return RX_DROP_UNUSABLE;
> }
> --
> 2.13.1
>
On Sun, 2017-06-18 at 23:31 +0300, Emmanuel Grumbach wrote:
> On Sun, Jun 18, 2017 at 10:18 PM, Jason A. Donenfeld <[email protected]
> > 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