On May 23, 2019, at 8:11 PM, Aleksa Sarai [off-list ref] wrote:

quoted

On 2019-05-23, Aleksa Sarai [off-list ref] wrote:

quoted

On 2019-05-22, Andy Lutomirski [off-list ref] wrote:
What are actual examples of uses for this exception?  Breaking
selftests is not, in and of itself, a huge problem.

Not as far as I know. All of the re-opening users I know of do re-opens
of O_PATH or are re-opening with the same (or fewer) privileges. I also
ran this for a few days on my laptop without this exception, and didn't
have any visible issues.

I have modified the patch to WARN_ON(may_open_magiclink() == -EACCES).

So far (in the past day on my openSUSE machines) I have only seen two
programs which have hit this case: kbd[1]'s "loadkeys" and "kbd_mode"
binaries. In addition to there not being any user-visible errors -- they
actually handle permission errors gracefully!

 static int
 open_a_console(const char *fnam)
 {
     int fd;

     /*
      * For ioctl purposes we only need some fd and permissions
      * do not matter. But setfont:activatemap() does a write.
      */
     fd = open(fnam, O_RDWR);
     if (fd < 0)
         fd = open(fnam, O_WRONLY);
     if (fd < 0)
         fd = open(fnam, O_RDONLY);
     if (fd < 0)
         return -1;
     return fd;
 }

The above gets called with "/proc/self/fd/0" as an argument (as well as
other console candidates like "/dev/console"). And setfont:activatemap()
actually does handle read-only fds:

 static void
 send_escseq(int fd, const char *seq, int n)
 {
     if (write(fd, seq, n) != n) /* maybe fd is read-only */
         printf("%s", seq);
 }

 void activatemap(int fd)
 {
     send_escseq(fd, "\033(K", 3);
 }

So, thus far, not only have I not seen anything go wrong -- the only
program which actually hits this case handles the error gracefully.
Obviously we got lucky here, but the lack of any users of this
mis-feature leads me to have some hope that we can block it without
anyone noticing.

But I emphatically do not want to break userspace here (except for
attackers, obviously).

Hmm. This will break any script that does echo foo >/dev/stdin too.

Just to throw an idea out there, what if the open were allowed if the file mode is sufficient or if the magic link target is openable with the correct mode without magic?  In other words, first check as in your code but without the exception and, if that check fails, then walk the same path that d_path would return and see if it would work as a normal open?  Of course, that second attempt would need to disable magic links to avoid recursing.  I’m not sure I love this idea...

Otherwise, I imagine we can live with the exception, especially if the new open API turns it off by default.