openssh unix dev - Feb 2022 - SCP in SFTP mode

Dear Damien,

Here is a proof-of-concept patch fixing this issue for me. If it could be
polished, it would be great.

Many thanks in advance!

On Wed, Feb 2, 2022 at 10:57 PM Damien Miller <djm at mindrot.org> wrote:
> I haven't had time to investigate this yet
>
> On Wed, 2 Feb 2022, Dmitry Belyavskiy wrote:
>
> > Dear Damien,
> > Sorry for being repetitive - but should we consider it to be a known
> > limitation?
> >
> > Many thanks in advance!
> >
> > On Fri, Jan 28, 2022 at 10:53 AM Dmitry Belyavskiy <dbelyavs at
redhat.com>
> > wrote:
> >       Dear Damien,
> >
> > On Fri, Jan 28, 2022 at 1:40 AM Damien Miller <djm at
mindrot.org> wrote:
> >       On Thu, 27 Jan 2022, Dmitry Belyavskiy wrote:
> >
> >       > Hello,
> >       >
> >       > When we use scp in sftp mode to copy folder to the
> >       machines with the "old"
> >       > version of ssh, we get the following error:
> >       > =====> >       > $ scp -vvv -r test/ touser at
machine:/home/touser
> >
> >       What version of OpenSSH are you using? There are a bunch
> >       of fixes in git
> >       HEAD after 8.8.
> >
> >       > If I understand correctly, the failure is caused by the
> >       lack of the "
> >       > expand-path at openssh.com"
> >       > extension to sftp protocol in old releases and (lack of)
> >       processing it on
> >       > the server side.
> >
> >       No, that extension is only invoked when a ~-prefixed path
> >       is used.
> >
> >       AFAIK this was juat a behaviour difference: scp in sftp
> >       mode wasn't
> >       creating the destination directory if it didn't exist,
> >       whereas legacy
> >       scp would do so. It was fixed in the commit you identify
> >       below.
> >
> >       > For the recent version self-compatibility it was fixed
> >       in
> >       >
> https://github.com/openssh/openssh-portable/commit/ac7c9ec894ed0825d04ef69c
> >       55babb49bab1d32e
> >
> >       -d
> >
> > I've just retested with the recent master
> > $ ./scp -S `pwd`/ssh -vvv -r -s `pwd`/openbsd-compat/ user at machine:
> >
> > debug2: subsystem request accepted on channel 0
> > ./scp: debug2: Remote version: 3
> > ./scp: debug2: Server supports extension "posix-rename at
openssh.com"
> > revision 1
> > ./scp: debug2: Server supports extension "statvfs at
openssh.com"
> > revision 2
> > ./scp: debug2: Server supports extension "fstatvfs at
openssh.com"
> > revision 2
> > ./scp: debug2: Server supports extension "hardlink at
openssh.com"
> > revision 1
> > ./scp: debug2: Server supports extension "fsync at
openssh.com" revision
> > 1
> > ./scp: debug2: Server supports extension "lsetstat at
openssh.com"
> > revision 1
> > ./scp: debug2: Sending SSH2_FXP_STAT "/root"
> > ./scp: debug3: Sent message fd 6 T:17 I:1
> > ./scp: debug3: Received stat reply T:105 I:1 F:0x000f M:40550
> > ./scp: debug3: source_sftp: copying local
> > /home/dbelyavs/work/upstream/openssh-portable/openbsd-compat to remote
> > /root/openbsd-compat
> > ./scp: debug2: Sending SSH2_FXP_REALPATH
"/root/openbsd-compat"
> > ./scp: debug3: Sent message fd 6 T:16 I:2
> > ./scp: realpath /root/openbsd-compat: No such file
> > ./scp: upload "/root/openbsd-compat": path canonicalization
failed
> > ./scp: failed to upload directory
> > /home/dbelyavs/work/upstream/openssh-portable/openbsd-compat to
> > /root/openbsd-compat
> >
> > The failure occurs even when I specify absolute paths on the
> > source/destination machine.
> >
> > The destination machine has OpenSSH_8.0p1 with patches.
> >
> > --
> > Dmitry Belyavskiy
> >
> >
> >
> > --
> > Dmitry Belyavskiy
> >
> >


-- 
Dmitry Belyavskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix_sftp_upload.diff
Type: text/x-patch
Size: 1649 bytes
Desc: not available
URL:
<http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20220203/37fd0ff5/attachment.bin>

On February 3, 2022 3:03:25 PM GMT+03:00, Dmitry Belyavskiy <dbelyavs at
redhat.com> wrote:
>Dear Damien,
>
>Here is a proof-of-concept patch fixing this issue for me. If it could be
>polished, it would be great.
>
Didn't you forget to free both error message string and ssh buffer in the
error path after do_mkdir?

Sincerely Yours, Dan.

On Thu, 3 Feb 2022, Dmitry Belyavskiy wrote:
> Dear Damien,
> 
> Here is a proof-of-concept patch fixing this issue for me. If it could be
> polished, it would be great.
> diff --git a/sftp-client.c b/sftp-client.c
> index c7565755..98f986d6 100644
> --- a/sftp-client.c
> +++ b/sftp-client.c
> @@ -1028,11 +1028,42 @@ do_realpath_expand(struct sftp_conn *conn, const
char *path, int expand)
...
> +		if (status != SSH2_FX_NO_SUCH_FILE) {
> +			error("%s %s: %s", expand ? "expand" :
"realpath",
> +			    path, *errmsg == '\0' ? fx2txt(status) : errmsg);
> +			free(errmsg);
> +			sshbuf_free(msg);
> +			return NULL;
> +		} else {
> +			if((r = do_mkdir(conn, path, &a, 0)) != 0) {
> +				return NULL;
> +			}
Sorry, but no way. This will attempt a directory creation any time a
realpath operations fails. Any "cd" in the sftp client will have the
chance to create a directory. scp without -r could create directories,
etc.

This server-side bug is difficult to fix in the client. Basically
you'd need to reimplement realpath() on the client side using remote
stat/readlink operations. It's possible, but not trivial, and a fair bit
of compat code for us to carry around in perpetuity.

I strongly recommend getting your employer to backport the existing
sftp-server fixes (569b650f93b and 53a6ebf1445).

-d