[PATCH] auth-pam: don't leak PAM env strings after pam_putenv()

[Avinash Duduskar]

pam_putenv(3) documents that it operates on a copy of name_value
and that the application retains ownership of the buffer:

  "pam_putenv() operates on a copy of name_value, which means in
   contrast to putenv(3), the application is responsible for
   freeing the data."

import_environments() carries a comment flagging this contract
as uncertain and chooses to leak env rather than free it. The
contract is unambiguous: libpam strdups name_value on entry
(libpam/pam_env.c, _pam_strdup) and frees the internal copy in
pam_end via _pam_drop_env. The caller's buffer is safe to free
immediately after pam_putenv returns, regardless of return
code.

Free the per-iteration env allocation produced by
sshbuf_get_cstring() after the pam_putenv() call and remove the
XXX block.

Verified under Valgrind 3.25 on V_10_3_P1 (2e6f9c6a4e59) built
with --with-pam --without-sandbox. PAM stack: pam_permit auth
+ pam_env exposing 2 env strings, exercised via
keyboard-interactive. Unpatched build reports on two
independent runs:

  63 bytes in 2 blocks are definitely lost
     at malloc
     by sshbuf_get_cstring (sshbuf-getput-basic.c:290)
     by import_environments (auth-pam.c:352)
     by sshpam_query (auth-pam.c:913)
     by mm_answer_pam_query (monitor.c:1205)
     by monitor_read (monitor.c:544)
     by monitor_child_preauth (monitor.c:299)
     by privsep_preauth (sshd-session.c:326)
     by main (sshd-session.c:1241)

Patched build: zero import_environments-rooted records;
definitely-lost delta exactly -63 bytes / -2 blocks in the
matched sshd-session log; other leak signatures (setproctitle,
monitor_read sshbuf_new) unchanged.

Scope: the leak fires on the sshpam_thread-based asynchronous
PAM auth path used by keyboard-interactive authentication and
ChallengeResponseAuthentication. Publickey authentication does
not engage sshpam_thread and so does not invoke
import_environments. Password authentication via
sshpam_auth_passwd is a separate synchronous path that also
bypasses this code. Deployments using publickey-only or
password-only auth are unaffected by both the pre-existing leak
and this fix.

Real-world PAM stacks push more env strings than the test
fixture, but the per-session leak is bounded by sshd-session's
per-connection lifetime and not user-visible. Hygiene cleanup
flowing from the contract clarification, not a fix for an
observed runtime issue.

Signed-off-by: Avinash Duduskar <avinash.duduskar at gmail.com>
---
 auth-pam.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/auth-pam.c b/auth-pam.c
index 29607e041..33a4d6c17 100644
--- a/auth-pam.c
+++ b/auth-pam.c
@@ -356,11 +356,7 @@ import_environments(struct sshbuf *b)
 			error("PAM: pam_putenv: %s",
 			    pam_strerror(sshpam_handle, r));
 		}
-		/*
-		 * XXX this possibly leaks env because it is not documented
-		 * what pam_putenv() does with it. Does it copy it? Does it
-		 * take ownership? We don't know, so it's safest just to leak.
-		 */
+		free(env);
 	}
 #endif
 }
-- 
2.54.0