Bug 3508

Summary:

Simplify handling of SSH_CONNECTION in auth-pam

Product:

Portable OpenSSH

Reporter:

miazgapow

Component:

PAM support

Assignee:

Assigned to nobody <unassigned-bugs>

Status:

CLOSED FIXED

Severity:

enhancement

CC:

djm, dtucker

Priority:

Version:

9.1p1

Hardware:

Other

OS:

Linux

Bug Depends on:

Bug Blocks:

3480

Attachments:

Description	Flags
Do not store sshpam_conninfo across calls to sshpam_init	none
Do not store sshpam_conninfo across calls to sshpam_init	djm: ok+

Description miazgapow 2022-12-09 05:47:41 AEDT

In auth-pam.c, in sshpam_init, where the SSH_CONNECTION environment variable is being set, xasprintf is used. It allocates memory for the formatted string and returns it via the global sshpam_conninfo, which is later passed to pam_putenv, which copies the string. So memory under sshpam_conninfo is never freed, and then a reference to it is lost on another run of sshpam_init

Comment 1 Darren Tucker 2022-12-16 14:56:21 AEDT

I agree with your analysis.  Putting on list for 9.2.

Comment 2 Darren Tucker 2022-12-16 15:14:05 AEDT

Actually looking more closely I don't think it can leak.

sshpam_conninfo is allocated at the same time as sshpam_rhost, which is also a global.  On subsequent calls to sshpam_init, sshpam_rhost will already be set so sshpam_conninfo will not be re-populated, but it will get used again for pam_putenv.

Freeing sshpam_conninfo would mean the during subsequent calls to sshpam_init it would not be available (either NULL if it was nulled out, or a use-after-free if not).

Comment 3 Darren Tucker 2022-12-16 15:35:28 AEDT

Created attachment 3643 [details]
Do not store sshpam_conninfo across calls to sshpam_init

Those things said, the content of sshpam_conninfo is constructed of things that are guaranteed to be available (remote host and port, local port and address) so we don't actually need to cache it across calls at all and thus it doesn't need to be static or global.  This puts all of the conninfo bits in one place.

Comment 4 Darren Tucker 2022-12-16 19:51:01 AEDT

Created attachment 3644 [details]
Do not store sshpam_conninfo across calls to sshpam_init

Damien points out that sshpam_init does get called without the ssh context, so my first patch is not correct.

Comment 5 Darren Tucker 2022-12-19 19:24:59 AEDT

Thanks for the report, we've made the SSH_CONNECTION handling hopefully clearer.

Comment 6 Damien Miller 2023-03-17 13:42:26 AEDT

OpenSSH 9.3 has been released. Close resolved bugs