[pacman-dev] [PATCH] can_remove_file: do not follow symlinks

[Joseph Burt]

On Sun, Aug 7, 2016 at 5:02 AM, Allan McRae <allan at archlinux.org> wrote:
>
> On 03/08/16 08:43, Joseph Burt wrote:
> > The symlink itself should be checked, not its target.
> >
> > Signed-off-by: Joseph Burt <caseorum at gmail.com>
> > ---
> >
> > This check also does more harm than good at the moment. If it fails once,
> > the rest of the package files are silently left on the filesystem.
> > Without this check, at least only the ones that failed to unlink would
> > be left, and the failures logged.
> >
> > What if the check for removal permissions happened at the very beginning
> > of a package removal, and aborted it on failure? Would I step on any toes
> > there?
>
> A check at the beginning of a package removal would be of interest.
>
> >  lib/libalpm/remove.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c
> > index 45f7c2f..48945ad 100644
> > --- a/lib/libalpm/remove.c
> > +++ b/lib/libalpm/remove.c
> > @@ -29,6 +29,7 @@
> >  #include <dirent.h>
> >  #include <regex.h>
> >  #include <unistd.h>
> > +#include <fcntl.h>
> >  #include <sys/stat.h>
> >  #include <sys/types.h>
> >
> > @@ -337,8 +338,8 @@ static int can_remove_file(alpm_handle_t *handle, const alpm_file_t *file)
> >
> >       /* If we fail write permissions due to a read-only filesystem, abort.
> >        * Assume all other possible failures are covered somewhere else */
> > -     if(_alpm_access(handle, NULL, filepath, W_OK) == -1) {
> > -             if(errno != EACCES && errno != ETXTBSY && access(filepath, F_OK) == 0) {
> > +     if(faccessat(AT_FDCWD, filepath, W_OK, AT_SYMLINK_NOFOLLOW) == -1) {
> > +             if(errno != EACCES && errno != ETXTBSY && faccessat(AT_FDCWD, filepath, F_OK, AT_SYMLINK_NOFOLLOW) == 0) {
>
> It would be better to fix _alpm_access and fix all occurrences of this
> rather than replace this one instance.
>
You're right of course, this is just the smallest change that would
correct the bug. Since _alpm_access can't tell from the W_OK mode
whether we mean to open for write (follow symlinks) or unlink (don't),
it's not enough. Thoughts on new util functions like
"_alpm_can_unlink"?

> Note AT_SYMLINK_NOFOLLOW is not available on OSX.  I believe pacman
> still builds there currently, but I can't remember if we decided not to
> support it...
>
Ok, so that means using lstat instead of faccessat.

There's also a TOCTOU bug here just for checking at all. Access or
lstat can't guarantee that open or unlink will succeed, so why bother?
Those failing should be dealt with properly anyway. The question is,
if a file fails to unlink, after logging it, should you remove the
package from the db, leaving orphaned files? Or leave it, but now
possibly missing files? Or keep FDs open for every file in a package
until all unlinks succeed, and if one fails, try to undo by copying
back the already unlnked files?
This is a design decision...

> >                       /* only return failure if the file ACTUALLY exists and we can't write to
> >                        * it - ignore "chmod -w" simple permission failures */
> >                       _alpm_log(handle, ALPM_LOG_ERROR, _("cannot remove file '%s': %s\n"),
> >