Fix SSH crash when started with -C argument /usr/sbin/sshd -T -C user=root -C host=nd-e013 -C addr=127.0.0.1 -C lport=22 #21995

asha-behera · 2025-03-11T12:17:49Z

Fixes issue #21997

Why I did it

SSH was crashing due to the patch 0003-Export-remote-info-for-authorization.patch when ssh is started with -C argumnets .
Running debian packaged sshd with '-T -C user=root -C host=nd-e013 -C addr=127.0.0.1 -C lport=22' arguments doesn't cause crash.
ssh NULL pointer was being accessed hence crash occured .
We are calling export_remote_info before ssh is assigned using ssh_alloc_session_state API later in code

Recreation of the crash:
(gdb) file /usr/sbin/sshd
(gdb) run -T -C user=root -C host=nd-e013 -C addr=127.0.0.1 -C lport=22
Starting program: /usr/sbin/sshd -T -C user=root -C host=nd-e013 -C addr=127.0.0.1 -C lport=22
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Program received signal SIGSEGV, Segmentation fault.
ssh_remote_ipaddr (ssh=ssh@entry=0x0) at ../../packet.c:532
Download failed: Invalid argument. Continuing without source file ./debian/build-deb/../../packet.c.
532 ../../packet.c: No such file or directory.
(gdb) bt
#0 ssh_remote_ipaddr (ssh=ssh@entry=0x0) at ../../packet.c:532
#1 0x0000555555572d6e in export_remote_info (ssh=ssh@entry=0x0) at ../../auth.c:1111
#2 0x00005555555620e2 in main (ac=10, av=0x555555633960) at ../../sshd.c:1672...

How I did it

Add NULL check in export_remote_info

How to verify it

Start ssh with arguments and check no crash

admin# /usr/sbin/sshd -T -C user=root -C host=nd-e013 -C addr=127.0.0.1 -C lport=22
addressfamily any
listenaddress 0.0.0.0:22
listenaddress [::]:22
usepam yes
logingracetime 120
x11displayoffset 10
maxauthtries 6
maxsessions 10
clientaliveinterval 600
....

linux-foundation-easycla · 2025-03-11T12:17:53Z

The committers listed above are authorized under a signed CLA.

✅ login: asha-behera (3327e08)

mssonicbld · 2025-03-11T12:19:35Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2025-03-11T12:19:46Z

Azure Pipelines successfully started running 1 pipeline(s).

asha-behera · 2025-03-11T13:04:38Z

@liuh-80 Please check this PR . Related to your changes in 1f88a3e

asha-behera · 2025-03-11T13:18:48Z

src/openssh/patch/0004-ssh-crash-fix.patch

+ 			break;
+ 		case 'C':
+ 			/* Export remote IP address and port for authorization. */
+-			export_remote_info(ssh);


@liuh-80 : The call of export_remote_info(ssh) is not right as ssh is assigned after call to ssh_packet_set_connection at line 2169 in file sshd.c .
I want to remove this API call to export_remote_info as it is a NOOP as ssh pointer will always be NULL here

Please make sure all TACACS related test case does not break.
We need export_remote_info for TACACS per-command authorization.
I think you can keeps those export_remote_info invoke code no change, because the fix in export_remote_info already return when ssh is empty.

@liuh-80 But if ssh argument to export_remote_info will always be NULL, then the call to export_remote_info is of no use. What is the usecase to keep the call here ?

For remove export_remote_info at line 89, if the ssh always be NULL, I agree with you, this can be removed.

asha-behera · 2025-03-12T06:02:44Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2025-03-12T06:02:50Z

Commenter does not have sufficient privileges for PR 21995 in repo sonic-net/sonic-buildimage

asha-behera · 2025-03-12T06:10:29Z

/azpw run Azure.sonic-buildimage

mssonicbld · 2025-03-12T06:10:32Z

/AzurePipelines run Azure.sonic-buildimage

azure-pipelines · 2025-03-12T06:10:42Z

Azure Pipelines successfully started running 1 pipeline(s).

liuh-80 · 2025-03-12T07:30:30Z

src/openssh/patch/0004-ssh-crash-fix.patch

+-		export_remote_info(ssh);
+                if (export_remote_info(ssh) != 0)
+                {
+		  return -1;


return here will change sshd behavior, suggest don't change.

liuh-80 · 2025-03-12T07:31:28Z

src/openssh/patch/0004-ssh-crash-fix.patch

+-	export_remote_info(ssh);
+        if (export_remote_info(ssh) != 0)
+        {
+          exit(1);


Suggest don't exit here, this will change sshd behavior.

liuh-80 · 2025-03-12T07:32:19Z

src/openssh/patch/0004-ssh-crash-fix.patch

+
+ 	/* Check whether we have cached the ipaddr. */
+-	if (ssh->remote_ipaddr == NULL) {
+	if (ssh != NULL && ssh->remote_ipaddr == NULL) {


This part of code belong to sshd, suggest don't change it's behavior.

@liuh-80 : Do you see any issue with the check ? It is good to have check of ssh NULL before accessing its contents remote_ipaddr.

This part of code never crashes in official openssh, so I think ssh will never be NULL here, if we make the change, the risk is this patch may break with future version.

I also agree with you that the new code does not have issue. so just a suggestion.

liuh-80 · 2025-03-12T07:34:35Z

I suggest fix the issue in 0003-Export-remote-info-for-authorization.patch:

+void
+export_remote_info(struct ssh *ssh)
+{
+      if (ssh == NULL)
+      {
+        return;
+      }
+	const char *remote_ip = ssh_remote_ipaddr(ssh);
+	const int remote_port = ssh_remote_port(ssh);
+	const char remote_addr_port[32 + INET6_ADDRSTRLEN];
+	snprintf(remote_addr_port, sizeof(remote_addr_port), "%s %d", remote_ip, remote_port);
+	setenv("SSH_CLIENT_IPADDR_PORT", remote_addr_port, 1);
+}

asha-behera · 2025-03-12T09:23:46Z

src/openssh/patch/0004-ssh-crash-fix.patch

+int
+ export_remote_info(struct ssh *ssh)
+ {
+        if (ssh == NULL)


@liuh-80 If we do not intend to do anything with return status of export_remote_info as it will change sshd behavior ...I will chnage as below : Hope it is ok ?
/* Export remote IP address and port for authorization. */
void
export_remote_info(struct ssh *ssh)
{
if (ssh != NULL)
{
const char *remote_ip = ssh_remote_ipaddr(ssh);
const int remote_port = ssh_remote_port(ssh);
const char remote_addr_port[32 + INET6_ADDRSTRLEN];
snprintf(remote_addr_port, sizeof(remote_addr_port), "%s %d", remote_ip, remote_port);
setenv("SSH_CLIENT_IPADDR_PORT", remote_addr_port, 1);
}
}

The code is OK, just a suggestion for improve nested code:

void
export_remote_info(struct ssh *ssh)
{
if (ssh == NULL)
{
return;
}
const char *remote_ip = ssh_remote_ipaddr(ssh);
const int remote_port = ssh_remote_port(ssh);
const char remote_addr_port[32 + INET6_ADDRSTRLEN];
snprintf(remote_addr_port, sizeof(remote_addr_port), "%s %d", remote_ip, remote_port);
setenv("SSH_CLIENT_IPADDR_PORT", remote_addr_port, 1);
}

mssonicbld · 2025-03-13T08:43:46Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2025-03-13T08:43:59Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-03-13T08:50:25Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2025-03-13T08:50:37Z

Azure Pipelines successfully started running 1 pipeline(s).

…ser=root -C host=nd-e013 -C addr=127.0.0.1 -C lport=22 Signed-off-by: Asha Behera <[email protected]>

mssonicbld · 2025-03-13T09:42:33Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2025-03-13T09:42:49Z

Azure Pipelines successfully started running 1 pipeline(s).

asha-behera requested a review from lguohan as a code owner March 11, 2025 12:17

asha-behera commented Mar 11, 2025

View reviewed changes

asha-behera changed the title ~~SSH crash when started with specific arguments /usr/sbin/sshd -T -C user=root -C host=nd-e013 -C addr=127.0.0.1 -C lport=22~~ Fix SSH crash when started with specific arguments /usr/sbin/sshd -T -C user=root -C host=nd-e013 -C addr=127.0.0.1 -C lport=22 Mar 11, 2025

asha-behera changed the title ~~Fix SSH crash when started with specific arguments /usr/sbin/sshd -T -C user=root -C host=nd-e013 -C addr=127.0.0.1 -C lport=22~~ Fix SSH crash when started with -C argument /usr/sbin/sshd -T -C user=root -C host=nd-e013 -C addr=127.0.0.1 -C lport=22 Mar 11, 2025

liuh-80 reviewed Mar 12, 2025

View reviewed changes

asha-behera commented Mar 12, 2025

View reviewed changes

asha-behera force-pushed the ssh_crash branch from 70e41af to c14b50a Compare March 13, 2025 08:43

asha-behera force-pushed the ssh_crash branch from c14b50a to 35345b7 Compare March 13, 2025 08:49

SSH crash when started with specific arguments /usr/sbin/sshd -T -C u…

3327e08

…ser=root -C host=nd-e013 -C addr=127.0.0.1 -C lport=22 Signed-off-by: Asha Behera <[email protected]>

asha-behera force-pushed the ssh_crash branch from 35345b7 to 3327e08 Compare March 13, 2025 09:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SSH crash when started with -C argument /usr/sbin/sshd -T -C user=root -C host=nd-e013 -C addr=127.0.0.1 -C lport=22 #21995

Fix SSH crash when started with -C argument /usr/sbin/sshd -T -C user=root -C host=nd-e013 -C addr=127.0.0.1 -C lport=22 #21995

asha-behera commented Mar 11, 2025 •

edited

Loading

linux-foundation-easycla bot commented Mar 11, 2025 •

edited

Loading

mssonicbld commented Mar 11, 2025

azure-pipelines bot commented Mar 11, 2025

asha-behera commented Mar 11, 2025

asha-behera Mar 11, 2025

liuh-80 Mar 12, 2025

asha-behera Mar 12, 2025 •

edited

Loading

liuh-80 Mar 12, 2025

asha-behera commented Mar 12, 2025

azure-pipelines bot commented Mar 12, 2025

asha-behera commented Mar 12, 2025

mssonicbld commented Mar 12, 2025

azure-pipelines bot commented Mar 12, 2025

liuh-80 Mar 12, 2025

liuh-80 Mar 12, 2025

liuh-80 Mar 12, 2025

asha-behera Mar 12, 2025

liuh-80 Mar 12, 2025

liuh-80 commented Mar 12, 2025 •

edited

Loading

asha-behera Mar 12, 2025 •

edited

Loading

liuh-80 Mar 12, 2025

mssonicbld commented Mar 13, 2025

azure-pipelines bot commented Mar 13, 2025

mssonicbld commented Mar 13, 2025

azure-pipelines bot commented Mar 13, 2025

mssonicbld commented Mar 13, 2025

azure-pipelines bot commented Mar 13, 2025

Fix SSH crash when started with -C argument /usr/sbin/sshd -T -C user=root -C host=nd-e013 -C addr=127.0.0.1 -C lport=22 #21995

Are you sure you want to change the base?

Fix SSH crash when started with -C argument /usr/sbin/sshd -T -C user=root -C host=nd-e013 -C addr=127.0.0.1 -C lport=22 #21995

Conversation

asha-behera commented Mar 11, 2025 • edited Loading

Why I did it

How I did it

How to verify it

linux-foundation-easycla bot commented Mar 11, 2025 • edited Loading

mssonicbld commented Mar 11, 2025

azure-pipelines bot commented Mar 11, 2025

asha-behera commented Mar 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asha-behera Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asha-behera commented Mar 12, 2025

azure-pipelines bot commented Mar 12, 2025

asha-behera commented Mar 12, 2025

mssonicbld commented Mar 12, 2025

azure-pipelines bot commented Mar 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuh-80 commented Mar 12, 2025 • edited Loading

asha-behera Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mssonicbld commented Mar 13, 2025

azure-pipelines bot commented Mar 13, 2025

mssonicbld commented Mar 13, 2025

azure-pipelines bot commented Mar 13, 2025

mssonicbld commented Mar 13, 2025

azure-pipelines bot commented Mar 13, 2025

asha-behera commented Mar 11, 2025 •

edited

Loading

linux-foundation-easycla bot commented Mar 11, 2025 •

edited

Loading

asha-behera Mar 12, 2025 •

edited

Loading

liuh-80 commented Mar 12, 2025 •

edited

Loading

asha-behera Mar 12, 2025 •

edited

Loading