getenv(3) vs secure_getenv(3)

[alejandro-colomar]

alx@devuan:~/src/shadow/shadow/master$ grep -rn '\<getenv *(' lib* src/
lib/ttytype.c:36:	if (getenv ("TERM") != NULL) {
lib/defines.h:193:#  define shadow_getenv(name) getenv(name)
lib/mail.c:36:	mailbox = getenv ("MAILDIR");
lib/mail.c:52:	mailbox = getenv ("MAIL");
src/su.c:898:		cp = getenv ("TERM");
src/su.c:911:		cp = getenv ("COLORTERM");
src/su.c:935:		cp = getenv ("DISPLAY");
src/su.c:939:		cp = getenv ("XAUTHORITY");
src/su.c:960:	if (getenv ("IFS") != NULL) {	/* don't export user IFS ... */
src/su.c:1059:		shellstr = getenv ("SHELL");
src/login.c:327:	tmp = getenv ("LANG");
src/login.c:336:	tmp = getenv ("TZ");
src/login.c:352:	tmp = getenv ("HZ");
src/login.c:569:		tmp = getenv ("TERM");
src/login.c:1009:	if (getenv ("IFS")) {	/* don't export user IFS ... */
src/nologin.c:29:	char *ssh_origcmd = getenv("SSH_ORIGINAL_COMMAND");
src/newgrp.c:747:	cp = getenv ("SHELL");
src/vipw.c:284:	editor = getenv ("VISUAL");
src/vipw.c:286:		editor = getenv ("EDITOR");

alx@devuan:~/src/shadow/shadow/master$ grep -rn '\<secure_getenv *(' lib* src/
lib/defines.h:191:#  define shadow_getenv(name) secure_getenv(name)

alx@devuan:~/src/shadow/shadow/master$ grep -rn '\<shadow_getenv *(' lib* src/
lib/defines.h:191:#  define shadow_getenv(name) secure_getenv(name)
lib/defines.h:193:#  define shadow_getenv(name) getenv(name)
lib/gettime.c:35:	source_date_epoch = shadow_getenv ("SOURCE_DATE_EPOCH");

We have one use of secure_getenv(3), but all other places use plain getenv(3). I suspect we should replace all such calls by the secure version. Of course, I should make sure the replacements are correct (I should check we don't assume the calls succeed). I also need to check the cases where we really want the insecure variant (if any).

Cc: @hallyn , @fweimer-rh , @ikerexxe

[fweimer-rh]

Pretty sure the call in newgrp should remain as-is, to preserve expected functionality. The su case looks like a security protection:

src/su.c:961:   if (getenv ("IFS") != NULL) {   /* don't export user IFS ... */
src/su.c-962-           addenv ("IFS= \t\n", NULL);     /* ... instead, set a safe IFS */
src/su.c-963-   }

Same for login, but it's not SUID on my system, so it wouldn't matter.

This needs a case-by-case analysis if the use of the environment variable value actually crosses a trust binary.

[alejandro-colomar]

Thanks! I'll close for now. Maybe I'll research this some time in the future. For now, there are lower hanging fruits.