From 2b3e6031eb3f39286a81f2a318e1f9ffbc5d4e2f Mon Sep 17 00:00:00 2001 From: Jakob Unterwurzacher Date: Sat, 25 Jun 2016 14:47:16 +0200 Subject: [PATCH 1/3] tests: open file with umask 0777 Regression test for https://github.com/vgough/encfs/issues/181 (fails at the moment, fixed in the next commits) --- tests/normal.t.pl | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/tests/normal.t.pl b/tests/normal.t.pl index 99e18b7..841c60a 100755 --- a/tests/normal.t.pl +++ b/tests/normal.t.pl @@ -2,7 +2,7 @@ # Test EncFS normal and paranoid mode -use Test::More tests => 102; +use Test::More tests => 104; use File::Path; use File::Copy; use File::Temp; @@ -46,6 +46,7 @@ sub runTests &renames; &internalModification; &grow; + &umask0777; &cleanup; } @@ -324,3 +325,12 @@ sub cleanup ok( ! -d $workingDir, "working dir removed"); } +# Test that we can create and write to a a file even if umask is set to 0777 +# Regression test for bug https://github.com/vgough/encfs/issues/181 +sub umask0777 +{ + my $old = umask(0777); + ok(open(my $fh, "+>$crypt/umask0777"), "open with umask 0777"); + close($fh); + umask($old); +} From 4a691dc0bb98cab3f3b3d38a836f1596bc19bff8 Mon Sep 17 00:00:00 2001 From: Jakob Unterwurzacher Date: Sat, 25 Jun 2016 14:48:45 +0200 Subject: [PATCH 2/3] Revert "Remove open_readonly_workaround" Caused a regression: https://github.com/vgough/encfs/issues/181 Without this, "umask 0777 ; echo foo > bar" fails. This reverts commit c2e046b694a42a6e17ec4a47a517638a098f928a. --- encfs/RawFileIO.cpp | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/encfs/RawFileIO.cpp b/encfs/RawFileIO.cpp index 5ec63f7..7c20310 100644 --- a/encfs/RawFileIO.cpp +++ b/encfs/RawFileIO.cpp @@ -75,6 +75,32 @@ RawFileIO::~RawFileIO() { Interface RawFileIO::interface() const { return RawFileIO_iface; } +/* + Workaround for opening a file for write when permissions don't allow. + Since the kernel has already checked permissions, we can assume it is ok to + provide access. So force it by changing permissions temporarily. Should + be called with a lock around it so that there won't be a race condition + with calls to lstat picking up the wrong permissions. + + This works around the problem described in https://github.com/vgough/encfs/issues/181 + Without this, "umask 0777 ; echo foo > bar" fails. +*/ +static int open_readonly_workaround(const char *path, int flags) { + int fd = -1; + struct stat stbuf; + memset(&stbuf, 0, sizeof(struct stat)); + if (lstat(path, &stbuf) != -1) { + // make sure user has read/write permission.. + chmod(path, stbuf.st_mode | 0600); + fd = ::open(path, flags); + chmod(path, stbuf.st_mode); + } else { + RLOG(INFO) << "can't stat file " << path; + } + + return fd; +} + /* We shouldn't have to support all possible open flags, so untaint the flags argument by only taking ones we understand and accept. @@ -107,6 +133,11 @@ int RawFileIO::open(int flags) { VLOG(1) << "open file with flags " << finalFlags << ", result = " << newFd; + if ((newFd == -1) && (errno == EACCES)) { + VLOG(1) << "using readonly workaround for open"; + newFd = open_readonly_workaround(name.c_str(), finalFlags); + } + if (newFd >= 0) { if (oldfd >= 0) { RLOG(ERROR) << "leaking FD?: oldfd = " << oldfd << ", fd = " << fd From 87ee4b7d99781630c467df265dbe0b66b98de3b5 Mon Sep 17 00:00:00 2001 From: Jakob Unterwurzacher Date: Sat, 25 Jun 2016 15:12:04 +0200 Subject: [PATCH 3/3] Revert "Remove "-o default_permissions" unless needed." Caused a regression: https://github.com/vgough/encfs/issues/112 , and removing open_readonly_workaround caused another one. So let's just keep it as it was. This reverts commit 82ceb88998fccfdbe21d2e66cd8764adef7d55e5. --- encfs/main.cpp | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/encfs/main.cpp b/encfs/main.cpp index d3cf573..d79ec79 100644 --- a/encfs/main.cpp +++ b/encfs/main.cpp @@ -405,17 +405,14 @@ static bool processArgs(int argc, char *argv[], PUSHARG("-o"); PUSHARG("use_ino"); - // "default_permissions" comes with a performance cost. Only enable - // it if makes sense. - for (int i = 0; i < out->fuseArgc; i++) { - if (out->fuseArgv[i] == NULL) { - continue; - } else if (strcmp(out->fuseArgv[i], "allow_other") == 0) { - PUSHARG("-o"); - PUSHARG("default_permissions"); - break; - } - } + // "default_permissions" comes with a performance cost, and only makes + // sense if "allow_other"" is used. + // But it works around the issues "open_readonly_workaround" causes, + // so enable it unconditionally. + // See https://github.com/vgough/encfs/issues/181 and + // https://github.com/vgough/encfs/issues/112 for more info. + PUSHARG("-o"); + PUSHARG("default_permissions"); #if defined(__APPLE__) // With OSXFuse, the 'local' flag selects a local filesystem mount icon in