Fix escape sequence injection in /var/log/auth.log

[skyler-ferrante]

See issue #959. su was sending argv[0] to auth.log, which can contain newlines or escape sequences, which can be used to hide messages.

[alejandro-colomar]

Other programs may also write to auth.log. For example, try passwd root from your user (for some reason, the escape sequence didn't work for me in the case of passwd(1), though). Maybe we should hardcode all (or most?) calls to log_set_progname() to be safe?

$ grep -rn log_set_progname -B1
lib/shadowlog.h-34-
lib/shadowlog.h:35:extern void log_set_progname(const char *);
--
lib/shadowlog.c-7-
lib/shadowlog.c:8:void log_set_progname(const char *progname)
--
libsubid/api.c-26-          return false;
libsubid/api.c:27:      log_set_progname(progname);
libsubid/api.c-28-  } else {
libsubid/api.c:29:      log_set_progname("(libsubid)");
--
src/vipw.c-474- Prog = Basename (argv[0]);
src/vipw.c:475: log_set_progname(Prog);
--
src/sulogin.c-75-   Prog = Basename (argv[0]);
src/sulogin.c:76:   log_set_progname(Prog);
--
src/pwunconv.c-118- Prog = Basename (argv[0]);
src/pwunconv.c:119: log_set_progname(Prog);
--
src/chage.c-774-    Prog = Basename (argv[0]);
src/chage.c:775:    log_set_progname(Prog);
--
src/chfn.c-627- Prog = Basename (argv[0]);
src/chfn.c:628: log_set_progname(Prog);
--
src/check_subid_range.c-30- Prog = Basename (argv[0]);
src/check_subid_range.c:31: log_set_progname(Prog);
--
src/gpasswd.c-945-  Prog = Basename (argv[0]);
src/gpasswd.c:946:  log_set_progname(Prog);
--
src/newgidmap.c-154-    Prog = Basename (argv[0]);
src/newgidmap.c:155:    log_set_progname(Prog);
--
src/lastlog.c-294-  Prog = Basename (argv[0]);
src/lastlog.c:295:  log_set_progname(Prog);
--
src/chsh.c-480- Prog = Basename (argv[0]);
src/chsh.c:481: log_set_progname(Prog);
--
src/chpasswd.c-453- Prog = Basename (argv[0]);
src/chpasswd.c:454: log_set_progname(Prog);
--
src/groupmod.c-757- Prog = Basename (argv[0]);
src/groupmod.c:758: log_set_progname(Prog);
--
src/newusers.c-1062-    Prog = Basename (argv[0]);
src/newusers.c:1063:    log_set_progname(Prog);
--
src/su.c-745-   Prog = Basename (argv[0]);
src/su.c:746:   log_set_progname(Prog);
--
src/groupmems.c-579-    Prog = Basename (argv[0]);
src/groupmems.c:580:    log_set_progname(Prog);
--
src/faillog.c-517-  Prog = Basename (argv[0]);
src/faillog.c:518:  log_set_progname(Prog);
--
src/groupdel.c-355- Prog = Basename (argv[0]);
src/groupdel.c:356: log_set_progname(Prog);
--
src/pwconv.c-158-   Prog = Basename (argv[0]);
src/pwconv.c:159:   log_set_progname(Prog);
--
src/logoutd.c-160-  Prog = Basename (argv[0]);
src/logoutd.c:161:  log_set_progname(Prog);
--
src/groupadd.c-578- Prog = Basename (argv[0]);
src/groupadd.c:579: log_set_progname(Prog);
--
src/getsubids.c-26- Prog = Basename (argv[0]);
src/getsubids.c:27: log_set_progname(Prog);
--
src/userdel.c-965-  Prog = Basename (argv[0]);
src/userdel.c:966:  log_set_progname(Prog);
--
src/new_subid_range.c-31-   Prog = Basename (argv[0]);
src/new_subid_range.c:32:   log_set_progname(Prog);
--
src/chgpasswd.c-426-    Prog = Basename (argv[0]);
src/chgpasswd.c:427:    log_set_progname(Prog);
--
src/usermod.c-2164- Prog = Basename (argv[0]);
src/usermod.c:2165: log_set_progname(Prog);
--
src/grpconv.c-129-  Prog = Basename (argv[0]);
src/grpconv.c:130:  log_set_progname(Prog);
--
src/newuidmap.c-83- Prog = Basename (argv[0]);
src/newuidmap.c:84: log_set_progname(Prog);
--
src/pwck.c-839- Prog = Basename (argv[0]);
src/pwck.c:840: log_set_progname(Prog);
--
src/newgrp.c-428-
src/newgrp.c:429:   log_set_progname(Prog);
--
src/grpunconv.c-127-    Prog = Basename (argv[0]);
src/grpunconv.c:128:    log_set_progname(Prog);
--
src/expiry.c-128-   Prog = Basename (argv[0]);
src/expiry.c:129:   log_set_progname(Prog);
--
src/passwd.c-736-   Prog = Basename (argv[0]);
src/passwd.c:737:   log_set_progname(Prog);
--
src/grpck.c-822-    Prog = Basename (argv[0]);
src/grpck.c:823:    log_set_progname(Prog);
--
src/login.c-523-    Prog = Basename (argv[0]);
src/login.c:524:    log_set_progname(Prog);
--
src/get_subid_owners.c-24-  Prog = Basename (argv[0]);
src/get_subid_owners.c:25:  log_set_progname(Prog);
--
src/free_subid_range.c-28-  Prog = Basename (argv[0]);
src/free_subid_range.c:29:  log_set_progname(Prog);
--
src/groups.c-103-   Prog = Basename (argv[0]);
src/groups.c:104:   log_set_progname(Prog);
--
src/useradd.c-2491- Prog = Basename (argv[0]);
src/useradd.c:2492: log_set_progname(Prog);

[alejandro-colomar]

Hmmm, something seems to be escaping control characters in the case of passwd(1). I see #033 in the log, and if I pass newlines, I see #012 in the log. For some reason, that escape is working in passwd(1), but not in su(1). Maybe there's a bug somewhere...

[alejandro-colomar]

Hmmm, the escaped version (e.g., passwd(1)) comes from SYSLOG(), which calls syslog(3), and seems to be escaping control characters. The problematic one (e.g., su(1)), I'm not sure what's calling, but it's likely fprintf(3).

[alejandro-colomar]

I think we can hard-code all program names, just to be safe. I don't see why one would want a different name in the logs.

[alejandro-colomar]

And I see that we're initializing openlog(3) (via OPENLOG()) with hard-coded names, so I don't see why we later pass argv[0] to syslog(3).

$ grep -rn OPENLOG src/ lib*
src/vipw.c:486: OPENLOG (do_vipw ? "vipw" : "vigr");
src/pwunconv.c:128: OPENLOG ("pwunconv");
src/chage.c:789:    OPENLOG ("chage");
src/chfn.c:644: OPENLOG ("chfn");
src/gpasswd.c:949:  OPENLOG ("gpasswd");
src/chsh.c:495: OPENLOG ("chsh");
src/chpasswd.c:479: OPENLOG ("chpasswd");
src/groupmod.c:768: OPENLOG ("groupmod");
src/newusers.c:1073:    OPENLOG ("newusers");
src/su.c:1021:  OPENLOG ("su");
src/groupmems.c:589:    OPENLOG ("groupmems");
src/groupdel.c:366: OPENLOG ("groupdel");
src/pwconv.c:168:   OPENLOG ("pwconv");
src/logoutd.c:164:  OPENLOG ("logoutd");
src/groupadd.c:589: OPENLOG ("groupadd");
src/userdel.c:975:  OPENLOG ("userdel");
src/chgpasswd.c:444:    OPENLOG ("chgpasswd");
src/usermod.c:2175: OPENLOG ("usermod");
src/grpconv.c:139:  OPENLOG ("grpconv");
src/pwck.c:849: OPENLOG ("pwck");
src/newgrp.c:431:   OPENLOG (Prog);
src/grpunconv.c:137:    OPENLOG ("grpunconv");
src/expiry.c:150:   OPENLOG ("expiry");
src/passwd.c:758:   OPENLOG ("passwd");
src/grpck.c:832:    OPENLOG ("grpck");
src/login.c:590:    OPENLOG ("login");
src/useradd.c:2503: OPENLOG ("useradd");
lib/defines.h:131:#define OPENLOG(progname) openlog(progname, SYSLOG_OPTIONS, SYSLOG_FACILITY)

[skyler-ferrante]

Other programs may also write to auth.log. For example, try passwd root from your user (for some reason, the escape sequence didn't work for me in the case of passwd(1), though).

Were you able to get anything other than su to send escape sequences? That's all I was able to. I was trying to modify behavior in the least amount possible, while still fixing the issue.

You should probably Prog = "su"; in the previous line. Otherwise, other uses of Prog below in fprintf(3) calls will have similar issues. Is it useful to read argv[0] at all?

It seems that fprintf is not used to print to log files in shadow. If we consider an attacker changing argv, printing ansi escape sequences to stdout/stderr doesn't seem like a threat surface. Printing to /var/log/auth.log is a threat surface, since a low privilege user can cause this to happen, and a high privilege user might look at them in a dangerous way (e.g. tail).

I agree that all program names should probably just be hard-coded. I only changed su since it seemed only su was affected. But I can send in a patch that hardcodes everything since that's probably better.

[skyler-ferrante]

Okay, I created a new patchset that changes every program in shadow to use a hard coded value for Prog. If these changes are considered too large, or we don't want to change the default Prog behavior, we can go back to just changing su.

[alejandro-colomar]

Other programs may also write to auth.log. For example, try passwd root from your user (for some reason, the escape sequence didn't work for me in the case of passwd(1), though).

Were you able to get anything other than su to send escape sequences? That's all I was able to. I was trying to modify behavior in the least amount possible, while still fixing the issue.

You should probably Prog = "su"; in the previous line. Otherwise, other uses of Prog below in fprintf(3) calls will have similar issues. Is it useful to read argv[0] at all?

It seems that fprintf is not used to print to log files in shadow. If we consider an attacker changing argv, printing ansi escape sequences to stdout/stderr doesn't seem like a threat surface. Printing to /var/log/auth.log is a threat surface, since a low privilege user can cause this to happen, and a high privilege user might look at them in a dangerous way (e.g. tail).

I agree that all program names should probably just be hard-coded. I only changed su since it seemed only su was affected. But I can send in a patch that hardcodes everything since that's probably better.

I've been trying to debug this, and again I couldn't find the source code line that prints the log entry that su(1) is producing.

And then I've noticed that Debian uses --without-su when building shadow:

https://salsa.debian.org/debian/shadow/-/blob/master/debian/rules?ref_type=heads#L20

su(1) seems to come from util-linux.

$ apt-file find -x /bin/su$
sudo-rs: /usr/lib/cargo/bin/su            
util-linux: /usr/bin/su

So, maybe we don't have this bug after all. In what system did you reproduce it?

[skyler-ferrante]

I've been trying to debug this, and again I couldn't find the source code line that prints the log entry that su(1) is producing.

And then I've noticed that Debian uses --without-su when building shadow:

https://salsa.debian.org/debian/shadow/-/blob/master/debian/rules?ref_type=heads#L20

su(1) seems to come from util-linux.

$ apt-file find -x /bin/su$
sudo-rs: /usr/lib/cargo/bin/su            
util-linux: /usr/bin/su

So, maybe we don't have this bug after all. In what system did you reproduce it?

Ahh, this makes sense.

$ dpkg -S /bin/su
util-linux: /bin/su

Yes, it seems that shadow-utils was handling this correctly all along. I tried to reproduce this with a local/manual build of shadow-utils su and I could not. It seems the problem is in util-linux instead. I can reach out to them.

At this point, do we want to still hardcode argv[0]? If so, I can change everything to be const like you mentioned. But since the original issue was not even there to start with, I'm not sure if it makes sense to change the current behavior.

[alejandro-colomar]

At this point, do we want to still hardcode argv[0]? If so, I can change everything to be const like you mentioned. But since the original issue was not even there to start with, I'm not sure if it makes sense to change the current behavior.

Since we're frozen for the 4.15 release, we would merge it after that, if we want it.

I do like the patch. I see no reason to print basename(argv[0]) instead of the hard-coded name of the program. If we were printing the entire path, it could be useful if you're debugging different versions of the same program, which you may have in different paths. But I don't see a scenario where the basename wouldn't be the same, other than evil cases.

Moreover, SYSLOG(), which calls syslog(3), already prefixes the log entry with a hard-coded program name (due to the initialization in OPENLOG(), which calls openlog(3)). I don't see why we would print the program basename again. So I would write a second patch after this one, and get rid of those unnecessary uses of Prog in SYSLOG calls.

What do you think @hallyn?

[hallyn]

What do you think @hallyn?

less processing is better processing.

lgtm, thanks.

[skyler-ferrante]

To make sure we don't mess with it, you could use

static const char  Prog[] = "chage";

As suggested I changed Prog to be a constant array when possible.

What do you think @hallyn?

less processing is better processing.

lgtm, thanks.

Okay, I think it should be good now. We now ignore argv for most commands. There are some exceptions, like newgrp and vipw, but those rely on the binary name for what command they run.

[alejandro-colomar]

Reviewed-by: Alejandro Colomar <alx@kernel.org>

(but we need to wait until the 4.15 release)

Thanks!

[alejandro-colomar]

I'm merging now, as per the private report. Thanks!

Cc: @ikerexxe @hallyn