All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] ltp: add tests for some recently-fixed keyrings bugs
@ 2017-10-10 17:51 ` Eric Biggers
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Biggers @ 2017-10-10 17:51 UTC (permalink / raw)
  To: keyrings

From: Eric Biggers <ebiggers@google.com>

Add tests to LTP for a few more longstanding bugs which were recently
fixed in the kernel keyrings API.

Eric Biggers (4):
  lapi/keyctl.h: add a few missing definitions
  syscalls/keyctl06: new test for keyring_read() buffer overrun
  syscalls/keyctl07: new test for oops when reading negative key
  syscalls/add_key03: new test for forging user keyrings

 include/lapi/keyctl.h                         |  20 +++++
 runtest/syscalls                              |   3 +
 testcases/kernel/syscalls/.gitignore          |   3 +
 testcases/kernel/syscalls/add_key/add_key03.c | 122 ++++++++++++++++++++++++++
 testcases/kernel/syscalls/keyctl/keyctl06.c   |  68 ++++++++++++++
 testcases/kernel/syscalls/keyctl/keyctl07.c   |  89 +++++++++++++++++++
 6 files changed, 305 insertions(+)
 create mode 100644 testcases/kernel/syscalls/add_key/add_key03.c
 create mode 100644 testcases/kernel/syscalls/keyctl/keyctl06.c
 create mode 100644 testcases/kernel/syscalls/keyctl/keyctl07.c

-- 
2.14.2.920.gcf0c67979c-goog


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [LTP] [PATCH 0/4] ltp: add tests for some recently-fixed keyrings bugs
@ 2017-10-10 17:51 ` Eric Biggers
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Biggers @ 2017-10-10 17:51 UTC (permalink / raw)
  To: ltp

From: Eric Biggers <ebiggers@google.com>

Add tests to LTP for a few more longstanding bugs which were recently
fixed in the kernel keyrings API.

Eric Biggers (4):
  lapi/keyctl.h: add a few missing definitions
  syscalls/keyctl06: new test for keyring_read() buffer overrun
  syscalls/keyctl07: new test for oops when reading negative key
  syscalls/add_key03: new test for forging user keyrings

 include/lapi/keyctl.h                         |  20 +++++
 runtest/syscalls                              |   3 +
 testcases/kernel/syscalls/.gitignore          |   3 +
 testcases/kernel/syscalls/add_key/add_key03.c | 122 ++++++++++++++++++++++++++
 testcases/kernel/syscalls/keyctl/keyctl06.c   |  68 ++++++++++++++
 testcases/kernel/syscalls/keyctl/keyctl07.c   |  89 +++++++++++++++++++
 6 files changed, 305 insertions(+)
 create mode 100644 testcases/kernel/syscalls/add_key/add_key03.c
 create mode 100644 testcases/kernel/syscalls/keyctl/keyctl06.c
 create mode 100644 testcases/kernel/syscalls/keyctl/keyctl07.c

-- 
2.14.2.920.gcf0c67979c-goog


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/4] lapi/keyctl.h: add a few missing definitions
  2017-10-10 17:51 ` [LTP] " Eric Biggers
@ 2017-10-10 17:51 ` Eric Biggers
  -1 siblings, 0 replies; 14+ messages in thread
From: Eric Biggers @ 2017-10-10 17:51 UTC (permalink / raw)
  To: keyrings

From: Eric Biggers <ebiggers@google.com>

KEYCTL_REVOKE, KEYCTL_READ, KEY_SPEC_PROCESS_KEYRING, and
KEY_SPEC_USER_SESSION_KEYRING are used by existing tests.
KEY_SPEC_USER_KEYRING will be used by an upcoming test.

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

diff --git a/include/lapi/keyctl.h b/include/lapi/keyctl.h
index 3e7ce4708..73f4fdc90 100644
--- a/include/lapi/keyctl.h
+++ b/include/lapi/keyctl.h
@@ -81,6 +81,10 @@ static inline long keyctl(int cmd, ...)
 # define KEYCTL_UPDATE 2
 #endif
 
+#ifndef KEYCTL_REVOKE
+# define KEYCTL_REVOKE 3
+#endif
+
 #ifndef KEYCTL_SETPERM
 # define KEYCTL_SETPERM 5
 #endif
@@ -89,14 +93,30 @@ static inline long keyctl(int cmd, ...)
 # define KEYCTL_UNLINK 9
 #endif
 
+#ifndef KEYCTL_READ
+# define KEYCTL_READ 11
+#endif
+
 #ifndef KEY_SPEC_THREAD_KEYRING
 # define KEY_SPEC_THREAD_KEYRING -1
 #endif
 
+#ifndef KEY_SPEC_PROCESS_KEYRING
+# define KEY_SPEC_PROCESS_KEYRING -2
+#endif
+
 #ifndef KEY_SPEC_SESSION_KEYRING
 # define KEY_SPEC_SESSION_KEYRING -3
 #endif
 
+#ifndef KEY_SPEC_USER_KEYRING
+# define KEY_SPEC_USER_KEYRING -4
+#endif
+
+#ifndef KEY_SPEC_USER_SESSION_KEYRING
+# define KEY_SPEC_USER_SESSION_KEYRING -5
+#endif
+
 #ifndef KEY_REQKEY_DEFL_THREAD_KEYRING
 # define KEY_REQKEY_DEFL_THREAD_KEYRING 1
 #endif
-- 
2.14.2.920.gcf0c67979c-goog


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [LTP] [PATCH 1/4] lapi/keyctl.h: add a few missing definitions
@ 2017-10-10 17:51 ` Eric Biggers
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Biggers @ 2017-10-10 17:51 UTC (permalink / raw)
  To: ltp

From: Eric Biggers <ebiggers@google.com>

KEYCTL_REVOKE, KEYCTL_READ, KEY_SPEC_PROCESS_KEYRING, and
KEY_SPEC_USER_SESSION_KEYRING are used by existing tests.
KEY_SPEC_USER_KEYRING will be used by an upcoming test.

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

diff --git a/include/lapi/keyctl.h b/include/lapi/keyctl.h
index 3e7ce4708..73f4fdc90 100644
--- a/include/lapi/keyctl.h
+++ b/include/lapi/keyctl.h
@@ -81,6 +81,10 @@ static inline long keyctl(int cmd, ...)
 # define KEYCTL_UPDATE 2
 #endif
 
+#ifndef KEYCTL_REVOKE
+# define KEYCTL_REVOKE 3
+#endif
+
 #ifndef KEYCTL_SETPERM
 # define KEYCTL_SETPERM 5
 #endif
@@ -89,14 +93,30 @@ static inline long keyctl(int cmd, ...)
 # define KEYCTL_UNLINK 9
 #endif
 
+#ifndef KEYCTL_READ
+# define KEYCTL_READ 11
+#endif
+
 #ifndef KEY_SPEC_THREAD_KEYRING
 # define KEY_SPEC_THREAD_KEYRING -1
 #endif
 
+#ifndef KEY_SPEC_PROCESS_KEYRING
+# define KEY_SPEC_PROCESS_KEYRING -2
+#endif
+
 #ifndef KEY_SPEC_SESSION_KEYRING
 # define KEY_SPEC_SESSION_KEYRING -3
 #endif
 
+#ifndef KEY_SPEC_USER_KEYRING
+# define KEY_SPEC_USER_KEYRING -4
+#endif
+
+#ifndef KEY_SPEC_USER_SESSION_KEYRING
+# define KEY_SPEC_USER_SESSION_KEYRING -5
+#endif
+
 #ifndef KEY_REQKEY_DEFL_THREAD_KEYRING
 # define KEY_REQKEY_DEFL_THREAD_KEYRING 1
 #endif
-- 
2.14.2.920.gcf0c67979c-goog


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/4] syscalls/keyctl06: new test for keyring_read() buffer overrun
  2017-10-10 17:51 ` [LTP] " Eric Biggers
@ 2017-10-10 17:51 ` Eric Biggers
  -1 siblings, 0 replies; 14+ messages in thread
From: Eric Biggers @ 2017-10-10 17:51 UTC (permalink / raw)
  To: keyrings

From: Eric Biggers <ebiggers@google.com>

Add a test for a bug which caused the kernel to write past the end of
the provided buffer when using KEYCTL_READ to read from a keyring.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 runtest/syscalls                            |  1 +
 testcases/kernel/syscalls/.gitignore        |  1 +
 testcases/kernel/syscalls/keyctl/keyctl06.c | 68 +++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+)
 create mode 100644 testcases/kernel/syscalls/keyctl/keyctl06.c

diff --git a/runtest/syscalls b/runtest/syscalls
index 2362a231d..67a7362ee 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -500,6 +500,7 @@ keyctl02 keyctl02
 keyctl03 keyctl03
 keyctl04 keyctl04
 keyctl05 keyctl05
+keyctl06 keyctl06
 
 kcmp01 kcmp01
 kcmp02 kcmp02
diff --git a/testcases/kernel/syscalls/.gitignore b/testcases/kernel/syscalls/.gitignore
index 930f3f999..1e573d9a4 100644
--- a/testcases/kernel/syscalls/.gitignore
+++ b/testcases/kernel/syscalls/.gitignore
@@ -462,6 +462,7 @@
 /keyctl/keyctl03
 /keyctl/keyctl04
 /keyctl/keyctl05
+/keyctl/keyctl06
 /kcmp/kcmp01
 /kcmp/kcmp02
 /kcmp/kcmp03
diff --git a/testcases/kernel/syscalls/keyctl/keyctl06.c b/testcases/kernel/syscalls/keyctl/keyctl06.c
new file mode 100644
index 000000000..88734313d
--- /dev/null
+++ b/testcases/kernel/syscalls/keyctl/keyctl06.c
@@ -0,0 +1,68 @@
+/*
+ * Copyright (c) 2017 Google, Inc.
+ *
+ * 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 e645016abc80 ("KEYS: fix writing past end of
+ * user-supplied buffer in keyring_read()").
+ */
+
+#include <errno.h>
+
+#include "tst_test.h"
+#include "lapi/keyctl.h"
+
+static key_serial_t add_test_key(const char *description)
+{
+	TEST(add_key("user", description, "payload", 7,
+		     KEY_SPEC_PROCESS_KEYRING));
+	if (TEST_RETURN < 0)
+		tst_brk(TBROK | TTERRNO, "Failed to add test key");
+	return TEST_RETURN;
+}
+
+static void do_test(void)
+{
+	key_serial_t key_ids[2];
+	key_serial_t key_id_1 = add_test_key("key1");
+	key_serial_t key_id_2 = add_test_key("key2");
+
+	memset(key_ids, 0, sizeof(key_ids));
+	TEST(keyctl(KEYCTL_READ, KEY_SPEC_PROCESS_KEYRING,
+		    (char *)key_ids, sizeof(key_serial_t)));
+	if (TEST_RETURN < 0)
+		tst_brk(TBROK | TTERRNO, "KEYCTL_READ failed");
+
+	if (key_ids[1] != 0)
+		tst_brk(TFAIL, "KEYCTL_READ overran the buffer");
+
+	if (key_ids[0] = 0)
+		tst_brk(TBROK, "KEYCTL_READ didn't read anything");
+
+	if (key_ids[0] != key_id_1 && key_ids[0] != key_id_2)
+		tst_brk(TBROK, "KEYCTL_READ didn't return correct key ID");
+
+	if (TEST_RETURN != sizeof(key_serial_t)) {
+		tst_brk(TBROK, "KEYCTL_READ returned %ld but expected %zu",
+			TEST_RETURN, sizeof(key_serial_t));
+	}
+
+	tst_res(TPASS, "KEYCTL_READ didn't overrun the buffer");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+};
-- 
2.14.2.920.gcf0c67979c-goog


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [LTP] [PATCH 2/4] syscalls/keyctl06: new test for keyring_read() buffer overrun
@ 2017-10-10 17:51 ` Eric Biggers
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Biggers @ 2017-10-10 17:51 UTC (permalink / raw)
  To: ltp

From: Eric Biggers <ebiggers@google.com>

Add a test for a bug which caused the kernel to write past the end of
the provided buffer when using KEYCTL_READ to read from a keyring.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 runtest/syscalls                            |  1 +
 testcases/kernel/syscalls/.gitignore        |  1 +
 testcases/kernel/syscalls/keyctl/keyctl06.c | 68 +++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+)
 create mode 100644 testcases/kernel/syscalls/keyctl/keyctl06.c

diff --git a/runtest/syscalls b/runtest/syscalls
index 2362a231d..67a7362ee 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -500,6 +500,7 @@ keyctl02 keyctl02
 keyctl03 keyctl03
 keyctl04 keyctl04
 keyctl05 keyctl05
+keyctl06 keyctl06
 
 kcmp01 kcmp01
 kcmp02 kcmp02
diff --git a/testcases/kernel/syscalls/.gitignore b/testcases/kernel/syscalls/.gitignore
index 930f3f999..1e573d9a4 100644
--- a/testcases/kernel/syscalls/.gitignore
+++ b/testcases/kernel/syscalls/.gitignore
@@ -462,6 +462,7 @@
 /keyctl/keyctl03
 /keyctl/keyctl04
 /keyctl/keyctl05
+/keyctl/keyctl06
 /kcmp/kcmp01
 /kcmp/kcmp02
 /kcmp/kcmp03
diff --git a/testcases/kernel/syscalls/keyctl/keyctl06.c b/testcases/kernel/syscalls/keyctl/keyctl06.c
new file mode 100644
index 000000000..88734313d
--- /dev/null
+++ b/testcases/kernel/syscalls/keyctl/keyctl06.c
@@ -0,0 +1,68 @@
+/*
+ * Copyright (c) 2017 Google, Inc.
+ *
+ * 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 e645016abc80 ("KEYS: fix writing past end of
+ * user-supplied buffer in keyring_read()").
+ */
+
+#include <errno.h>
+
+#include "tst_test.h"
+#include "lapi/keyctl.h"
+
+static key_serial_t add_test_key(const char *description)
+{
+	TEST(add_key("user", description, "payload", 7,
+		     KEY_SPEC_PROCESS_KEYRING));
+	if (TEST_RETURN < 0)
+		tst_brk(TBROK | TTERRNO, "Failed to add test key");
+	return TEST_RETURN;
+}
+
+static void do_test(void)
+{
+	key_serial_t key_ids[2];
+	key_serial_t key_id_1 = add_test_key("key1");
+	key_serial_t key_id_2 = add_test_key("key2");
+
+	memset(key_ids, 0, sizeof(key_ids));
+	TEST(keyctl(KEYCTL_READ, KEY_SPEC_PROCESS_KEYRING,
+		    (char *)key_ids, sizeof(key_serial_t)));
+	if (TEST_RETURN < 0)
+		tst_brk(TBROK | TTERRNO, "KEYCTL_READ failed");
+
+	if (key_ids[1] != 0)
+		tst_brk(TFAIL, "KEYCTL_READ overran the buffer");
+
+	if (key_ids[0] == 0)
+		tst_brk(TBROK, "KEYCTL_READ didn't read anything");
+
+	if (key_ids[0] != key_id_1 && key_ids[0] != key_id_2)
+		tst_brk(TBROK, "KEYCTL_READ didn't return correct key ID");
+
+	if (TEST_RETURN != sizeof(key_serial_t)) {
+		tst_brk(TBROK, "KEYCTL_READ returned %ld but expected %zu",
+			TEST_RETURN, sizeof(key_serial_t));
+	}
+
+	tst_res(TPASS, "KEYCTL_READ didn't overrun the buffer");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+};
-- 
2.14.2.920.gcf0c67979c-goog


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/4] syscalls/keyctl07: new test for oops when reading negative key
  2017-10-10 17:51 ` [LTP] " Eric Biggers
@ 2017-10-10 17:51 ` Eric Biggers
  -1 siblings, 0 replies; 14+ messages in thread
From: Eric Biggers @ 2017-10-10 17:51 UTC (permalink / raw)
  To: keyrings

From: Eric Biggers <ebiggers@google.com>

Add a test for a bug which caused the kernel to dereference a bogus
pointer when using KEYCTL_READ to read from a negative key.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 runtest/syscalls                            |  1 +
 testcases/kernel/syscalls/.gitignore        |  1 +
 testcases/kernel/syscalls/keyctl/keyctl07.c | 89 +++++++++++++++++++++++++++++
 3 files changed, 91 insertions(+)
 create mode 100644 testcases/kernel/syscalls/keyctl/keyctl07.c

diff --git a/runtest/syscalls b/runtest/syscalls
index 67a7362ee..649dbfa6c 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -501,6 +501,7 @@ keyctl03 keyctl03
 keyctl04 keyctl04
 keyctl05 keyctl05
 keyctl06 keyctl06
+keyctl07 keyctl07
 
 kcmp01 kcmp01
 kcmp02 kcmp02
diff --git a/testcases/kernel/syscalls/.gitignore b/testcases/kernel/syscalls/.gitignore
index 1e573d9a4..53ad7ca2b 100644
--- a/testcases/kernel/syscalls/.gitignore
+++ b/testcases/kernel/syscalls/.gitignore
@@ -463,6 +463,7 @@
 /keyctl/keyctl04
 /keyctl/keyctl05
 /keyctl/keyctl06
+/keyctl/keyctl07
 /kcmp/kcmp01
 /kcmp/kcmp02
 /kcmp/kcmp03
diff --git a/testcases/kernel/syscalls/keyctl/keyctl07.c b/testcases/kernel/syscalls/keyctl/keyctl07.c
new file mode 100644
index 000000000..c41d7040f
--- /dev/null
+++ b/testcases/kernel/syscalls/keyctl/keyctl07.c
@@ -0,0 +1,89 @@
+/*
+ * Copyright (c) 2017 Google, Inc.
+ *
+ * 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 37863c43b2c6 ("KEYS: prevent KEYCTL_READ on
+ * negative key").
+ */
+
+#include <errno.h>
+
+#include "tst_test.h"
+#include "lapi/keyctl.h"
+
+static void do_test(void)
+{
+	key_serial_t key_id;
+	char buffer[128];
+
+	/*
+	 * Create a negatively instantiated key of the "user" key type.  This
+	 * key type is chosen because it has a ->read() method (which makes the
+	 * bug reachable) and is available whenever CONFIG_KEYS is enabled.
+	 *
+	 * request_key() will result in the creation of a negative key provided
+	 * that /sbin/request-key isn't configured to positively instantiate the
+	 * key, based on the provided type, description, and callout_info.  If
+	 * /sbin/request-key doesn't exist, errno will be ENOENT; while if it
+	 * does exist and we specify some random unprefixed description, errno
+	 * should be ENOKEY (since /sbin/request-key should not be configured to
+	 * instantiate random user keys).  In either case a negative key should
+	 * be created and we can continue on with the test.  Negative keys last
+	 * for 60 seconds so there should be plenty of time for the test.
+	 */
+	TEST(request_key("user", "description", "callout_info",
+			 KEY_SPEC_PROCESS_KEYRING));
+	if (TEST_RETURN != -1)
+		tst_brk(TBROK, "request_key() unexpectedly succeeded");
+
+	if (TEST_ERRNO != ENOKEY && TEST_ERRNO != ENOENT) {
+		tst_brk(TBROK | TTERRNO,
+			"request_key() failed with unexpected error");
+	}
+
+	/* Get the ID of the negative key by reading the keyring */
+	TEST(keyctl(KEYCTL_READ, KEY_SPEC_PROCESS_KEYRING,
+		    &key_id, sizeof(key_id)));
+	if (TEST_RETURN < 0)
+		tst_brk(TBROK | TTERRNO, "KEYCTL_READ unexpectedly failed");
+	if (TEST_RETURN != sizeof(key_id)) {
+		tst_brk(TBROK, "KEYCTL_READ returned %ld but expected %zu",
+			TEST_RETURN, sizeof(key_id));
+	}
+
+	/*
+	 * Now try to read the negative key.  Unpatched kernels will oops trying
+	 * to read from memory address 0x00000000ffffff92.
+	 */
+	tst_res(TINFO, "trying to read from the negative key...");
+	TEST(keyctl(KEYCTL_READ, key_id, buffer, sizeof(buffer)));
+	if (TEST_RETURN != -1) {
+		tst_res(TFAIL,
+			"KEYCTL_READ on negative key unexpectedly succeeded");
+		return;
+	}
+	if (TEST_ERRNO != ENOKEY) {
+		tst_res(TFAIL | TTERRNO,
+			"KEYCTL_READ on negative key failed with unexpected error");
+		return;
+	}
+	tst_res(TPASS, "KEYCTL_READ on negative key expectedly failed with ENOKEY");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+};
-- 
2.14.2.920.gcf0c67979c-goog


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [LTP] [PATCH 3/4] syscalls/keyctl07: new test for oops when reading negative key
@ 2017-10-10 17:51 ` Eric Biggers
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Biggers @ 2017-10-10 17:51 UTC (permalink / raw)
  To: ltp

From: Eric Biggers <ebiggers@google.com>

Add a test for a bug which caused the kernel to dereference a bogus
pointer when using KEYCTL_READ to read from a negative key.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 runtest/syscalls                            |  1 +
 testcases/kernel/syscalls/.gitignore        |  1 +
 testcases/kernel/syscalls/keyctl/keyctl07.c | 89 +++++++++++++++++++++++++++++
 3 files changed, 91 insertions(+)
 create mode 100644 testcases/kernel/syscalls/keyctl/keyctl07.c

diff --git a/runtest/syscalls b/runtest/syscalls
index 67a7362ee..649dbfa6c 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -501,6 +501,7 @@ keyctl03 keyctl03
 keyctl04 keyctl04
 keyctl05 keyctl05
 keyctl06 keyctl06
+keyctl07 keyctl07
 
 kcmp01 kcmp01
 kcmp02 kcmp02
diff --git a/testcases/kernel/syscalls/.gitignore b/testcases/kernel/syscalls/.gitignore
index 1e573d9a4..53ad7ca2b 100644
--- a/testcases/kernel/syscalls/.gitignore
+++ b/testcases/kernel/syscalls/.gitignore
@@ -463,6 +463,7 @@
 /keyctl/keyctl04
 /keyctl/keyctl05
 /keyctl/keyctl06
+/keyctl/keyctl07
 /kcmp/kcmp01
 /kcmp/kcmp02
 /kcmp/kcmp03
diff --git a/testcases/kernel/syscalls/keyctl/keyctl07.c b/testcases/kernel/syscalls/keyctl/keyctl07.c
new file mode 100644
index 000000000..c41d7040f
--- /dev/null
+++ b/testcases/kernel/syscalls/keyctl/keyctl07.c
@@ -0,0 +1,89 @@
+/*
+ * Copyright (c) 2017 Google, Inc.
+ *
+ * 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 37863c43b2c6 ("KEYS: prevent KEYCTL_READ on
+ * negative key").
+ */
+
+#include <errno.h>
+
+#include "tst_test.h"
+#include "lapi/keyctl.h"
+
+static void do_test(void)
+{
+	key_serial_t key_id;
+	char buffer[128];
+
+	/*
+	 * Create a negatively instantiated key of the "user" key type.  This
+	 * key type is chosen because it has a ->read() method (which makes the
+	 * bug reachable) and is available whenever CONFIG_KEYS is enabled.
+	 *
+	 * request_key() will result in the creation of a negative key provided
+	 * that /sbin/request-key isn't configured to positively instantiate the
+	 * key, based on the provided type, description, and callout_info.  If
+	 * /sbin/request-key doesn't exist, errno will be ENOENT; while if it
+	 * does exist and we specify some random unprefixed description, errno
+	 * should be ENOKEY (since /sbin/request-key should not be configured to
+	 * instantiate random user keys).  In either case a negative key should
+	 * be created and we can continue on with the test.  Negative keys last
+	 * for 60 seconds so there should be plenty of time for the test.
+	 */
+	TEST(request_key("user", "description", "callout_info",
+			 KEY_SPEC_PROCESS_KEYRING));
+	if (TEST_RETURN != -1)
+		tst_brk(TBROK, "request_key() unexpectedly succeeded");
+
+	if (TEST_ERRNO != ENOKEY && TEST_ERRNO != ENOENT) {
+		tst_brk(TBROK | TTERRNO,
+			"request_key() failed with unexpected error");
+	}
+
+	/* Get the ID of the negative key by reading the keyring */
+	TEST(keyctl(KEYCTL_READ, KEY_SPEC_PROCESS_KEYRING,
+		    &key_id, sizeof(key_id)));
+	if (TEST_RETURN < 0)
+		tst_brk(TBROK | TTERRNO, "KEYCTL_READ unexpectedly failed");
+	if (TEST_RETURN != sizeof(key_id)) {
+		tst_brk(TBROK, "KEYCTL_READ returned %ld but expected %zu",
+			TEST_RETURN, sizeof(key_id));
+	}
+
+	/*
+	 * Now try to read the negative key.  Unpatched kernels will oops trying
+	 * to read from memory address 0x00000000ffffff92.
+	 */
+	tst_res(TINFO, "trying to read from the negative key...");
+	TEST(keyctl(KEYCTL_READ, key_id, buffer, sizeof(buffer)));
+	if (TEST_RETURN != -1) {
+		tst_res(TFAIL,
+			"KEYCTL_READ on negative key unexpectedly succeeded");
+		return;
+	}
+	if (TEST_ERRNO != ENOKEY) {
+		tst_res(TFAIL | TTERRNO,
+			"KEYCTL_READ on negative key failed with unexpected error");
+		return;
+	}
+	tst_res(TPASS, "KEYCTL_READ on negative key expectedly failed with ENOKEY");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+};
-- 
2.14.2.920.gcf0c67979c-goog


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/4] syscalls/add_key03: new test for forging user keyrings
  2017-10-10 17:51 ` [LTP] " Eric Biggers
@ 2017-10-10 17:51 ` Eric Biggers
  -1 siblings, 0 replies; 14+ messages in thread
From: Eric Biggers @ 2017-10-10 17:51 UTC (permalink / raw)
  To: keyrings

From: Eric Biggers <ebiggers@google.com>

Add a test for a bug which allowed a user to create the user and user
session keyrings for another user.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 runtest/syscalls                              |   1 +
 testcases/kernel/syscalls/.gitignore          |   1 +
 testcases/kernel/syscalls/add_key/add_key03.c | 122 ++++++++++++++++++++++++++
 3 files changed, 124 insertions(+)
 create mode 100644 testcases/kernel/syscalls/add_key/add_key03.c

diff --git a/runtest/syscalls b/runtest/syscalls
index 649dbfa6c..0d2254393 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -13,6 +13,7 @@ acct01 acct01
 
 add_key01 add_key01
 add_key02 add_key02
+add_key03 add_key03
 
 adjtimex01 adjtimex01
 adjtimex02 adjtimex02
diff --git a/testcases/kernel/syscalls/.gitignore b/testcases/kernel/syscalls/.gitignore
index 53ad7ca2b..7dea3b2c7 100644
--- a/testcases/kernel/syscalls/.gitignore
+++ b/testcases/kernel/syscalls/.gitignore
@@ -8,6 +8,7 @@
 /acct/acct01
 /add_key/add_key01
 /add_key/add_key02
+/add_key/add_key03
 /adjtimex/adjtimex01
 /adjtimex/adjtimex02
 /alarm/alarm01
diff --git a/testcases/kernel/syscalls/add_key/add_key03.c b/testcases/kernel/syscalls/add_key/add_key03.c
new file mode 100644
index 000000000..8c68f8635
--- /dev/null
+++ b/testcases/kernel/syscalls/add_key/add_key03.c
@@ -0,0 +1,122 @@
+/*
+ * Copyright (c) 2017 Google, Inc.
+ *
+ * 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 237bbd29f7a0 ("KEYS: prevent creating a different
+ * user's keyrings").  The bug allowed any random user to create a keyring named
+ * "_uid.$UID" (or "_uid_ses.$UID"), and it would become the user keyring (or
+ * user session keyring) for user $UID, provided that it hadn't already been
+ * created.
+ *
+ * This test must be run as root so that it has permission to switch to another
+ * user ID and check whether the keyrings are wrong.  However, the underlying
+ * bug is actually reachable/exploitable by a non-root user.
+ */
+
+#include <errno.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "tst_test.h"
+#include "lapi/keyctl.h"
+
+static key_serial_t create_keyring(const char *description)
+{
+	TEST(add_key("keyring", description, NULL, 0,
+		     KEY_SPEC_PROCESS_KEYRING));
+	if (TEST_RETURN < 0) {
+		tst_brk(TBROK | TTERRNO,
+			"unable to create keyring '%s'", description);
+	}
+	return TEST_RETURN;
+}
+
+static key_serial_t get_keyring_id(key_serial_t special_id)
+{
+	TEST(keyctl(KEYCTL_GET_KEYRING_ID, special_id, 1));
+	if (TEST_RETURN < 0) {
+		tst_brk(TBROK | TTERRNO,
+			"unable to get ID of keyring %d", special_id);
+	}
+	return TEST_RETURN;
+}
+
+static void unlink_keyring(key_serial_t id)
+{
+	TEST(keyctl(KEYCTL_UNLINK, id, KEY_SPEC_PROCESS_KEYRING));
+	if (TEST_RETURN < 0) {
+		tst_brk(TBROK | TTERRNO,
+			"unable to unlink the keyring we created");
+	}
+}
+
+static void do_test(void)
+{
+	int i;
+
+	/*
+	 * Try with multiple user IDs before reporting success.  By chance, some
+	 * users may already have an existing user keyring; the bug will not be
+	 * reproducible for them.
+	 */
+	for (i = 0; i < 10; i++) {
+		char description[32];
+		uid_t uid;
+		key_serial_t fake_user_keyring;
+		key_serial_t fake_user_session_keyring;
+
+		uid = rand();
+		if (uid = 0)
+			continue;
+
+		sprintf(description, "_uid.%u", uid);
+		fake_user_keyring = create_keyring(description);
+		sprintf(description, "_uid_ses.%u", uid);
+		fake_user_session_keyring = create_keyring(description);
+
+		TEST(setreuid(uid, 0));
+		if (TEST_RETURN < 0) {
+			tst_brk(TBROK | TTERRNO,
+				"unable to set real uid to %u", uid);
+		}
+
+		if (fake_user_keyring = get_keyring_id(KEY_SPEC_USER_KEYRING)) {
+			tst_brk(TFAIL,
+				"created user keyring for another user");
+		}
+
+		if (fake_user_session_keyring =
+		    get_keyring_id(KEY_SPEC_USER_SESSION_KEYRING)) {
+			tst_brk(TFAIL,
+				"created user session keyring for another user");
+		}
+
+		TEST(setreuid(0, 0));
+		if (TEST_RETURN < 0)
+			tst_brk(TBROK | TTERRNO, "unable to reset real uid");
+		uid++;
+
+		unlink_keyring(fake_user_keyring);
+		unlink_keyring(fake_user_session_keyring);
+	}
+	tst_res(TPASS, "expectedly could not create another user's keyrings");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_root = 1,
+};
-- 
2.14.2.920.gcf0c67979c-goog


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [LTP] [PATCH 4/4] syscalls/add_key03: new test for forging user keyrings
@ 2017-10-10 17:51 ` Eric Biggers
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Biggers @ 2017-10-10 17:51 UTC (permalink / raw)
  To: ltp

From: Eric Biggers <ebiggers@google.com>

Add a test for a bug which allowed a user to create the user and user
session keyrings for another user.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 runtest/syscalls                              |   1 +
 testcases/kernel/syscalls/.gitignore          |   1 +
 testcases/kernel/syscalls/add_key/add_key03.c | 122 ++++++++++++++++++++++++++
 3 files changed, 124 insertions(+)
 create mode 100644 testcases/kernel/syscalls/add_key/add_key03.c

diff --git a/runtest/syscalls b/runtest/syscalls
index 649dbfa6c..0d2254393 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -13,6 +13,7 @@ acct01 acct01
 
 add_key01 add_key01
 add_key02 add_key02
+add_key03 add_key03
 
 adjtimex01 adjtimex01
 adjtimex02 adjtimex02
diff --git a/testcases/kernel/syscalls/.gitignore b/testcases/kernel/syscalls/.gitignore
index 53ad7ca2b..7dea3b2c7 100644
--- a/testcases/kernel/syscalls/.gitignore
+++ b/testcases/kernel/syscalls/.gitignore
@@ -8,6 +8,7 @@
 /acct/acct01
 /add_key/add_key01
 /add_key/add_key02
+/add_key/add_key03
 /adjtimex/adjtimex01
 /adjtimex/adjtimex02
 /alarm/alarm01
diff --git a/testcases/kernel/syscalls/add_key/add_key03.c b/testcases/kernel/syscalls/add_key/add_key03.c
new file mode 100644
index 000000000..8c68f8635
--- /dev/null
+++ b/testcases/kernel/syscalls/add_key/add_key03.c
@@ -0,0 +1,122 @@
+/*
+ * Copyright (c) 2017 Google, Inc.
+ *
+ * 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 237bbd29f7a0 ("KEYS: prevent creating a different
+ * user's keyrings").  The bug allowed any random user to create a keyring named
+ * "_uid.$UID" (or "_uid_ses.$UID"), and it would become the user keyring (or
+ * user session keyring) for user $UID, provided that it hadn't already been
+ * created.
+ *
+ * This test must be run as root so that it has permission to switch to another
+ * user ID and check whether the keyrings are wrong.  However, the underlying
+ * bug is actually reachable/exploitable by a non-root user.
+ */
+
+#include <errno.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "tst_test.h"
+#include "lapi/keyctl.h"
+
+static key_serial_t create_keyring(const char *description)
+{
+	TEST(add_key("keyring", description, NULL, 0,
+		     KEY_SPEC_PROCESS_KEYRING));
+	if (TEST_RETURN < 0) {
+		tst_brk(TBROK | TTERRNO,
+			"unable to create keyring '%s'", description);
+	}
+	return TEST_RETURN;
+}
+
+static key_serial_t get_keyring_id(key_serial_t special_id)
+{
+	TEST(keyctl(KEYCTL_GET_KEYRING_ID, special_id, 1));
+	if (TEST_RETURN < 0) {
+		tst_brk(TBROK | TTERRNO,
+			"unable to get ID of keyring %d", special_id);
+	}
+	return TEST_RETURN;
+}
+
+static void unlink_keyring(key_serial_t id)
+{
+	TEST(keyctl(KEYCTL_UNLINK, id, KEY_SPEC_PROCESS_KEYRING));
+	if (TEST_RETURN < 0) {
+		tst_brk(TBROK | TTERRNO,
+			"unable to unlink the keyring we created");
+	}
+}
+
+static void do_test(void)
+{
+	int i;
+
+	/*
+	 * Try with multiple user IDs before reporting success.  By chance, some
+	 * users may already have an existing user keyring; the bug will not be
+	 * reproducible for them.
+	 */
+	for (i = 0; i < 10; i++) {
+		char description[32];
+		uid_t uid;
+		key_serial_t fake_user_keyring;
+		key_serial_t fake_user_session_keyring;
+
+		uid = rand();
+		if (uid == 0)
+			continue;
+
+		sprintf(description, "_uid.%u", uid);
+		fake_user_keyring = create_keyring(description);
+		sprintf(description, "_uid_ses.%u", uid);
+		fake_user_session_keyring = create_keyring(description);
+
+		TEST(setreuid(uid, 0));
+		if (TEST_RETURN < 0) {
+			tst_brk(TBROK | TTERRNO,
+				"unable to set real uid to %u", uid);
+		}
+
+		if (fake_user_keyring == get_keyring_id(KEY_SPEC_USER_KEYRING)) {
+			tst_brk(TFAIL,
+				"created user keyring for another user");
+		}
+
+		if (fake_user_session_keyring ==
+		    get_keyring_id(KEY_SPEC_USER_SESSION_KEYRING)) {
+			tst_brk(TFAIL,
+				"created user session keyring for another user");
+		}
+
+		TEST(setreuid(0, 0));
+		if (TEST_RETURN < 0)
+			tst_brk(TBROK | TTERRNO, "unable to reset real uid");
+		uid++;
+
+		unlink_keyring(fake_user_keyring);
+		unlink_keyring(fake_user_session_keyring);
+	}
+	tst_res(TPASS, "expectedly could not create another user's keyrings");
+}
+
+static struct tst_test test = {
+	.test_all = do_test,
+	.needs_root = 1,
+};
-- 
2.14.2.920.gcf0c67979c-goog


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [LTP] [PATCH 4/4] syscalls/add_key03: new test for forging user keyrings
  2017-10-10 17:51 ` [LTP] " Eric Biggers
@ 2017-10-11 13:47 ` Cyril Hrubis
  -1 siblings, 0 replies; 14+ messages in thread
From: Cyril Hrubis @ 2017-10-11 13:47 UTC (permalink / raw)
  To: keyrings

Hi!
> +static key_serial_t create_keyring(const char *description)
> +{
> +	TEST(add_key("keyring", description, NULL, 0,
> +		     KEY_SPEC_PROCESS_KEYRING));
> +	if (TEST_RETURN < 0) {
> +		tst_brk(TBROK | TTERRNO,
> +			"unable to create keyring '%s'", description);
> +	}
> +	return TEST_RETURN;
> +}
> +
> +static key_serial_t get_keyring_id(key_serial_t special_id)
> +{
> +	TEST(keyctl(KEYCTL_GET_KEYRING_ID, special_id, 1));
> +	if (TEST_RETURN < 0) {
> +		tst_brk(TBROK | TTERRNO,
> +			"unable to get ID of keyring %d", special_id);
> +	}
> +	return TEST_RETURN;
> +}
> +
> +static void unlink_keyring(key_serial_t id)
> +{
> +	TEST(keyctl(KEYCTL_UNLINK, id, KEY_SPEC_PROCESS_KEYRING));
> +	if (TEST_RETURN < 0) {
> +		tst_brk(TBROK | TTERRNO,
> +			"unable to unlink the keyring we created");
> +	}
> +}
> +
> +static void do_test(void)
> +{
> +	int i;
> +
> +	/*
> +	 * Try with multiple user IDs before reporting success.  By chance, some
> +	 * users may already have an existing user keyring; the bug will not be
> +	 * reproducible for them.
> +	 */
> +	for (i = 0; i < 10; i++) {
> +		char description[32];
> +		uid_t uid;
> +		key_serial_t fake_user_keyring;
> +		key_serial_t fake_user_session_keyring;
> +
> +		uid = rand();
> +		if (uid = 0)
> +			continue;

We have testcases that look for unused uid with this loop:

	for (i = 1; i < 1000; i++) {
		if (!getpwuid(i))
			return i;
	}

What about using this instead of doing 10 random tries?

> +		sprintf(description, "_uid.%u", uid);
> +		fake_user_keyring = create_keyring(description);
> +		sprintf(description, "_uid_ses.%u", uid);
> +		fake_user_session_keyring = create_keyring(description);
> +
> +		TEST(setreuid(uid, 0));
> +		if (TEST_RETURN < 0) {
> +			tst_brk(TBROK | TTERRNO,
> +				"unable to set real uid to %u", uid);
> +		}

I guess that we should add SAFE_SETREUID() to the tst_safe_macros.h
library. We do have SAFE_SETRESUID() though, so we may as well use
SAFE_SETRESUID(uid, -1, -1) here.

> +		if (fake_user_keyring = get_keyring_id(KEY_SPEC_USER_KEYRING)) {
> +			tst_brk(TFAIL,
> +				"created user keyring for another user");
> +		}
> +
> +		if (fake_user_session_keyring =
> +		    get_keyring_id(KEY_SPEC_USER_SESSION_KEYRING)) {
> +			tst_brk(TFAIL,
> +				"created user session keyring for another user");
> +		}
> +
> +		TEST(setreuid(0, 0));
> +		if (TEST_RETURN < 0)
> +			tst_brk(TBROK | TTERRNO, "unable to reset real uid");
> +		uid++;
> +
> +		unlink_keyring(fake_user_keyring);
> +		unlink_keyring(fake_user_session_keyring);
> +	}
> +	tst_res(TPASS, "expectedly could not create another user's keyrings");
> +}
> +
> +static struct tst_test test = {
> +	.test_all = do_test,
> +	.needs_root = 1,
> +};
> -- 
> 2.14.2.920.gcf0c67979c-goog
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [LTP] [PATCH 4/4] syscalls/add_key03: new test for forging user keyrings
@ 2017-10-11 13:47 ` Cyril Hrubis
  0 siblings, 0 replies; 14+ messages in thread
From: Cyril Hrubis @ 2017-10-11 13:47 UTC (permalink / raw)
  To: ltp

Hi!
> +static key_serial_t create_keyring(const char *description)
> +{
> +	TEST(add_key("keyring", description, NULL, 0,
> +		     KEY_SPEC_PROCESS_KEYRING));
> +	if (TEST_RETURN < 0) {
> +		tst_brk(TBROK | TTERRNO,
> +			"unable to create keyring '%s'", description);
> +	}
> +	return TEST_RETURN;
> +}
> +
> +static key_serial_t get_keyring_id(key_serial_t special_id)
> +{
> +	TEST(keyctl(KEYCTL_GET_KEYRING_ID, special_id, 1));
> +	if (TEST_RETURN < 0) {
> +		tst_brk(TBROK | TTERRNO,
> +			"unable to get ID of keyring %d", special_id);
> +	}
> +	return TEST_RETURN;
> +}
> +
> +static void unlink_keyring(key_serial_t id)
> +{
> +	TEST(keyctl(KEYCTL_UNLINK, id, KEY_SPEC_PROCESS_KEYRING));
> +	if (TEST_RETURN < 0) {
> +		tst_brk(TBROK | TTERRNO,
> +			"unable to unlink the keyring we created");
> +	}
> +}
> +
> +static void do_test(void)
> +{
> +	int i;
> +
> +	/*
> +	 * Try with multiple user IDs before reporting success.  By chance, some
> +	 * users may already have an existing user keyring; the bug will not be
> +	 * reproducible for them.
> +	 */
> +	for (i = 0; i < 10; i++) {
> +		char description[32];
> +		uid_t uid;
> +		key_serial_t fake_user_keyring;
> +		key_serial_t fake_user_session_keyring;
> +
> +		uid = rand();
> +		if (uid == 0)
> +			continue;

We have testcases that look for unused uid with this loop:

	for (i = 1; i < 1000; i++) {
		if (!getpwuid(i))
			return i;
	}

What about using this instead of doing 10 random tries?

> +		sprintf(description, "_uid.%u", uid);
> +		fake_user_keyring = create_keyring(description);
> +		sprintf(description, "_uid_ses.%u", uid);
> +		fake_user_session_keyring = create_keyring(description);
> +
> +		TEST(setreuid(uid, 0));
> +		if (TEST_RETURN < 0) {
> +			tst_brk(TBROK | TTERRNO,
> +				"unable to set real uid to %u", uid);
> +		}

I guess that we should add SAFE_SETREUID() to the tst_safe_macros.h
library. We do have SAFE_SETRESUID() though, so we may as well use
SAFE_SETRESUID(uid, -1, -1) here.

> +		if (fake_user_keyring == get_keyring_id(KEY_SPEC_USER_KEYRING)) {
> +			tst_brk(TFAIL,
> +				"created user keyring for another user");
> +		}
> +
> +		if (fake_user_session_keyring ==
> +		    get_keyring_id(KEY_SPEC_USER_SESSION_KEYRING)) {
> +			tst_brk(TFAIL,
> +				"created user session keyring for another user");
> +		}
> +
> +		TEST(setreuid(0, 0));
> +		if (TEST_RETURN < 0)
> +			tst_brk(TBROK | TTERRNO, "unable to reset real uid");
> +		uid++;
> +
> +		unlink_keyring(fake_user_keyring);
> +		unlink_keyring(fake_user_session_keyring);
> +	}
> +	tst_res(TPASS, "expectedly could not create another user's keyrings");
> +}
> +
> +static struct tst_test test = {
> +	.test_all = do_test,
> +	.needs_root = 1,
> +};
> -- 
> 2.14.2.920.gcf0c67979c-goog
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [LTP] [PATCH 4/4] syscalls/add_key03: new test for forging user keyrings
  2017-10-11 13:47 ` Cyril Hrubis
@ 2017-10-11 13:53   ` Cyril Hrubis
  -1 siblings, 0 replies; 14+ messages in thread
From: Cyril Hrubis @ 2017-10-11 13:53 UTC (permalink / raw)
  To: keyrings

Hi!
> > +static void do_test(void)
> > +{
> > +	int i;
> > +
> > +	/*
> > +	 * Try with multiple user IDs before reporting success.  By chance, some
> > +	 * users may already have an existing user keyring; the bug will not be
> > +	 * reproducible for them.
> > +	 */
> > +	for (i = 0; i < 10; i++) {
> > +		char description[32];
> > +		uid_t uid;
> > +		key_serial_t fake_user_keyring;
> > +		key_serial_t fake_user_session_keyring;
> > +
> > +		uid = rand();
> > +		if (uid = 0)
> > +			continue;
> 
> We have testcases that look for unused uid with this loop:
> 
> 	for (i = 1; i < 1000; i++) {
> 		if (!getpwuid(i))
> 			return i;
> 	}
> 
> What about using this instead of doing 10 random tries?

Or even better try to get the keyring as an user and proceed with the
test if there is none?

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [LTP] [PATCH 4/4] syscalls/add_key03: new test for forging user keyrings
@ 2017-10-11 13:53   ` Cyril Hrubis
  0 siblings, 0 replies; 14+ messages in thread
From: Cyril Hrubis @ 2017-10-11 13:53 UTC (permalink / raw)
  To: ltp

Hi!
> > +static void do_test(void)
> > +{
> > +	int i;
> > +
> > +	/*
> > +	 * Try with multiple user IDs before reporting success.  By chance, some
> > +	 * users may already have an existing user keyring; the bug will not be
> > +	 * reproducible for them.
> > +	 */
> > +	for (i = 0; i < 10; i++) {
> > +		char description[32];
> > +		uid_t uid;
> > +		key_serial_t fake_user_keyring;
> > +		key_serial_t fake_user_session_keyring;
> > +
> > +		uid = rand();
> > +		if (uid == 0)
> > +			continue;
> 
> We have testcases that look for unused uid with this loop:
> 
> 	for (i = 1; i < 1000; i++) {
> 		if (!getpwuid(i))
> 			return i;
> 	}
> 
> What about using this instead of doing 10 random tries?

Or even better try to get the keyring as an user and proceed with the
test if there is none?

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2017-10-11 13:53 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-11 13:47 [LTP] [PATCH 4/4] syscalls/add_key03: new test for forging user keyrings Cyril Hrubis
2017-10-11 13:47 ` Cyril Hrubis
2017-10-11 13:53 ` Cyril Hrubis
2017-10-11 13:53   ` Cyril Hrubis
  -- strict thread matches above, loose matches on Subject: below --
2017-10-10 17:51 Eric Biggers
2017-10-10 17:51 ` [LTP] " Eric Biggers
2017-10-10 17:51 [PATCH 3/4] syscalls/keyctl07: new test for oops when reading negative key Eric Biggers
2017-10-10 17:51 ` [LTP] " Eric Biggers
2017-10-10 17:51 [PATCH 2/4] syscalls/keyctl06: new test for keyring_read() buffer overrun Eric Biggers
2017-10-10 17:51 ` [LTP] " Eric Biggers
2017-10-10 17:51 [PATCH 1/4] lapi/keyctl.h: add a few missing definitions Eric Biggers
2017-10-10 17:51 ` [LTP] " Eric Biggers
2017-10-10 17:51 [PATCH 0/4] ltp: add tests for some recently-fixed keyrings bugs Eric Biggers
2017-10-10 17:51 ` [LTP] " Eric Biggers

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.