[LTP] [PATCH v2 0/3] ltp: new test for crypto_user information leak bug

[LTP] [PATCH v2 0/3] ltp: new test for crypto_user information leak bug

[Eric Biggers]

This series adds a test for an information leak bug in the crypto user
configuration API which was recently fixed.

Changed since v1:
  - Check all attributes, not just the first of each type
  - Work around dependency bug in old kernel headers
  - Rename __check_for_leaks() to do_check_for_leaks()
  - Use SPDX license identifier

Eric Biggers (3):
  lapi/cryptouser.h: add more declarations
  tst_netlink: inline functions in header
  crypto/crypto_user01.c: new test for information leak bug

 include/lapi/cryptouser.h               |  73 +++++++++
 include/tst_netlink.h                   |  11 +-
 runtest/crypto                          |   1 +
 runtest/cve                             |   2 +
 testcases/kernel/crypto/.gitignore      |   1 +
 testcases/kernel/crypto/crypto_user01.c | 195 ++++++++++++++++++++++++
 6 files changed, 278 insertions(+), 5 deletions(-)
 create mode 100644 testcases/kernel/crypto/crypto_user01.c

-- 
2.19.2

[LTP] [PATCH v2 1/3] lapi/cryptouser.h: add more declarations

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Add some missing declarations for the crypto user configuration API,
a.k.a. NETLINK_CRYPTO.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 include/lapi/cryptouser.h | 73 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/include/lapi/cryptouser.h b/include/lapi/cryptouser.h
index 61ff21ad3..94d641446 100644
--- a/include/lapi/cryptouser.h
+++ b/include/lapi/cryptouser.h
@@ -34,6 +34,24 @@ enum {
 	__CRYPTO_MSG_MAX
 };
 
+enum crypto_attr_type_t {
+	CRYPTOCFGA_UNSPEC,
+	CRYPTOCFGA_PRIORITY_VAL,	/* uint32_t */
+	CRYPTOCFGA_REPORT_LARVAL,	/* struct crypto_report_larval */
+	CRYPTOCFGA_REPORT_HASH,		/* struct crypto_report_hash */
+	CRYPTOCFGA_REPORT_BLKCIPHER,	/* struct crypto_report_blkcipher */
+	CRYPTOCFGA_REPORT_AEAD,		/* struct crypto_report_aead */
+	CRYPTOCFGA_REPORT_COMPRESS,	/* struct crypto_report_comp */
+	CRYPTOCFGA_REPORT_RNG,		/* struct crypto_report_rng */
+	CRYPTOCFGA_REPORT_CIPHER,	/* struct crypto_report_cipher */
+	CRYPTOCFGA_REPORT_AKCIPHER,	/* struct crypto_report_akcipher */
+	CRYPTOCFGA_REPORT_KPP,		/* struct crypto_report_kpp */
+	CRYPTOCFGA_REPORT_ACOMP,	/* struct crypto_report_acomp */
+	__CRYPTOCFGA_MAX
+
+#define CRYPTOCFGA_MAX (__CRYPTOCFGA_MAX - 1)
+};
+
 struct crypto_user_alg {
 	char cru_name[CRYPTO_MAX_NAME];
 	char cru_driver_name[CRYPTO_MAX_NAME];
@@ -44,6 +62,61 @@ struct crypto_user_alg {
 	uint32_t cru_flags;
 };
 
+struct crypto_report_larval {
+	char type[CRYPTO_MAX_NAME];
+};
+
+struct crypto_report_hash {
+	char type[CRYPTO_MAX_NAME];
+	unsigned int blocksize;
+	unsigned int digestsize;
+};
+
+struct crypto_report_cipher {
+	char type[CRYPTO_MAX_NAME];
+	unsigned int blocksize;
+	unsigned int min_keysize;
+	unsigned int max_keysize;
+};
+
+struct crypto_report_blkcipher {
+	char type[CRYPTO_MAX_NAME];
+	char geniv[CRYPTO_MAX_NAME];
+	unsigned int blocksize;
+	unsigned int min_keysize;
+	unsigned int max_keysize;
+	unsigned int ivsize;
+};
+
+struct crypto_report_aead {
+	char type[CRYPTO_MAX_NAME];
+	char geniv[CRYPTO_MAX_NAME];
+	unsigned int blocksize;
+	unsigned int maxauthsize;
+	unsigned int ivsize;
+};
+
+struct crypto_report_comp {
+	char type[CRYPTO_MAX_NAME];
+};
+
+struct crypto_report_rng {
+	char type[CRYPTO_MAX_NAME];
+	unsigned int seedsize;
+};
+
+struct crypto_report_akcipher {
+	char type[CRYPTO_MAX_NAME];
+};
+
+struct crypto_report_kpp {
+	char type[CRYPTO_MAX_NAME];
+};
+
+struct crypto_report_acomp {
+	char type[CRYPTO_MAX_NAME];
+};
+
 #endif	/* HAVE_LINUX_CRYPTOUSER_H */
 
 /* These are taken from include/crypto.h in the kernel tree. They are not
-- 
2.19.2

[LTP] [PATCH v2 2/3] tst_netlink: inline functions in header

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

safe_netlink_send() and safe_netlink_recv() need to be static inline,
since they're defined in a .h file.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 include/tst_netlink.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/tst_netlink.h b/include/tst_netlink.h
index 42232a169..892ef8f78 100644
--- a/include/tst_netlink.h
+++ b/include/tst_netlink.h
@@ -33,9 +33,9 @@
 #endif
 
 /** @private */
-ssize_t safe_netlink_send(const char *file, const int lineno,
-			  int fd, const struct nlmsghdr *nh,
-			  const void *payload)
+static inline ssize_t safe_netlink_send(const char *file, const int lineno,
+					int fd, const struct nlmsghdr *nh,
+					const void *payload)
 {
 	struct sockaddr_nl sa = { .nl_family = AF_NETLINK };
 	struct iovec iov[2] = {
@@ -68,8 +68,9 @@ ssize_t safe_netlink_send(const char *file, const int lineno,
 	safe_netlink_send(__FILE__, __LINE__, fd, nl_header, payload)
 
 /** @private */
-ssize_t safe_netlink_recv(const char *file, const int lineno,
-			  int fd, char *nl_headers_buf, size_t buf_len)
+static inline ssize_t safe_netlink_recv(const char *file, const int lineno,
+					int fd, char *nl_headers_buf,
+					size_t buf_len)
 {
 	struct iovec iov = { nl_headers_buf, buf_len };
 	struct sockaddr_nl sa;
-- 
2.19.2

[LTP] [PATCH v2 3/3] crypto/crypto_user01.c: new test for information leak bug

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Test for a bug in the crypto user configuration API (NETLINK_CRYPTO)
that leaked uninitialized memory to userspace.  This bug was assigned
CVE-2018-19854, and it was also a re-introduction of CVE-2013-2547.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 runtest/crypto                          |   1 +
 runtest/cve                             |   2 +
 testcases/kernel/crypto/.gitignore      |   1 +
 testcases/kernel/crypto/crypto_user01.c | 195 ++++++++++++++++++++++++
 4 files changed, 199 insertions(+)
 create mode 100644 testcases/kernel/crypto/crypto_user01.c

diff --git a/runtest/crypto b/runtest/crypto
index e5ba61e5e..cdbc44cc8 100644
--- a/runtest/crypto
+++ b/runtest/crypto
@@ -1 +1,2 @@
 pcrypt_aead01 pcrypt_aead01
+crypto_user01 crypto_user01
diff --git a/runtest/cve b/runtest/cve
index c4ba74186..78a5d8db2 100644
--- a/runtest/cve
+++ b/runtest/cve
@@ -3,6 +3,7 @@ cve-2011-0999 thp01 -I 120
 cve-2011-2183 ksm05 -I 10
 cve-2011-2496 vma03
 cve-2012-0957 uname04
+cve-2013-2547 crypto_user01
 cve-2014-0196 cve-2014-0196
 cve-2015-0235 gethostbyname_r01
 cve-2015-7550 keyctl02
@@ -36,3 +37,4 @@ cve-2017-17053 cve-2017-17053
 cve-2017-18075 pcrypt_aead01
 cve-2018-5803 sctp_big_chunk
 cve-2018-1000001 realpath01
+cve-2018-19854 crypto_user01
diff --git a/testcases/kernel/crypto/.gitignore b/testcases/kernel/crypto/.gitignore
index fafe5c972..759592fbd 100644
--- a/testcases/kernel/crypto/.gitignore
+++ b/testcases/kernel/crypto/.gitignore
@@ -1 +1,2 @@
 pcrypt_aead01
+crypto_user01
diff --git a/testcases/kernel/crypto/crypto_user01.c b/testcases/kernel/crypto/crypto_user01.c
new file mode 100644
index 000000000..163d7a2dd
--- /dev/null
+++ b/testcases/kernel/crypto/crypto_user01.c
@@ -0,0 +1,195 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2018 Google LLC
+ */
+
+/*
+ * Regression test for commit f43f39958beb ("crypto: user - fix leaking
+ * uninitialized memory to userspace"), or CVE-2018-19854; it was also a
+ * re-introduction of CVE-2013-2547.  This bug caused uninitialized kernel stack
+ * memory to be leaked in some string fields in the replies to CRYPTO_MSG_GETALG
+ * messages over NETLINK_CRYPTO.  To try to detect the bug, this test dumps all
+ * algorithms using NLM_F_DUMP mode and checks all string fields for unexpected
+ * nonzero bytes.
+ */
+
+#include <stdlib.h>
+
+#include "tst_test.h"
+#include "tst_crypto.h"
+#include "tst_netlink.h"
+
+/*
+ * include after <sys/socket.h> (via tst_test.h), to work around dependency bug
+ * in old kernel headers (https://www.spinics.net/lists/netdev/msg171764.html)
+ */
+#include <linux/rtnetlink.h>
+
+static struct tst_crypto_session ses = TST_CRYPTO_SESSION_INIT;
+
+static void setup(void)
+{
+	tst_crypto_open(&ses);
+}
+
+static void do_check_for_leaks(const char *name, const char *value, size_t vlen)
+{
+	size_t i;
+
+	for (i = strnlen(value, vlen); i < vlen; i++) {
+		if (value[i] != '\0')
+			tst_brk(TFAIL, "information leak in field '%s'", name);
+	}
+}
+
+#define check_for_leaks(name, field)  \
+	do_check_for_leaks(name, field, sizeof(field))
+
+static void validate_attr(const struct rtattr *rta)
+{
+	switch (rta->rta_type) {
+	case CRYPTOCFGA_REPORT_LARVAL: {
+		const struct crypto_report_larval *p = RTA_DATA(rta);
+
+		check_for_leaks("crypto_report_larval::type", p->type);
+		break;
+	}
+	case CRYPTOCFGA_REPORT_HASH: {
+		const struct crypto_report_hash *p = RTA_DATA(rta);
+
+		check_for_leaks("crypto_report_hash::type", p->type);
+		break;
+	}
+	case CRYPTOCFGA_REPORT_BLKCIPHER: {
+		const struct crypto_report_blkcipher *p = RTA_DATA(rta);
+
+		check_for_leaks("crypto_report_blkcipher::type", p->type);
+		check_for_leaks("crypto_report_blkcipher::geniv", p->geniv);
+		break;
+	}
+	case CRYPTOCFGA_REPORT_AEAD: {
+		const struct crypto_report_aead *p = RTA_DATA(rta);
+
+		check_for_leaks("crypto_report_aead::type", p->type);
+		check_for_leaks("crypto_report_aead::geniv", p->geniv);
+		break;
+	}
+	case CRYPTOCFGA_REPORT_COMPRESS: {
+		const struct crypto_report_comp *p = RTA_DATA(rta);
+
+		check_for_leaks("crypto_report_comp::type", p->type);
+		break;
+	}
+	case CRYPTOCFGA_REPORT_RNG: {
+		const struct crypto_report_rng *p = RTA_DATA(rta);
+
+		check_for_leaks("crypto_report_rng::type", p->type);
+		break;
+	}
+	case CRYPTOCFGA_REPORT_CIPHER: {
+		const struct crypto_report_cipher *p = RTA_DATA(rta);
+
+		check_for_leaks("crypto_report_cipher::type", p->type);
+		break;
+	}
+	case CRYPTOCFGA_REPORT_AKCIPHER: {
+		const struct crypto_report_akcipher *p = RTA_DATA(rta);
+
+		check_for_leaks("crypto_report_akcipher::type", p->type);
+		break;
+	}
+	case CRYPTOCFGA_REPORT_KPP: {
+		const struct crypto_report_kpp *p = RTA_DATA(rta);
+
+		check_for_leaks("crypto_report_kpp::type", p->type);
+		break;
+	}
+	case CRYPTOCFGA_REPORT_ACOMP: {
+		const struct crypto_report_acomp *p = RTA_DATA(rta);
+
+		check_for_leaks("crypto_report_acomp::type", p->type);
+		break;
+	}
+	} /* end switch */
+}
+
+static void validate_one_alg(const struct nlmsghdr *nh)
+{
+	const struct crypto_user_alg *alg = NLMSG_DATA(nh);
+	const struct rtattr *rta = (void *)alg + NLMSG_ALIGN(sizeof(*alg));
+	size_t remaining = NLMSG_PAYLOAD(nh, sizeof(*alg));
+
+	check_for_leaks("crypto_user_alg::cru_name", alg->cru_name);
+	check_for_leaks("crypto_user_alg::cru_driver_name",
+			alg->cru_driver_name);
+	check_for_leaks("crypto_user_alg::cru_module_name",
+			alg->cru_module_name);
+
+	while (RTA_OK(rta, remaining)) {
+		validate_attr(rta);
+		rta = RTA_NEXT(rta, remaining);
+	}
+}
+
+static void validate_alg_list(const void *buf, size_t remaining)
+{
+	const struct nlmsghdr *nh;
+
+	for (nh = buf; NLMSG_OK(nh, remaining);
+	     nh = NLMSG_NEXT(nh, remaining)) {
+		if (nh->nlmsg_seq != ses.seq_num) {
+			tst_brk(TBROK,
+				"Message out of sequence; type=0%hx, seq_num=%u (not %u)",
+				nh->nlmsg_type, nh->nlmsg_seq, ses.seq_num);
+		}
+		if (nh->nlmsg_type == NLMSG_DONE)
+			return;
+		if (nh->nlmsg_type != CRYPTO_MSG_GETALG) {
+			tst_brk(TBROK,
+				"Unexpected message type; type=0x%hx, seq_num=%u",
+				nh->nlmsg_type, nh->nlmsg_seq);
+		}
+		validate_one_alg(nh);
+	}
+}
+
+static void run(void)
+{
+	struct crypto_user_alg payload = { 0 };
+	struct nlmsghdr nh = {
+		.nlmsg_len = sizeof(payload),
+		.nlmsg_type = CRYPTO_MSG_GETALG,
+		.nlmsg_flags = NLM_F_REQUEST | NLM_F_DUMP,
+		.nlmsg_seq = ++(ses.seq_num),
+		.nlmsg_pid = 0,
+	};
+	/*
+	 * Due to an apparent kernel bug, this API cannot be used incrementally,
+	 * so we just use a large recvmsg() buffer.  This is good enough since
+	 * we don't necessarily have to check every algorithm for this test to
+	 * be effective...
+	 */
+	const size_t bufsize = 1048576;
+	void *buf = SAFE_MALLOC(bufsize);
+	size_t res;
+
+	SAFE_NETLINK_SEND(ses.fd, &nh, &payload);
+
+	res = SAFE_NETLINK_RECV(ses.fd, buf, bufsize);
+
+	validate_alg_list(buf, res);
+
+	free(buf);
+	tst_res(TPASS, "No information leaks found");
+}
+
+static void cleanup(void)
+{
+	tst_crypto_close(&ses);
+}
+
+static struct tst_test test = {
+	.setup = setup,
+	.test_all = run,
+	.cleanup = cleanup,
+};
-- 
2.19.2

[LTP] [PATCH v2 0/3] ltp: new test for crypto_user information leak bug

[Richard Palethorpe]

Hello Eric,

Eric Biggers <ebiggers@kernel.org> writes:

> This series adds a test for an information leak bug in the crypto user
> configuration API which was recently fixed.
>
> Changed since v1:
>   - Check all attributes, not just the first of each type
>   - Work around dependency bug in old kernel headers
>   - Rename __check_for_leaks() to do_check_for_leaks()
>   - Use SPDX license identifier

Thanks, I tested it and it LGTM.

-- 
Thank you,
Richard.

[LTP] [PATCH v2 3/3] crypto/crypto_user01.c: new test for information leak bug

[Cyril Hrubis]

Hi!
> --- a/runtest/crypto
> +++ b/runtest/crypto
> @@ -1 +1,2 @@
>  pcrypt_aead01 pcrypt_aead01
> +crypto_user01 crypto_user01
> diff --git a/runtest/cve b/runtest/cve
> index c4ba74186..78a5d8db2 100644
> --- a/runtest/cve
> +++ b/runtest/cve
> @@ -3,6 +3,7 @@ cve-2011-0999 thp01 -I 120
>  cve-2011-2183 ksm05 -I 10
>  cve-2011-2496 vma03
>  cve-2012-0957 uname04
> +cve-2013-2547 crypto_user01
>  cve-2014-0196 cve-2014-0196
>  cve-2015-0235 gethostbyname_r01
>  cve-2015-7550 keyctl02
> @@ -36,3 +37,4 @@ cve-2017-17053 cve-2017-17053
>  cve-2017-18075 pcrypt_aead01
>  cve-2018-5803 sctp_big_chunk
>  cve-2018-1000001 realpath01
> +cve-2018-19854 crypto_user01

Does it really make sense to run this test twice just under different
name?

BTW: I'm working on test tags and metadata patches that would make
failed tests print list of CVEs and kernel commits that fixed them on a
failure and also under the -h help switch, which should fix this once
for all.

See:

http://lists.linux.it/pipermail/ltp/2018-November/009895.html
http://lists.linux.it/pipermail/ltp/2018-November/010118.html

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 3/3] crypto/crypto_user01.c: new test for information leak bug

[Eric Biggers]

Hi Cyril,

On Tue, Dec 11, 2018 at 01:32:52PM +0100, Cyril Hrubis wrote:
> Hi!
> > --- a/runtest/crypto
> > +++ b/runtest/crypto
> > @@ -1 +1,2 @@
> >  pcrypt_aead01 pcrypt_aead01
> > +crypto_user01 crypto_user01
> > diff --git a/runtest/cve b/runtest/cve
> > index c4ba74186..78a5d8db2 100644
> > --- a/runtest/cve
> > +++ b/runtest/cve
> > @@ -3,6 +3,7 @@ cve-2011-0999 thp01 -I 120
> >  cve-2011-2183 ksm05 -I 10
> >  cve-2011-2496 vma03
> >  cve-2012-0957 uname04
> > +cve-2013-2547 crypto_user01
> >  cve-2014-0196 cve-2014-0196
> >  cve-2015-0235 gethostbyname_r01
> >  cve-2015-7550 keyctl02
> > @@ -36,3 +37,4 @@ cve-2017-17053 cve-2017-17053
> >  cve-2017-18075 pcrypt_aead01
> >  cve-2018-5803 sctp_big_chunk
> >  cve-2018-1000001 realpath01
> > +cve-2018-19854 crypto_user01
> 
> Does it really make sense to run this test twice just under different
> name?
> 
> BTW: I'm working on test tags and metadata patches that would make
> failed tests print list of CVEs and kernel commits that fixed them on a
> failure and also under the -h help switch, which should fix this once
> for all.
> 
> See:
> 
> http://lists.linux.it/pipermail/ltp/2018-November/009895.html
> http://lists.linux.it/pipermail/ltp/2018-November/010118.html
> 

No it doesn't really make sense to run a test twice, but this has two associated
CVE numbers, resulting in two runs when tests are run by CVE number.  Are you
saying you'd prefer that it be listed under just the more recent CVE?  Or are
you saying the runtest/cve file will be going away and replaced by something
else anyway?

- Eric

[LTP] [PATCH v2 3/3] crypto/crypto_user01.c: new test for information leak bug

[Cyril Hrubis]

Hi!
> No it doesn't really make sense to run a test twice, but this has two associated
> CVE numbers, resulting in two runs when tests are run by CVE number.  Are you
> saying you'd prefer that it be listed under just the more recent CVE?  Or are
> you saying the runtest/cve file will be going away and replaced by something
> else anyway?

For now I would just put it under the newer CVE number.

For a future I would like to be able to export all the metadata from the
tests to the tesrunner, so the testrunner would know, among other
things, which tests are associated with CVEs. That, in turn, would
eliminate the need to maintain various different sets of runtest files.

But given that I just started to work on that I do not expect it to be
finished anytime soon.

Does that sounds reasonable to you?

Also no need to resent the patchset, I can remove the runtest entry when
applying the patch.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 3/3] crypto/crypto_user01.c: new test for information leak bug

[Eric Biggers]

Hi Cyril,

On Tue, Dec 11, 2018 at 08:15:43PM +0100, Cyril Hrubis wrote:
> Hi!
> > No it doesn't really make sense to run a test twice, but this has two associated
> > CVE numbers, resulting in two runs when tests are run by CVE number.  Are you
> > saying you'd prefer that it be listed under just the more recent CVE?  Or are
> > you saying the runtest/cve file will be going away and replaced by something
> > else anyway?
> 
> For now I would just put it under the newer CVE number.
> 
> For a future I would like to be able to export all the metadata from the
> tests to the tesrunner, so the testrunner would know, among other
> things, which tests are associated with CVEs. That, in turn, would
> eliminate the need to maintain various different sets of runtest files.
> 
> But given that I just started to work on that I do not expect it to be
> finished anytime soon.
> 
> Does that sounds reasonable to you?
> 
> Also no need to resent the patchset, I can remove the runtest entry when
> applying the patch.
> 

Yes that sounds reasonable to me, please feel free to just put it under the
newer CVE number.  Thanks!

- Eric

[LTP] [PATCH v2 3/3] crypto/crypto_user01.c: new test for information leak bug

[Cyril Hrubis]

Hi!
> Yes that sounds reasonable to me, please feel free to just put it under the
> newer CVE number.  Thanks!

Pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz