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

[LTP] [PATCH 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.

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 | 208 ++++++++++++++++++++++++
 6 files changed, 291 insertions(+), 5 deletions(-)
 create mode 100644 testcases/kernel/crypto/crypto_user01.c

-- 
2.20.0.rc1.387.gf8505762e3-goog

[LTP] [PATCH 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.20.0.rc1.387.gf8505762e3-goog

[LTP] [PATCH 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.20.0.rc1.387.gf8505762e3-goog

[LTP] [PATCH 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 | 208 ++++++++++++++++++++++++
 4 files changed, 212 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..b648fcbdc
--- /dev/null
+++ b/testcases/kernel/crypto/crypto_user01.c
@@ -0,0 +1,208 @@
+/*
+ * Copyright 2018 Google LLC
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program, if not, see <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * 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 <linux/rtnetlink.h>
+
+#include "tst_test.h"
+#include "tst_crypto.h"
+#include "tst_netlink.h"
+
+static struct tst_crypto_session ses = TST_CRYPTO_SESSION_INIT;
+
+static void setup(void)
+{
+	tst_crypto_open(&ses);
+}
+
+static void __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)  \
+	__check_for_leaks(name, field, sizeof(field))
+
+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));
+	const struct rtattr *tb[CRYPTOCFGA_MAX + 1] = { 0 };
+	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)) {
+		if (rta->rta_type < CRYPTOCFGA_MAX && !tb[rta->rta_type])
+			tb[rta->rta_type] = rta;
+		rta = RTA_NEXT(rta, remaining);
+	}
+
+	rta = tb[CRYPTOCFGA_REPORT_LARVAL];
+	if (rta) {
+		const struct crypto_report_larval *p = RTA_DATA(rta);
+
+		check_for_leaks("crypto_report_larval::type", p->type);
+	}
+
+	rta = tb[CRYPTOCFGA_REPORT_HASH];
+	if (rta) {
+		const struct crypto_report_hash *p = RTA_DATA(rta);
+
+		check_for_leaks("crypto_report_hash::type", p->type);
+	}
+
+	rta = tb[CRYPTOCFGA_REPORT_BLKCIPHER];
+	if (rta) {
+		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);
+	}
+
+	rta = tb[CRYPTOCFGA_REPORT_AEAD];
+	if (rta) {
+		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);
+	}
+
+	rta = tb[CRYPTOCFGA_REPORT_COMPRESS];
+	if (rta) {
+		const struct crypto_report_comp *p = RTA_DATA(rta);
+
+		check_for_leaks("crypto_report_comp::type", p->type);
+	}
+
+	rta = tb[CRYPTOCFGA_REPORT_RNG];
+	if (rta) {
+		const struct crypto_report_rng *p = RTA_DATA(rta);
+
+		check_for_leaks("crypto_report_rng::type", p->type);
+	}
+
+	rta = tb[CRYPTOCFGA_REPORT_CIPHER];
+	if (rta) {
+		const struct crypto_report_cipher *p = RTA_DATA(rta);
+
+		check_for_leaks("crypto_report_cipher::type", p->type);
+	}
+
+	rta = tb[CRYPTOCFGA_REPORT_AKCIPHER];
+	if (rta) {
+		const struct crypto_report_akcipher *p = RTA_DATA(rta);
+
+		check_for_leaks("crypto_report_akcipher::type", p->type);
+	}
+
+	rta = tb[CRYPTOCFGA_REPORT_KPP];
+	if (rta) {
+		const struct crypto_report_kpp *p = RTA_DATA(rta);
+
+		check_for_leaks("crypto_report_kpp::type", p->type);
+	}
+
+	rta = tb[CRYPTOCFGA_REPORT_ACOMP];
+	if (rta) {
+		const struct crypto_report_acomp *p = RTA_DATA(rta);
+
+		check_for_leaks("crypto_report_acomp::type", p->type);
+	}
+}
+
+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.20.0.rc1.387.gf8505762e3-goog

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

[Richard Palethorpe]

Hello,

Eric Biggers <ebiggers@kernel.org> writes:

> 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 | 208 ++++++++++++++++++++++++
>  4 files changed, 212 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..b648fcbdc
> --- /dev/null
> +++ b/testcases/kernel/crypto/crypto_user01.c
> @@ -0,0 +1,208 @@
> +/*
> + * Copyright 2018 Google LLC
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program, if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/*
> + * 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 <linux/rtnetlink.h>
> +
> +#include "tst_test.h"
> +#include "tst_crypto.h"
> +#include "tst_netlink.h"
> +
> +static struct tst_crypto_session ses = TST_CRYPTO_SESSION_INIT;
> +
> +static void setup(void)
> +{
> +	tst_crypto_open(&ses);
> +}
> +
> +static void __check_for_leaks(const char *name, const char *value,
> size_t vlen)

IIRC we don't allow functions beginning with underscores because they
are reserved by the compiler or C library.

> +{
> +	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)  \
> +	__check_for_leaks(name, field, sizeof(field))
> +
> +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));
> +	const struct rtattr *tb[CRYPTOCFGA_MAX + 1] = { 0 };
> +	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)) {
> +		if (rta->rta_type < CRYPTOCFGA_MAX &&
> !tb[rta->rta_type])

So does this mean we get multiple instances of the same RTA type and we
just check the first? If so I wonder if there is any advantage to testing
all of them?


-- 
Thank you,
Richard.

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

[Richard Palethorpe]

Hello again,

Eric Biggers <ebiggers@kernel.org> writes:

> 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 | 208 ++++++++++++++++++++++++
>  4 files changed, 212 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..b648fcbdc
> --- /dev/null
> +++ b/testcases/kernel/crypto/crypto_user01.c
> @@ -0,0 +1,208 @@
> +/*
> + * Copyright 2018 Google LLC
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program, if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/*
> + * 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 <linux/rtnetlink.h>
> +
> +#include "tst_test.h"
> +#include "tst_crypto.h"
> +#include "tst_netlink.h"
> +

It seems that on SLE11 there is a bug in the kernel headers which means
compilation fails if you include linux/rtnetlink.h before
linux/netlink.h. If you switch the order then it compiles OK.

-- 
Thank you,
Richard.

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

[Petr Vorel]

Hi Eric, Richard,

> > +#include <linux/rtnetlink.h>
> > +
> > +#include "tst_test.h"
> > +#include "tst_crypto.h"
> > +#include "tst_netlink.h"

> It seems that on SLE11 there is a bug in the kernel headers which means
> compilation fails if you include linux/rtnetlink.h before
> linux/netlink.h. If you switch the order then it compiles OK.

Correct, it suffers from bug:
https://www.spinics.net/lists/netdev/msg171764.html
How about adding include <linux/rtnetlink.h> into include/tst_netlink.h?

> > +static void __check_for_leaks(const char *name, const char *value,
> > size_t vlen)
> 
> IIRC we don't allow functions beginning with underscores because they
> are reserved by the compiler or C library.
Agree.


Kind regards,
Petr

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

[Richard Palethorpe]

Hello,

Petr Vorel <pvorel@suse.cz> writes:

> Hi Eric, Richard,
>
>> > +#include <linux/rtnetlink.h>
>> > +
>> > +#include "tst_test.h"
>> > +#include "tst_crypto.h"
>> > +#include "tst_netlink.h"
>
>> It seems that on SLE11 there is a bug in the kernel headers which means
>> compilation fails if you include linux/rtnetlink.h before
>> linux/netlink.h. If you switch the order then it compiles OK.
>
> Correct, it suffers from bug:
> https://www.spinics.net/lists/netdev/msg171764.html
> How about adding include <linux/rtnetlink.h> into
> include/tst_netlink.h?

I think we would also have to include sys/socket.h in tst_netlink.h to
really solve the problem.

I am not sure if that is a good idea.
>
>> > +static void __check_for_leaks(const char *name, const char *value,
>> > size_t vlen)
>>
>> IIRC we don't allow functions beginning with underscores because they
>> are reserved by the compiler or C library.
> Agree.
>
>
> Kind regards,
> Petr


--
Thank you,
Richard.

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

[Cyril Hrubis]

Hi!
> > +static void __check_for_leaks(const char *name, const char *value,
> > size_t vlen)
> 
> IIRC we don't allow functions beginning with underscores because they
> are reserved by the compiler or C library.

We do append underscores at the end of the identifiers in order not to
redefine symbols from compiler/libc.

-- 
Cyril Hrubis
chrubis@suse.cz

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

[Cyril Hrubis]

Hi!
> +/*
> + * Copyright 2018 Google LLC
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program, if not, see <http://www.gnu.org/licenses/>.
> + */

We started to adopt the SPDX licencse identifier instead of this license
text because it's shorter and machine parseable.

In this case it would shorten the license to:

// SPDX-License-Identifier: GPL-2.0-or-later
/*
 * Copyright 2018 Google LLC
 */


Other than the minor issues pointed out by ritchie and peter the code
looks good to me.

-- 
Cyril Hrubis
chrubis@suse.cz

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

[Eric Biggers]

On Mon, Dec 10, 2018 at 09:59:46AM +0100, Richard Palethorpe wrote:
> Hello,
> 
> Petr Vorel <pvorel@suse.cz> writes:
> 
> > Hi Eric, Richard,
> >
> >> > +#include <linux/rtnetlink.h>
> >> > +
> >> > +#include "tst_test.h"
> >> > +#include "tst_crypto.h"
> >> > +#include "tst_netlink.h"
> >
> >> It seems that on SLE11 there is a bug in the kernel headers which means
> >> compilation fails if you include linux/rtnetlink.h before
> >> linux/netlink.h. If you switch the order then it compiles OK.
> >
> > Correct, it suffers from bug:
> > https://www.spinics.net/lists/netdev/msg171764.html
> > How about adding include <linux/rtnetlink.h> into
> > include/tst_netlink.h?
> 
> I think we would also have to include sys/socket.h in tst_netlink.h to
> really solve the problem.
> 
> I am not sure if that is a good idea.

tst_netlink.h already has an implicit dependency on safe_net_fn.h which already
includes <sys/socket.h>.  So it appears the real issue is including
<linux/rtnetlink.h> (which includes <linux/netlink.h>) before tst_test.h.

But various other <linux/*> headers include <linux/netlink.h> too, and I don't
think all should be included in tst_netlink.h, so I guess I'll just move the
include within the test .c file itself...

BTW, I have no system to reproduce this problem on, so you'll just have to tell
me whether it works.  I tried CentOS 6, but this is already fixed there.

- Eric

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

[Eric Biggers]

Hi Richard,

On Fri, Dec 07, 2018 at 03:11:04PM +0100, Richard Palethorpe wrote:
> Hello,
> 
> Eric Biggers <ebiggers@kernel.org> writes:
> 
> > 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 | 208 ++++++++++++++++++++++++
> >  4 files changed, 212 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..b648fcbdc
> > --- /dev/null
> > +++ b/testcases/kernel/crypto/crypto_user01.c
> > @@ -0,0 +1,208 @@
> > +/*
> > + * Copyright 2018 Google LLC
> > + *
> > + * This program is free software: you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation, either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program, if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +/*
> > + * 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 <linux/rtnetlink.h>
> > +
> > +#include "tst_test.h"
> > +#include "tst_crypto.h"
> > +#include "tst_netlink.h"
> > +
> > +static struct tst_crypto_session ses = TST_CRYPTO_SESSION_INIT;
> > +
> > +static void setup(void)
> > +{
> > +	tst_crypto_open(&ses);
> > +}
> > +
> > +static void __check_for_leaks(const char *name, const char *value,
> > size_t vlen)
> 
> IIRC we don't allow functions beginning with underscores because they
> are reserved by the compiler or C library.
> 
> > +{
> > +	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)  \
> > +	__check_for_leaks(name, field, sizeof(field))
> > +
> > +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));
> > +	const struct rtattr *tb[CRYPTOCFGA_MAX + 1] = { 0 };
> > +	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)) {
> > +		if (rta->rta_type < CRYPTOCFGA_MAX &&
> > !tb[rta->rta_type])
> 
> So does this mean we get multiple instances of the same RTA type and we
> just check the first? If so I wonder if there is any advantage to testing
> all of them?
> 

I don't believe the API returns multiple of any type currently, but we can check
all just as easily, so I'll do it that way instead.  Thanks!

- Eric

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

[Petr Vorel]

Hi Eric, Richard,

...
> > > Correct, it suffers from bug:
> > > https://www.spinics.net/lists/netdev/msg171764.html
> > > How about adding include <linux/rtnetlink.h> into
> > > include/tst_netlink.h?

> > I think we would also have to include sys/socket.h in tst_netlink.h to
> > really solve the problem.

> > I am not sure if that is a good idea.

> tst_netlink.h already has an implicit dependency on safe_net_fn.h which already
> includes <sys/socket.h>.  So it appears the real issue is including
> <linux/rtnetlink.h> (which includes <linux/netlink.h>) before tst_test.h.

> But various other <linux/*> headers include <linux/netlink.h> too, and I don't
> think all should be included in tst_netlink.h, so I guess I'll just move the
> include within the test .c file itself...
If it's the only change, we can do it before merge (no need to repost whole
patchset).

> BTW, I have no system to reproduce this problem on, so you'll just have to tell
> me whether it works.  I tried CentOS 6, but this is already fixed there.
Sure, we'll test it (I can reproduce it as well).

> - Eric
Petr