Move getcwd05 to realpath01 and fix for old distro

Message ID 20180801134717.29909-1-mmoese@suse.de
State Accepted
Delegated to: Petr Vorel
Headers show
Series
  • Move getcwd05 to realpath01 and fix for old distro
Related show

Commit Message

Michael Moese Aug. 1, 2018, 1:47 p.m.
The testcase getcwd05 is a regression test for cve-2018-1000001 [1].
However, there were changes in the behavior of libc functions, that some older
distributions refused to backport.
The testcase was two testcases, one for getcwd() and one for realpath().
While the behavior of getcwd() changed, it is totally independent from
the thestcase for the vulnerability in realpath. So, this test should be
moved to realpath/realpath01.c. In addition, the test of getcwd() is
totally unneeded here to test realpath() for the fix.

[1] https://sourceware.org/git/gitweb.cgi?p=glibc.git;a=commitdiff;h=52a713fdd0a30e1bd79818e2e3c4ab44ddca1a94

Signed-off-by: Michael Moese <mmoese@suse.de>
---
 runtest/cve                                   |  2 +-
 runtest/syscalls                              |  3 ++-
 testcases/kernel/syscalls/getcwd/.gitignore   |  1 -
 testcases/kernel/syscalls/realpath/.gitignore |  1 +
 testcases/kernel/syscalls/realpath/Makefile   | 23 ++++++++++++++++
 .../getcwd05.c => realpath/realpath01.c}      | 26 +++++--------------
 6 files changed, 33 insertions(+), 23 deletions(-)
 create mode 100644 testcases/kernel/syscalls/realpath/.gitignore
 create mode 100644 testcases/kernel/syscalls/realpath/Makefile
 rename testcases/kernel/syscalls/{getcwd/getcwd05.c => realpath/realpath01.c} (72%)

Comments

Jan Stancek Aug. 2, 2018, 10:53 a.m. | #1
----- Original Message -----
> The testcase getcwd05 is a regression test for cve-2018-1000001 [1].
> However, there were changes in the behavior of libc functions, that some
> older
> distributions refused to backport.
> The testcase was two testcases, one for getcwd() and one for realpath().
> While the behavior of getcwd() changed, it is totally independent from
> the thestcase for the vulnerability in realpath. So, this test should be
> moved to realpath/realpath01.c. In addition, the test of getcwd() is
> totally unneeded here to test realpath() for the fix.
> 
> [1]
> https://sourceware.org/git/gitweb.cgi?p=glibc.git;a=commitdiff;h=52a713fdd0a30e1bd79818e2e3c4ab44ddca1a94
> 
> Signed-off-by: Michael Moese <mmoese@suse.de>

Looks good to me, ack.
Adding Petr, as he wrote the original test.

Regards,
Jan
Petr Vorel Aug. 13, 2018, 1:41 p.m. | #2
Hi Michael,

> The testcase getcwd05 is a regression test for cve-2018-1000001 [1].
> However, there were changes in the behavior of libc functions, that some older
> distributions refused to backport.
> The testcase was two testcases, one for getcwd() and one for realpath().
> While the behavior of getcwd() changed, it is totally independent from
> the thestcase for the vulnerability in realpath. So, this test should be
> moved to realpath/realpath01.c. In addition, the test of getcwd() is
> totally unneeded here to test realpath() for the fix.

> [1] https://sourceware.org/git/gitweb.cgi?p=glibc.git;a=commitdiff;h=52a713fdd0a30e1bd79818e2e3c4ab44ddca1a94

> Signed-off-by: Michael Moese <mmoese@suse.de>
Acked-by: Petr Vorel <pvorel@suse.cz>

Thanks for your patch. ACK with 2 minor issues bellow.
I can fix them (no need to repost a patch) if you agree.

>  rename testcases/kernel/syscalls/{getcwd/getcwd05.c => realpath/realpath01.c} (72%)
I wonder whether file should be named realpath_buffer_underflow.c to be more
descriptive.

> diff --git a/testcases/kernel/syscalls/realpath/Makefile b/testcases/kernel/syscalls/realpath/Makefile
> new file mode 100644
> index 000000000..bd617d806
> --- /dev/null
> +++ b/testcases/kernel/syscalls/realpath/Makefile
> @@ -0,0 +1,23 @@
> +#
> +#  Copyright (c) International Business Machines  Corp., 2001
Copy paste error. + I'd prefer to use 'SPDX-License-Identifier: GPL-2.0-or-later' as it's shorter.
> +#
> +#  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, write to the Free Software
> +#  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA


Kind regards,
Petr
Cyril Hrubis Aug. 13, 2018, 2:26 p.m. | #3
Hi!
> >  rename testcases/kernel/syscalls/{getcwd/getcwd05.c => realpath/realpath01.c} (72%)
> I wonder whether file should be named realpath_buffer_underflow.c to be more
> descriptive.

We usually keep just the names short and simple, so I would keep it as
it is.

> > diff --git a/testcases/kernel/syscalls/realpath/Makefile b/testcases/kernel/syscalls/realpath/Makefile
> > new file mode 100644
> > index 000000000..bd617d806
> > --- /dev/null
> > +++ b/testcases/kernel/syscalls/realpath/Makefile
> > @@ -0,0 +1,23 @@
> > +#
> > +#  Copyright (c) International Business Machines  Corp., 2001
> Copy paste error. + I'd prefer to use 'SPDX-License-Identifier: GPL-2.0-or-later' as it's shorter.

Just fix this before you push the patch.

> > +#
> > +#  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, write to the Free Software
> > +#  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
Petr Vorel Aug. 14, 2018, 7:13 a.m. | #4
Hi Cyril,

> Hi!
> > >  rename testcases/kernel/syscalls/{getcwd/getcwd05.c => realpath/realpath01.c} (72%)
> > I wonder whether file should be named realpath_buffer_underflow.c to be more
> > descriptive.

> We usually keep just the names short and simple, so I would keep it as
> it is.
I think CVE related are the only exception (see Eric Biggers's post [1]), but
I'm ok with keeping it only realpath01.c.


Kind regards,
Petr

[1] http://lists.linux.it/pipermail/ltp/2018-March/007388.html
Cyril Hrubis Aug. 14, 2018, 2:17 p.m. | #5
Hi!
> > We usually keep just the names short and simple, so I would keep it as
> > it is.
> I think CVE related are the only exception (see Eric Biggers's post [1]), but
> I'm ok with keeping it only realpath01.c.

Let's keep the names short, we do have a bunch of regression tests named
as syscallXY.c anyways. And the CVE was in realpath() implementation
anyways, so it's not like we named wrongly to begin with.

> [1] http://lists.linux.it/pipermail/ltp/2018-March/007388.html
Petr Vorel Aug. 14, 2018, 3:55 p.m. | #6
Hi,

> > > We usually keep just the names short and simple, so I would keep it as
> > > it is.
> > I think CVE related are the only exception (see Eric Biggers's post [1]), but
> > I'm ok with keeping it only realpath01.c.

> Let's keep the names short, we do have a bunch of regression tests named
> as syscallXY.c anyways. And the CVE was in realpath() implementation
> anyways, so it's not like we named wrongly to begin with.
OK, pushed with the original name.
Thanks for your patch, Michael.

I did tiny changes:
Fixed copyright in Makefile and state reproducer info ("cve-2018-1000001
realpath buffer underflow") in source file. This should have been done by me in
the original post (when renaming it form cve-2018-1000001.c to getcwd05.c in
later versions of my original post).


Kind regards,
Petr

> > [1] http://lists.linux.it/pipermail/ltp/2018-March/007388.html

Patch

diff --git a/runtest/cve b/runtest/cve
index 58d8f12b2..b38fb3503 100644
--- a/runtest/cve
+++ b/runtest/cve
@@ -34,4 +34,4 @@  cve-2017-16939 cve-2017-16939
 cve-2017-17053 cve-2017-17053
 cve-2017-18075 pcrypt_aead01
 cve-2018-5803 sctp_big_chunk
-cve-2018-1000001 getcwd05
+cve-2018-1000001 realpath01
diff --git a/runtest/syscalls b/runtest/syscalls
index dc72484cb..4e813e9a7 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -348,7 +348,6 @@  getcwd01 getcwd01
 getcwd02 getcwd02
 getcwd03 getcwd03
 getcwd04 getcwd04
-getcwd05 getcwd05
 
 getdents01 getdents01
 getdents02 getdents02
@@ -895,6 +894,8 @@  readv01 readv01
 readv02 readv02
 readv03 readv03
 
+realpath01 realpath01
+
 reboot01 reboot01
 reboot02 reboot02
 
diff --git a/testcases/kernel/syscalls/getcwd/.gitignore b/testcases/kernel/syscalls/getcwd/.gitignore
index 99b2ba19f..338852b62 100644
--- a/testcases/kernel/syscalls/getcwd/.gitignore
+++ b/testcases/kernel/syscalls/getcwd/.gitignore
@@ -2,4 +2,3 @@ 
 /getcwd02
 /getcwd03
 /getcwd04
-/getcwd05
diff --git a/testcases/kernel/syscalls/realpath/.gitignore b/testcases/kernel/syscalls/realpath/.gitignore
new file mode 100644
index 000000000..54860c088
--- /dev/null
+++ b/testcases/kernel/syscalls/realpath/.gitignore
@@ -0,0 +1 @@ 
+/realpath01
diff --git a/testcases/kernel/syscalls/realpath/Makefile b/testcases/kernel/syscalls/realpath/Makefile
new file mode 100644
index 000000000..bd617d806
--- /dev/null
+++ b/testcases/kernel/syscalls/realpath/Makefile
@@ -0,0 +1,23 @@ 
+#
+#  Copyright (c) International Business Machines  Corp., 2001
+#
+#  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, write to the Free Software
+#  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#
+
+top_srcdir		?= ../../../..
+
+include $(top_srcdir)/include/mk/testcases.mk
+
+include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/syscalls/getcwd/getcwd05.c b/testcases/kernel/syscalls/realpath/realpath01.c
similarity index 72%
rename from testcases/kernel/syscalls/getcwd/getcwd05.c
rename to testcases/kernel/syscalls/realpath/realpath01.c
index f39df4b1d..dcd6785fc 100644
--- a/testcases/kernel/syscalls/getcwd/getcwd05.c
+++ b/testcases/kernel/syscalls/realpath/realpath01.c
@@ -20,36 +20,22 @@  static void setup(void)
 	SAFE_CHROOT(CHROOT_DIR);
 }
 
-static void run(unsigned int i)
+static void run(void)
 {
-	int fail = 0;
-
-	if (i) {
-		tst_res(TINFO, "testing realpath()");
-		TESTPTR(realpath(".", NULL));
-	} else {
-		tst_res(TINFO, "testing getcwd()");
-		TESTPTR(getcwd(NULL, 0));
-	}
+	TESTPTR(realpath(".", NULL));
 
 	if (TST_ERR != ENOENT) {
 		tst_res(TFAIL | TTERRNO, "returned unexpected errno");
-		fail = 1;
-	}
-
-	if (TST_RET_PTR != NULL) {
+	} else	if (TST_RET_PTR != NULL) {
 		tst_res(TFAIL, "syscall didn't return NULL: '%s'",
 				(char *)TST_RET_PTR);
-		fail = 1;
-	}
-
-	if (!fail)
+	} else {
 		tst_res(TPASS, "bug not reproduced");
+	}
 }
 
 static struct tst_test test = {
-	.test = run,
-	.tcnt = 2,
+	.test_all = run,
 	.setup = setup,
 	.needs_root = 1,
 	.needs_tmpdir = 1,