#1013924 coreutils: runcon -c getfscon()s program verbatim but execve()s it; trojan moment?

Package:
coreutils
Source:
coreutils
Description:
GNU core utilities
Submitter:
наб
Date:
2022-06-27 18:15:04 UTC
Severity:
normal
#1013924#5
Date:
2022-06-27 15:04:00 UTC
From:
To:
Dear Maintainer,

The strace for runcon -c true true (after a > true) contains
  getxattr("true", "security.selinux", "unconfined_u:object_r:user_tmp_t", 255) = 36
  execve("/usr/local/sbin/true", ["true", "true"]) = -1 ENOENT
  execve("/usr/local/bin/true", ["true", "true"]) = -1 ENOENT
  execve("/sbin/true", ["true", "true"]) = -1 ENOENT
  execve("/bin/true", ["true", "true"]) = 0

This corresponds to getfscon("true"), execvp("true", ["true", NULL]).
(of course, this also errors if ./true doesn't exist).

So, uh: is this intentional? It certainly feels wrong? All invocations
take a PATH executable except this one which takes a PATH executable
that must *also* be a valid file? And also invites a trivial trojan
because the precomputed transition is to the file in the cwd, but the
program executed lives somewhere in PATH? Should -c just execv()
instead? Am I misunderstanding the usefulness of this?

Best,
наб

#1013924#10
Date:
2022-06-27 18:13:53 UTC
From:
To:
This is a fair point.
I.e. the following patch would be more correct operation.
I'll propose this upstream.

Now existing scripts would need to pass absolute paths to `runcon -c`
to work in the first place, so I don't know how much of a security
issue this is in practice.

thanks,
Pádraig

iff --git a/src/runcon.c b/src/runcon.c
index c4227c784..d85411c79 100644
--- a/src/runcon.c
+++ b/src/runcon.c
@@ -255,7 +255,7 @@ main (int argc, char **argv)
    if (cur_context != NULL)
      freecon (cur_context);

-  execvp (argv[optind], argv + optind);
+  (compute_trans ? execv : execvp) (argv[optind], argv + optind);

    int exit_status = errno == ENOENT ? EXIT_ENOENT : EXIT_CANNOT_INVOKE;
    error (0, errno, "%s", quote (argv[optind]));

#1013924#15
Date:
2022-06-27 18:13:53 UTC
From:
To:
This is a fair point.
I.e. the following patch would be more correct operation.
I'll propose this upstream.

Now existing scripts would need to pass absolute paths to `runcon -c`
to work in the first place, so I don't know how much of a security
issue this is in practice.

thanks,
Pádraig

iff --git a/src/runcon.c b/src/runcon.c
index c4227c784..d85411c79 100644
--- a/src/runcon.c
+++ b/src/runcon.c
@@ -255,7 +255,7 @@ main (int argc, char **argv)
    if (cur_context != NULL)
      freecon (cur_context);

-  execvp (argv[optind], argv + optind);
+  (compute_trans ? execv : execvp) (argv[optind], argv + optind);

    int exit_status = errno == ENOENT ? EXIT_ENOENT : EXIT_CANNOT_INVOKE;
    error (0, errno, "%s", quote (argv[optind]));