[LTP] [PATCH v2 1/1] tst_af_alg: Another fix for disabled weak cipher

[LTP] [PATCH v2 1/1] tst_af_alg: Another fix for disabled weak cipher

[Petr Vorel]

e.g. md5 on enabled FIPS.
Similar fix to 4fa302ef9d. It fixes:

./af_alg01
tst_af_alg.c:84: TBROK: unexpected error binding AF_ALG socket to hash algorithm 'md5': ELIBBAD (80)
become
tst_fips.c:22: TINFO: FIPS: on
tst_af_alg.c:82: TCONF: FIPS enabled => hash algorithm 'md5' disabled
tst_fips.c:22: TINFO: FIPS: on
tst_af_alg.c:82: TCONF: FIPS enabled => hash algorithm 'md5-generic' disabled

./af_alg02
tst_af_alg.c:37: TBROK: unexpected error binding AF_ALG socket to skcipher algorithm 'salsa20': ELIBBAD (80)
become
tst_fips.c:22: TINFO: FIPS: on
tst_af_alg.c:36: TCONF: FIPS enabled => skcipher algorithm 'salsa20' disabled

./af_alg04
tst_af_alg.c:81: TBROK: unexpected error binding AF_ALG socket to hash algorithm 'vmac64(aes)': ELIBBAD (80)
become
tst_fips.c:22: TINFO: FIPS: on
tst_af_alg.c:82: TCONF: FIPS enabled => hash algorithm 'vmac64(aes)' disabled
af_alg04.c:32: TCONF: kernel doesn't have hash algorithm 'vmac(aes)'
af_alg04.c:32: TCONF: kernel doesn't have hash algorithm 'vmac(sm4)'
af_alg04.c:32: TCONF: kernel doesn't have hash algorithm 'vmac(sm4-generic)'

af_alg01.c adjusted not to print TCONF twice.

Tested on Debian stable bullseye and SLES 15-SP4.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Hi,

I was wrong, although SUSE has some custom patches for FIPS to disable
ciphers in drivers/crypto, patch is for mainline, because it returns
ELIBBAD for algorithms it considers non-FIPS-approved.

Also, while it's not that easy to run fips=1 on current openSUSE
Tumbleweed or Fedora 34 (there are probably some restricted ciphers
boot (systemd?) depends on), at least Debian stable boots and restrict
ciphers as expected.

NOTE: do we want to optimize repeated fips detection or repeated output?
(didn't see any elegant solution).

Kind regards,
Petr

 include/tst_af_alg.h               |  3 ++-
 lib/tst_af_alg.c                   | 16 +++++++++++++++-
 testcases/kernel/crypto/af_alg01.c |  6 ++++--
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/include/tst_af_alg.h b/include/tst_af_alg.h
index fd2ff06478..264e226a2c 100644
--- a/include/tst_af_alg.h
+++ b/include/tst_af_alg.h
@@ -61,7 +61,8 @@ void tst_alg_bind(int algfd, const char *algtype, const char *algname);
  * @param algname The name of the algorithm, such as "sha256" or "xts(aes)"
  *
  * Return true if the algorithm is available, or false if unavailable.
- * If another error occurs, tst_brk() is called with TBROK.
+ * If another error occurs, tst_brk() is called with TBROK,
+ * unless algorithm enabled due FIPS mode (errno ELIBBAD).
  */
 bool tst_have_alg(const char *algtype, const char *algname);
 
diff --git a/lib/tst_af_alg.c b/lib/tst_af_alg.c
index 05caa63016..9325a98432 100644
--- a/lib/tst_af_alg.c
+++ b/lib/tst_af_alg.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * Copyright 2019 Google LLC
+ * Copyright (c) Linux Test Project, 2019-2021
  */
 
 #include <errno.h>
@@ -30,10 +31,18 @@ void tst_alg_bind_addr(int algfd, const struct sockaddr_alg *addr)
 
 	if (ret == 0)
 		return;
+
+	if (errno == ELIBBAD && tst_fips_enabled()) {
+		tst_brk(TCONF,
+			"FIPS enabled => %s algorithm '%s' disabled",
+			addr->salg_type, addr->salg_name);
+	}
+
 	if (errno == ENOENT) {
 		tst_brk(TCONF, "kernel doesn't support %s algorithm '%s'",
 			addr->salg_type, addr->salg_name);
 	}
+
 	tst_brk(TBROK | TERRNO,
 		"unexpected error binding AF_ALG socket to %s algorithm '%s'",
 		addr->salg_type, addr->salg_name);
@@ -77,11 +86,16 @@ bool tst_have_alg(const char *algtype, const char *algname)
 
 	ret = bind(algfd, (const struct sockaddr *)&addr, sizeof(addr));
 	if (ret != 0) {
-		if (errno != ENOENT) {
+		if (errno == ELIBBAD && tst_fips_enabled()) {
+			tst_res(TCONF,
+				"FIPS enabled => %s algorithm '%s' disabled",
+				algtype, algname);
+		} else if (errno != ENOENT) {
 			tst_brk(TBROK | TERRNO,
 				"unexpected error binding AF_ALG socket to %s algorithm '%s'",
 				algtype, algname);
 		}
+
 		have_alg = false;
 	}
 
diff --git a/testcases/kernel/crypto/af_alg01.c b/testcases/kernel/crypto/af_alg01.c
index 47292ee328..e31126fe01 100644
--- a/testcases/kernel/crypto/af_alg01.c
+++ b/testcases/kernel/crypto/af_alg01.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * Copyright 2019 Google LLC
+ * Copyright (c) Linux Test Project, 2019-2021
  */
 
 /*
@@ -22,8 +23,9 @@ static void test_with_hash_alg(const char *hash_algname)
 	char key[4096] = { 0 };
 
 	if (!tst_have_alg("hash", hash_algname)) {
-		tst_res(TCONF, "kernel doesn't have hash algorithm '%s'",
-			hash_algname);
+		if (errno != ELIBBAD)
+			tst_res(TCONF, "kernel doesn't have hash algorithm '%s'",
+				hash_algname);
 		return;
 	}
 	sprintf(hmac_algname, "hmac(%s)", hash_algname);
-- 
2.34.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/1] tst_af_alg: Another fix for disabled weak cipher

[Cyril Hrubis]

Hi!
> e.g. md5 on enabled FIPS.
> Similar fix to 4fa302ef9d. It fixes:
> 
> ./af_alg01
> tst_af_alg.c:84: TBROK: unexpected error binding AF_ALG socket to hash algorithm 'md5': ELIBBAD (80)
> become
> tst_fips.c:22: TINFO: FIPS: on
> tst_af_alg.c:82: TCONF: FIPS enabled => hash algorithm 'md5' disabled
> tst_fips.c:22: TINFO: FIPS: on
> tst_af_alg.c:82: TCONF: FIPS enabled => hash algorithm 'md5-generic' disabled
> 
> ./af_alg02
> tst_af_alg.c:37: TBROK: unexpected error binding AF_ALG socket to skcipher algorithm 'salsa20': ELIBBAD (80)
> become
> tst_fips.c:22: TINFO: FIPS: on
> tst_af_alg.c:36: TCONF: FIPS enabled => skcipher algorithm 'salsa20' disabled
> 
> ./af_alg04
> tst_af_alg.c:81: TBROK: unexpected error binding AF_ALG socket to hash algorithm 'vmac64(aes)': ELIBBAD (80)
> become
> tst_fips.c:22: TINFO: FIPS: on
> tst_af_alg.c:82: TCONF: FIPS enabled => hash algorithm 'vmac64(aes)' disabled
> af_alg04.c:32: TCONF: kernel doesn't have hash algorithm 'vmac(aes)'
> af_alg04.c:32: TCONF: kernel doesn't have hash algorithm 'vmac(sm4)'
> af_alg04.c:32: TCONF: kernel doesn't have hash algorithm 'vmac(sm4-generic)'
> 
> af_alg01.c adjusted not to print TCONF twice.
> 
> Tested on Debian stable bullseye and SLES 15-SP4.
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> Hi,
> 
> I was wrong, although SUSE has some custom patches for FIPS to disable
> ciphers in drivers/crypto, patch is for mainline, because it returns
> ELIBBAD for algorithms it considers non-FIPS-approved.
> 
> Also, while it's not that easy to run fips=1 on current openSUSE
> Tumbleweed or Fedora 34 (there are probably some restricted ciphers
> boot (systemd?) depends on), at least Debian stable boots and restrict
> ciphers as expected.
> 
> NOTE: do we want to optimize repeated fips detection or repeated output?
> (didn't see any elegant solution).
> 
> Kind regards,
> Petr
> 
>  include/tst_af_alg.h               |  3 ++-
>  lib/tst_af_alg.c                   | 16 +++++++++++++++-
>  testcases/kernel/crypto/af_alg01.c |  6 ++++--
>  3 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/include/tst_af_alg.h b/include/tst_af_alg.h
> index fd2ff06478..264e226a2c 100644
> --- a/include/tst_af_alg.h
> +++ b/include/tst_af_alg.h
> @@ -61,7 +61,8 @@ void tst_alg_bind(int algfd, const char *algtype, const char *algname);
>   * @param algname The name of the algorithm, such as "sha256" or "xts(aes)"
>   *
>   * Return true if the algorithm is available, or false if unavailable.
> - * If another error occurs, tst_brk() is called with TBROK.
> + * If another error occurs, tst_brk() is called with TBROK,
> + * unless algorithm enabled due FIPS mode (errno ELIBBAD).
                      ^
		      is disabled
>   */
>  bool tst_have_alg(const char *algtype, const char *algname);
>  
> diff --git a/lib/tst_af_alg.c b/lib/tst_af_alg.c
> index 05caa63016..9325a98432 100644
> --- a/lib/tst_af_alg.c
> +++ b/lib/tst_af_alg.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  /*
>   * Copyright 2019 Google LLC
> + * Copyright (c) Linux Test Project, 2019-2021
>   */
>  
>  #include <errno.h>
> @@ -30,10 +31,18 @@ void tst_alg_bind_addr(int algfd, const struct sockaddr_alg *addr)
>  
>  	if (ret == 0)
>  		return;
> +
> +	if (errno == ELIBBAD && tst_fips_enabled()) {
> +		tst_brk(TCONF,
> +			"FIPS enabled => %s algorithm '%s' disabled",
> +			addr->salg_type, addr->salg_name);
> +	}
> +
>  	if (errno == ENOENT) {
>  		tst_brk(TCONF, "kernel doesn't support %s algorithm '%s'",
>  			addr->salg_type, addr->salg_name);
>  	}
> +
>  	tst_brk(TBROK | TERRNO,
>  		"unexpected error binding AF_ALG socket to %s algorithm '%s'",
>  		addr->salg_type, addr->salg_name);
> @@ -77,11 +86,16 @@ bool tst_have_alg(const char *algtype, const char *algname)
>  
>  	ret = bind(algfd, (const struct sockaddr *)&addr, sizeof(addr));
>  	if (ret != 0) {
> -		if (errno != ENOENT) {
> +		if (errno == ELIBBAD && tst_fips_enabled()) {
> +			tst_res(TCONF,
> +				"FIPS enabled => %s algorithm '%s' disabled",
> +				algtype, algname);
> +		} else if (errno != ENOENT) {
>  			tst_brk(TBROK | TERRNO,
>  				"unexpected error binding AF_ALG socket to %s algorithm '%s'",
>  				algtype, algname);
>  		}
> +
>  		have_alg = false;
>  	}
>  
> diff --git a/testcases/kernel/crypto/af_alg01.c b/testcases/kernel/crypto/af_alg01.c
> index 47292ee328..e31126fe01 100644
> --- a/testcases/kernel/crypto/af_alg01.c
> +++ b/testcases/kernel/crypto/af_alg01.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  /*
>   * Copyright 2019 Google LLC
> + * Copyright (c) Linux Test Project, 2019-2021
>   */
>  
>  /*
> @@ -22,8 +23,9 @@ static void test_with_hash_alg(const char *hash_algname)
>  	char key[4096] = { 0 };
>  
>  	if (!tst_have_alg("hash", hash_algname)) {
> -		tst_res(TCONF, "kernel doesn't have hash algorithm '%s'",
> -			hash_algname);
> +		if (errno != ELIBBAD)
> +			tst_res(TCONF, "kernel doesn't have hash algorithm '%s'",
> +				hash_algname);

What about moving the tst_res(TCONF, ...) in the case of ENOENT to the
tst_have_alg() function?

That way we would have here just

	if (!tst_have_alg("hash", hash_algname))
		return;

Other than these two minor things this version does look good:

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

>  		return;
>  	}
>  	sprintf(hmac_algname, "hmac(%s)", hash_algname);
> -- 
> 2.34.1
> 

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/1] tst_af_alg: Another fix for disabled weak cipher

[Petr Vorel]

Hi Cyril,

[ Cc Eric ]
> Hi!
> > e.g. md5 on enabled FIPS.
> > Similar fix to 4fa302ef9d. It fixes:

> > ./af_alg01
> > tst_af_alg.c:84: TBROK: unexpected error binding AF_ALG socket to hash algorithm 'md5': ELIBBAD (80)
> > become
> > tst_fips.c:22: TINFO: FIPS: on
> > tst_af_alg.c:82: TCONF: FIPS enabled => hash algorithm 'md5' disabled
> > tst_fips.c:22: TINFO: FIPS: on
> > tst_af_alg.c:82: TCONF: FIPS enabled => hash algorithm 'md5-generic' disabled

> > ./af_alg02
> > tst_af_alg.c:37: TBROK: unexpected error binding AF_ALG socket to skcipher algorithm 'salsa20': ELIBBAD (80)
> > become
> > tst_fips.c:22: TINFO: FIPS: on
> > tst_af_alg.c:36: TCONF: FIPS enabled => skcipher algorithm 'salsa20' disabled

> > ./af_alg04
> > tst_af_alg.c:81: TBROK: unexpected error binding AF_ALG socket to hash algorithm 'vmac64(aes)': ELIBBAD (80)
> > become
> > tst_fips.c:22: TINFO: FIPS: on
> > tst_af_alg.c:82: TCONF: FIPS enabled => hash algorithm 'vmac64(aes)' disabled
> > af_alg04.c:32: TCONF: kernel doesn't have hash algorithm 'vmac(aes)'
> > af_alg04.c:32: TCONF: kernel doesn't have hash algorithm 'vmac(sm4)'
> > af_alg04.c:32: TCONF: kernel doesn't have hash algorithm 'vmac(sm4-generic)'

> > af_alg01.c adjusted not to print TCONF twice.

> > Tested on Debian stable bullseye and SLES 15-SP4.

> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> > Hi,

> > I was wrong, although SUSE has some custom patches for FIPS to disable
> > ciphers in drivers/crypto, patch is for mainline, because it returns
> > ELIBBAD for algorithms it considers non-FIPS-approved.

> > Also, while it's not that easy to run fips=1 on current openSUSE
> > Tumbleweed or Fedora 34 (there are probably some restricted ciphers
> > boot (systemd?) depends on), at least Debian stable boots and restrict
> > ciphers as expected.

> > NOTE: do we want to optimize repeated fips detection or repeated output?
> > (didn't see any elegant solution).

> > Kind regards,
> > Petr

> >  include/tst_af_alg.h               |  3 ++-
> >  lib/tst_af_alg.c                   | 16 +++++++++++++++-
> >  testcases/kernel/crypto/af_alg01.c |  6 ++++--
> >  3 files changed, 21 insertions(+), 4 deletions(-)

> > diff --git a/include/tst_af_alg.h b/include/tst_af_alg.h
> > index fd2ff06478..264e226a2c 100644
> > --- a/include/tst_af_alg.h
> > +++ b/include/tst_af_alg.h
> > @@ -61,7 +61,8 @@ void tst_alg_bind(int algfd, const char *algtype, const char *algname);
> >   * @param algname The name of the algorithm, such as "sha256" or "xts(aes)"
> >   *
> >   * Return true if the algorithm is available, or false if unavailable.
> > - * If another error occurs, tst_brk() is called with TBROK.
> > + * If another error occurs, tst_brk() is called with TBROK,
> > + * unless algorithm enabled due FIPS mode (errno ELIBBAD).
>                       ^
> 		      is disabled

Thanks!

> >   */
> >  bool tst_have_alg(const char *algtype, const char *algname);

> > diff --git a/lib/tst_af_alg.c b/lib/tst_af_alg.c
> > index 05caa63016..9325a98432 100644
> > --- a/lib/tst_af_alg.c
> > +++ b/lib/tst_af_alg.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0-or-later
> >  /*
> >   * Copyright 2019 Google LLC
> > + * Copyright (c) Linux Test Project, 2019-2021
> >   */

> >  #include <errno.h>
> > @@ -30,10 +31,18 @@ void tst_alg_bind_addr(int algfd, const struct sockaddr_alg *addr)

> >  	if (ret == 0)
> >  		return;
> > +
> > +	if (errno == ELIBBAD && tst_fips_enabled()) {
> > +		tst_brk(TCONF,
> > +			"FIPS enabled => %s algorithm '%s' disabled",
> > +			addr->salg_type, addr->salg_name);
> > +	}
> > +
> >  	if (errno == ENOENT) {
> >  		tst_brk(TCONF, "kernel doesn't support %s algorithm '%s'",
> >  			addr->salg_type, addr->salg_name);
> >  	}
> > +
> >  	tst_brk(TBROK | TERRNO,
> >  		"unexpected error binding AF_ALG socket to %s algorithm '%s'",
> >  		addr->salg_type, addr->salg_name);
> > @@ -77,11 +86,16 @@ bool tst_have_alg(const char *algtype, const char *algname)

> >  	ret = bind(algfd, (const struct sockaddr *)&addr, sizeof(addr));
> >  	if (ret != 0) {
> > -		if (errno != ENOENT) {
> > +		if (errno == ELIBBAD && tst_fips_enabled()) {
> > +			tst_res(TCONF,
> > +				"FIPS enabled => %s algorithm '%s' disabled",
> > +				algtype, algname);
> > +		} else if (errno != ENOENT) {
> >  			tst_brk(TBROK | TERRNO,
> >  				"unexpected error binding AF_ALG socket to %s algorithm '%s'",
> >  				algtype, algname);
> >  		}
> > +
> >  		have_alg = false;
> >  	}

> > diff --git a/testcases/kernel/crypto/af_alg01.c b/testcases/kernel/crypto/af_alg01.c
> > index 47292ee328..e31126fe01 100644
> > --- a/testcases/kernel/crypto/af_alg01.c
> > +++ b/testcases/kernel/crypto/af_alg01.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0-or-later
> >  /*
> >   * Copyright 2019 Google LLC
> > + * Copyright (c) Linux Test Project, 2019-2021
> >   */

> >  /*
> > @@ -22,8 +23,9 @@ static void test_with_hash_alg(const char *hash_algname)
> >  	char key[4096] = { 0 };

> >  	if (!tst_have_alg("hash", hash_algname)) {
> > -		tst_res(TCONF, "kernel doesn't have hash algorithm '%s'",
> > -			hash_algname);
> > +		if (errno != ELIBBAD)
> > +			tst_res(TCONF, "kernel doesn't have hash algorithm '%s'",
> > +				hash_algname);

> What about moving the tst_res(TCONF, ...) in the case of ENOENT to the
> tst_have_alg() function?

> That way we would have here just

> 	if (!tst_have_alg("hash", hash_algname))
> 		return;

Hm, if I haven't overlook anything, it could work even for af_alg04.c,
which uses it !tst_have_alg() once without TCONF:

 28     sprintf(vmac_algname, "vmac64(%s)", symm_enc_algname);
 29     if (!tst_have_alg("hash", vmac_algname)) {
 30         sprintf(vmac_algname, "vmac(%s)", symm_enc_algname);

Moved to tst_have_alg():
tst_fips.c:22: TINFO: FIPS: on
tst_af_alg.c:90: TCONF: FIPS enabled => hash algorithm 'vmac64(aes)' disabled
tst_af_alg.c:94: TCONF: kernel doesn't have hash algorithm 'vmac(aes)'
tst_af_alg.c:94: TCONF: kernel doesn't have hash algorithm 'vmac64(sm4)'
tst_af_alg.c:94: TCONF: kernel doesn't have hash algorithm 'vmac(sm4)'
tst_af_alg.c:94: TCONF: kernel doesn't have hash algorithm 'vmac64(sm4-generic)'
tst_af_alg.c:94: TCONF: kernel doesn't have hash algorithm 'vmac(sm4-generic)'

=> I'll send v3.

Kind regards,
Petr

> Other than these two minor things this version does look good:

> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

> >  		return;
> >  	}
> >  	sprintf(hmac_algname, "hmac(%s)", hash_algname);
> > -- 
> > 2.34.1

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/1] tst_af_alg: Another fix for disabled weak cipher

[Petr Vorel]

Hi Cyril,

...
> > >  	if (!tst_have_alg("hash", hash_algname)) {
> > > -		tst_res(TCONF, "kernel doesn't have hash algorithm '%s'",
> > > -			hash_algname);
> > > +		if (errno != ELIBBAD)
> > > +			tst_res(TCONF, "kernel doesn't have hash algorithm '%s'",
> > > +				hash_algname);

> > What about moving the tst_res(TCONF, ...) in the case of ENOENT to the
> > tst_have_alg() function?

> > That way we would have here just

> > 	if (!tst_have_alg("hash", hash_algname))
> > 		return;

> Hm, if I haven't overlook anything, it could work even for af_alg04.c,
> which uses it !tst_have_alg() once without TCONF:

>  28     sprintf(vmac_algname, "vmac64(%s)", symm_enc_algname);
>  29     if (!tst_have_alg("hash", vmac_algname)) {
>  30         sprintf(vmac_algname, "vmac(%s)", symm_enc_algname);

> Moved to tst_have_alg():
> tst_fips.c:22: TINFO: FIPS: on
> tst_af_alg.c:90: TCONF: FIPS enabled => hash algorithm 'vmac64(aes)' disabled
> tst_af_alg.c:94: TCONF: kernel doesn't have hash algorithm 'vmac(aes)'
> tst_af_alg.c:94: TCONF: kernel doesn't have hash algorithm 'vmac64(sm4)'
> tst_af_alg.c:94: TCONF: kernel doesn't have hash algorithm 'vmac(sm4)'
> tst_af_alg.c:94: TCONF: kernel doesn't have hash algorithm 'vmac64(sm4-generic)'
> tst_af_alg.c:94: TCONF: kernel doesn't have hash algorithm 'vmac(sm4-generic)'

> => I'll send v3.

OK, this would not work for af_alg03.c, where false positive TCONF would be
printed:
tst_test.c:1426: TINFO: Timeout per run is 0h 05m 00s
tst_af_alg.c:81: TCONF: kernel doesn't have aead algorithm 'rfc7539(chacha20,sha256)'
af_alg03.c:24: TPASS: couldn't instantiate rfc7539 template with wrong digest size

Kind regards,
Petr

> Kind regards,
> Petr

> > Other than these two minor things this version does look good:

> > Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

> > >  		return;
> > >  	}
> > >  	sprintf(hmac_algname, "hmac(%s)", hash_algname);
> > > -- 
> > > 2.34.1

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/1] tst_af_alg: Another fix for disabled weak cipher

[Eric Biggers]

On Mon, Dec 20, 2021 at 10:27:56PM +0100, Petr Vorel wrote:
> tst_af_alg.c:84: TBROK: unexpected error binding AF_ALG socket to hash algorithm 'md5': ELIBBAD (80)

This seems like a kernel bug; shouldn't the kernel report ENOENT for the
algorithms that fips_enabled isn't allowing, just like other algorithms that
aren't available?  Have you checked with linux-crypto@vger.kernel.org that the
current behavior is actually intentional?

> @@ -77,11 +86,16 @@ bool tst_have_alg(const char *algtype, const char *algname)
>  
>  	ret = bind(algfd, (const struct sockaddr *)&addr, sizeof(addr));
>  	if (ret != 0) {
> -		if (errno != ENOENT) {
> +		if (errno == ELIBBAD && tst_fips_enabled()) {
> +			tst_res(TCONF,
> +				"FIPS enabled => %s algorithm '%s' disabled",
> +				algtype, algname);
> +		} else if (errno != ENOENT) {
>  			tst_brk(TBROK | TERRNO,
>  				"unexpected error binding AF_ALG socket to %s algorithm '%s'",
>  				algtype, algname);
>  		}
> +
>  		have_alg = false;
>  	}

This function is supposed to return false if the algorithm isn't available; it
shouldn't be skipping the test.

> @@ -22,8 +23,9 @@ static void test_with_hash_alg(const char *hash_algname)
>  	char key[4096] = { 0 };
>  
>  	if (!tst_have_alg("hash", hash_algname)) {
> -		tst_res(TCONF, "kernel doesn't have hash algorithm '%s'",
> -			hash_algname);
> +		if (errno != ELIBBAD)
> +			tst_res(TCONF, "kernel doesn't have hash algorithm '%s'",
> +				hash_algname);
>  		return;
>  	}
>  	sprintf(hmac_algname, "hmac(%s)", hash_algname);

Why treat this case any differently from any other hash algorithm that isn't
available?

- Eric

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/1] tst_af_alg: Another fix for disabled weak cipher

[Cyril Hrubis]

Hi!
> OK, this would not work for af_alg03.c, where false positive TCONF would be
> printed:
> tst_test.c:1426: TINFO: Timeout per run is 0h 05m 00s
> tst_af_alg.c:81: TCONF: kernel doesn't have aead algorithm 'rfc7539(chacha20,sha256)'
> af_alg03.c:24: TPASS: couldn't instantiate rfc7539 template with wrong digest size

Hmm, so af_alg actually passes wrong data to the tst_have_alg() on
purpose.

I guess that if we want to move the TCONF to the library we either have
to add a flag to the function or split it into a two. Not sure which one
is actually better.

Maybe we should split it into two functions, one that wouldn't do
anything but return the errno and one that would switch over that errno
and print messages as well. Something as:


int tst_try_alg(const char *algtype, const char *algname)
{
	...
	int retval = 0;

	if (ret != 0)
		retval = errno;

	close(algfd);
	return retval;
}


bool tst_have_alg(const char *algtype, const char *algname)
{
	ret = tst_try_alg(algtype, algname);

	switch (ret) {
	case 0:
		return true;
	case ENOENT:
		tst_res(TCONF, ...);
		return false;
	case ELIBBAD:
		if (tst_fips_enabled())
			return false;
	/* fallthrough */
	default:
		errno = ret;
		tst_brk(TBROK | TERRNO, ...);
	break;
	}
}


The the af_alg03 can call tst_try_alg() and check if the retval is
ENOENT.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/1] tst_af_alg: Another fix for disabled weak cipher

[Petr Vorel]

Hi Eric,

> Hi!
> > OK, this would not work for af_alg03.c, where false positive TCONF would be
> > printed:
> > tst_test.c:1426: TINFO: Timeout per run is 0h 05m 00s
> > tst_af_alg.c:81: TCONF: kernel doesn't have aead algorithm 'rfc7539(chacha20,sha256)'
> > af_alg03.c:24: TPASS: couldn't instantiate rfc7539 template with wrong digest size

> Hmm, so af_alg actually passes wrong data to the tst_have_alg() on
> purpose.

> I guess that if we want to move the TCONF to the library we either have
> to add a flag to the function or split it into a two. Not sure which one
> is actually better.
I was thinking of both (preferred split), but wasn't sure if it's worth of just
just to filter out "tst_fips.c:22: TINFO: FIPS: on" output.

> Maybe we should split it into two functions, one that wouldn't do
> anything but return the errno and one that would switch over that errno
> and print messages as well. Something as:


> int tst_try_alg(const char *algtype, const char *algname)
> {
> 	...
> 	int retval = 0;

> 	if (ret != 0)
> 		retval = errno;

> 	close(algfd);
> 	return retval;
> }


> bool tst_have_alg(const char *algtype, const char *algname)
> {
> 	ret = tst_try_alg(algtype, algname);

> 	switch (ret) {
> 	case 0:
> 		return true;
> 	case ENOENT:
> 		tst_res(TCONF, ...);
> 		return false;
> 	case ELIBBAD:
> 		if (tst_fips_enabled())
> 			return false;
> 	/* fallthrough */
> 	default:
> 		errno = ret;
> 		tst_brk(TBROK | TERRNO, ...);
> 	break;
> 	}
> }

> The the af_alg03 can call tst_try_alg() and check if the retval is
> ENOENT.
This looks good, thx! Simple enough to merge directly, but I'll send v3.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/1] tst_af_alg: Another fix for disabled weak cipher

[Petr Vorel]

Hi Eric,

> On Mon, Dec 20, 2021 at 10:27:56PM +0100, Petr Vorel wrote:
> > tst_af_alg.c:84: TBROK: unexpected error binding AF_ALG socket to hash algorithm 'md5': ELIBBAD (80)

> This seems like a kernel bug; shouldn't the kernel report ENOENT for the
> algorithms that fips_enabled isn't allowing, just like other algorithms that
> aren't available?  Have you checked with linux-crypto@vger.kernel.org that the
> current behavior is actually intentional?
It reports ELIBBAD. Am I missing something?

> > @@ -77,11 +86,16 @@ bool tst_have_alg(const char *algtype, const char *algname)

> >  	ret = bind(algfd, (const struct sockaddr *)&addr, sizeof(addr));
> >  	if (ret != 0) {
> > -		if (errno != ENOENT) {
> > +		if (errno == ELIBBAD && tst_fips_enabled()) {
> > +			tst_res(TCONF,
> > +				"FIPS enabled => %s algorithm '%s' disabled",
> > +				algtype, algname);
> > +		} else if (errno != ENOENT) {
> >  			tst_brk(TBROK | TERRNO,
> >  				"unexpected error binding AF_ALG socket to %s algorithm '%s'",
> >  				algtype, algname);
> >  		}
> > +
> >  		have_alg = false;
> >  	}

> This function is supposed to return false if the algorithm isn't available; it
> shouldn't be skipping the test.
Sure, but split into 2 functions (add tst_try_alg() and use it in
tst_have_alg()) suggested by Cyril should solve it.

> > @@ -22,8 +23,9 @@ static void test_with_hash_alg(const char *hash_algname)
> >  	char key[4096] = { 0 };

> >  	if (!tst_have_alg("hash", hash_algname)) {
> > -		tst_res(TCONF, "kernel doesn't have hash algorithm '%s'",
> > -			hash_algname);
> > +		if (errno != ELIBBAD)
> > +			tst_res(TCONF, "kernel doesn't have hash algorithm '%s'",
> > +				hash_algname);
> >  		return;
> >  	}
> >  	sprintf(hmac_algname, "hmac(%s)", hash_algname);

> Why treat this case any differently from any other hash algorithm that isn't
> available?
I'm sorry The addition is left over from testing, it should have not been here.

Kind regards,
Petr

> - Eric

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/1] tst_af_alg: Another fix for disabled weak cipher

[Eric Biggers]

On Wed, Dec 22, 2021 at 06:01:37PM +0100, Petr Vorel wrote:
> Hi Eric,
> 
> > On Mon, Dec 20, 2021 at 10:27:56PM +0100, Petr Vorel wrote:
> > > tst_af_alg.c:84: TBROK: unexpected error binding AF_ALG socket to hash algorithm 'md5': ELIBBAD (80)
> 
> > This seems like a kernel bug; shouldn't the kernel report ENOENT for the
> > algorithms that fips_enabled isn't allowing, just like other algorithms that
> > aren't available?  Have you checked with linux-crypto@vger.kernel.org that the
> > current behavior is actually intentional?
> It reports ELIBBAD. Am I missing something?
> 

It does.  I am asking whether it should.  The purpose of LTP is to find kernel
bugs; perhaps it found a bug here?

- Eric

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/1] tst_af_alg: Another fix for disabled weak cipher

[Petr Vorel]

Hi Eric,

> On Wed, Dec 22, 2021 at 06:01:37PM +0100, Petr Vorel wrote:
> > Hi Eric,

> > > On Mon, Dec 20, 2021 at 10:27:56PM +0100, Petr Vorel wrote:
> > > > tst_af_alg.c:84: TBROK: unexpected error binding AF_ALG socket to hash algorithm 'md5': ELIBBAD (80)

> > > This seems like a kernel bug; shouldn't the kernel report ENOENT for the
> > > algorithms that fips_enabled isn't allowing, just like other algorithms that
> > > aren't available?  Have you checked with linux-crypto@vger.kernel.org that the
> > > current behavior is actually intentional?
> > It reports ELIBBAD. Am I missing something?


> It does.  I am asking whether it should.  The purpose of LTP is to find kernel
> bugs; perhaps it found a bug here?

Well, if I understand mainline kernel code correctly, although crypto/testmgr.c
in alg_test() returns -EINVAL for non-fips allowed algorithms, but that means
failing crypto API test. And the API in crypto_alg_lookup() returns -ELIBBAD for failed test.

But I'll check it with Herbert Xu.

Kind regards,
Petr

> - Eric

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/1] tst_af_alg: Another fix for disabled weak cipher

[Petr Vorel]

Hi all,

[Cc Herbert and Eric ]

FYI Herbert's view for using ELIBBAD instead of ENOENT (reply to Eric's question
whether using ELIBBAD in kernel is a good approach or bug) [1]:

"For the purpose of identifying FIPS-disabled algorithm (as opposed
to an algorithm that's not enabled in the kernel at all), I think
it is perfectly safe to use ELIBBAD instead of ENOENT in user-space."

I suppose that's justify my proposed changes (i.e. testing also ELIBBAD when
fips enabled).

@Herbert if you care, you can post your Acked-by: tag.

Kind regards,
Petr

[1] https://lore.kernel.org/linux-crypto/YclURQzYKqCMHWx6@gondor.apana.org.au/

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/1] tst_af_alg: Another fix for disabled weak cipher

[Herbert Xu]

On Tue, Jan 04, 2022 at 12:54:46PM +0100, Petr Vorel wrote:
> Hi all,
> 
> [Cc Herbert and Eric ]
> 
> FYI Herbert's view for using ELIBBAD instead of ENOENT (reply to Eric's question
> whether using ELIBBAD in kernel is a good approach or bug) [1]:
> 
> "For the purpose of identifying FIPS-disabled algorithm (as opposed
> to an algorithm that's not enabled in the kernel at all), I think
> it is perfectly safe to use ELIBBAD instead of ENOENT in user-space."
> 
> I suppose that's justify my proposed changes (i.e. testing also ELIBBAD when
> fips enabled).
> 
> @Herbert if you care, you can post your Acked-by: tag.

Please hold the horses on this patch.

I'm about to post a series of patches that aims to disable algorithms
such as sha1 in FIPS mode while still allowing compound algorithms such
as hmac(sha1) to work.

As a result of this series, ENOENT will again be returned for FIPS-
disallowed algorithms when in FIPS mode.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/1] tst_af_alg: Another fix for disabled weak cipher

[Petr Vorel]

Hi Herbert,

> On Tue, Jan 04, 2022 at 12:54:46PM +0100, Petr Vorel wrote:
> > Hi all,

> > [Cc Herbert and Eric ]

> > FYI Herbert's view for using ELIBBAD instead of ENOENT (reply to Eric's question
> > whether using ELIBBAD in kernel is a good approach or bug) [1]:

> > "For the purpose of identifying FIPS-disabled algorithm (as opposed
> > to an algorithm that's not enabled in the kernel at all), I think
> > it is perfectly safe to use ELIBBAD instead of ENOENT in user-space."

> > I suppose that's justify my proposed changes (i.e. testing also ELIBBAD when
> > fips enabled).

> > @Herbert if you care, you can post your Acked-by: tag.

> Please hold the horses on this patch.

I'm sorry, too late, already merged. But never mind, LTP is not tight to
particular kernel version (we tried to cover also very old releases), thus the
old releases will be covered with this commit, never ones with the default check
for ENOENT (regardless FIPS).

> I'm about to post a series of patches that aims to disable algorithms
> such as sha1 in FIPS mode while still allowing compound algorithms such
> as hmac(sha1) to work.
Thanks for notifying.

> As a result of this series, ENOENT will again be returned for FIPS-
> disallowed algorithms when in FIPS mode.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp