[pacman-dev] [PATCH] can_remove_file: do not follow symlinks
The symlink itself should be checked, not its target. Signed-off-by: Joseph Burt <caseorum@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? 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) { /* 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"), -- 2.9.2
On 03/08/16 08:43, Joseph Burt wrote:
The symlink itself should be checked, not its target.
Signed-off-by: Joseph Burt <caseorum@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. 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...
/* 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"),
On Sun, Aug 7, 2016 at 5:02 AM, Allan McRae <allan@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@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"),
On Sun, Aug 7, 2016 at 8:17 PM, Joseph Burt <caseorum@gmail.com> wrote:
On Sun, Aug 7, 2016 at 5:02 AM, Allan McRae <allan@archlinux.org> wrote:
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...
Allan, any thoughts on what you'd like here? I'm currently working around this bug by removing the can_remove_file check entirely.
On 09/25/16 at 01:43pm, Joseph Burt wrote:
On Sun, Aug 7, 2016 at 8:17 PM, Joseph Burt <caseorum@gmail.com> wrote:
On Sun, Aug 7, 2016 at 5:02 AM, Allan McRae <allan@archlinux.org> wrote:
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"?
Looking at our other uses of _alpm_access, this appears to actually be the only one where we would want to operate directly on the symlink.
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.
I'm not overly concerned with supporting OSX where it deviates from POSIX, but, in this case, AT_SYMLINK_NOFOLLOW is a non-POSIX extension for 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...
Allan, any thoughts on what you'd like here? I'm currently working around this bug by removing the can_remove_file check entirely.
Half-finished operations are never a good thing, so we do our best to make sure that once the operation starts it will be able to run to completion. Unfortunately, to the best of my knowledge, there is no way for us to perform these checks in a way that guarantees that the results are still valid by the time we go to perform the actual operation or reliably undo failed operations. The best we can do is check as much as possible prior to beginning the operation, assume that other processes aren't going to be fiddling with pacman-owned files during the operation, and handle errors as gracefully as possible. I agree that this is the wrong place to perform this check. This should be performed during the prepare phase so that we can prevent the doomed operation from ever starting. apg
participants (3)
-
Allan McRae
-
Andrew Gregory
-
Joseph Burt