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

[Avinash Duduskar]

import_environments() carries a comment flagging the
pam_putenv() ownership contract as uncertain and leaks env
rather than free it. Per review feedback on v1, the contract
was verified on each PAM library OpenSSH supports where
source is publicly readable:

  Linux-PAM: libpam/pam_env.c calls _pam_strdup on entry;
             pam_end frees the internal copy via
             _pam_drop_env.
  OpenPAM:   lib/libpam/pam_putenv.c strdups namevalue at
             both the replace and append paths (lines 73,
             91); pam_end frees via openpam_destroy_pamh.
  illumos:   usr/src/lib/libpam/pam_framework.c
             pam_putenv (lines 1366-1478) splits the input
             into local name and value buffers, strdups
             both into the handle's env_list, and frees
             the locals before return. The input pointer
             is never retained.

XSSO is silent on ownership, so the contract is
implementation-defined. illumos is the 2010 open-source
fork of the Sun PAM codebase from which Oracle Solaris is
also derived; the public pam_putenv(3PAM) page does not
document ownership, but the fork-time inheritance and
Solaris's ABI stability make the illumos contract a
reasonable proxy. HP-UX 11 is flagged Sun-derived in
configure.ac (PAM_SUN_CODEBASE).

The strdup-on-entry contract is stable across all
auditable releases: Linux-PAM 0.72 (~2001), OpenPAM
20050616 (Jun 2005), and illumos 7c478bd (OpenSolaris
Launch, 14 Jun 2005). Pre-tag Linux-PAM and
pre-OpenSolaris-Launch Sun PAM are outside any plausible
current deployment window.

AIX PAM is proprietary; IBM's public pam_putenv(3) page
(ibm.com/docs/en/aix/7.3.0?topic=p-pam-putenv-subroutine)
describes the function's behaviour without committing on
the ownership semantics of name_value. Free the
per-iteration env allocation produced by
sshbuf_get_cstring() after the pam_putenv() call on every
platform except AIX, whose ownership semantics are
unconfirmed. Remove the XXX block on the verified path.

Verified empirically on Linux-PAM (Valgrind 3.25, OpenSSH
V_10_3_P1 in an Arch container, --with-pam --without-sandbox,
keyboard-interactive auth with 2 pam_env strings): unpatched
builds report 63 bytes / 2 blocks definitely lost rooted at
import_environments() on two independent runs; patched builds
show the leak eliminated with no other signature changes.
OpenPAM and illumos rest on source verification only.

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.

The per-session leak is bounded by sshd-session's
per-connection lifetime and not user-visible.

Suggested-by: Theo de Raadt <deraadt at openbsd.org>
Suggested-by: Simon Josefsson <simon at josefsson.org>
Suggested-by: Marco Trevisan <marco.trevisan at canonical.com>
Signed-off-by: Avinash Duduskar <avinash.duduskar at gmail.com>
---
 auth-pam.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

v2 -> v3: invert the conditional so AIX is the explicit
  "#if defined(_AIX)" case carrying the comment, with free(env)
  in the "#else"; commenting on the else of an explicit if read
  awkwardly. No behaviour change. (Marco Trevisan's review of v2.)
v1 -> v2: gate the free under the PAM implementation rather than
  free unconditionally, after auditing the pam_putenv() ownership
  contract on each open implementation; AIX left conservative.

diff --git a/auth-pam.c b/auth-pam.c
index 29607e041..df2f0e287 100644
--- a/auth-pam.c
+++ b/auth-pam.c
@@ -356,11 +356,14 @@ import_environments(struct sshbuf *b)
 			error("PAM: pam_putenv: %s",
 			    pam_strerror(sshpam_handle, r));
 		}
+#if defined(_AIX)
 		/*
-		 * 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.
+		 * AIX PAM: pam_putenv() ownership semantics not
+		 * publicly documented; conservative leak preserved.
 		 */
+#else
+		free(env);
+#endif
 	}
 #endif
 }

base-commit: 66847768ffd5a2a004891c8d3bd79eaba12625b7
-- 
2.54.0