Bug 2773 - Potential segfault from ssh_remote_ipaddr()
Summary: Potential segfault from ssh_remote_ipaddr()
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: sshd (show other bugs)
Version: 7.5p1
Hardware: Other Linux
: P5 enhancement
Assignee: Assigned to nobody
URL:
Keywords:
Depends on:
Blocks: V_7_8
  Show dependency treegraph
 
Reported: 2017-09-04 21:00 AEST by Jakub Jelen
Modified: 2021-04-23 15:10 AEST (History)
1 user (show)

See Also:


Attachments
do not dereference NULL pointer before checking its validity (572 bytes, text/plain)
2017-09-04 21:00 AEST, Jakub Jelen
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jelen 2017-09-04 21:00:29 AEST
Created attachment 3050 [details]
do not dereference NULL pointer before checking its validity

The function ssh_remote_ipaddr() is dereferencing ssh->state pointer before it is checking it is not NULL.

This function is probably not called with closed connections in upstream OpenSSH, but we are attaching audit messages explaining reasons for failures and they can be used after the connection got closed so we can see segfaults in some occasions.

For more information and debugging, see the RH bugzilla:
https://bugzilla.redhat.com/show_bug.cgi?id=1488083
Comment 1 Damien Miller 2017-09-05 10:13:47 AEST
None of the packet layer functions work after the connection has been freed.

I think the main problem here is that sshd is calling audit_event(SSH_CONNECTION_ABANDON) and you seem to have patched audit-linux.c to try to handle that case. You should handle it separately, e.g. calling it early or skipping the ssh_remote_ipaddr() call.
Comment 2 Jakub Jelen 2017-09-05 19:05:08 AEST
Yes, it is a patched audit code, but I don't see a reason why the packet layer could not return the cached values even after the connection is cleaned up (this was one of the use cases why this data is cached in the first place).

If it is a design decision, that it should not work, the code should certainly not dereference NULL pointers before checking it and should not segfault instead of gracefully failing (or falling back to UNKNOWN) in this case, even though you are sure it can not be called from your code without valid state.

We probably need the IP address for auditing reasons and I don't see a simple way to call that earlier.
Comment 3 Damien Miller 2018-06-01 14:05:49 AEST
similar fix applied; this will be in the 7.8 release
Comment 4 Damien Miller 2021-04-23 15:10:02 AEST
closing resolved bugs as of 8.6p1 release