Skip to contentSkip to conversation

ltp: new test for crypto_user information leak bug

PATCH New Eric Biggers 3 participants 10 messages Dec 11, 2018 – Dec 17, 2018

lore
Share & tools
lore
#0 lore raw

v2 cover ltp: new test for crypto_user information leak bug

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
Signature · 1 line
2.19.2
#1 lore raw

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

From: Eric Biggers <[email protected]>

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

---
 include/lapi/cryptouser.h | 73 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)
include/lapi/cryptouser.h +73 -0
index 61ff21ad3..94d641446 100644
@@ -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
Signature · 1 line
2.19.2
#2 lore raw

PATCH v2 2/3 tst_netlink: inline functions in header

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

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

---
 include/tst_netlink.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
include/tst_netlink.h +6 -5
index 42232a169..892ef8f78 100644
@@ -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;
Signature · 1 line
2.19.2
#3 lore raw

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

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

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.

---
 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
runtest/crypto +1 -0
index e5ba61e5e..cdbc44cc8 100644
@@ -1 +1,2 @@
 pcrypt_aead01 pcrypt_aead01
+crypto_user01 crypto_user01
runtest/cve +2 -0
index c4ba74186..78a5d8db2 100644
@@ -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
testcases/kernel/crypto/.gitignore +1 -0
index fafe5c972..759592fbd 100644
@@ -1 +1,2 @@
 pcrypt_aead01
+crypto_user01
testcases/kernel/crypto/crypto_user01.c new file +195 -0

195 lines not on this page · raw message

Signature · 1 line
2.19.2
#4 lore raw

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

Hi!

Quoted 18 lines
runtest/crypto +1 -0
@@ -1 +1,2 @@
 pcrypt_aead01 pcrypt_aead01
+crypto_user01 crypto_user01
runtest/cve +1 -0
index c4ba74186..78a5d8db2 100644
@@ -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

runtest/cve cve-2017-17053 cve-2017-17053 · line 40

Quoted 4 lines
runtest/cve +1 -0
@@ -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

Signature · 2 lines
Cyril Hrubis
[email protected]
#5 lore raw

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

Hi Cyril,

Quoted 32 lines · Cyril Hrubis

Hi!

Quoted 18 lines
runtest/crypto +1 -0
@@ -1 +1,2 @@
 pcrypt_aead01 pcrypt_aead01
+crypto_user01 crypto_user01
runtest/cve +1 -0
index c4ba74186..78a5d8db2 100644
@@ -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

runtest/cve cve-2017-17053 cve-2017-17053 · line 40

Quoted 4 lines
runtest/cve +1 -0
@@ -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.

Quoted 5 lines

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

#6 lore raw

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

Hi!

Quoted 5 lines

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.

Signature · 2 lines
Cyril Hrubis
[email protected]
#7 lore raw

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

Hi Cyril,

Quoted 14 lines · Cyril Hrubis

Hi!

Quoted 5 lines

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.

Quoted 8 lines

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

#8 lore raw

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

Hi!

Quoted 2 lines

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

Pushed, thanks.

Signature · 2 lines
Cyril Hrubis
[email protected]
#9 lore raw

v2 cover ltp: new test for crypto_user information leak bug

In reply to Eric Biggers

Hello Eric,

Quoted 8 lines · 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

Thanks, I tested it and it LGTM.

Signature · 2 lines
Thank you,
Richard.