Hi Solene,
Solene Rapenne wrote on Fri, Oct 30, 2020 at 06:34:09PM +0100:
> Following diff changes accton(8) behavior.
>
> If the file given as parameter doesn't exists, accton will create it.
> Then it will check the ownership and will make it root:wheel if
> it's different.
>
> I added a check to be sure it's run as root because it's has no use if
> not run as root.
If it naturally runs into privileged system calls anyway and the
error message is comprehensible, that is not necessarily needed.
Currently, the error message seems OK to me:
$ accton
accton: Operation not permitted
> I don't often write C, if the logic is good but the C implementation
> wrong I'm open to critic.
>
> The -f test and touch in /etc/rc won't be needed anymore with this
> change.
>
>
> Index: accton.8
> ===================================================================
> RCS file: /home/reposync/src/usr.sbin/accton/accton.8,v
> retrieving revision 1.11
> diff -u -p -r1.11 accton.8
> --- accton.8 10 Nov 2019 20:51:53 -0000 1.11
> +++ accton.8 30 Oct 2020 17:27:36 -0000
> @@ -36,7 +36,7 @@
> .Nm accton
> .Op Ar file
> .Sh DESCRIPTION
> -With an argument naming an existing
> +With an argument naming a
> .Ar file ,
> .Nm
> causes system accounting information for every process executed
*If* we change accton(8) to create the file if it does not exist,
the manual should probably be more explicit and say that an
existing file is appended to, but that it is created if it does
not exist.
Maybe it should even say something like
Unlike other implementations, this version of
.Nm
creates the
.Ar file
if it does not exist.
in order to not set a trap for experienced users.
> Index: accton.c
> ===================================================================
> RCS file: /home/reposync/src/usr.sbin/accton/accton.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 accton.c
> --- accton.c 27 Oct 2009 23:59:50 -0000 1.8
> +++ accton.c 30 Oct 2020 17:26:31 -0000
> @@ -27,6 +27,7 @@
> * SUCH DAMAGE.
> */
>
> +#include <sys/stat.h>
> #include <sys/types.h>
> #include <err.h>
> #include <stdio.h>
> @@ -47,6 +48,10 @@ int
> main(int argc, char *argv[])
> {
> int ch;
> + struct stat info_file;
> +
> + if(getuid() != 0)
> + err(1, "need root privileges");
>
> while ((ch = getopt(argc, argv, "")) != -1)
> switch(ch) {
> @@ -63,6 +68,15 @@ main(int argc, char *argv[])
> err(1, NULL);
> break;
> case 1:
> + if(stat(*argv,&info_file) != 0)
> + if(fopen(*argv,"w"))
Uh oh, that looks like a TOCTOU race condition to me.
> + if(stat(*argv,&info_file))
And that looks like another TOCTOU race condition.
Checking the return value of fopen(3) might be better.
Leaking a file descriptor from open(2) is also unusual.
Arguably, it may not be a problem here because the program
promptly exits anyway, but is is not nice for auditors.
Finally, what do you need fopen(3) for?
Wouldn't open(2) be sufficient?
> + err(1, "file %s couldn't be created",
> *argv);
> +
> + if (info_file.st_uid != 0 || info_file.st_gid != 0)
> + if(chown(*argv, 0, 0) != 0)
> + err(1, "Impossible to fix %s ownership", *argv);
> +
Isn't that yet another TOCTOU race condition?
I guess using fchown(2) might be more appropriate?
Also, fchmod(2) seems to be lacking, or even better, couldn't that
be covered by the third argument to open(2)?
> if (acct(*argv))
> err(1, "%s", *argv);
Arguably, there might also be a race condition between userland and
the kernel, but i'm not so sure about kernel land. Given how system
calls like open(2) work - taking a path, returning an int to avoid
race conditions - wouldn't it be more natural to have the acct(2)
system call create the file on the kernel side if necessary?
Yours,
Ingo
