Re: Should rm(1) -Pf change file permission?

2018-04-18 Thread Grégoire Jadi
Ingo Schwarze  writes:

Hello Ingo,

> Hi Gregoire,
>
> Gregoire Jadi wrote on Tue, Apr 17, 2018 at 11:44:41AM +0200:
>> Ingo Schwarze  writes:
>
>>> Feedback is welcome both on the general idea and on the specific
>>> implementation.
>
> The result of that feeback was "we don't want it, the whole
> idea of changing this is pointless and dangerous", so i deleted
> my patch quite some time ago.

Ok.

> The conversation resulted in a clarification in the manual page,
> though.

Thanks for the clarification. It's clear now it's a "best effort".

>> Thank you for working on this.  I have tested your patch
>> and both `rm -P` and `rm -Pf` work as expected.
>
> Sorry, it won't be committed, it was definitely rejected.
> Besides, it wasn't correct, there were race conditions.
> Working on improving it would be a waste of time.

Ok. So, this issue is closed. :)

Best,

> Yours,
>   Ingo



Re: Should rm(1) -Pf change file permission?

2018-04-17 Thread Ingo Schwarze
Hi Gregoire,

Gregoire Jadi wrote on Tue, Apr 17, 2018 at 11:44:41AM +0200:
> Ingo Schwarze  writes:

>> Feedback is welcome both on the general idea and on the specific
>> implementation.

The result of that feeback was "we don't want it, the whole
idea of changing this is pointless and dangerous", so i deleted
my patch quite some time ago.

The conversation resulted in a clarification in the manual page,
though.

> Thank you for working on this.  I have tested your patch
> and both `rm -P` and `rm -Pf` work as expected.

Sorry, it won't be committed, it was definitely rejected.
Besides, it wasn't correct, there were race conditions.
Working on improving it would be a waste of time.

Yours,
  Ingo



Re: Should rm(1) -Pf change file permission?

2018-04-17 Thread Grégoire Jadi
Ingo Schwarze  writes:

Hi,

> Hi,
>
> Gregoire Jadi wrote on Fri, Mar 30, 2018 at 06:07:42PM +0200:
>
>> While working on a port of keyringer, I observed the following behavior
>> of rm(1) with the -P option: if the file does not have write permission,
>> the file is removed without being overwritten.
>> 
>> This is not the same behavior as shred(1) (from sysutils/coreutils) which
>> do not remove the file if it cannot be overwritten. If the -f option is
>> used with shred(1), the permission of the file are changed to allow
>> writing if necessary.
>> 
>> Is the current behavior desired? (need to update the manpage?)
>> 
>> Or should `rm -P` and `rm -Pf` have the same behavior as shred. That is:
>> `rm -P` should fail without removing the file if the file cannot be
>> overwritten and `rm -Pf` should change the permission and overwrite
>> file.
>
> That makes more sense to me than the current behaviour.
>
> In addition, when running with -P and without -f and the user
> interactive confirms that the missing write protection shall be
> overriden, changing the permissions seems right to me, too.
>
> Here is a patch implementing that.
>
> Lightly tested by hand by running various combinations of rm(1)
>  * with and without -P
>  * with and without -f
>  * on files with different permissions
>
> Feedback is welcome both on the general idea and on the specific
> implementation.

Thank you for working on this. I have tested your patch and both `rm -P`
and `rm -Pf` work as expected.

I have no opinion on your implementation but it does solve the problem
and make rm(1) more consistent with gshred(1).

Best,

> Note that the return value of rm_overwrite() is currently unused.
> So we are free to change it to reflect success or failure, such
> that deleting the file can be skipped in case of failure even
> after trying to relax the permissions.  Also note that checking
> fflag in rw_overwrite is not needed (and would even be wrong)
> because the program doesn't get there in the first place unless
> fflag was given or deletion was interactively confirmed.
>
> Yours,
>   Ingo
>
> Index: rm.c
> ===
> RCS file: /cvs/src/bin/rm/rm.c,v
> retrieving revision 1.42
> diff -u -p -r1.42 rm.c
> --- rm.c  27 Jun 2017 21:49:47 -  1.42
> +++ rm.c  31 Mar 2018 17:02:37 -
> @@ -103,7 +103,7 @@ main(int argc, char *argv[])
>   argv += optind;
>
>   if (Pflag) {
> - if (pledge("stdio rpath wpath cpath getpw", NULL) == -1)
> + if (pledge("stdio rpath wpath cpath fattr getpw", NULL) == -1)
>   err(1, "pledge");
>   } else {
>   if (pledge("stdio rpath cpath getpw", NULL) == -1)
> @@ -213,9 +213,9 @@ rm_tree(char **argv)
>
>   case FTS_F:
>   case FTS_NSOK:
> - if (Pflag)
> - rm_overwrite(p->fts_accpath, p->fts_info ==
> - FTS_NSOK ? NULL : p->fts_statp);
> + if (Pflag && !rm_overwrite(p->fts_accpath,
> + p->fts_info == FTS_NSOK ? NULL : p->fts_statp))
> + continue;
>   /* FALLTHROUGH */
>   default:
>   if (!unlink(p->fts_accpath) ||
> @@ -264,8 +264,8 @@ rm_file(char **argv)
>   else if (S_ISDIR(sb.st_mode))
>   rval = rmdir(f);
>   else {
> - if (Pflag)
> - rm_overwrite(f, );
> + if (Pflag && !rm_overwrite(f, ))
> + continue;
>   rval = unlink(f);
>   }
>   if (rval && (!fflag || errno != ENOENT)) {
> @@ -308,9 +308,12 @@ rm_overwrite(char *file, struct stat *sb
>   if (sbp->st_nlink > 1) {
>   warnx("%s (inode %llu): not overwritten due to multiple links",
>   file, (unsigned long long)sbp->st_ino);
> - return (0);
> + return (1);
>   }
> - if ((fd = open(file, O_WRONLY|O_NONBLOCK|O_NOFOLLOW, 0)) == -1)
> + if ((fd = open(file, O_WRONLY|O_NONBLOCK|O_NOFOLLOW, 0)) == -1 &&
> + (errno != EACCES ||
> +  chmod(file, sb.st_mode | S_IWUSR) == -1 ||
> +  (fd = open(file, O_WRONLY|O_NONBLOCK|O_NOFOLLOW, 0)) == -1))
>   goto err;
>   if (fstat(fd, ))
>   goto err;



Re: Should rm(1) -Pf change file permission?

2018-03-31 Thread Ingo Schwarze
Hi,

Gregoire Jadi wrote on Fri, Mar 30, 2018 at 06:07:42PM +0200:

> While working on a port of keyringer, I observed the following behavior
> of rm(1) with the -P option: if the file does not have write permission,
> the file is removed without being overwritten.
> 
> This is not the same behavior as shred(1) (from sysutils/coreutils) which
> do not remove the file if it cannot be overwritten. If the -f option is
> used with shred(1), the permission of the file are changed to allow
> writing if necessary.
> 
> Is the current behavior desired? (need to update the manpage?)
> 
> Or should `rm -P` and `rm -Pf` have the same behavior as shred. That is:
> `rm -P` should fail without removing the file if the file cannot be
> overwritten and `rm -Pf` should change the permission and overwrite
> file.

That makes more sense to me than the current behaviour.

In addition, when running with -P and without -f and the user
interactive confirms that the missing write protection shall be
overriden, changing the permissions seems right to me, too.

Here is a patch implementing that.

Lightly tested by hand by running various combinations of rm(1)
 * with and without -P
 * with and without -f
 * on files with different permissions

Feedback is welcome both on the general idea and on the specific
implementation.

Note that the return value of rm_overwrite() is currently unused.
So we are free to change it to reflect success or failure, such
that deleting the file can be skipped in case of failure even
after trying to relax the permissions.  Also note that checking
fflag in rw_overwrite is not needed (and would even be wrong)
because the program doesn't get there in the first place unless
fflag was given or deletion was interactively confirmed.

Yours,
  Ingo


Index: rm.c
===
RCS file: /cvs/src/bin/rm/rm.c,v
retrieving revision 1.42
diff -u -p -r1.42 rm.c
--- rm.c27 Jun 2017 21:49:47 -  1.42
+++ rm.c31 Mar 2018 17:02:37 -
@@ -103,7 +103,7 @@ main(int argc, char *argv[])
argv += optind;
 
if (Pflag) {
-   if (pledge("stdio rpath wpath cpath getpw", NULL) == -1)
+   if (pledge("stdio rpath wpath cpath fattr getpw", NULL) == -1)
err(1, "pledge");
} else {
if (pledge("stdio rpath cpath getpw", NULL) == -1)
@@ -213,9 +213,9 @@ rm_tree(char **argv)
 
case FTS_F:
case FTS_NSOK:
-   if (Pflag)
-   rm_overwrite(p->fts_accpath, p->fts_info ==
-   FTS_NSOK ? NULL : p->fts_statp);
+   if (Pflag && !rm_overwrite(p->fts_accpath,
+   p->fts_info == FTS_NSOK ? NULL : p->fts_statp))
+   continue;
/* FALLTHROUGH */
default:
if (!unlink(p->fts_accpath) ||
@@ -264,8 +264,8 @@ rm_file(char **argv)
else if (S_ISDIR(sb.st_mode))
rval = rmdir(f);
else {
-   if (Pflag)
-   rm_overwrite(f, );
+   if (Pflag && !rm_overwrite(f, ))
+   continue;
rval = unlink(f);
}
if (rval && (!fflag || errno != ENOENT)) {
@@ -308,9 +308,12 @@ rm_overwrite(char *file, struct stat *sb
if (sbp->st_nlink > 1) {
warnx("%s (inode %llu): not overwritten due to multiple links",
file, (unsigned long long)sbp->st_ino);
-   return (0);
+   return (1);
}
-   if ((fd = open(file, O_WRONLY|O_NONBLOCK|O_NOFOLLOW, 0)) == -1)
+   if ((fd = open(file, O_WRONLY|O_NONBLOCK|O_NOFOLLOW, 0)) == -1 &&
+   (errno != EACCES ||
+chmod(file, sb.st_mode | S_IWUSR) == -1 ||
+(fd = open(file, O_WRONLY|O_NONBLOCK|O_NOFOLLOW, 0)) == -1))
goto err;
if (fstat(fd, ))
goto err;



Re: Should rm(1) -Pf change file permission?

2018-03-31 Thread Craig Skinner
Hi Grégoire/all,

On Fri, 30 Mar 2018 18:07:42 +0200 Grégoire Jadi wrote:
> ... here is a small test to demonstrate ...

Same behaviour noticed and previously bugged:-
http://openbsd-archive.7691.n7.nabble.com/rm-P-doesn-t-overwrite-a-user-owned-read-only-file-td266276.html

Regards,
-- 
Craig Skinner | http://linkd.in/yGqkv7



Should rm(1) -Pf change file permission?

2018-03-30 Thread Grégoire Jadi
Hello,

While working on a port of keyringer, I observed the following behavior
of rm(1) with the -P option: if the file does not have write permission,
the file is removed without being overwritten.

This is not the same behavior as shred(1) (from sysutils/coreutils) which do
not remove the file if it cannot be overwritten. If the -f option is
used with shred(1), the permission of the file are changed to allow
writing if necessary.

Is the current behavior desired? (need to update the manpage?)

Or should `rm -P` and `rm -Pf` have the same behavior as shred. That is:
`rm -P` should fail without removing the file if the file cannot be
overwritten and `rm -Pf` should change the permission and overwrite
file.


Here are excerpts from rm(1) and shred(1) manpages:
rm(1)
 -f  Attempt to remove the files without prompting for confirmation,
 regardless of the file's permissions.  If the file does not
 exist, do not display a diagnostic message or modify the exit
 status to reflect an error.  The -f option overrides any previous
 -i options.

 -P  Overwrite regular files before deleting them.  Files are
 overwritten once with a random pattern.  Files with multiple
 links will be unlinked but not overwritten.

shred(1)
   -f, --force
  change permissions to allow writing if necessary

And here is a small test to demonstrate this behavior:

$ echo bar > foo
$ chmod -w foo
$ rm -P foo
override r--r--r--  daimrod/wheel for foo? y
rm: foo: Permission denied
$ ls -l foo
ls: foo: No such file or directory

$ echo bar > foo
$ chmod -w foo
$ rm -Pf foo
rm: foo: Permission denied
$ ls -l foo
ls: foo: No such file or directory

$ echo bar > foo
$ chmod -w foo
$ gshred -u foo
gshred: foo: failed to open for writing: Permission denied
$ ls -l foo
-r--r--r--  1 daimrod  wheel  4 Mar 30 17:45 foo
$ gshred -uf foo
$ ls -l foo
ls: foo: No such file or directory

Best,