by Aleksa Sarai

[permalink] [raw]

Subject: Re: [PATCH RFC v8 01/10] namei: obey trailing magic-link DAC permissions

On 2019-05-22, Andy Lutomirski <[email protected]> wrote:
> On Mon, May 20, 2019 at 6:34 AM Aleksa Sarai <[email protected]> wrote:
> > One final exception is given, which is that non-O_PATH file descriptors
> > are given re-open rights equivalent to the permissions available at
> > open-time. This allows for O_RDONLY file descriptors to be re-opened
> > O_RDWR as long as the user had MAY_WRITE access at the time of opening
> > the O_RDONLY descriptor. This is necessary to avoid breaking userspace
> > (some of the kernel's own selftests depended on this "feature").
>
> Can you clarify this exception a bit? I'd like to make sure it's not
> such a huge exception that it invalidates the whole point of the
> patch.

Sure. This exception applies to regular file opens, and the idea is that
the user had permissions to open the file O_RDWR originally (even if
they opened it O_RDONLY) so re-opening it O_RDWR later should not be an
issue (they could've just opened it O_RDWR in the first place). These
permissions still get masked by nd->opath_mask, so opening a magic-link
or including an O_PATH doesn't increase the permission set.

This does mean that an O_RDONLY open (if the user could've done an
O_RDWR and still done the open) results in an FMODE_PATH_WRITE. To be
honest, I'm still on the fence whether this is a great idea or not (and
I'd prefer to not include it). Though, I don't think it invalidates the
patch though, since the attack scenario of a read-only file being
re-opened later as read-write is still blocked.

The main reason for including it is the concern that there is some
program from 1993 running in a basement somewhere that depends on this
that we don't know about. Though, as a counter-example, I have run this
patchset (without this exception) on my laptop for a few days without
any visible issues.

> If you open a file for execute, by actually exec()ing it or by using
> something like the proposed O_MAYEXEC, and you have inode_permission
> to write, do you still end up with FMODE_PATH_WRITE? The code looks
> like it does, and this seems like it might be a mistake.

I'm not sure about the execve(2) example -- after all, you don't get an
fd from execve(2) and /proc/self/exe still has a mode a+rx.

I'm also not sure what the semantics of a hypothetical O_MAYEXEC would
be -- but we'd probably want to discuss re-opening semantics when it
gets included. I would argue that since O_MAYEXEC would likely be merged
after this, no userspace code would depend on this mis-feature and we
could decide to not include FMODE_EXECv2 in the handling of additional
permissions.

As an aside, I did originally implement RESOLVE_UPGRADE_NOEXEC (and the
corresponding FMODE_PATH_EXEC handling). It worked for the most part,
though execveat(AT_EMPTY_PATH) would need some additional changes to do
the may_open_magiclink() checks and I decided against including it here
until we had an O_MAYEXEC.

> Is there any way for user code to read out these new file mode bits?

There is, but it's not exactly trivial. You could check the mode of
/proc/self/fd and then compare it to the acc_mode of the "flags"
/proc/self/fdinfo. The bits present in /proc/self/fd but not in acc_mode
are the FMODE_PATH_* bits.

However, this is quite an ugly way of doing it. I see two options to
make it easier:

1. We can add additional information to fdinfo so it includes that
FMODE_PATH_* bits to indicate how the fd can be upgraded.

2. Previously, only the u bits of the fd mode were used to represent the
open flags. We could add the FMODE_PATH_* permissions to the g bits
and change how the permission check in trailing_symlink() operates.

The really neat thing here is that we could then know for sure which
fmode bits are set during name lookup of a magic-link rather than
assuming they're all FMODE_PATH_* bits.

In addition, userspace that depends on checking the u bits of an fd
mode would continue to work (though I'm not aware of any userspace
code that does depend on this).

Option 2 seems nicer to me in some respects, but it has the additional
cost of making the permission check less obvious (it's no longer an
"inode_permission" and is instead something different with a weird new
set of semantics). Then again, the modes of magic-links weren't obeyed
in the first place so I'd argue these semantics are entirely up for us
to decide.

> 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.

--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

Attachments:

(No filename) (4.84 kB)
signature.asc (849.00 B)
Download all attachments

2019-05-24 03:14:12

by Aleksa Sarai

[permalink] [raw]

Subject: Re: [PATCH RFC v8 01/10] namei: obey trailing magic-link DAC permissions

On 2019-05-23, Aleksa Sarai <[email protected]> wrote:
> On 2019-05-22, Andy Lutomirski <[email protected]> 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).

[1]: http://git.altlinux.org/people/legion/packages/kbd.git

--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

Attachments:

(No filename) (2.14 kB)
signature.asc (849.00 B)
Download all attachments

2019-05-29 15:13:04

by Andy Lutomirski

[permalink] [raw]

Subject: Re: [PATCH RFC v8 01/10] namei: obey trailing magic-link DAC permissions

> On May 23, 2019, at 8:11 PM, Aleksa Sarai <[email protected]> wrote:
>
>> On 2019-05-23, Aleksa Sarai <[email protected]> wrote:
>>> On 2019-05-22, Andy Lutomirski <[email protected]> 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.