Re: wall(1) unveil for non-root users
Philip Guenther wrote: > On Wed, May 15, 2019 at 10:54 PM Theo de Raadt wrote: > > Martijn van Duren wrote: > > > I don't see much point in the check. > > > > If we don't have write permissions open(2) will fail. > > If we open it based on S_IWOTH permissions than checking for S_IWGRP > > without considering who is in that group seems really absurd to me. > > > > So I'd be OK with patch 1 > > There are 3 write permission checks which could permit the open, > but then it checks the specific one. > > I believe this is similar to comsat / biff, which in those programs > are used to indicate "I am ok with messages to my tty". > > That is an old behaviour, but I bet that is the history of this S_IWGRP > check. > > Yep. mesg(1) manipulates your tty's group-write bit so that wall(1), > talk(1), and write(1) can check it. Amusing, write(1) has a very similar TOCTOU. These are not serious, but if they were fixes there are opportunities for unveil and pledge to maybe prevent other bad behaviour in the future..
Re: wall(1) unveil for non-root users
On Wed, May 15, 2019 at 10:54 PM Theo de Raadt wrote: > Martijn van Duren wrote: > > > I don't see much point in the check. > > > > If we don't have write permissions open(2) will fail. > > If we open it based on S_IWOTH permissions than checking for S_IWGRP > > without considering who is in that group seems really absurd to me. > > > > So I'd be OK with patch 1 > > There are 3 write permission checks which could permit the open, > but then it checks the specific one. > > I believe this is similar to comsat / biff, which in those programs > are used to indicate "I am ok with messages to my tty". > > That is an old behaviour, but I bet that is the history of this S_IWGRP > check. > Yep. mesg(1) manipulates your tty's group-write bit so that wall(1), talk(1), and write(1) can check it. Philip Guenther
Re: wall(1) unveil for non-root users
On 5/16/19 7:53 AM, Theo de Raadt wrote: > Martijn van Duren wrote: > >> I don't see much point in the check. >> >> If we don't have write permissions open(2) will fail. >> If we open it based on S_IWOTH permissions than checking for S_IWGRP >> without considering who is in that group seems really absurd to me. >> >> So I'd be OK with patch 1 > > There are 3 write permission checks which could permit the open, > but then it checks the specific one. > > I believe this is similar to comsat / biff, which in those programs > are used to indicate "I am ok with messages to my tty". > > That is an old behaviour, but I bet that is the history of this S_IWGRP check. > Sounds valid. Personally I think it's a rather obscure feature that can go as far as I'm concerned, but if people want it I'm OK with either diff. martijn@
Re: wall(1) unveil for non-root users
Martijn van Duren wrote: > I don't see much point in the check. > > If we don't have write permissions open(2) will fail. > If we open it based on S_IWOTH permissions than checking for S_IWGRP > without considering who is in that group seems really absurd to me. > > So I'd be OK with patch 1 There are 3 write permission checks which could permit the open, but then it checks the specific one. I believe this is similar to comsat / biff, which in those programs are used to indicate "I am ok with messages to my tty". That is an old behaviour, but I bet that is the history of this S_IWGRP check.
Re: wall(1) unveil for non-root users
I don't see much point in the check.
If we don't have write permissions open(2) will fail.
If we open it based on S_IWOTH permissions than checking for S_IWGRP
without considering who is in that group seems really absurd to me.
So I'd be OK with patch 1
martijn@
On 5/16/19 12:46 AM, Theo de Raadt wrote:
> Anton Borowka wrote:
>
>> wall(1) does not work correctly for non-root users at the moment because
>> ttymsg() needs read access for the tty devices, but only write access is
>> unveiled. Because it cannot access any devices nothing is printed.
>>
>> This patch adds read access for /dev. Don't know if it makes sense to
>> only unveil read access for non-root users.
>
> Apparently it stat()'s the devices [thereby desiring "r"], before calling
> open O_WRONLY [which only needs "w"].
>
> It is a TOCTOU. Here are two proposals.
>
> This one lets non-root write to any fd it can manage to open.
>
> Index: ttymsg.c
> ===
> RCS file: /cvs/src/usr.bin/wall/ttymsg.c,v
> retrieving revision 1.17
> diff -u -p -u -r1.17 ttymsg.c
> --- ttymsg.c 5 Nov 2015 22:20:11 - 1.17
> +++ ttymsg.c 15 May 2019 22:39:44 -
> @@ -87,13 +87,6 @@ ttymsg(iov, iovcnt, line, tmout)
> return (errbuf);
> }
>
> - if (getuid()) {
> - if (stat(device, ) < 0)
> - return (NULL);
> - if ((st.st_mode & S_IWGRP) == 0)
> - return (NULL);
> - }
> -
> /*
>* open will fail on slip lines or exclusive-use lines
>* if not running as root; not an error.
>
> This one is a bit more cautious, it checks if specifically g+w
> is allowed on the tty, that is more true to the original behaviour
> isn't it?
>
> Index: ttymsg.c
> ===
> RCS file: /cvs/src/usr.bin/wall/ttymsg.c,v
> retrieving revision 1.17
> diff -u -p -u -r1.17 ttymsg.c
> --- ttymsg.c 5 Nov 2015 22:20:11 - 1.17
> +++ ttymsg.c 15 May 2019 22:42:58 -
> @@ -87,12 +87,6 @@ ttymsg(iov, iovcnt, line, tmout)
> return (errbuf);
> }
>
> - if (getuid()) {
> - if (stat(device, ) < 0)
> - return (NULL);
> - if ((st.st_mode & S_IWGRP) == 0)
> - return (NULL);
> - }
>
> /*
>* open will fail on slip lines or exclusive-use lines
> @@ -104,6 +98,17 @@ ttymsg(iov, iovcnt, line, tmout)
> (void) snprintf(errbuf, sizeof(errbuf),
> "%s: %s", device, strerror(errno));
> return (errbuf);
> + }
> +
> + if (getuid()) {
> + if (fstat(fd, ) < 0) {
> + close(fd);
> + return (NULL);
> + }
> + if ((st.st_mode & S_IWGRP) == 0) {
> + close(fd);
> + return (NULL);
> + }
> }
>
> for (cnt = left = 0; cnt < iovcnt; ++cnt)
>
Re: wall(1) unveil for non-root users
"Theo de Raadt" writes:
> Anton Borowka wrote:
>
>> wall(1) does not work correctly for non-root users at the moment because
>> ttymsg() needs read access for the tty devices, but only write access is
>> unveiled. Because it cannot access any devices nothing is printed.
>>
>> This patch adds read access for /dev. Don't know if it makes sense to
>> only unveil read access for non-root users.
>
> Apparently it stat()'s the devices [thereby desiring "r"], before calling
> open O_WRONLY [which only needs "w"].
>
> It is a TOCTOU. Here are two proposals.
>
> This one lets non-root write to any fd it can manage to open.
>
> Index: ttymsg.c
> ===
> RCS file: /cvs/src/usr.bin/wall/ttymsg.c,v
> retrieving revision 1.17
> diff -u -p -u -r1.17 ttymsg.c
> --- ttymsg.c 5 Nov 2015 22:20:11 - 1.17
> +++ ttymsg.c 15 May 2019 22:39:44 -
> @@ -87,13 +87,6 @@ ttymsg(iov, iovcnt, line, tmout)
> return (errbuf);
> }
>
> - if (getuid()) {
> - if (stat(device, ) < 0)
> - return (NULL);
> - if ((st.st_mode & S_IWGRP) == 0)
> - return (NULL);
> - }
> -
> /*
>* open will fail on slip lines or exclusive-use lines
>* if not running as root; not an error.
>
> This one is a bit more cautious, it checks if specifically g+w
> is allowed on the tty, that is more true to the original behaviour
> isn't it?
>
> Index: ttymsg.c
> ===
> RCS file: /cvs/src/usr.bin/wall/ttymsg.c,v
> retrieving revision 1.17
> diff -u -p -u -r1.17 ttymsg.c
> --- ttymsg.c 5 Nov 2015 22:20:11 - 1.17
> +++ ttymsg.c 15 May 2019 22:42:58 -
> @@ -87,12 +87,6 @@ ttymsg(iov, iovcnt, line, tmout)
> return (errbuf);
> }
>
> - if (getuid()) {
> - if (stat(device, ) < 0)
> - return (NULL);
> - if ((st.st_mode & S_IWGRP) == 0)
> - return (NULL);
> - }
>
> /*
>* open will fail on slip lines or exclusive-use lines
> @@ -104,6 +98,17 @@ ttymsg(iov, iovcnt, line, tmout)
> (void) snprintf(errbuf, sizeof(errbuf),
> "%s: %s", device, strerror(errno));
> return (errbuf);
> + }
> +
> + if (getuid()) {
> + if (fstat(fd, ) < 0) {
> + close(fd);
> + return (NULL);
> + }
> + if ((st.st_mode & S_IWGRP) == 0) {
> + close(fd);
> + return (NULL);
> + }
> }
>
> for (cnt = left = 0; cnt < iovcnt; ++cnt)
While looking at the wall(1) code I also thought about the first diff,
but the comments above ttymesg() said something about it being used by
talkd(8). Apparently I misunderstood or the comment is outdated, but I
didn't look into it.
I don't know about the practical implications of the second diff. A user
can not read his own messages, because for example the tty is not group
writable (but writable by the user)?
As you said the second one is more conservative and keeps to the old
behaviour. I personally don't see anything wrong with the first diff,
but I'm not an expert.
Best regards
Anton
Re: wall(1) unveil for non-root users
Anton Borowka wrote:
> wall(1) does not work correctly for non-root users at the moment because
> ttymsg() needs read access for the tty devices, but only write access is
> unveiled. Because it cannot access any devices nothing is printed.
>
> This patch adds read access for /dev. Don't know if it makes sense to
> only unveil read access for non-root users.
Apparently it stat()'s the devices [thereby desiring "r"], before calling
open O_WRONLY [which only needs "w"].
It is a TOCTOU. Here are two proposals.
This one lets non-root write to any fd it can manage to open.
Index: ttymsg.c
===
RCS file: /cvs/src/usr.bin/wall/ttymsg.c,v
retrieving revision 1.17
diff -u -p -u -r1.17 ttymsg.c
--- ttymsg.c5 Nov 2015 22:20:11 - 1.17
+++ ttymsg.c15 May 2019 22:39:44 -
@@ -87,13 +87,6 @@ ttymsg(iov, iovcnt, line, tmout)
return (errbuf);
}
- if (getuid()) {
- if (stat(device, ) < 0)
- return (NULL);
- if ((st.st_mode & S_IWGRP) == 0)
- return (NULL);
- }
-
/*
* open will fail on slip lines or exclusive-use lines
* if not running as root; not an error.
This one is a bit more cautious, it checks if specifically g+w
is allowed on the tty, that is more true to the original behaviour
isn't it?
Index: ttymsg.c
===
RCS file: /cvs/src/usr.bin/wall/ttymsg.c,v
retrieving revision 1.17
diff -u -p -u -r1.17 ttymsg.c
--- ttymsg.c5 Nov 2015 22:20:11 - 1.17
+++ ttymsg.c15 May 2019 22:42:58 -
@@ -87,12 +87,6 @@ ttymsg(iov, iovcnt, line, tmout)
return (errbuf);
}
- if (getuid()) {
- if (stat(device, ) < 0)
- return (NULL);
- if ((st.st_mode & S_IWGRP) == 0)
- return (NULL);
- }
/*
* open will fail on slip lines or exclusive-use lines
@@ -104,6 +98,17 @@ ttymsg(iov, iovcnt, line, tmout)
(void) snprintf(errbuf, sizeof(errbuf),
"%s: %s", device, strerror(errno));
return (errbuf);
+ }
+
+ if (getuid()) {
+ if (fstat(fd, ) < 0) {
+ close(fd);
+ return (NULL);
+ }
+ if ((st.st_mode & S_IWGRP) == 0) {
+ close(fd);
+ return (NULL);
+ }
}
for (cnt = left = 0; cnt < iovcnt; ++cnt)
wall(1) unveil for non-root users
wall(1) does not work correctly for non-root users at the moment because ttymsg() needs read access for the tty devices, but only write access is unveiled. Because it cannot access any devices nothing is printed. This patch adds read access for /dev. Don't know if it makes sense to only unveil read access for non-root users. Index: usr.bin/wall/wall.c === RCS file: /cvs/src/usr.bin/wall/wall.c,v retrieving revision 1.34 diff -u -p -u -r1.34 wall.c --- usr.bin/wall/wall.c 28 Jan 2019 20:17:51 - 1.34 +++ usr.bin/wall/wall.c 15 May 2019 17:06:31 - @@ -117,7 +117,7 @@ main(int argc, char **argv) if (unveil(_PATH_UTMP, "r") == -1) err(1, "unveil"); - if (unveil(_PATH_DEV, "w") == -1) + if (unveil(_PATH_DEV, "rw") == -1) err(1, "unveil"); if (unveil(_PATH_DEVDB, "r") == -1) err(1, "unveil");
