Bug 3508 - Simplify handling of SSH_CONNECTION in auth-pam
Summary: Simplify handling of SSH_CONNECTION in auth-pam
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: PAM support (show other bugs)
Version: 9.1p1
Hardware: Other Linux
: P5 enhancement
Assignee: Assigned to nobody
URL:
Keywords:
Depends on:
Blocks: V_9_2
  Show dependency treegraph
 
Reported: 2022-12-09 05:47 AEDT by miazgapow
Modified: 2023-03-17 13:42 AEDT (History)
2 users (show)

See Also:


Attachments
Do not store sshpam_conninfo across calls to sshpam_init (1.72 KB, patch)
2022-12-16 15:35 AEDT, Darren Tucker
no flags Details | Diff
Do not store sshpam_conninfo across calls to sshpam_init (1.72 KB, patch)
2022-12-16 19:51 AEDT, Darren Tucker
djm: ok+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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