[Bug 2966] New: scp rev 1.202 fix doesn't quite hit the mark

[bugzilla-daemon at bugzilla.mindrot.org]

https://bugzilla.mindrot.org/show_bug.cgi?id=2966

            Bug ID: 2966
           Summary: scp rev 1.202 fix doesn't quite hit the mark
           Product: Portable OpenSSH
           Version: 7.9p1
          Hardware: Other
                OS: All
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: scp
          Assignee: unassigned-bugs at mindrot.org
          Reporter: norman at teach.cs.toronto.edu

Revision 1.202 to ssh/scp.c means to protect against a rogue
server that writes to unexpected file names, by checking that
the file name returned matches the original glob pattern.

The fix has two bugs:

1.  If the requested filename contains no / characters, e.g.

scp remote:'[xyz]*' .

or even

scp remote:safefile .

no check is done; the remote is permitted to send any file name
(hence overwrite any file) it likes.  The trouble is that the
new code does

        if ((restrict_pattern = strrchr(src_copy, '/')) != NULL) {
                *restrict_pattern++ = '\0';
        }

then, later,

        if (restrict_pattern != NULL &&
           fnmatch(restrict_pattern, cp, 0) != 0)
                SCREWUP("filename does not match request");

If there is no /, strrchr returns NULL, restrict_pattern is set
to NULL, and fnmatch is not called.

One simple fix might be:

        if ((restrict_pattern = strrchr(src_copy, '/')) != NULL) {
                *restrict_pattern++ = '\0';
        } else
                restrict_pattern = src_copy;

2.  Rather more subtly: before the fix, one could copy several
files while connecting and authenticating only once by doing

scp remote:'dir1/file1 dir2/file2 dir3/file3' .

or even

scp remote:'dir1/*.c dir2/*.[ch] dir3/*.o' .

Since the fix doesn't account for white space, it would
allow only file3 to be copied in the first case, and only
file names matching *.o (regardless of which directory they
came from) in the second.

One can quibble about whether multiple file names should
have been allowed like that in the first place, but it has
worked for a long time and some* have written scripts that
expect it.  More to the point, it makes the current fix
do the test wrong.

Fixing it to preserve the old behaviour seems worthwhile,
but adds complexity (I guess you'd need to allocate a
table to hold all the patterns, or just split remote
names into white-space-separated pieces at a much-higher
level in the code).  I can see the argument for not doing
that, or at least deferring it, since an insistent user
can use -T as a workaround.

But if the code isn't that white-space-aware, it should
at least check for and reject remote names containing
white space, since white space breaks the safety check.

Thanks,

Norman Wilson
Computer Science, University of Toronto

-- 
You are receiving this mail because:
You are watching the assignee of the bug.