public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/1] Fix permission checking by NFS client for open-create with mode 000
@ 2014-07-09 21:54 Frank S. Filz
  2014-07-09 22:17 ` Trond Myklebust
  0 siblings, 1 reply; 13+ messages in thread
From: Frank S. Filz @ 2014-07-09 21:54 UTC (permalink / raw)
  To: linux-nfs, linux-kernel; +Cc: Trond Myklebust, Frank S. Filz

From: "Frank S. Filz" <ffilzlnx@mindspring.com>

The NFS v4 client sends a COMPOUND with an OPEN and an ACCESS.

The ACCESS is required to verify an open for read is actually
allowed because RFC 3530 indicates OPEN for read only must succeed
for an execute only file.

The old code expected to have read access if the requested access
was O_RDWR.

We can expect the OPEN to properly permission check as long as
the open is O_WRONLY or O_RDWR.

Signed-off-by: Frank S. Filz <ffilzlnx@mindspring.com>
---
 fs/nfs/nfs4proc.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 4bf3d97..9742054 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1966,15 +1966,30 @@ static int nfs4_opendata_access(struct rpc_cred *cred,
 		return 0;
 
 	mask = 0;
-	/* don't check MAY_WRITE - a newly created file may not have
-	 * write mode bits, but POSIX allows the creating process to write.
-	 * use openflags to check for exec, because fmode won't
-	 * always have FMODE_EXEC set when file open for exec. */
+	/* Don't trust the permission check on OPEN if open for exec or for
+	 * read only. Since FMODE_EXEC doesn't go across the wire, the server
+	 * has no way to distinguish between an open to read an executable file
+	 * and an open to read a readable file. Write access is properly checked
+	 * and permission SHOULD always be granted if the file was created as a
+	 * result of this OPEN, no matter what mode the file was created with.
+	 *
+	 * NOTE: If the case of a OPEN CREATE READ-ONLY with a mode that does
+	 *       not allow read access, this test will produce an incorrect
+	 *       EACCES error.
+	 */
 	if (openflags & __FMODE_EXEC) {
 		/* ONLY check for exec rights */
 		mask = MAY_EXEC;
-	} else if (fmode & FMODE_READ)
+	} else if (!(fmode & FMODE_WRITE)) {
+		/* In case the file was execute only, check for read permission
+		 * ONLY if write access was not requested. It is expected that
+		 * an OPEN for write will fail if the file is execute only.
+		 * Note that if the file was newly created, the fmode SHOULD
+		 * include FMODE_WRITE, especially if the file will be created
+		 * with a restrictive mode.
+		 */
 		mask = MAY_READ;
+	}
 
 	cache.cred = cred;
 	cache.jiffies = jiffies;
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] Fix permission checking by NFS client for open-create with mode 000
@ 2014-07-09 21:55 Frank S. Filz
  0 siblings, 0 replies; 13+ messages in thread
From: Frank S. Filz @ 2014-07-09 21:55 UTC (permalink / raw)
  To: linux-nfs, linux-kernel; +Cc: Trond Myklebust, Frank S. Filz

From: "Frank S. Filz" <ffilzlnx@mindspring.com>

I used the following program to test the patch:

It does report 4 failures, as expected, when the file does not already exist:

	open("foo", O_CREAT | O_TRUNC | O_RDONLY, 000);
	open("foo", O_CREAT | O_TRUNC | O_RDONLY, 111);
	open("foo", O_CREAT | O_TRUNC | O_RDONLY, 222);
	open("foo", O_CREAT | O_TRUNC | O_RDONLY, 333);

These will not fail on a local filesystem.

---
#include <stdio.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <errno.h>
#include <strings.h>
#include <string.h>

static inline int expect(int required, int actual)
{
	return (actual & required) == required;
}

void check_one(char *name, int flags, int required, int mode)
{
	int fd, rc, access = flags & O_ACCMODE;
	char flags_s[1024], open_s[1024];
	int expected = expect(required, mode);

	sprintf(flags_s, "%s%s%s",
		flags & O_CREAT ? "O_CREAT | " : "",
		flags & O_TRUNC ? "O_TRUNC | " : "",
		access == O_RDONLY ? "O_RDONLY" :
		access == O_WRONLY ? "O_WRONLY" :
		access == O_RDWR ? "O_RDWR" :
		"UNKNOWN");

	if (flags & O_CREAT) {
		strcat(flags_s, ",");
		sprintf(open_s, "EXISTS: open(%s, %-29s %03o)",
			name, flags_s, mode);
		fd = open(name, flags, mode);
	} else {
		sprintf(open_s, "EXISTS: open(%s, %-29s)    ",
			name, flags_s);
		fd = open(name, flags);
	}

	if (fd == -1) {
		rc = errno;

		if (expected || (rc != EACCES))
			printf("FAIL - %s - Error %s (%d)\n",
			       open_s,
			       strerror(rc), rc);
		else
			printf("PASS - %s - Expected Error %s (%d)\n",
			       open_s,
			       strerror(rc), rc);

		return;
	}

	if (!expected) {
		printf("FAIL - %s - Unexpected Success\n",
		       open_s);
	} else {
		printf("PASS - %s - Success\n",
		       open_s);
	}

	rc = close(fd);

	if (rc == -1) {
		printf("Error %s (%d) closing %s\n",
		       strerror(errno), errno, name);
		return;
	}
}


int check_create(char *name, int access, int mode)
{
	int fd, rc, len, flags = access | O_CREAT | O_TRUNC;
	char flags_s[1024], open_s[1024];
	char buffer_w[1024], buffer_r[1024];

	sprintf(buffer_w,"File: %s\n", name);

	/* Make sure file is unlinked */
	rc = unlink(name);
	if (rc == -1) {
		rc = errno;
		if (rc != ENOENT) {
			printf("FAIL - Error %s (%d) unlinking %s\n",
			       strerror(errno), errno, name);
			return 0;
		}
	}

	sprintf(flags_s, "O_CREAT | O_TRUNC | %s,",
		access == O_RDONLY ? "O_RDONLY" :
		access == O_WRONLY ? "O_WRONLY" :
		access == O_RDWR ? "O_RDWR" :
		"UNKNOWN");

	/* Now do the open and create */
	sprintf(open_s, "CREATE: open(%s, %-29s %03o)",
		name, flags_s, mode);
	fd = open(name, flags, mode);

	if (fd == -1) {
		printf("FAIL - %s - Error %s (%d)\n",
		       open_s,
		       strerror(errno), errno);
		return 0;
	}

	if (access == O_RDONLY)
		goto close_it;

	len = strlen(buffer_w);

	rc = write(fd, buffer_w, len);

	if (rc == -1) {
		printf("FAIL - Error %s (%d) writing %s\n",
		       strerror(errno), errno, name);
		return 0;
	}

	if (access == O_WRONLY)
		goto close_it;

	rc = lseek(fd, 0, SEEK_SET);

	if (rc == -1) {
		printf("FAIL - Error %s (%d) seeking %s\n",
		       strerror(errno), errno, name);
		return 0;
	}

	rc = read(fd, buffer_r, len);

	if (rc == -1) {
		printf("FAIL - Error %s (%d) reading %s\n",
		       strerror(errno), errno, name);
		return 0;
	}

	buffer_r[rc] = '\0';

	if (rc != len) {
		printf("FAIL - Mismatch only read %d bytes from %s = %s\n",
		       rc, name, buffer_r);
	}

close_it:

	rc = close(fd);

	if (rc == -1) {
		printf("FAIL - Error %s (%d) closing %s\n",
		       strerror(errno), errno, name);
		return 0;
	}

	printf("PASS - %s - Success\n",
	       open_s);

	return 1;
}

void test_one(char *base, int mode)
{
	char name[1024];
	int rc;

	sprintf(name, "%s.%03o", base, mode);

	(void) check_create(name, O_RDONLY, mode);
	(void) check_create(name, O_WRONLY, mode);
	rc =   check_create(name, O_RDWR, mode);

	if (rc == 0) {
		printf("FAIL - can't continue with mode %03o\n");
		return;
	}

	/* Now try to open the file in various modes */
	check_one(name, O_RDONLY, S_IRUSR, mode);
	check_one(name, O_WRONLY, S_IWUSR, mode);
	check_one(name, O_RDWR, S_IRUSR | S_IWUSR, mode);
	check_one(name, O_CREAT | O_WRONLY, S_IWUSR, mode);
	check_one(name, O_CREAT | O_RDONLY, S_IRUSR, mode);
	check_one(name, O_CREAT | O_RDWR, S_IRUSR | S_IWUSR, mode);
}

int main(int argc, char **argv)
{
	int i;
	char *name;
	int mode;

	if (argc > 1) {
		name = argv[1];
	} else {
		printf("usage %s {file-name}\n",
		       argv[0]);
		return 1;
	}

	for (i = 0; i <= 7; i++) {
		mode = i * S_IXUSR + i * S_IXGRP + i * S_IXOTH;
		test_one(name, mode);
	}
	return 0;
}

^ permalink raw reply	[flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-07-11 20:46 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-07-09 21:54 [PATCH 1/1] Fix permission checking by NFS client for open-create with mode 000 Frank S. Filz
2014-07-09 22:17 ` Trond Myklebust
2014-07-09 22:42   ` Frank Filz
2014-07-09 23:06     ` Trond Myklebust
2014-07-09 23:12       ` Trond Myklebust
2014-07-10  4:26         ` Frank Filz
2014-07-10  4:32           ` Trond Myklebust
2014-07-10  5:22             ` Frank Filz
2014-07-10 12:42               ` Trond Myklebust
2014-07-10 14:23                 ` Frank Filz
2014-07-11 20:20         ` J. Bruce Fields
2014-07-11 20:46           ` Trond Myklebust
  -- strict thread matches above, loose matches on Subject: below --
2014-07-09 21:55 Frank S. Filz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox