Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Date: Tue, 30 Jan 2018 23:32:37 +0300
From: Alexander Monakov <amonakov@...ras.ru>
To: musl@...ts.openwall.com
Subject: [PATCH] faccessat: fix error code on setreXid failure

Commit 316d6741b68b485205d7233c98bd6c795bb80370 changed one use of
SYS_exit in 'checker' without changing another just three lines above,
and then commit f9fb20b42da0e755d93de229a5a737d79a0e8f60 changed the
meaning of return value, causing EACCES to be reported instead of EBUSY
if preparatory setregid/setreuid fail.
---

This is the minimal fix for the issue, but it appears there's another:
collecting checker's exit code and reaping the zombie is implemented as

		int status;
		do {
			__syscall(SYS_wait4, pid, &status, __WCLONE, 0);
		} while (!WIFEXITED(status) && !WIFSIGNALED(status));

but I don't understand why this retry loop is required and correct:

- if another thread won the race to collect the zombie by doing something
  like waitpid(-1, 0, __WALL), it fails to check syscall's return value and
  uses uninitialized 'status', possibly causing an infinite loop or OOB access
  in the parent;

- the code seems to assume that the zombie will not be auto-collected even if
  SIGCHLD disposition is set to SIG_IGN; this sounds logical, but not explicitly
  documented as far as I can tell;

- if the two problems above don't arise, I don't see how the test in while ()
  condition can fail; we have signals blocked, so waitpid can only return when
  the child no longer exists.

Plus, using CLONE_VM | CLONE_VFORK would help conserve resources.

Alexander

 src/unistd/faccessat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/unistd/faccessat.c b/src/unistd/faccessat.c
index 33478959..954cbdb4 100644
--- a/src/unistd/faccessat.c
+++ b/src/unistd/faccessat.c
@@ -25,7 +25,7 @@ static int checker(void *p)
 	int i;
 	if (__syscall(SYS_setregid, __syscall(SYS_getegid), -1)
 	    || __syscall(SYS_setreuid, __syscall(SYS_geteuid), -1))
-		__syscall(SYS_exit, 1);
+		return sizeof errors/sizeof *errors - 1;
 	ret = __syscall(SYS_faccessat, c->fd, c->filename, c->amode, 0);
 	for (i=0; i < sizeof errors/sizeof *errors - 1 && ret!=errors[i]; i++);
 	return i;
-- 
2.11.0

Powered by blists - more mailing lists

Your e-mail address:

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.