Re: scp: now using SFTP protocol by default

[Date Prev] [Date Next] [Thread Prev] [Thread Next] [Date Index] [Thread Index]

 



On Thu, 9 Sep 2021, Damien Miller wrote:

> FYI: the next release will have scp using the SFTP protocol by
> default.
> 
> There are two known incompatibilities:
> 
> Use of the SFTP protocol avoids interpretation of remote paths by
> the shell. We consider this a feature, but it does change (simplify
> really) necessary quoting of shell characters.
> 
> Remote paths with a ~user/ prefix require a SFTP protocol extension
> that was included in OpenSSH 8.7's sftp-server.
> 
> The original scp/rcp protocol remains available via "scp -O ..."
> 
> If you're in a position to test snapshots/git prior to release
> (ETA October), then it would be appreciated.

FYI, I'm rolling this back for the release that will happen in a few
days. We want to give people a bit more time to pick up the sftp-server
"expand-path@xxxxxxxxxxx" extension to support ~user paths.

If you're an OpenSSH maintainer for an operating system distribution
consider either updating your stable OpenSSH to the 8.8 release when
it ships or backporting the "expand-path@xxxxxxxxxxx" extension to
your stable OpenSSH sftp-server. Attached are some patches to do this
for OpenSSH 8.2 and should be fairly easily adaptable to other
versions.

Removing this backwards-compatibility problem in popular distributions
will hasten the time when we can turn scp protocol off by default.

-d
From 8b85f54a2fdce9b749ea69ffe4f69afe559be591 Mon Sep 17 00:00:00 2001
From: Damien Miller <djm@xxxxxxxxxx>
Date: Mon, 20 Sep 2021 11:30:50 +1000
Subject: [PATCH 1/2] Cherrypick extension advertisment code (6653c6120)

---
 regress/sftp-perm.sh |  8 -----
 sftp-server.c        | 82 ++++++++++++++++++++++++++++----------------
 2 files changed, 53 insertions(+), 37 deletions(-)

diff --git a/regress/sftp-perm.sh b/regress/sftp-perm.sh
index 304ca0ac..798966d1 100644
--- a/regress/sftp-perm.sh
+++ b/regress/sftp-perm.sh
@@ -220,14 +220,6 @@ perm_test \
 	"test ! -d ${COPY}.dd" \
 	"test -d ${COPY}.dd"
 
-perm_test \
-	"posix-rename" \
-	"realpath,stat,lstat" \
-	"rename $COPY ${COPY}.1" \
-	"touch $COPY" \
-	"test -f ${COPY}.1 -a ! -f $COPY" \
-	"test -f $COPY -a ! -f ${COPY}.1"
-
 perm_test \
 	"rename" \
 	"realpath,stat,lstat" \
diff --git a/sftp-server.c b/sftp-server.c
index 359204fa..b7b54d7c 100644
--- a/sftp-server.c
+++ b/sftp-server.c
@@ -155,6 +155,18 @@ static const struct sftp_handler extended_handlers[] = {
 	{ NULL, NULL, 0, NULL, 0 }
 };
 
+static const struct sftp_handler *
+extended_handler_byname(const char *name)
+{
+	int i;
+
+	for (i = 0; extended_handlers[i].handler != NULL; i++) {
+		if (strcmp(name, extended_handlers[i].ext_name) == 0)
+			return &extended_handlers[i];
+	}
+	return NULL;
+}
+
 static int
 request_permitted(const struct sftp_handler *h)
 {
@@ -641,6 +653,29 @@ send_statvfs(u_int32_t id, struct statvfs *st)
 	sshbuf_free(msg);
 }
 
+/*
+ * Prepare SSH2_FXP_VERSION extension advertisement for a single extension.
+ * The extension is checked for permission prior to advertisment.
+ */
+static int
+compose_extension(struct sshbuf *msg, const char *name, const char *ver)
+{
+	int r;
+	const struct sftp_handler *exthnd;
+
+	if ((exthnd = extended_handler_byname(name)) == NULL)
+		fatal("%s: internal error: no handler for %s", __func__, name);
+	if (!request_permitted(exthnd)) {
+		debug2("%s: refusing to advertise disallowed extension %s",
+		    __func__, name);
+		return 0;
+	}
+	if ((r = sshbuf_put_cstring(msg, name)) != 0 ||
+	    (r = sshbuf_put_cstring(msg, ver)) != 0)
+		fatal("%s: buffer error: %s", __func__, ssh_err(r));
+	return 0;
+}
+
 /* parse incoming */
 
 static void
@@ -655,25 +690,17 @@ process_init(void)
 	if ((msg = sshbuf_new()) == NULL)
 		fatal("%s: sshbuf_new failed", __func__);
 	if ((r = sshbuf_put_u8(msg, SSH2_FXP_VERSION)) != 0 ||
-	    (r = sshbuf_put_u32(msg, SSH2_FILEXFER_VERSION)) != 0 ||
-	    /* POSIX rename extension */
-	    (r = sshbuf_put_cstring(msg, "posix-rename@xxxxxxxxxxx")) != 0 ||
-	    (r = sshbuf_put_cstring(msg, "1")) != 0 || /* version */
-	    /* statvfs extension */
-	    (r = sshbuf_put_cstring(msg, "statvfs@xxxxxxxxxxx")) != 0 ||
-	    (r = sshbuf_put_cstring(msg, "2")) != 0 || /* version */
-	    /* fstatvfs extension */
-	    (r = sshbuf_put_cstring(msg, "fstatvfs@xxxxxxxxxxx")) != 0 ||
-	    (r = sshbuf_put_cstring(msg, "2")) != 0 || /* version */
-	    /* hardlink extension */
-	    (r = sshbuf_put_cstring(msg, "hardlink@xxxxxxxxxxx")) != 0 ||
-	    (r = sshbuf_put_cstring(msg, "1")) != 0 || /* version */
-	    /* fsync extension */
-	    (r = sshbuf_put_cstring(msg, "fsync@xxxxxxxxxxx")) != 0 ||
-	    (r = sshbuf_put_cstring(msg, "1")) != 0 || /* version */
-	    (r = sshbuf_put_cstring(msg, "lsetstat@xxxxxxxxxxx")) != 0 ||
-	    (r = sshbuf_put_cstring(msg, "1")) != 0) /* version */
+	    (r = sshbuf_put_u32(msg, SSH2_FILEXFER_VERSION)) != 0)
 		fatal("%s: buffer error: %s", __func__, ssh_err(r));
+
+	 /* extension advertisments */
+	compose_extension(msg, "posix-rename@xxxxxxxxxxx", "1");
+	compose_extension(msg, "statvfs@xxxxxxxxxxx", "2");
+	compose_extension(msg, "fstatvfs@xxxxxxxxxxx", "2");
+	compose_extension(msg, "hardlink@xxxxxxxxxxx", "1");
+	compose_extension(msg, "fsync@xxxxxxxxxxx", "1");
+	compose_extension(msg, "lsetstat@xxxxxxxxxxx", "1");
+
 	send_msg(msg);
 	sshbuf_free(msg);
 }
@@ -1440,22 +1467,19 @@ static void
 process_extended(u_int32_t id)
 {
 	char *request;
-	int i, r;
+	int r;
+	const struct sftp_handler *exthand;
 
 	if ((r = sshbuf_get_cstring(iqueue, &request, NULL)) != 0)
 		fatal("%s: buffer error: %s", __func__, ssh_err(r));
-	for (i = 0; extended_handlers[i].handler != NULL; i++) {
-		if (strcmp(request, extended_handlers[i].ext_name) == 0) {
-			if (!request_permitted(&extended_handlers[i]))
-				send_status(id, SSH2_FX_PERMISSION_DENIED);
-			else
-				extended_handlers[i].handler(id);
-			break;
-		}
-	}
-	if (extended_handlers[i].handler == NULL) {
+	if ((exthand = extended_handler_byname(request)) == NULL) {
 		error("Unknown extended request \"%.100s\"", request);
 		send_status(id, SSH2_FX_OP_UNSUPPORTED);	/* MUST */
+	} else {
+		if (!request_permitted(exthand))
+			send_status(id, SSH2_FX_PERMISSION_DENIED);
+		else
+			exthand->handler(id);
 	}
 	free(request);
 }
-- 
2.33.0.464.g1972c5931b-goog

From 43dc7a3fa6d1661af675ddc8d8e2060eeb4cde9e Mon Sep 17 00:00:00 2001
From: Damien Miller <djm@xxxxxxxxxx>
Date: Mon, 20 Sep 2021 11:38:38 +1000
Subject: [PATCH 2/2] Cherrypick expand-path@xxxxxxxxxxx ext (2ab864010)

---
 misc.c        | 47 ++++++++++++++++++++++++++++-----------
 misc.h        |  1 +
 sftp-server.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+), 13 deletions(-)

diff --git a/misc.c b/misc.c
index 3a31d5c1..bcfed9a0 100644
--- a/misc.c
+++ b/misc.c
@@ -1010,29 +1010,37 @@ freeargs(arglist *args)
  * Expands tildes in the file name.  Returns data allocated by xmalloc.
  * Warning: this calls getpw*.
  */
-char *
-tilde_expand_filename(const char *filename, uid_t uid)
+int
+tilde_expand(const char *filename, uid_t uid, char **retp)
 {
 	const char *path, *sep;
 	char user[128], *ret;
 	struct passwd *pw;
 	u_int len, slash;
 
-	if (*filename != '~')
-		return (xstrdup(filename));
+	if (*filename != '~') {
+		*retp = xstrdup(filename);
+		return 0;
+	}
 	filename++;
 
 	path = strchr(filename, '/');
 	if (path != NULL && path > filename) {		/* ~user/path */
 		slash = path - filename;
-		if (slash > sizeof(user) - 1)
-			fatal("tilde_expand_filename: ~username too long");
+		if (slash > sizeof(user) - 1) {
+			error("%s ~username too long", __func__);
+			return -1;
+		}
 		memcpy(user, filename, slash);
 		user[slash] = '\0';
-		if ((pw = getpwnam(user)) == NULL)
-			fatal("tilde_expand_filename: No such user %s", user);
-	} else if ((pw = getpwuid(uid)) == NULL)	/* ~/path */
-		fatal("tilde_expand_filename: No such uid %ld", (long)uid);
+		if ((pw = getpwnam(user)) == NULL) {
+			error("%s: No such user %s", __func__, user);
+			return -1;
+		}
+	} else if ((pw = getpwuid(uid)) == NULL) {	/* ~/path */
+		error("%s: No such uid %ld", __func__, (long)uid);
+		return -1;
+	}
 
 	/* Make sure directory has a trailing '/' */
 	len = strlen(pw->pw_dir);
@@ -1045,10 +1053,23 @@ tilde_expand_filename(const char *filename, uid_t uid)
 	if (path != NULL)
 		filename = path + 1;
 
-	if (xasprintf(&ret, "%s%s%s", pw->pw_dir, sep, filename) >= PATH_MAX)
-		fatal("tilde_expand_filename: Path too long");
+	if (xasprintf(&ret, "%s%s%s", pw->pw_dir, sep, filename) >= PATH_MAX) {
+		error("%s: Path too long", __func__);
+		return -1;
+	}
 
-	return (ret);
+	*retp = ret;
+	return 0;
+}
+
+char *
+tilde_expand_filename(const char *filename, uid_t uid)
+{
+	char *ret;
+
+	if (tilde_expand(filename, uid, &ret) != 0)
+		cleanup_exit(255);
+	return ret;
 }
 
 /*
diff --git a/misc.h b/misc.h
index 4a05db2d..a090028c 100644
--- a/misc.h
+++ b/misc.h
@@ -66,6 +66,7 @@ int	 parse_user_host_path(const char *, char **, char **, char **);
 int	 parse_user_host_port(const char *, char **, char **, int *);
 int	 parse_uri(const char *, const char *, char **, char **, int *, char **);
 long	 convtime(const char *);
+int	 tilde_expand(const char *, uid_t, char **);
 char	*tilde_expand_filename(const char *, uid_t);
 char	*percent_expand(const char *, ...) __attribute__((__sentinel__));
 char	*tohex(const void *, size_t);
diff --git a/sftp-server.c b/sftp-server.c
index b7b54d7c..41bbe344 100644
--- a/sftp-server.c
+++ b/sftp-server.c
@@ -110,6 +110,7 @@ static void process_extended_fstatvfs(u_int32_t id);
 static void process_extended_hardlink(u_int32_t id);
 static void process_extended_fsync(u_int32_t id);
 static void process_extended_lsetstat(u_int32_t id);
+static void process_extended_expand(u_int32_t id);
 static void process_extended(u_int32_t id);
 
 struct sftp_handler {
@@ -152,6 +153,8 @@ static const struct sftp_handler extended_handlers[] = {
 	{ "hardlink", "hardlink@xxxxxxxxxxx", 0, process_extended_hardlink, 1 },
 	{ "fsync", "fsync@xxxxxxxxxxx", 0, process_extended_fsync, 1 },
 	{ "lsetstat", "lsetstat@xxxxxxxxxxx", 0, process_extended_lsetstat, 1 },
+	{ "expand-path", "expand-path@xxxxxxxxxxx", 0,
+	    process_extended_expand, 0 },
 	{ NULL, NULL, 0, NULL, 0 }
 };
 
@@ -700,6 +703,7 @@ process_init(void)
 	compose_extension(msg, "hardlink@xxxxxxxxxxx", "1");
 	compose_extension(msg, "fsync@xxxxxxxxxxx", "1");
 	compose_extension(msg, "lsetstat@xxxxxxxxxxx", "1");
+	compose_extension(msg, "expand-path@xxxxxxxxxxx", "1");
 
 	send_msg(msg);
 	sshbuf_free(msg);
@@ -1463,6 +1467,63 @@ process_extended_lsetstat(u_int32_t id)
 	free(name);
 }
 
+static void
+process_extended_expand(u_int32_t id)
+{
+	char cwd[PATH_MAX], resolvedname[PATH_MAX];
+	char *path, *npath;
+	int r;
+	Stat s;
+
+	if ((r = sshbuf_get_cstring(iqueue, &path, NULL)) != 0)
+		fatal("%s: parse message: %s", __func__, ssh_err(r));
+	if (getcwd(cwd, sizeof(cwd)) == NULL) {
+		send_status(id, errno_to_portable(errno));
+		goto out;
+	}
+
+	debug3("request %u: expand, original \"%s\"", id, path);
+	if (path[0] == '\0') {
+		/* empty path */
+		free(path);
+		path = xstrdup(".");
+	} else if (*path == '~') {
+		/* ~ expand path */
+		/* Special-case for "~" and "~/" to respect homedir flag */
+		if (strcmp(path, "~") == 0) {
+			free(path);
+			path = xstrdup(cwd);
+		} else if (strncmp(path, "~/", 2) == 0) {
+			npath = xstrdup(path + 2);
+			free(path);
+			xasprintf(&path, "%s/%s", cwd, npath);
+		} else {
+			/* ~user expansions */
+			if (tilde_expand(path, pw->pw_uid, &npath) != 0) {
+				send_status(id, errno_to_portable(EINVAL));
+				goto out;
+			}
+			free(path);
+			path = npath;
+		}
+	} else if (*path != '/') {
+		/* relative path */
+		xasprintf(&npath, "%s/%s", cwd, path);
+		free(path);
+		path = npath;
+	}
+	verbose("expand \"%s\"", path);
+	if (sftp_realpath(path, resolvedname) == NULL) {
+		send_status(id, errno_to_portable(errno));
+		goto out;
+	}
+	attrib_clear(&s.attrib);
+	s.name = s.long_name = resolvedname;
+	send_names(id, 1, &s);
+ out:
+	free(path);
+}
+
 static void
 process_extended(u_int32_t id)
 {
-- 
2.33.0.464.g1972c5931b-goog

_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@xxxxxxxxxxx
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev

[Date Prev] [Date Next] [Thread Prev] [Thread Next] [Date Index] [Thread Index]

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux