openssh unix dev - Feb 2019 - [Bug 2966] New: scp rev 1.202 fix doesn't quite hit the mark

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.

Some filesystems have files with whitespace in the names which may
add an additional wrinkle to pattern matching fixes.

	-- Mark

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org

--- Comment #1 from Damien Miller <djm at mindrot.org> ---
Thanks - the changes in rev 1.202 weren't complete, e.g. they don't
include support for {foo,bar} alternations. I've committed a fix that
also corrected the paths-without-slash problem along the way.

I'll take a look at the space-separated path thing; it might be
difficult to discern a space-separated source path that refers to
multiple files from one that refers to a single file with spaces in its
name. It really depends on the server's rules, and the client has no
way of knowing these.

In practice, /bin/sh is likely to be doing the dequoting/expansion,
except when it isn't.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

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

Jordan <jborean93 at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jborean93 at gmail.com

--- Comment #2 from Jordan <jborean93 at gmail.com> ---
Just to add a bit more to this problem, I've found that using single
quotes for the path component also causes it to fail with 'filename
does not match request'. For example If I was to do;

    scp -o User=username "[host]:'/tmp/file.txt'"

or even

    scp -o User=username
'[host]:'"'"'/tmp/file.txt'"'"''

It will fail with

    protocol error: filename does not match request

This worked perfectly fine before this patch and I don't see a reason
why it shouldn't continue to work going forward. Quotes aren't used all
the time but if the path we are trying to fetch has a space it is
needed. We also use single quotes so we don't have to escape any
metachars, e.g.

    scp -o User=username '[host]:'"'"'/tmp/file
$TERM.txt'"'"''

Would fetch the literal path '/tmp/file $TERM.txt' and not expand
$TERM. An alternate way would be to escape paths and meta chars with
'\' but that seems to be dependent on the shell used on the remote and
potentially fraught with danger. Being able to just enclose the path
with a single quote is a lot simpler.

https://github.com/ansible/ansible/issues/52640 has some more issues
and how we came across this problem.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|scp rev 1.202 fix doesn't   |scp client-side filename
                   |quite hit the mark          |matching problems

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
I honestly don't know how far we're willing to chase this down in scp -
these are all extremely fiddly and easy to get wrong in the client.

To do it right means basically having to re-implement the entirety of
shell quote handling in scp. Even then, it all goes out the window if
the server happens to be using a different shell. 

In any case, it's too late to get fixes for this into OpenSSH 8.0. I
suggest adjusting your workflows to use sftp or otherwise using the new
scp -T flag to disable the extra client-side checking.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

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

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
I honestly don't know how far we're willing to chase this down in scp -
these are all extremely fiddly and easy to get wrong in the client.

To do it right means basically having to re-implement the entirety of
shell quote handling in scp. Even then, it all goes out the window if
the server happens to be using a different shell. 

In any case, it's too late to get fixes for this into OpenSSH 8.0. I
suggest adjusting your workflows to use sftp or otherwise using the new
scp -T flag to disable the extra client-side checking.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

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

Josef Cejka <jcejka at suse.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jcejka at suse.com

--- Comment #5 from Josef Cejka <jcejka at suse.com> ---
Created attachment 3331
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3331&action=edit
0001-scp-show-filename-match-patterns-in-verbose-mode.patch

With current partial implementation of expansion isn't easy to check
why the expression was rejected and the error message isn't much
helpful.

Would be acceptable to print out on failure the received filename and
expected patterns? The attached patch prints that in verbose mode.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #6 from Damien Miller <djm at mindrot.org> ---
I just committed something similar:
https://github.com/openssh/openssh-portable/commit/c97520d23d1fe53d30725a2af25d2dddd6f2faff

Since this bug was opened, we also switched the default protocol for
scp from the old rcp protocol to SFTP, which performs all glob
expansion on the client and so doesn't suffer from these problems.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

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

Senku <197r1a0546 at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |197r1a0546 at gmail.com
         Resolution|FIXED                       |---
             Status|RESOLVED                    |REOPENED

--- Comment #7 from Senku <197r1a0546 at gmail.com> ---
I'm trying to fetch files from a remote VM to the agent machine.
The file path is "C:\\scripts\\template_windows.log"
It has no spaces and non ASCII characters, still I'm getting the error
"protocol error: filename does not match request"

any reason for this error? and any solutions
Thank you

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

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

--- Comment #8 from Damien Miller <djm at mindrot.org> ---
Is your client Windows or Unix?

Could you attach a debug trace? If you're able to compile and use
openssh git HEAD then there are were some extra diagnostics added in
https://github.com/openssh/openssh-portable/commits/c97520d23d1fe53d30725a2af25d2dddd6f2faff
that are likely to help figure out what is going wrong.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

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

--- Comment #9 from Senku <197r1a0546 at gmail.com> ---
Created attachment 3757
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3757&action=edit
Error Trace

Error Trace

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

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

--- Comment #10 from Senku <197r1a0546 at gmail.com> ---
It's a Windows client
I have attached the error trace.
The file I'm trying to fetch is "template_nt64.log", for copying
from
the TeamCity Agent machine to the remote VM I'm not getting any error
but for fetching, I'm getting the protocol error
I have also added export ANSIBLE_SCP_EXTRA_ARGS="-T" to the build
steps.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

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

--- Comment #11 from Damien Miller <djm at mindrot.org> ---
Sorry, that trace isn't any use because 1) it isn't from scp and 2)
it's not from scp in verbose mode (scp -vvv)

If you can attach a debug trace from scp that shows the problem
compiled from either github HEAD or from openssh-9.6 (which will be
released soon) then we can diagnose this further.

Alternately, if you're using Microsoft's version of OpenSSH on the
client then you should contact them for support.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.