diff mbox series

[backport,4.4] mac80211: Fix TKIP replay protection immediately after key setup

Message ID 20200215195407.GA10344@amd (mailing list archive)
State Not Applicable
Headers show
Series [backport,4.4] mac80211: Fix TKIP replay protection immediately after key setup | expand

Commit Message

Pavel Machek Feb. 15, 2020, 7:54 p.m. UTC 
Hi!

So... this is first backport patch. I'll need to reformat a changelog.

The patch should pass our tests on gitlab, but I somehow don't think
those tests involved wifi at all... At least it compiles.

Can someone test it easily? Should I just submit it to stable
explaining I did not test it?

Do you have other patches that should go to 4.4/4.19?

Best regards,
								Pavel

commit 911e21ed055f6700fa80d0f7a818ba223999bb2a
Author: Pavel Machek <pavel@ucw.cz>
Date:   Thu Feb 13 22:56:46 2020 +0100

    Author: Jouni Malinen <j@w1.fi>
    Date:   Tue Jan 7 17:35:45 2020 +0200
    
    commit fa73f24d1b119b85b32cd8f217a73d108888097e
    
        mac80211: Fix TKIP replay protection immediately after key setup
    
            TKIP replay protection was skipped for the very first frame received
            after a new key is configured. While this is potentially needed to avoid
            dropping a frame in some cases, this does leave a window for replay
            attacks with group-addressed frames at the station side. Any earlier
            frame sent by the AP using the same key would be accepted as a valid
            frame and the internal RSC would then be updated to the TSC from that
            frame. This would allow multiple previously transmitted group-addressed
            frames to be replayed until the next valid new group-addressed frame
            from the AP is received by the station.
    
            Fix this by limiting the no-replay-protection exception to apply only
            for the case where TSC=0, i.e., when this is for the very first frame
            protected using the new key, and the local RSC had not been set to a
            higher value when configuring the key (which may happen with GTK).
    
    Signed-off-by: Jouni Malinen <j@w1.fi>
    Link: https://lore.kernel.org/r/20200107153545.10934-1-j@w1.fi
    Signed-off-by: Johannes Berg <johannes.berg@intel.com>
    [pavel@ucw.cz: port to 4.4]
    Signed-off-by: Pavel Machek <pavel@ucw.cz>

Comments

Nobuhiro Iwamatsu Feb. 17, 2020, 11:14 p.m. UTC | #1 
Hi Pavel,

> -----Original Message-----
> From: cip-dev [mailto:cip-dev-bounces@lists.cip-project.org] On Behalf
> Of Pavel Machek
> Sent: Sunday, February 16, 2020 4:54 AM
> To: cip-dev@lists.cip-project.org; Chris.Paterson2@renesas.com
> Subject: [cip-dev] [backport 4.4] mac80211: Fix TKIP replay protection
> immediately after key setup
> 
> Hi!
> 
> So... this is first backport patch. I'll need to reformat a changelog.
> 
> The patch should pass our tests on gitlab, but I somehow don't think those
> tests involved wifi at all... At least it compiles.
> 
> Can someone test it easily? Should I just submit it to stable explaining
> I did not test it?

If testing is difficult, how about sending a patch to stable ML as RFC?
We may get reviews from the patch authors.

> 
> Do you have other patches that should go to 4.4/4.19?
> 

I don't think there are other patches.


> Best regards,
> 								Pavel
> 

Best regards,
  Nobuhiro

> commit 911e21ed055f6700fa80d0f7a818ba223999bb2a
> Author: Pavel Machek <pavel@ucw.cz>
> Date:   Thu Feb 13 22:56:46 2020 +0100
> 
>     Author: Jouni Malinen <j@w1.fi>
>     Date:   Tue Jan 7 17:35:45 2020 +0200
> 
>     commit fa73f24d1b119b85b32cd8f217a73d108888097e
> 
>         mac80211: Fix TKIP replay protection immediately after key setup
> 
>             TKIP replay protection was skipped for the very first frame
> received
>             after a new key is configured. While this is potentially
> needed to avoid
>             dropping a frame in some cases, this does leave a window for
> replay
>             attacks with group-addressed frames at the station side. Any
> earlier
>             frame sent by the AP using the same key would be accepted
> as a valid
>             frame and the internal RSC would then be updated to the TSC
> from that
>             frame. This would allow multiple previously transmitted
> group-addressed
>             frames to be replayed until the next valid new
> group-addressed frame
>             from the AP is received by the station.
> 
>             Fix this by limiting the no-replay-protection exception to
> apply only
>             for the case where TSC=0, i.e., when this is for the very
> first frame
>             protected using the new key, and the local RSC had not been
> set to a
>             higher value when configuring the key (which may happen with
> GTK).
> 
>     Signed-off-by: Jouni Malinen <j@w1.fi>
>     Link: https://lore.kernel.org/r/20200107153545.10934-1-j@w1.fi
>     Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>     [pavel@ucw.cz: port to 4.4]
>     Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/net/mac80211/tkip.c b/net/mac80211/tkip.c index
> 0ae207771a58..d09d24d04f8a 100644
> --- a/net/mac80211/tkip.c
> +++ b/net/mac80211/tkip.c
> @@ -265,10 +265,21 @@ int ieee80211_tkip_decrypt_data(struct
> crypto_cipher *tfm,
>  	if ((keyid >> 6) != key->conf.keyidx)
>  		return TKIP_DECRYPT_INVALID_KEYIDX;
> 
> -	if (key->u.tkip.rx[queue].state != TKIP_STATE_NOT_INIT &&
> -	    (iv32 < key->u.tkip.rx[queue].iv32 ||
> -	     (iv32 == key->u.tkip.rx[queue].iv32 &&
> -	      iv16 <= key->u.tkip.rx[queue].iv16)))
> +	/* Reject replays if the received TSC is smaller than or equal
> to the
> +	 * last received value in a valid message, but with an exception
> for
> +	 * the case where a new key has been set and no valid frame using
> that
> +	 * key has yet received and the local RSC was initialized to 0.
> This
> +	 * exception allows the very first frame sent by the transmitter
> to be
> +	 * accepted even if that transmitter were to use TSC 0 (IEEE 802.11
> +	 * described TSC to be initialized to 1 whenever a new key is
> taken into
> +	 * use).
> +	 */
> +	if (iv32 < key->u.tkip.rx[queue].iv32 ||
> +	    (iv32 == key->u.tkip.rx[queue].iv32 &&
> +	     (iv16 < key->u.tkip.rx[queue].iv16 ||
> +	      (iv16 == key->u.tkip.rx[queue].iv16 &&
> +	       (key->u.tkip.rx[queue].iv32 ||
> key->u.tkip.rx[queue].iv16 ||
> +		key->u.tkip.rx[queue].state !=
> TKIP_STATE_NOT_INIT)))))
>  		return TKIP_DECRYPT_REPLAY;
> 
>  	if (only_iv) {
> 
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures)
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Ben Hutchings March 20, 2020, 9:41 p.m. UTC | #2 
On Sat, 2020-02-15 at 20:54 +0100, Pavel Machek wrote:
> Hi!
> 
> So... this is first backport patch. I'll need to reformat a changelog.
> 
> The patch should pass our tests on gitlab, but I somehow don't think
> those tests involved wifi at all... At least it compiles.
> 
> Can someone test it easily? Should I just submit it to stable
> explaining I did not test it?

That's what I would do.

> Do you have other patches that should go to 4.4/4.19?
> 
> Best regards,
> 								Pavel
> 
> commit 911e21ed055f6700fa80d0f7a818ba223999bb2a
> Author: Pavel Machek <pavel@ucw.cz>
> Date:   Thu Feb 13 22:56:46 2020 +0100
> 
>     Author: Jouni Malinen <j@w1.fi>
>     Date:   Tue Jan 7 17:35:45 2020 +0200
>
>     commit fa73f24d1b119b85b32cd8f217a73d108888097e

This reference is wrong; the upstream commit is
6f601265215a421f425ba3a4850a35861d024643.  Also the usual format for
this reference has "upstream." after the commit hash.

[...]
> --- a/net/mac80211/tkip.c
> +++ b/net/mac80211/tkip.c
> @@ -265,10 +265,21 @@ int ieee80211_tkip_decrypt_data(struct crypto_cipher *tfm,
>  	if ((keyid >> 6) != key->conf.keyidx)
>  		return TKIP_DECRYPT_INVALID_KEYIDX;
>  
> -	if (key->u.tkip.rx[queue].state != TKIP_STATE_NOT_INIT &&
> -	    (iv32 < key->u.tkip.rx[queue].iv32 ||
> -	     (iv32 == key->u.tkip.rx[queue].iv32 &&
> -	      iv16 <= key->u.tkip.rx[queue].iv16)))
> +	/* Reject replays if the received TSC is smaller than or equal to the
> +	 * last received value in a valid message, but with an exception for
> +	 * the case where a new key has been set and no valid frame using that
> +	 * key has yet received and the local RSC was initialized to 0. This
> +	 * exception allows the very first frame sent by the transmitter to be
> +	 * accepted even if that transmitter were to use TSC 0 (IEEE 802.11
> +	 * described TSC to be initialized to 1 whenever a new key is taken into
> +	 * use).
> +	 */
> +	if (iv32 < key->u.tkip.rx[queue].iv32 ||
> +	    (iv32 == key->u.tkip.rx[queue].iv32 &&
> +	     (iv16 < key->u.tkip.rx[queue].iv16 ||
> +	      (iv16 == key->u.tkip.rx[queue].iv16 &&
> +	       (key->u.tkip.rx[queue].iv32 || key->u.tkip.rx[queue].iv16 ||
> +		key->u.tkip.rx[queue].state != TKIP_STATE_NOT_INIT)))))
>  		return TKIP_DECRYPT_REPLAY;
>  
>  	if (only_iv) {

This backport makes sense to me.  Please can you send it to the stable
list, with the fixed commit message?

Ben.
Ben Hutchings March 20, 2020, 9:42 p.m. UTC | #3 
On Mon, 2020-02-17 at 23:14 +0000, nobuhiro1.iwamatsu@toshiba.co.jp wrote:
> Hi Pavel,
> 
> > -----Original Message-----
> > From: cip-dev [mailto:cip-dev-bounces@lists.cip-project.org] On Behalf
> > Of Pavel Machek
> > Sent: Sunday, February 16, 2020 4:54 AM
> > To: cip-dev@lists.cip-project.org; Chris.Paterson2@renesas.com
> > Subject: [cip-dev] [backport 4.4] mac80211: Fix TKIP replay protection
> > immediately after key setup
> > 
> > Hi!
> > 
> > So... this is first backport patch. I'll need to reformat a changelog.
> > 
> > The patch should pass our tests on gitlab, but I somehow don't think those
> > tests involved wifi at all... At least it compiles.
> > 
> > Can someone test it easily? Should I just submit it to stable explaining
> > I did not test it?
> 
> If testing is difficult, how about sending a patch to stable ML as RFC?
> We may get reviews from the patch authors.
> 
> > Do you have other patches that should go to 4.4/4.19?
> > 
> 
> I don't think there are other patches.

The security tracker shows a lot of fixes missing from 4.4.

Ben.
diff mbox series

Patch

diff --git a/net/mac80211/tkip.c b/net/mac80211/tkip.c
index 0ae207771a58..d09d24d04f8a 100644
--- a/net/mac80211/tkip.c
+++ b/net/mac80211/tkip.c
@@ -265,10 +265,21 @@  int ieee80211_tkip_decrypt_data(struct crypto_cipher *tfm,
 	if ((keyid >> 6) != key->conf.keyidx)
 		return TKIP_DECRYPT_INVALID_KEYIDX;
 
-	if (key->u.tkip.rx[queue].state != TKIP_STATE_NOT_INIT &&
-	    (iv32 < key->u.tkip.rx[queue].iv32 ||
-	     (iv32 == key->u.tkip.rx[queue].iv32 &&
-	      iv16 <= key->u.tkip.rx[queue].iv16)))
+	/* Reject replays if the received TSC is smaller than or equal to the
+	 * last received value in a valid message, but with an exception for
+	 * the case where a new key has been set and no valid frame using that
+	 * key has yet received and the local RSC was initialized to 0. This
+	 * exception allows the very first frame sent by the transmitter to be
+	 * accepted even if that transmitter were to use TSC 0 (IEEE 802.11
+	 * described TSC to be initialized to 1 whenever a new key is taken into
+	 * use).
+	 */
+	if (iv32 < key->u.tkip.rx[queue].iv32 ||
+	    (iv32 == key->u.tkip.rx[queue].iv32 &&
+	     (iv16 < key->u.tkip.rx[queue].iv16 ||
+	      (iv16 == key->u.tkip.rx[queue].iv16 &&
+	       (key->u.tkip.rx[queue].iv32 || key->u.tkip.rx[queue].iv16 ||
+		key->u.tkip.rx[queue].state != TKIP_STATE_NOT_INIT)))))
 		return TKIP_DECRYPT_REPLAY;
 
 	if (only_iv) {