| Message ID | 20200702101947.682-5-ardb@kernel.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: get rid of ecb(arc4) | expand |
[+linux-wireless, Marcel Holtmann, and Denis Kenzior] On Thu, Jul 02, 2020 at 12:19:44PM +0200, Ard Biesheuvel wrote: > Remove the generic ecb(arc4) skcipher, which is slightly cumbersome from > a maintenance perspective, since it does not quite behave like other > skciphers do in terms of key vs IV lifetime. Since we are leaving the > library interface in place, which is used by the various WEP and TKIP > implementations we have in the tree, we can safely drop this code now > it no longer has any users. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Last year there was a discussion where it was mentioned that iwd uses "ecb(arc4)" via AF_ALG. So can we really remove it yet? See https://lkml.kernel.org/r/97BB95F6-4A4C-4984-9EAB-6069E19B4A4F@holtmann.org Note that the code isn't in "iwd" itself but rather in "libell" which iwd depends on: https://git.kernel.org/pub/scm/libs/ell/ell.git/ Apparently it also uses md4 and ecb(des) too. Marcel and Denis, what's your deprecation plan for these obsolete and insecure algorithms? - Eric
On Thu, 2 Jul 2020 at 19:50, Eric Biggers <ebiggers@kernel.org> wrote: > > [+linux-wireless, Marcel Holtmann, and Denis Kenzior] > > On Thu, Jul 02, 2020 at 12:19:44PM +0200, Ard Biesheuvel wrote: > > Remove the generic ecb(arc4) skcipher, which is slightly cumbersome from > > a maintenance perspective, since it does not quite behave like other > > skciphers do in terms of key vs IV lifetime. Since we are leaving the > > library interface in place, which is used by the various WEP and TKIP > > implementations we have in the tree, we can safely drop this code now > > it no longer has any users. > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > Last year there was a discussion where it was mentioned that iwd uses > "ecb(arc4)" via AF_ALG. So can we really remove it yet? > See https://lkml.kernel.org/r/97BB95F6-4A4C-4984-9EAB-6069E19B4A4F@holtmann.org > Note that the code isn't in "iwd" itself but rather in "libell" which iwd > depends on: https://git.kernel.org/pub/scm/libs/ell/ell.git/ > > Apparently it also uses md4 and ecb(des) too. > Ah yes, I remember now :-( > Marcel and Denis, what's your deprecation plan for these obsolete and insecure > algorithms? > Given Denis's statement: It sounds to me like it was broken and should be fixed. So our vote / preference is to have ARC4 fixed to follow the proper semantics. We can deal with the kernel behavioral change on our end easily enough; the required workarounds are the worse evil. I would think that an ABI break is not the end of the world for them, and given how trivial it is to implement RC4 in C, the workaround should be to simply implement RC4 in user space, and not even bother trying to use AF_ALG to get at ecb(arc4) (same applies to md4 and ecb(des) btw) There will always be a long tail of use cases, and at some point, we just have to draw the line and remove obsolete and insecure cruft, especially when it impedes progress on other fronts. Full implementation of arc4 aka ecb(arc4) below. void arc4_crypt(struct arc4_ctx *ctx, u8 *out, const u8 *in, unsigned int len) { u32 *const S = ctx->S; u32 x, y, a, b; u32 ty, ta, tb; if (len == 0) return; x = ctx->x; y = ctx->y; a = S[x]; y = (y + a) & 0xff; b = S[y]; do { S[y] = a; a = (a + b) & 0xff; S[x] = b; x = (x + 1) & 0xff; ta = S[x]; ty = (y + ta) & 0xff; tb = S[ty]; *out++ = *in++ ^ S[a]; if (--len == 0) break; y = ty; a = ta; b = tb; } while (true); ctx->x = x; ctx->y = y; }
On Thu, 2 Jul 2020 at 20:21, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Thu, 2 Jul 2020 at 19:50, Eric Biggers <ebiggers@kernel.org> wrote: > > > > [+linux-wireless, Marcel Holtmann, and Denis Kenzior] > > > > On Thu, Jul 02, 2020 at 12:19:44PM +0200, Ard Biesheuvel wrote: > > > Remove the generic ecb(arc4) skcipher, which is slightly cumbersome from > > > a maintenance perspective, since it does not quite behave like other > > > skciphers do in terms of key vs IV lifetime. Since we are leaving the > > > library interface in place, which is used by the various WEP and TKIP > > > implementations we have in the tree, we can safely drop this code now > > > it no longer has any users. > > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > > Last year there was a discussion where it was mentioned that iwd uses > > "ecb(arc4)" via AF_ALG. So can we really remove it yet? > > See https://lkml.kernel.org/r/97BB95F6-4A4C-4984-9EAB-6069E19B4A4F@holtmann.org > > Note that the code isn't in "iwd" itself but rather in "libell" which iwd > > depends on: https://git.kernel.org/pub/scm/libs/ell/ell.git/ > > > > Apparently it also uses md4 and ecb(des) too. > > > > Ah yes, I remember now :-( > > > Marcel and Denis, what's your deprecation plan for these obsolete and insecure > > algorithms? > > > > Given Denis's statement: > > It sounds to me like it was broken and should be fixed. So our vote / > preference is to have ARC4 fixed to follow the proper semantics. We > can deal with the kernel behavioral change on our end easily enough; > the required workarounds are the worse evil. > > I would think that an ABI break is not the end of the world for them, > and given how trivial it is to implement RC4 in C, the workaround > should be to simply implement RC4 in user space, and not even bother > trying to use AF_ALG to get at ecb(arc4) > > (same applies to md4 and ecb(des) btw) > > There will always be a long tail of use cases, and at some point, we > just have to draw the line and remove obsolete and insecure cruft, > especially when it impedes progress on other fronts. > I have ported iwd to Nettle's LGPL 2.1 implementation of ARC4, and the diffstat is src/crypto.c | 80 ++++++++++++-------- src/main.c | 8 -- unit/test-eapol.c | 3 +- 3 files changed, 51 insertions(+), 40 deletions(-) https://git.kernel.org/pub/scm/linux/kernel/git/ardb/iwd.git/log/?h=arc4-cleanup
On Fri, 3 Jul 2020 at 02:04, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Thu, 2 Jul 2020 at 20:21, Ard Biesheuvel <ardb@kernel.org> wrote: > > > > On Thu, 2 Jul 2020 at 19:50, Eric Biggers <ebiggers@kernel.org> wrote: > > > > > > [+linux-wireless, Marcel Holtmann, and Denis Kenzior] > > > > > > On Thu, Jul 02, 2020 at 12:19:44PM +0200, Ard Biesheuvel wrote: > > > > Remove the generic ecb(arc4) skcipher, which is slightly cumbersome from > > > > a maintenance perspective, since it does not quite behave like other > > > > skciphers do in terms of key vs IV lifetime. Since we are leaving the > > > > library interface in place, which is used by the various WEP and TKIP > > > > implementations we have in the tree, we can safely drop this code now > > > > it no longer has any users. > > > > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > > > > Last year there was a discussion where it was mentioned that iwd uses > > > "ecb(arc4)" via AF_ALG. So can we really remove it yet? > > > See https://lkml.kernel.org/r/97BB95F6-4A4C-4984-9EAB-6069E19B4A4F@holtmann.org > > > Note that the code isn't in "iwd" itself but rather in "libell" which iwd > > > depends on: https://git.kernel.org/pub/scm/libs/ell/ell.git/ > > > > > > Apparently it also uses md4 and ecb(des) too. > > > > > > > Ah yes, I remember now :-( > > > > > Marcel and Denis, what's your deprecation plan for these obsolete and insecure > > > algorithms? > > > > > > > Given Denis's statement: > > > > It sounds to me like it was broken and should be fixed. So our vote / > > preference is to have ARC4 fixed to follow the proper semantics. We > > can deal with the kernel behavioral change on our end easily enough; > > the required workarounds are the worse evil. > > > > I would think that an ABI break is not the end of the world for them, > > and given how trivial it is to implement RC4 in C, the workaround > > should be to simply implement RC4 in user space, and not even bother > > trying to use AF_ALG to get at ecb(arc4) > > > > (same applies to md4 and ecb(des) btw) > > > > There will always be a long tail of use cases, and at some point, we > > just have to draw the line and remove obsolete and insecure cruft, > > especially when it impedes progress on other fronts. > > > > I have ported iwd to Nettle's LGPL 2.1 implementation of ARC4, and the > diffstat is > > src/crypto.c | 80 ++++++++++++-------- > src/main.c | 8 -- > unit/test-eapol.c | 3 +- > 3 files changed, 51 insertions(+), 40 deletions(-) > > https://git.kernel.org/pub/scm/linux/kernel/git/ardb/iwd.git/log/?h=arc4-cleanup Marcel, Denis, Do you have any objections to the ecb(arc4) skcipher being dropped from the kernel, given the fallback i proposed above (which is a much better way of doing rc4 in user space anyway)? For libell, I would suggest dropping rc4 entirely, once iwd stops relying on it, as using rc4 for tls is obsolete as well.
On Sat, 18 Jul 2020 at 11:18, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Fri, 3 Jul 2020 at 02:04, Ard Biesheuvel <ardb@kernel.org> wrote: > > > > On Thu, 2 Jul 2020 at 20:21, Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > > On Thu, 2 Jul 2020 at 19:50, Eric Biggers <ebiggers@kernel.org> wrote: > > > > > > > > [+linux-wireless, Marcel Holtmann, and Denis Kenzior] > > > > > > > > On Thu, Jul 02, 2020 at 12:19:44PM +0200, Ard Biesheuvel wrote: > > > > > Remove the generic ecb(arc4) skcipher, which is slightly cumbersome from > > > > > a maintenance perspective, since it does not quite behave like other > > > > > skciphers do in terms of key vs IV lifetime. Since we are leaving the > > > > > library interface in place, which is used by the various WEP and TKIP > > > > > implementations we have in the tree, we can safely drop this code now > > > > > it no longer has any users. > > > > > > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > > > > > > Last year there was a discussion where it was mentioned that iwd uses > > > > "ecb(arc4)" via AF_ALG. So can we really remove it yet? > > > > See https://lkml.kernel.org/r/97BB95F6-4A4C-4984-9EAB-6069E19B4A4F@holtmann.org > > > > Note that the code isn't in "iwd" itself but rather in "libell" which iwd > > > > depends on: https://git.kernel.org/pub/scm/libs/ell/ell.git/ > > > > > > > > Apparently it also uses md4 and ecb(des) too. > > > > > > > > > > Ah yes, I remember now :-( > > > > > > > Marcel and Denis, what's your deprecation plan for these obsolete and insecure > > > > algorithms? > > > > > > > > > > Given Denis's statement: > > > > > > It sounds to me like it was broken and should be fixed. So our vote / > > > preference is to have ARC4 fixed to follow the proper semantics. We > > > can deal with the kernel behavioral change on our end easily enough; > > > the required workarounds are the worse evil. > > > > > > I would think that an ABI break is not the end of the world for them, > > > and given how trivial it is to implement RC4 in C, the workaround > > > should be to simply implement RC4 in user space, and not even bother > > > trying to use AF_ALG to get at ecb(arc4) > > > > > > (same applies to md4 and ecb(des) btw) > > > > > > There will always be a long tail of use cases, and at some point, we > > > just have to draw the line and remove obsolete and insecure cruft, > > > especially when it impedes progress on other fronts. > > > > > > > I have ported iwd to Nettle's LGPL 2.1 implementation of ARC4, and the > > diffstat is > > > > src/crypto.c | 80 ++++++++++++-------- > > src/main.c | 8 -- > > unit/test-eapol.c | 3 +- > > 3 files changed, 51 insertions(+), 40 deletions(-) > > > > https://git.kernel.org/pub/scm/linux/kernel/git/ardb/iwd.git/log/?h=arc4-cleanup > > Marcel, Denis, > > Do you have any objections to the ecb(arc4) skcipher being dropped > from the kernel, given the fallback i proposed above (which is a much > better way of doing rc4 in user space anyway)? > > For libell, I would suggest dropping rc4 entirely, once iwd stops > relying on it, as using rc4 for tls is obsolete as well. Ping?
On Sat, 25 Jul 2020 at 10:06, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Sat, 18 Jul 2020 at 11:18, Ard Biesheuvel <ardb@kernel.org> wrote: > > > > On Fri, 3 Jul 2020 at 02:04, Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > > On Thu, 2 Jul 2020 at 20:21, Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > > > > On Thu, 2 Jul 2020 at 19:50, Eric Biggers <ebiggers@kernel.org> wrote: > > > > > > > > > > [+linux-wireless, Marcel Holtmann, and Denis Kenzior] > > > > > > > > > > On Thu, Jul 02, 2020 at 12:19:44PM +0200, Ard Biesheuvel wrote: > > > > > > Remove the generic ecb(arc4) skcipher, which is slightly cumbersome from > > > > > > a maintenance perspective, since it does not quite behave like other > > > > > > skciphers do in terms of key vs IV lifetime. Since we are leaving the > > > > > > library interface in place, which is used by the various WEP and TKIP > > > > > > implementations we have in the tree, we can safely drop this code now > > > > > > it no longer has any users. > > > > > > > > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > > > > > > > > Last year there was a discussion where it was mentioned that iwd uses > > > > > "ecb(arc4)" via AF_ALG. So can we really remove it yet? > > > > > See https://lkml.kernel.org/r/97BB95F6-4A4C-4984-9EAB-6069E19B4A4F@holtmann.org > > > > > Note that the code isn't in "iwd" itself but rather in "libell" which iwd > > > > > depends on: https://git.kernel.org/pub/scm/libs/ell/ell.git/ > > > > > > > > > > Apparently it also uses md4 and ecb(des) too. > > > > > > > > > > > > > Ah yes, I remember now :-( > > > > > > > > > Marcel and Denis, what's your deprecation plan for these obsolete and insecure > > > > > algorithms? > > > > > > > > > > > > > Given Denis's statement: > > > > > > > > It sounds to me like it was broken and should be fixed. So our vote / > > > > preference is to have ARC4 fixed to follow the proper semantics. We > > > > can deal with the kernel behavioral change on our end easily enough; > > > > the required workarounds are the worse evil. > > > > > > > > I would think that an ABI break is not the end of the world for them, > > > > and given how trivial it is to implement RC4 in C, the workaround > > > > should be to simply implement RC4 in user space, and not even bother > > > > trying to use AF_ALG to get at ecb(arc4) > > > > > > > > (same applies to md4 and ecb(des) btw) > > > > > > > > There will always be a long tail of use cases, and at some point, we > > > > just have to draw the line and remove obsolete and insecure cruft, > > > > especially when it impedes progress on other fronts. > > > > > > > > > > I have ported iwd to Nettle's LGPL 2.1 implementation of ARC4, and the > > > diffstat is > > > > > > src/crypto.c | 80 ++++++++++++-------- > > > src/main.c | 8 -- > > > unit/test-eapol.c | 3 +- > > > 3 files changed, 51 insertions(+), 40 deletions(-) > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/ardb/iwd.git/log/?h=arc4-cleanup > > > > Marcel, Denis, > > > > Do you have any objections to the ecb(arc4) skcipher being dropped > > from the kernel, given the fallback i proposed above (which is a much > > better way of doing rc4 in user space anyway)? > > > > For libell, I would suggest dropping rc4 entirely, once iwd stops > > relying on it, as using rc4 for tls is obsolete as well. > > Ping? Denis was kind enough to take the changes to iwd and libell that remove all dependencies on the ecb(arc4) skcipher exposed by the kernel, so we can at least deprecate it in the short term, and hopefully remove it entirely at a later stage. Perhaps we should introduce a Kconfig symbol that needs to be set to enable deprecated algorithms? That way, we can work with the distros to phase out the old junk that is piling up, but in a way that doesn't break people's systems.
diff --git a/crypto/Kconfig b/crypto/Kconfig index 091c0a0bbf26..fd0d1f78ac47 100644 --- a/crypto/Kconfig +++ b/crypto/Kconfig @@ -1197,18 +1197,6 @@ config CRYPTO_ANUBIS <https://www.cosic.esat.kuleuven.be/nessie/reports/> <http://www.larc.usp.br/~pbarreto/AnubisPage.html> -config CRYPTO_ARC4 - tristate "ARC4 cipher algorithm" - select CRYPTO_SKCIPHER - select CRYPTO_LIB_ARC4 - help - ARC4 cipher algorithm. - - ARC4 is a stream cipher using keys ranging from 8 bits to 2048 - bits in length. This algorithm is required for driver-based - WEP, but it should not be for other purposes because of the - weakness of the algorithm. - config CRYPTO_BLOWFISH tristate "Blowfish cipher algorithm" select CRYPTO_ALGAPI diff --git a/crypto/Makefile b/crypto/Makefile index 4ca12b6044f7..af88c7e30b3c 100644 --- a/crypto/Makefile +++ b/crypto/Makefile @@ -128,7 +128,6 @@ obj-$(CONFIG_CRYPTO_CAMELLIA) += camellia_generic.o obj-$(CONFIG_CRYPTO_CAST_COMMON) += cast_common.o obj-$(CONFIG_CRYPTO_CAST5) += cast5_generic.o obj-$(CONFIG_CRYPTO_CAST6) += cast6_generic.o -obj-$(CONFIG_CRYPTO_ARC4) += arc4.o obj-$(CONFIG_CRYPTO_TEA) += tea.o obj-$(CONFIG_CRYPTO_KHAZAD) += khazad.o obj-$(CONFIG_CRYPTO_ANUBIS) += anubis.o diff --git a/crypto/arc4.c b/crypto/arc4.c deleted file mode 100644 index aa79571dbd49..000000000000 --- a/crypto/arc4.c +++ /dev/null @@ -1,76 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-or-later -/* - * Cryptographic API - * - * ARC4 Cipher Algorithm - * - * Jon Oberheide <jon@oberheide.org> - */ - -#include <crypto/algapi.h> -#include <crypto/arc4.h> -#include <crypto/internal/skcipher.h> -#include <linux/init.h> -#include <linux/module.h> - -static int crypto_arc4_setkey(struct crypto_skcipher *tfm, const u8 *in_key, - unsigned int key_len) -{ - struct arc4_ctx *ctx = crypto_skcipher_ctx(tfm); - - return arc4_setkey(ctx, in_key, key_len); -} - -static int crypto_arc4_crypt(struct skcipher_request *req) -{ - struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req); - struct arc4_ctx *ctx = crypto_skcipher_ctx(tfm); - struct skcipher_walk walk; - int err; - - err = skcipher_walk_virt(&walk, req, false); - - while (walk.nbytes > 0) { - arc4_crypt(ctx, walk.dst.virt.addr, walk.src.virt.addr, - walk.nbytes); - err = skcipher_walk_done(&walk, 0); - } - - return err; -} - -static struct skcipher_alg arc4_alg = { - /* - * For legacy reasons, this is named "ecb(arc4)", not "arc4". - * Nevertheless it's actually a stream cipher, not a block cipher. - */ - .base.cra_name = "ecb(arc4)", - .base.cra_driver_name = "ecb(arc4)-generic", - .base.cra_priority = 100, - .base.cra_blocksize = ARC4_BLOCK_SIZE, - .base.cra_ctxsize = sizeof(struct arc4_ctx), - .base.cra_module = THIS_MODULE, - .min_keysize = ARC4_MIN_KEY_SIZE, - .max_keysize = ARC4_MAX_KEY_SIZE, - .setkey = crypto_arc4_setkey, - .encrypt = crypto_arc4_crypt, - .decrypt = crypto_arc4_crypt, -}; - -static int __init arc4_init(void) -{ - return crypto_register_skcipher(&arc4_alg); -} - -static void __exit arc4_exit(void) -{ - crypto_unregister_skcipher(&arc4_alg); -} - -subsys_initcall(arc4_init); -module_exit(arc4_exit); - -MODULE_LICENSE("GPL"); -MODULE_DESCRIPTION("ARC4 Cipher Algorithm"); -MODULE_AUTHOR("Jon Oberheide <jon@oberheide.org>"); -MODULE_ALIAS_CRYPTO("ecb(arc4)"); diff --git a/drivers/net/wireless/intel/ipw2x00/Kconfig b/drivers/net/wireless/intel/ipw2x00/Kconfig index d00386915a9d..82b7eea3495f 100644 --- a/drivers/net/wireless/intel/ipw2x00/Kconfig +++ b/drivers/net/wireless/intel/ipw2x00/Kconfig @@ -160,7 +160,6 @@ config LIBIPW select WIRELESS_EXT select WEXT_SPY select CRYPTO - select CRYPTO_ARC4 select CRYPTO_ECB select CRYPTO_AES select CRYPTO_MICHAEL_MIC diff --git a/drivers/net/wireless/intersil/hostap/Kconfig b/drivers/net/wireless/intersil/hostap/Kconfig index 6ad88299432f..428fb6f55f51 100644 --- a/drivers/net/wireless/intersil/hostap/Kconfig +++ b/drivers/net/wireless/intersil/hostap/Kconfig @@ -5,7 +5,6 @@ config HOSTAP select WEXT_SPY select WEXT_PRIV select CRYPTO - select CRYPTO_ARC4 select CRYPTO_ECB select CRYPTO_AES select CRYPTO_MICHAEL_MIC
Remove the generic ecb(arc4) skcipher, which is slightly cumbersome from a maintenance perspective, since it does not quite behave like other skciphers do in terms of key vs IV lifetime. Since we are leaving the library interface in place, which is used by the various WEP and TKIP implementations we have in the tree, we can safely drop this code now it no longer has any users. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- crypto/Kconfig | 12 ---- crypto/Makefile | 1 - crypto/arc4.c | 76 -------------------- drivers/net/wireless/intel/ipw2x00/Kconfig | 1 - drivers/net/wireless/intersil/hostap/Kconfig | 1 - 5 files changed, 91 deletions(-)