[PATCH 0/2] drop RC4 support

[PATCH 0/2] drop RC4 support

[Ard Biesheuvel]

[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]

RC4 support in libell is based on the ecb(arc4) skcipher exposed by the
Linux crypto API, which is an odd beast given that it does not
distinguish between the key and the IV, and therefore does not fit the
skcipher API very well.

Now that work is underway in the linux-crypto kernel community to
implement chained requests for skciphers, we are running into problems
with this: the state that needs to be kept between skcipher requests to
implement chaining is currently kept in the TFM object (which holds the
key) in the case of ecb(arc4), and moving this into the request object
(to match the way chaining is implemented for other skcipher modes) may
enable key reuse (and thus IV reuse in the ARC4 case), which amount to
catastrophic failure for any stream cipher.

So in order to address this in a safe and robust manner, we intend to
retire the ecb(arc4) skcipher entirely on the Linux side. This obviously
requires work in the userland side as well, hence this series.

Ard Biesheuvel (2):
  tls: remove support for RC4 cipher suites
  cipher: remove obsolete arc4 support

 ell/cipher.c       |  8 +--
 ell/cipher.h       |  4 +-
 ell/tls-suites.c   | 41 ++------------
 unit/test-cipher.c | 56 --------------------
 unit/test-tls.c    |  3 +-
 5 files changed, 11 insertions(+), 101 deletions(-)

-- 
2.20.1

[PATCH 1/2] tls: remove support for RC4 cipher suites

[Ard Biesheuvel]

[-- Attachment #1: Type: text/plain, Size: 3512 bytes --]

The RC4 based TLS suites rely on a quirky kernel implementation (the
ecb(arc4) skcipher), are not covered by any tests, and RFC 7465, which
was issued in 2015, explicitly forbids the use of RC4 in TLS under any
circumstances.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ell/tls-suites.c | 41 ++------------------
 unit/test-tls.c  |  3 +-
 2 files changed, 4 insertions(+), 40 deletions(-)

diff --git a/ell/tls-suites.c b/ell/tls-suites.c
index 91bc2cfe2622..7ec49a86c9ab 100644
--- a/ell/tls-suites.c
+++ b/ell/tls-suites.c
@@ -1096,11 +1096,7 @@ static struct tls_key_exchange_algorithm tls_dhe = {
 	.free_params = tls_free_dhe_params,
 };
 
-static struct tls_bulk_encryption_algorithm tls_rc4 = {
-	.cipher_type = TLS_CIPHER_STREAM,
-	.l_id = L_CIPHER_ARC4,
-	.key_length = 16,
-}, tls_aes128 = {
+static struct tls_bulk_encryption_algorithm tls_aes128 = {
 	.cipher_type = TLS_CIPHER_BLOCK,
 	.l_id = L_CIPHER_AES_CBC,
 	.key_length = 16,
@@ -1134,11 +1130,7 @@ static struct tls_bulk_encryption_algorithm tls_rc4 = {
 	.auth_tag_length = 16,
 };
 
-static struct tls_mac_algorithm tls_md5 = {
-	.id = 1,
-	.hmac_type = L_CHECKSUM_MD5,
-	.mac_length = 16,
-}, tls_sha = {
+static struct tls_mac_algorithm tls_sha = {
 	.id = 2,
 	.hmac_type = L_CHECKSUM_SHA1,
 	.mac_length = 20,
@@ -1152,23 +1144,7 @@ static struct tls_mac_algorithm tls_md5 = {
 	.mac_length = 48,
 };
 
-static struct tls_cipher_suite tls_rsa_with_rc4_128_md5 = {
-	.id = { 0x00, 0x04 },
-	.name = "TLS_RSA_WITH_RC4_128_MD5",
-	.verify_data_length = 12,
-	.encryption = &tls_rc4,
-	.mac = &tls_md5,
-	.signature = &tls_rsa_signature,
-	.key_xchg = &tls_rsa_key_xchg,
-}, tls_rsa_with_rc4_128_sha = {
-	.id = { 0x00, 0x05 },
-	.name = "TLS_RSA_WITH_RC4_128_SHA",
-	.verify_data_length = 12,
-	.encryption = &tls_rc4,
-	.mac = &tls_sha,
-	.signature = &tls_rsa_signature,
-	.key_xchg = &tls_rsa_key_xchg,
-}, tls_rsa_with_3des_ede_cbc_sha = {
+static struct tls_cipher_suite tls_rsa_with_3des_ede_cbc_sha = {
 	.id = { 0x00, 0x0a },
 	.name = "TLS_RSA_WITH_3DES_EDE_CBC_SHA",
 	.verify_data_length = 12,
@@ -1278,14 +1254,6 @@ static struct tls_cipher_suite tls_rsa_with_rc4_128_md5 = {
 	.prf_hmac = L_CHECKSUM_SHA384,
 	.signature = &tls_rsa_signature,
 	.key_xchg = &tls_dhe,
-}, tls_ecdhe_rsa_with_rc4_128_sha = {
-	.id = { 0xc0, 0x11 },
-	.name = "TLS_ECDHE_RSA_WITH_RC4_128_SHA",
-	.verify_data_length = 12,
-	.encryption = &tls_rc4,
-	.mac = &tls_sha,
-	.signature = &tls_rsa_signature,
-	.key_xchg = &tls_ecdhe,
 }, tls_ecdhe_rsa_with_3des_ede_cbc_sha = {
 	.id = { 0xc0, 0x12 },
 	.name = "TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA",
@@ -1366,8 +1334,5 @@ struct tls_cipher_suite *tls_cipher_suite_pref[] = {
 	&tls_ecdhe_rsa_with_3des_ede_cbc_sha,
 	&tls_dhe_rsa_with_3des_ede_cbc_sha,
 	&tls_rsa_with_3des_ede_cbc_sha,
-	&tls_ecdhe_rsa_with_rc4_128_sha,
-	&tls_rsa_with_rc4_128_sha,
-	&tls_rsa_with_rc4_128_md5,
 	NULL,
 };
diff --git a/unit/test-tls.c b/unit/test-tls.c
index 89dcb9ad3215..9503f6b0a493 100644
--- a/unit/test-tls.c
+++ b/unit/test-tls.c
@@ -956,8 +956,7 @@ int main(int argc, char *argv[])
 	}
 
 	if (!l_cipher_is_supported(L_CIPHER_DES3_EDE_CBC) ||
-			!l_cipher_is_supported(L_CIPHER_AES_CBC) ||
-			!l_cipher_is_supported(L_CIPHER_ARC4)) {
+			!l_cipher_is_supported(L_CIPHER_AES_CBC)) {
 		printf("Needed ciphers missing, "
 				"skipping TLS connection tests...\n");
 		goto done;
-- 
2.20.1

[PATCH 2/2] cipher: remove obsolete arc4 support

[Ard Biesheuvel]

[-- Attachment #1: Type: text/plain, Size: 3944 bytes --]

The rc4 cipher is obsolete, and is only relevant for very specific use
cases where compatibility with legacy hardware or software is needed.
Now that iwd has been updated to use its own arc4 implementation, which
is tailored to its needs, and support for the RC4 based TLS cipher suites
has been dropped, we can drop the implementation from libell.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ell/cipher.c       |  8 +--
 ell/cipher.h       |  4 +-
 unit/test-cipher.c | 56 --------------------
 3 files changed, 7 insertions(+), 61 deletions(-)

diff --git a/ell/cipher.c b/ell/cipher.c
index 4a27b5e3556f..300d7c0dd3b1 100644
--- a/ell/cipher.c
+++ b/ell/cipher.c
@@ -149,8 +149,6 @@ static const char *cipher_type_to_name(enum l_cipher_type type)
 		return "cbc(aes)";
 	case L_CIPHER_AES_CTR:
 		return "ctr(aes)";
-	case L_CIPHER_ARC4:
-		return "ecb(arc4)";
 	case L_CIPHER_DES:
 		return "ecb(des)";
 	case L_CIPHER_DES_CBC:
@@ -619,7 +617,11 @@ static void init_supported()
 	strcpy((char *) salg.salg_type, "skcipher");
 
 	for (c = L_CIPHER_AES; c <= L_CIPHER_DES3_EDE_CBC; c++) {
-		strcpy((char *) salg.salg_name, cipher_type_to_name(c));
+		const char *name = cipher_type_to_name(c);
+
+		if (!name)
+			continue;
+		strcpy((char *) salg.salg_name, name);
 
 		if (bind(sk, (struct sockaddr *) &salg, sizeof(salg)) < 0)
 			continue;
diff --git a/ell/cipher.h b/ell/cipher.h
index 84f29888253f..e1a4bda14f81 100644
--- a/ell/cipher.h
+++ b/ell/cipher.h
@@ -33,8 +33,8 @@ enum l_cipher_type {
 	L_CIPHER_AES = 0,
 	L_CIPHER_AES_CBC,
 	L_CIPHER_AES_CTR,
-	L_CIPHER_ARC4,
-	L_CIPHER_DES,
+	/* L_CIPHER_ARC4, */
+	L_CIPHER_DES = 4,
 	L_CIPHER_DES_CBC,
 	L_CIPHER_DES3_EDE_CBC,
 };
diff --git a/unit/test-cipher.c b/unit/test-cipher.c
index 9469e0eefe62..7fe32a21ba57 100644
--- a/unit/test-cipher.c
+++ b/unit/test-cipher.c
@@ -98,59 +98,6 @@ static void test_aes_ctr(const void *data)
 	l_cipher_free(cipher);
 }
 
-static void test_arc4(const void *data)
-{
-	struct l_cipher *cipher;
-	char buf[256];
-	int r;
-
-	static const unsigned char expect_plaintext[] = {
-		0xbb, 0xf3, 0x16, 0xe8, 0xd9, 0x40, 0xaf, 0x0a, 0xd3,
-	};
-	static const unsigned char expect_pedia[] = {
-		0x10, 0x21, 0xbf, 0x04, 0x20,
-	};
-	static const unsigned char expect_attack[] = {
-		0x45, 0xa0, 0x1f, 0x64, 0x5f, 0xc3, 0x5b, 0x38, 0x35, 0x52,
-		0x54, 0x4b, 0x9b, 0xf5,
-	};
-
-	cipher = l_cipher_new(L_CIPHER_ARC4, "Key", 3);
-	assert(cipher);
-	l_cipher_encrypt(cipher, "Plaintext", buf, 9);
-	assert(!memcmp(buf, expect_plaintext, 9));
-	l_cipher_free(cipher);
-
-	cipher = l_cipher_new(L_CIPHER_ARC4, "Wiki", 4);
-	assert(cipher);
-	l_cipher_encrypt(cipher, "pedia", buf, 5);
-	assert(!memcmp(buf, expect_pedia, 5));
-	l_cipher_free(cipher);
-
-	cipher = l_cipher_new(L_CIPHER_ARC4, "Secret", 6);
-	assert(cipher);
-	l_cipher_encrypt(cipher, "Attack@dawn", buf, 14);
-	assert(!memcmp(buf, expect_attack, 14));
-	l_cipher_free(cipher);
-
-	cipher = l_cipher_new(L_CIPHER_ARC4, KEY_STR, KEY_LEN);
-	assert(cipher);
-
-	memcpy(buf, FIXED_STR, FIXED_LEN);
-
-	l_cipher_encrypt(cipher, buf, buf, FIXED_LEN);
-
-	r = memcmp(buf, FIXED_STR, FIXED_LEN);
-	assert(r);
-
-	l_cipher_decrypt(cipher, buf, buf, FIXED_LEN);
-
-	r = memcmp(buf, FIXED_STR, FIXED_LEN);
-	assert(!r);
-
-	l_cipher_free(cipher);
-}
-
 struct aead_test_vector {
 	enum l_aead_cipher_type type;
 	char *aad;
@@ -385,9 +332,6 @@ int main(int argc, char *argv[])
 	if (l_cipher_is_supported(L_CIPHER_AES_CTR))
 		l_test_add("aes_ctr", test_aes_ctr, NULL);
 
-	if (l_cipher_is_supported(L_CIPHER_ARC4))
-		l_test_add("arc4", test_arc4, NULL);
-
 	if (l_aead_cipher_is_supported(L_AEAD_CIPHER_AES_CCM)) {
 		l_test_add("aes_ccm long nonce", test_aead, &ccm_long_nonce);
 		l_test_add("aes_ccm short nonce", test_aead, &ccm_short_nonce);
-- 
2.20.1

Re: [PATCH 0/2] drop RC4 support

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1514 bytes --]

Hi Ard,

On 8/3/20 5:54 AM, Ard Biesheuvel wrote:
> RC4 support in libell is based on the ecb(arc4) skcipher exposed by the
> Linux crypto API, which is an odd beast given that it does not
> distinguish between the key and the IV, and therefore does not fit the
> skcipher API very well.
> 
> Now that work is underway in the linux-crypto kernel community to
> implement chained requests for skciphers, we are running into problems
> with this: the state that needs to be kept between skcipher requests to
> implement chaining is currently kept in the TFM object (which holds the
> key) in the case of ecb(arc4), and moving this into the request object
> (to match the way chaining is implemented for other skcipher modes) may
> enable key reuse (and thus IV reuse in the ARC4 case), which amount to
> catastrophic failure for any stream cipher.
> 
> So in order to address this in a safe and robust manner, we intend to
> retire the ecb(arc4) skcipher entirely on the Linux side. This obviously
> requires work in the userland side as well, hence this series.
> 
> Ard Biesheuvel (2):
>    tls: remove support for RC4 cipher suites
>    cipher: remove obsolete arc4 support
> 
>   ell/cipher.c       |  8 +--
>   ell/cipher.h       |  4 +-
>   ell/tls-suites.c   | 41 ++------------
>   unit/test-cipher.c | 56 --------------------
>   unit/test-tls.c    |  3 +-
>   5 files changed, 11 insertions(+), 101 deletions(-)
> 

Both applied (minus the S-o-B lines).

Regards,
-Denis