user/3169: Race fix for sftp-server rename breaks dir and symlink renames

From: <srp(at)srparish.net>
Date: Tue Mar 25 2003 - 15:40:07 EST

>Number: 3169
>Category: user
>Synopsis: Race fix for sftp-server rename breaks dir and symlink renames
>Confidential: yes

	System      : OpenBSD 3.3
	Architecture: OpenBSD.i386
	Machine     : i386

>Description:

The -current sftp-server includes a patch to fix a race condition in rename and symlink. This patch uses link(2) to prevent the race, but this causes renames for directories and symlinks to fail.

>How-To-Repeat:

$ cd /tmp
$ mkdir foo
$ sftp 127.0.0.1

sftp> cd /tmp
sftp> rename foo bar
Couldn't rename file "/tmp/bar" to "/tmp/foo": Permission Denied

>Fix:

The following patch fixes this problem, although it leave a few race conditions for cases when:

	+ source is dir and raced target is an empty dir
	+ source is symlink and raced target is a file


diff -ur ssh-3.6/sftp-server.c ssh-patch/sftp-server.c
--- ssh-3.6/sftp-server.c	Sun Mar 16 18:15:49 2003
+++ ssh-patch/sftp-server.c	Tue Mar 25 20:23:11 2003

@@ -814,22 +814,32 @@
 process_rename(void)
 {

 	u_int32_t id;
+	struct stat ops, nps;
 	char *oldpath, *newpath;
-	int status;
+	int ret, 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 (link(oldpath, newpath) == -1)
+	if (lstat(oldpath, &ops))
 		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;
+	/* avoid race for regular files */
+	else if (S_ISREG(ops.st_mode)) {
+		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;
+	}
+	else if (stat(newpath, &nps) == -1) {
+		ret = rename(oldpath, newpath);
+		status = (ret == -1) ? errno_to_portable(errno) : SSH2_FX_OK;
+	}
 	send_status(id, status);
 	xfree(oldpath);
 	xfree(newpath);

>Release-Note:
Received on Tue Mar 25 15:54:02 2003