openssh unix dev - Feb 2003 - Minor races in sftp-server.c

There are a couple of races in sftp-server as this patch shows:

--- sftp-server.c	28 Jan 2003 18:06:53 -0000	1.1.1.2
+++ sftp-server.c	5 Feb 2003 19:19:42 -0000
@@ -832,19 +832,22 @@
 process_rename(void)
 {
 	u_int32_t id;
-	struct stat st;
 	char *oldpath, *newpath;
-	int ret, status = SSH2_FX_FAILURE;
+	int status;
 
 	id = get_int();
 	oldpath = get_string(NULL);
 	newpath = get_string(NULL);
 	TRACE("rename id %u old %s new %s", id, oldpath, newpath);
 	/* fail if 'newpath' exists */
-	if (stat(newpath, &st) == -1) {
-		ret = rename(oldpath, newpath);
-		status = (ret == -1) ? errno_to_portable(errno) : SSH2_FX_OK;
-	}
+	if (link(oldpath, newpath) == -1)
+		status = errno_to_portable(errno);
+	else if (unlink(oldpath) == -1) {
+		status = errno_to_portable(errno);
+		/* clean spare link */
+		unlink(newpath);
+	} else
+		status = SSH2_FX_OK;
 	send_status(id, status);
 	xfree(oldpath);
 	xfree(newpath);
@@ -878,19 +881,16 @@
 process_symlink(void)
 {
 	u_int32_t id;
-	struct stat st;
 	char *oldpath, *newpath;
-	int ret, status = SSH2_FX_FAILURE;
+	int ret, status;
 
 	id = get_int();
 	oldpath = get_string(NULL);
 	newpath = get_string(NULL);
 	TRACE("symlink id %u old %s new %s", id, oldpath, newpath);
-	/* fail if 'newpath' exists */
-	if (stat(newpath, &st) == -1) {
-		ret = symlink(oldpath, newpath);
-		status = (ret == -1) ? errno_to_portable(errno) : SSH2_FX_OK;
-	}
+	/* this will fail if 'newpath' exists */
+	ret = symlink(oldpath, newpath);
+	status = (ret == -1) ? errno_to_portable(errno) : SSH2_FX_OK;
 	send_status(id, status);
 	xfree(oldpath);
 	xfree(newpath);


Tony.
-- 
f.a.n.finch  <dot at dotat.at>  http://dotat.at/
CAPE WRATH TO RATTRAY HEAD INCLUDING ORKNEY: WEST 4 BACKING SOUTH 5 TO 7
LOCALLY GALE 8, LATER VEERING WEST TO SOUTHWEST AND DECREASING 5 OR 6. RAIN
SPREADING FROM THE WEST, CLEARING LATER. GOOD DECREASING MODERATE IN RAIN.
SLIGHT TO MODERATE LOCALLY ROUGH.

On Wed, Feb 05, 2003 at 07:26:04PM +0000, Tony Finch
wrote:
> There are a couple of races in sftp-server as this patch shows:
i think we can just remove the calls to stat.

On Wed, Feb 05, 2003 at 10:35:01PM +0100, Markus Friedl
wrote:
> On Wed, Feb 05, 2003 at 07:26:04PM +0000, Tony Finch wrote:
> > There are a couple of races in sftp-server as this patch shows:
> 
> i think we can just remove the calls to stat.
No, that changes the semantics of the rename operation.
symlink(2) returns EEXIST if the second argument exists,
but rename(2) unlinks it silently. The current code tries
to prevent the user from renaming A to B when B exists.
This should be clear from the way my patch treats the
two cases differently, whereas they are treated the same
in the old code.

Tony.
-- 
f.a.n.finch  <dot at dotat.at>  http://dotat.at/
WIGHT PORTLAND PLYMOUTH: VARIABLE BECOMING SOUTHWESTERLY 3 OR 4, OCCASIONALLY
5. RAIN OR DRIZZLE. GOOD BECOMING MODERATE, OCCASIONALLY POOR.