sftp: don't crash when glob(3) results lack stat information - #707
Open
maycuatroi1 wants to merge 1 commit into
Open
sftp: don't crash when glob(3) results lack stat information#707maycuatroi1 wants to merge 1 commit into
maycuatroi1 wants to merge 1 commit into
Conversation
glob(3) run with GLOB_KEEPSTAT can leave gl_statv[i] NULL for result
entries that could not be stat'ed, e.g. for a GLOB_NOCHECK literal
appended for a non-matching brace expansion when the remote SFTP
server fails SSH2_FXP_STAT/LSTAT for it. The listing code already
handles this case ("no stat information"), but two other consumers
of the glob result did not:
- sglob_comp(), the comparator used by ls -t/-S sorting, dereferenced
gl_statv[a]/gl_statv[b] unconditionally
- collect_ids_from_glob(), which collects uids/gids for the
users-groups-by-id@openssh.com extension, dereferenced
gl_statv[i]->st_uid/st_gid unconditionally
A malicious or compromised SFTP server could crash the client this
way (NULL deref, read-only). Order entries without stat information
last in the comparator and skip them during id collection.
Reproduced on master @ 5280556 and
on Ubuntu 24.04 stock 9.6p1 client against a mock SFTP server that
returns SSH_FX_FAILURE for STAT/LSTAT of a non-matching brace
alternative; with this change both ls {*,zzz} (extension advertised)
and ls -t {*,zzz} complete normally.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The sftp(1) client crashes with a NULL pointer dereference when a glob(3) result contains entries whose
gl_statv[]stat pointer is NULL. Two consumers of the glob result indo_globbed_ls()dereference these entries without NULL checks, while the display loop in the same function already handles NULL correctly ("no stat information", sftp.c:1046).How NULL stat entries arise
do_globbed_ls()runs glob(3) withGLOB_MARK|GLOB_NOCHECK|GLOB_BRACE|GLOB_KEEPSTAT|GLOB_NOSORT(sftp.c:980). When one brace alternative matches real entries while another alternative matches nothing (e.g.ls {*,zzz}), the NOCHECK fallback appends the non-matching literal viaglobextend(..., NULL), which stores a NULLgl_statv[]entry for it (openbsd-compat/glob.c:563, 850-851). The stat callbacksfudge_stat/fudge_lstat(sftp-glob.c:110-133) fail whenever the SFTP server replies failure to SSH2_FXP_STAT/LSTAT, so a malicious or compromised server fully controls which entries carry NULL stat pointers. This is server-controlled data, no server bug needed.Confirmed sinks
collect_ids_from_glob()(sftp-usergroup.c:158) dereferencesgl_statv[i]->st_uid/st_gid. Reached from a plainls <glob>whenever the server advertisesusers-groups-by-id@openssh.com(feature since 9.1).sglob_comp()(sftp.c:950-956, 961) dereferencesgl_statv[a]/gl_statv[bforls -t/ls -Ssorting. No extension required; comparator present since at least 8.x.Reproduction (deterministic, minimal-impact)
Mock SFTP subsystem (Python, stdio protocol, fixed fake data, no filesystem access): READDIR returns
poc_a/poc_b/poc_c; STAT/LSTAT succeed for/dataand/data/poc_*, return SSH_FX_FAILURE for anything else (thezzzliteral). Loopback sshd, container build.ls {*,zzz}with extension advertised: SEGV read at 0x1c (struct stat .st_uidoffset), stackcollect_ids_from_glob<-parse_dispatch_commandsftp.c:1039ls -t {*,zzz}: SEGV read at 0x58 (.st_mtimoffset), stacksglob_compsftp.c:951 <- qsort <- sftp.c:1035ls -l {*,zzz}still prints "no stat information for zzz";-S/-ltrorder the no-stat entry deterministicallyImpact: remote client-side crash (read of a NULL-derived address; no write primitive). Severity low/medium DoS - but it is a memory-safety violation on server-controlled data in code that clearly intends to handle the NULL case elsewhere.
Code changes
sftp.csglob_comp(): order entries without stat information last (reverse-aware viarmul)sftp-usergroup.ccollect_ids_from_glob(): skip entries with NULL statReport credit: Binhna3