Created attachment 53691 [details] Patch to allow the use of real access() calls Hi, this is a bug inherited from OOo (#101891, unfixed for more than 2 years): File permissions are determined by a stat() call and manually checking the permission map. This is plain wrong when filesystems not using UNIX semantics (e.g. OpenAFS or Linux ACLs) are in place, and causes perfectly writeable files (haven't checked that behaviour with libreoffice, but its wrong anyways) to open write-protected. Instead access() calls must be used to determine readability and writeablitity. On the other side it seems there have been problems using access() in the past: > We don't use access(...) because access follows links which > may cause performance problems see #97133. (comment in sal/osl/unx/file_stat.cxx) If that's still the case it would be nice to have a compile time option to switch between the two behaviours. Patch against master is attached.
I suggest we instead try to see why the code wants to check file access before attempting to open a file (if that is what it does), and if possible just try to get rid of such checks. I very much doubt even access() can get it right in all circumstances. For instance, surely mounting volumes from Windows servers on Unix clients is much more common than using OpenAFS or Linux ACLs. Does acess() work correctly if the file has some funky ACL on the SMB side? Isn't it in general wrong to try to check if something is possible before doing it? See http://en.wikipedia.org/wiki/Time-of-check-to-time-of-use . Instead, just try to do what you want to do (like open a file for reading or writing), and if it doesn't succeed, fail gracefully at that point.
I can guess that the code e.g. wants to put the document in "READ ONLY" mode / show some UI difference when the file it was opened from cannot be written to. Anyway, if we really need to check for permission without actually trying to do the action, we can solve the "access() follows symlink" problem by using faccessat(-1, "/absolute/path", AT_EACCESS | AT_SYMLINK_NOFOLLOW) or faccessat(AT_FDCWD, "relative/or/absolute/path", AT_EACCESS | AT_SYMLINK_NOFOLLOW)
And that is portable to all Unix systems we care for? And symlinks are the only thing that can make access() misleading?
But whatever, getting more contributors is important, I am not opposing, will apply the patch tomorrow when I get to work.
(In reply to comment #3) > And that is portable to all Unix systems we care for? Which systems is that? Standards-wise, faccessat() is in POSIX.1-2008 and Open Group Extended API Set, Part 2. It is supported by GNU/Linux and FreeBSD: GNU/Linux: by default since glibc 2.10 (released in May 2009), needs "#define _ATFILE_SOURCE 1" in older glibc. Needs kernel 2.6.16 (released 20 March 2006) or later. FreeBSD: version 8.0 and later. Release: 25 November 25 2009. However, FreeBSD and "Open Group Extended API Set, Part 2" do not document the AT_SYMLINK_NOFOLLOW flag for faccessat :-( So, hmm. > And symlinks are the only thing that can make access() misleading? If LibreOffice is setuid-foo, then access() checks permission for the invoking user, not for foo. That is fixed in FreeBSD and GNU/Linux with the eaccess() function, but that is not standard. That's also what the AT_EACCESS flag to faccessat does. However, now that I think about it, checking permissions of the symlink rather than permissions of symlink's target is probably the wrong thing anyway.
> Which systems is that? Well, Mac OS X is the first one that comes to mind. Interestingly, the Mac OS X manpage for plain old access() says: "CAVEAT: Access() is a potential security hole and should never be used" ;) And my point whether "symlinks are the only thing that can make access() misleading" was not security issues, but just simply that I wouldn't trust that access() (or faccessat()), even on the most bleeding edge Linux, would necessarily be able to tell the truth beforehand whether you will actually succeed in actually opening some pathname for reading or writing in various scenarios.
(In reply to comment #6) >> Which systems is that? > Well, Mac OS X is the first one that comes to mind. Ah, I had temporarily forgotten it is a Unix. Indeed, it does not support it, nor does it support eaccess: http://lists.apple.com/archives/darwin-dev/2009/Mar/msg00110.html > Interestingly, the Mac OS X manpage for plain old access() says: "CAVEAT: > Access() is a potential security hole and should never be used" ;) Well, it should never by used to authorise an action by a privileged process, as there is a race condition. The FreeBSD man page says the same, but then continues saying "but you can use it to give UI hints to the user".
Should be fixed now with: commit a35140f245d774745b806ef12346aa77f0256395 Author: Tor Lillqvist <tlillqvist@suse.com> Date: Fri Dec 2 11:41:33 2011 +0200 Fix library layer mapping for the URELIB ones, fdo#42826
Argh, ignore previous comment, wrong bug...
Had second thoughts; will apply only after 3.5 has been branched.
Stephan volunteered to have a closer look at all involved issues, re-assigning.
Looks like there is only a modes number of calls to set_file_access_rights, so should be safe to include the attached patch. (The performance issue 97133 mentioned in the original code appears to reference a no-longer accessible Hamburg-internal bug-tracking system, so hard to tell what the exact problem was there.) Pushed now as <http://cgit.freedesktop.org/libreoffice/core/commit/?id=32a6a0891fb5f2d893cca656cd44afd0bcbe3272> with following clean up <http://cgit.freedesktop.org/libreoffice/core/commit/?id=fb2078addcbd96662283b2481206eee7ce3d50b6>. Moritz, I assume you make available your patch under the standard LO terms (LGPLv3+/MPL)? Please update <http://wiki.documentfoundation.org/Development/Developers> accordingly.
Already confirmed that on the mailinglist, added the reference to the wiki. Thanks to all the people involved.