[kernel-hardening] symlink/hardlink/FIFO restrictions

[kernel-hardening] symlink/hardlink/FIFO restrictions

[Solar Designer]

Hi,

FWIW, I happened to compare the implementations of symlink and hardlink
restrictions in -ow patches vs. grsecurity vs. mainline.  Here are some
observations:

The symlink restrictions in -ow vs. grsecurity look similar to me, but
Kees' patches that eventually got merged upstream introduce an extra
limitation on when the restrictions (do not) apply.  In Kees' patches as
posted in here, it was the "bool sensitive" parameter to follow_link(),
which one of the calls set to false.  In upstream code, the check is now
only in the new trailing_symlink() function, which I guess is called in
similar cases.  As the name suggests, this probably means the symlink
restrictions are only applied to last ("trailing") symlinks in a chain
(and hopefully to symlinks on their own as well, without a chain).
I guess this was suggested at some point, perhaps with some rationale
given, but I didn't watch those many threads closely and missed it.
Why wouldn't a "nested" symlink be used for a successful attack?  The
attacker can then provide their own "trailing" symlink in a non-+t
directory pointing to the ultimate target.  But maybe I misunderstand
what is called "trailing" vs. "nested" in there?

The hardlink restrictions differ between -ow vs. grsecurity and upstream.
In -ow, I had the check in vfs_link().  In grsecurity (oldest I checked
now is grsecurity-2.1.11-2.4.35-200708071800), it's in sys_link() and
later in sys_linkat(), which is also called by sys_link().  Upstream
also has it right in the linkat() syscall function.  Maybe there were
good reasons not to do it in vfs_link(), but I am unaware of those.
We need to know what currently happens and what we'd like to happen for
other callers to vfs_link(), such as kernel nfsd and CRIU - do we want
the restrictions to apply there?  Maybe for some of those other callers,
but not for all?  Do the same checks work correctly when called from
those other contexts or do we need revised checks there?

As to the FIFO restrictions, those don't appear to have been merged
upstream yet.

There are probably many other subtle differences, but I thought I'd post
whatever I happened to notice now.

Alexander

Re: [kernel-hardening] symlink/hardlink/FIFO restrictions

[Solar Designer]

On Tue, Jun 06, 2017 at 11:55:34PM +0200, Solar Designer wrote:
> The symlink restrictions in -ow vs. grsecurity look similar to me, but
> Kees' patches that eventually got merged upstream introduce an extra
> limitation on when the restrictions (do not) apply.  In Kees' patches as
> posted in here, it was the "bool sensitive" parameter to follow_link(),
> which one of the calls set to false.  In upstream code, the check is now
> only in the new trailing_symlink() function, which I guess is called in
> similar cases.  As the name suggests, this probably means the symlink
> restrictions are only applied to last ("trailing") symlinks in a chain
> (and hopefully to symlinks on their own as well, without a chain).
> I guess this was suggested at some point, perhaps with some rationale
> given, but I didn't watch those many threads closely and missed it.
> Why wouldn't a "nested" symlink be used for a successful attack?  The
> attacker can then provide their own "trailing" symlink in a non-+t
> directory pointing to the ultimate target.  But maybe I misunderstand
> what is called "trailing" vs. "nested" in there?

I spoke too soon - should read code rather than derive meaning from
names.  Looks like it's nested vs. trailing components in a pathname.
So we're only protecting against bad symlinks for the last pathname
component - not for upper directories in the path.  Indeed, for typical
vulnerable programs it's the last pathname component that would be
attacked, but I am not sure if it's always the case nor whether we
needed this limitation in this security feature for some desirable uses.

Does this mean symlink attacks against not-yet-created directories like
/tmp/.X11-unix (so perhaps during the system's bootup, maybe when it
already started sshd but not yet X) are still possible even with the
feature enabled?

Alexander

Re: [kernel-hardening] symlink/hardlink/FIFO restrictions

[Solar Designer]

On Wed, Jun 07, 2017 at 12:16:17AM +0200, Solar Designer wrote:
> So we're only protecting against bad symlinks for the last pathname
> component - not for upper directories in the path.  Indeed, for typical
> vulnerable programs it's the last pathname component that would be
> attacked, but I am not sure if it's always the case nor whether we
> needed this limitation in this security feature for some desirable uses.
> 
> Does this mean symlink attacks against not-yet-created directories like
> /tmp/.X11-unix (so perhaps during the system's bootup, maybe when it
> already started sshd but not yet X) are still possible even with the
> feature enabled?

Confirmed this by testing (with a dummy user-owned directory in /tmp).

I think this is actually fine, because we were not protecting against a
variation of those attacks anyway: where the attacker could have created
a non-+t directory of their own in place of /tmp/.X11-unix or the like.
Then symlinks placed in that directory wouldn't be protected anyway.

If we tried to protect against such "directory attacks", it'd be very
similar to the "no unsafe writes" policy (and maybe "no unsafe reads"
too) I mentioned in the thread about TPE here.  (Even though I admit
it's out of scope / off-topic for TPE.)

Alexander

Re: [kernel-hardening] symlink/hardlink/FIFO restrictions

[Kees Cook]

On Tue, Jun 6, 2017 at 3:57 PM, Solar Designer <solar@openwall.com> wrote:
> On Wed, Jun 07, 2017 at 12:16:17AM +0200, Solar Designer wrote:
>> So we're only protecting against bad symlinks for the last pathname
>> component - not for upper directories in the path.  Indeed, for typical
>> vulnerable programs it's the last pathname component that would be
>> attacked, but I am not sure if it's always the case nor whether we
>> needed this limitation in this security feature for some desirable uses.
>>
>> Does this mean symlink attacks against not-yet-created directories like
>> /tmp/.X11-unix (so perhaps during the system's bootup, maybe when it
>> already started sshd but not yet X) are still possible even with the
>> feature enabled?
>
> Confirmed this by testing (with a dummy user-owned directory in /tmp).
>
> I think this is actually fine, because we were not protecting against a
> variation of those attacks anyway: where the attacker could have created
> a non-+t directory of their own in place of /tmp/.X11-unix or the like.
> Then symlinks placed in that directory wouldn't be protected anyway.

IIRC, the "trailing" part was requested by Al Viro in his various
reviews of my port of the defense, and I came to the same conclusion:
we can't really protected the non-+t directory case without some very
different semantics.

I couldn't come up with a situation where the intended defense didn't
work as expected, but maybe I lacked imagination.

-Kees

-- 
Kees Cook
Pixel Security

Re: [kernel-hardening] symlink/hardlink/FIFO restrictions

[Kees Cook]

On Tue, Jun 6, 2017 at 2:55 PM, Solar Designer <solar@openwall.com> wrote:
> The hardlink restrictions differ between -ow vs. grsecurity and upstream.
> In -ow, I had the check in vfs_link().  In grsecurity (oldest I checked
> now is grsecurity-2.1.11-2.4.35-200708071800), it's in sys_link() and
> later in sys_linkat(), which is also called by sys_link().  Upstream
> also has it right in the linkat() syscall function.  Maybe there were
> good reasons not to do it in vfs_link(), but I am unaware of those.
> We need to know what currently happens and what we'd like to happen for
> other callers to vfs_link(), such as kernel nfsd and CRIU - do we want
> the restrictions to apply there?  Maybe for some of those other callers,
> but not for all?  Do the same checks work correctly when called from
> those other contexts or do we need revised checks there?

Hm, yeah, I don't remember sys_linkat() vs vfs_link() coming up during
review, but it's been a while. The nfsd thing is interesting... would
it be right to refuse a client's linkat based on the server's link
restriction sysctl? Hmm.

> As to the FIFO restrictions, those don't appear to have been merged
> upstream yet.

Do the FIFO restrictions exist in -ow too? I wonder what enabling that
protection might break... (better asked as "omg, what puts FIFOs in +t
directories?")

-Kees

-- 
Kees Cook
Pixel Security

Re: [kernel-hardening] symlink/hardlink/FIFO restrictions

[Solar Designer]

On Tue, Jun 06, 2017 at 09:10:43PM -0700, Kees Cook wrote:
> On Tue, Jun 6, 2017 at 2:55 PM, Solar Designer <solar@openwall.com> wrote:
> > The hardlink restrictions differ between -ow vs. grsecurity and upstream.
> > In -ow, I had the check in vfs_link().  In grsecurity (oldest I checked
> > now is grsecurity-2.1.11-2.4.35-200708071800), it's in sys_link() and
> > later in sys_linkat(), which is also called by sys_link().  Upstream
> > also has it right in the linkat() syscall function.  Maybe there were
> > good reasons not to do it in vfs_link(), but I am unaware of those.
> > We need to know what currently happens and what we'd like to happen for
> > other callers to vfs_link(), such as kernel nfsd and CRIU - do we want
> > the restrictions to apply there?  Maybe for some of those other callers,
> > but not for all?  Do the same checks work correctly when called from
> > those other contexts or do we need revised checks there?
> 
> Hm, yeah, I don't remember sys_linkat() vs vfs_link() coming up during
> review, but it's been a while. The nfsd thing is interesting... would
> it be right to refuse a client's linkat based on the server's link
> restriction sysctl? Hmm.

Probably it would, because otherwise we have a bypass in case the
server's filesystem is or will be later also mounted elsewhere.

> > As to the FIFO restrictions, those don't appear to have been merged
> > upstream yet.
> 
> Do the FIFO restrictions exist in -ow too?

Yes, and they pre-date grsecurity too.

> I wonder what enabling that protection might break...

I was adding notes on what broke to the -ow patch FAQ, but it lists
nothing for the FIFOs.  So apparently nothing of note broke, maybe
because the restrictions were limited to what was required (e.g., didn't
apply to FIFOs opened without O_CREAT).

+Restricted FIFOs in /tmp
+CONFIG_HARDEN_FIFO
+  In addition to restricting links, you might also want to restrict
+  writes into untrusted FIFOs (named pipes), to make data spoofing
+  attacks harder. Enabling this option disallows writing into FIFOs
+  not owned by the user in +t directories, unless the owner is the
+  same as that of the directory or the FIFO is opened without the
+  O_CREAT flag.

@@ -1084,6 +1126,32 @@ do_last:
        /*
         * It already exists.
         */
+
+#ifdef CONFIG_HARDEN_FIFO
+       /*
+        * Don't write to FIFOs that we don't own in +t directories,
+        * unless the FIFO is owned by the owner of the directory.
+        *
+        * Do this check early while we hold the directory.
+        */
+       inode = dentry->d_inode;
+       if (S_ISFIFO(inode->i_mode) && !(flag & O_EXCL) &&
+           (dir->d_inode->i_mode & S_ISVTX) &&
+           inode->i_uid != dir->d_inode->i_uid &&
+           current->fsuid != inode->i_uid) {
+               up(&dir->d_inode->i_sem);
+               if (!permission(inode, acc_mode))
+               security_alert("denied writing FIFO of %d.%d "
+                       "by UID %d, EUID %d, process %s:%d",
+                       "writes into a FIFO denied",
+                       inode->i_uid, inode->i_gid,
+                       current->uid, current->euid,
+                       current->comm, current->pid);
+               error = -EACCES;
+               goto exit_dput;
+       }
+#endif

The intent was to detect unintentional writes to FIFOs, where the
program expected to create a regular file with this very same call.

An argument against restricting this is that similar attacks are also
possible via attacker-created regular files, especially now that we have
inotify, although FIFOs made the attacks particularly simple and
reliable (no need to win a race).

Alexander

Re: [kernel-hardening] symlink/hardlink/FIFO restrictions

[Kees Cook]

On Wed, Jun 7, 2017 at 5:06 AM, Solar Designer <solar@openwall.com> wrote:
> On Tue, Jun 06, 2017 at 09:10:43PM -0700, Kees Cook wrote:
>> On Tue, Jun 6, 2017 at 2:55 PM, Solar Designer <solar@openwall.com> wrote:
>> > The hardlink restrictions differ between -ow vs. grsecurity and upstream.
>> > In -ow, I had the check in vfs_link().  In grsecurity (oldest I checked
>> > now is grsecurity-2.1.11-2.4.35-200708071800), it's in sys_link() and
>> > later in sys_linkat(), which is also called by sys_link().  Upstream
>> > also has it right in the linkat() syscall function.  Maybe there were
>> > good reasons not to do it in vfs_link(), but I am unaware of those.
>> > We need to know what currently happens and what we'd like to happen for
>> > other callers to vfs_link(), such as kernel nfsd and CRIU - do we want
>> > the restrictions to apply there?  Maybe for some of those other callers,
>> > but not for all?  Do the same checks work correctly when called from
>> > those other contexts or do we need revised checks there?
>>
>> Hm, yeah, I don't remember sys_linkat() vs vfs_link() coming up during
>> review, but it's been a while. The nfsd thing is interesting... would
>> it be right to refuse a client's linkat based on the server's link
>> restriction sysctl? Hmm.
>
> Probably it would, because otherwise we have a bypass in case the
> server's filesystem is or will be later also mounted elsewhere.
>
>> > As to the FIFO restrictions, those don't appear to have been merged
>> > upstream yet.
>>
>> Do the FIFO restrictions exist in -ow too?
>
> Yes, and they pre-date grsecurity too.
>
>> I wonder what enabling that protection might break...
>
> I was adding notes on what broke to the -ow patch FAQ, but it lists
> nothing for the FIFOs.  So apparently nothing of note broke, maybe
> because the restrictions were limited to what was required (e.g., didn't
> apply to FIFOs opened without O_CREAT).
>
> +Restricted FIFOs in /tmp
> +CONFIG_HARDEN_FIFO
> +  In addition to restricting links, you might also want to restrict
> +  writes into untrusted FIFOs (named pipes), to make data spoofing
> +  attacks harder. Enabling this option disallows writing into FIFOs
> +  not owned by the user in +t directories, unless the owner is the
> +  same as that of the directory or the FIFO is opened without the
> +  O_CREAT flag.
>
> @@ -1084,6 +1126,32 @@ do_last:
>         /*
>          * It already exists.
>          */
> +
> +#ifdef CONFIG_HARDEN_FIFO
> +       /*
> +        * Don't write to FIFOs that we don't own in +t directories,
> +        * unless the FIFO is owned by the owner of the directory.
> +        *
> +        * Do this check early while we hold the directory.
> +        */
> +       inode = dentry->d_inode;
> +       if (S_ISFIFO(inode->i_mode) && !(flag & O_EXCL) &&
> +           (dir->d_inode->i_mode & S_ISVTX) &&
> +           inode->i_uid != dir->d_inode->i_uid &&
> +           current->fsuid != inode->i_uid) {
> +               up(&dir->d_inode->i_sem);
> +               if (!permission(inode, acc_mode))
> +               security_alert("denied writing FIFO of %d.%d "
> +                       "by UID %d, EUID %d, process %s:%d",
> +                       "writes into a FIFO denied",
> +                       inode->i_uid, inode->i_gid,
> +                       current->uid, current->euid,
> +                       current->comm, current->pid);
> +               error = -EACCES;
> +               goto exit_dput;
> +       }
> +#endif
>
> The intent was to detect unintentional writes to FIFOs, where the
> program expected to create a regular file with this very same call.
>
> An argument against restricting this is that similar attacks are also
> possible via attacker-created regular files, especially now that we have
> inotify, although FIFOs made the attacks particularly simple and
> reliable (no need to win a race).

[trying to get back to older emails...]

Looks reasonable. Do you want to put together a patch for this? It
seems like it'd be a useful addition to complement the link
restrictions.

-Kees

-- 
Kees Cook
Pixel Security

Re: [kernel-hardening] symlink/hardlink/FIFO restrictions

[Solar Designer]

On Thu, Jun 22, 2017 at 09:27:10PM -0700, Kees Cook wrote:
> On Wed, Jun 7, 2017 at 5:06 AM, Solar Designer <solar@openwall.com> wrote:
> > On Tue, Jun 06, 2017 at 09:10:43PM -0700, Kees Cook wrote:
> >> On Tue, Jun 6, 2017 at 2:55 PM, Solar Designer <solar@openwall.com> wrote:
> >> > As to the FIFO restrictions, those don't appear to have been merged
> >> > upstream yet.
> >>
> >> Do the FIFO restrictions exist in -ow too?
> >
> > Yes, and they pre-date grsecurity too.
> >
> >> I wonder what enabling that protection might break...
> >
> > I was adding notes on what broke to the -ow patch FAQ, but it lists
> > nothing for the FIFOs.  So apparently nothing of note broke, maybe
> > because the restrictions were limited to what was required (e.g., didn't
> > apply to FIFOs opened without O_CREAT).
> >
> > +Restricted FIFOs in /tmp
> > +CONFIG_HARDEN_FIFO
> > +  In addition to restricting links, you might also want to restrict
> > +  writes into untrusted FIFOs (named pipes), to make data spoofing
> > +  attacks harder. Enabling this option disallows writing into FIFOs
> > +  not owned by the user in +t directories, unless the owner is the
> > +  same as that of the directory or the FIFO is opened without the
> > +  O_CREAT flag.
> >
> > @@ -1084,6 +1126,32 @@ do_last:
> >         /*
> >          * It already exists.
> >          */
> > +
> > +#ifdef CONFIG_HARDEN_FIFO
> > +       /*
> > +        * Don't write to FIFOs that we don't own in +t directories,
> > +        * unless the FIFO is owned by the owner of the directory.
> > +        *
> > +        * Do this check early while we hold the directory.
> > +        */
> > +       inode = dentry->d_inode;
> > +       if (S_ISFIFO(inode->i_mode) && !(flag & O_EXCL) &&
> > +           (dir->d_inode->i_mode & S_ISVTX) &&
> > +           inode->i_uid != dir->d_inode->i_uid &&
> > +           current->fsuid != inode->i_uid) {
> > +               up(&dir->d_inode->i_sem);
> > +               if (!permission(inode, acc_mode))
> > +               security_alert("denied writing FIFO of %d.%d "
> > +                       "by UID %d, EUID %d, process %s:%d",
> > +                       "writes into a FIFO denied",
> > +                       inode->i_uid, inode->i_gid,
> > +                       current->uid, current->euid,
> > +                       current->comm, current->pid);
> > +               error = -EACCES;
> > +               goto exit_dput;
> > +       }
> > +#endif
> >
> > The intent was to detect unintentional writes to FIFOs, where the
> > program expected to create a regular file with this very same call.
> >
> > An argument against restricting this is that similar attacks are also
> > possible via attacker-created regular files, especially now that we have
> > inotify, although FIFOs made the attacks particularly simple and
> > reliable (no need to win a race).
> 
> [trying to get back to older emails...]
> 
> Looks reasonable. Do you want to put together a patch for this?

No, I'd prefer that someone else does.  If a new KSPP contributor does,
then we need to provide some mentoring and be especially careful with
patch review - perhaps you'd help with that.  I lack time and I'm not up
to date with latest kernels.

> It seems like it'd be a useful addition to complement the link
> restrictions.

Yes, although not as useful as it used to be, unless:

Maybe at this time we can also afford a restriction on regular files,
even if perhaps disabled by default?  Disallow O_CREAT without O_EXCL in
+t directories, even for regular files.  A separate knob from the FIFOs,
but sharing code with them (skip the S_ISFIFO() check).

The primary thing I expect this to break is people's manual misuse of
/tmp and the like.  Maybe those trying to get rid of this bad habit will
enable this policy enforcement.  Then these people's use of the systems
and their problem reports will also uncover some issues and hardening
opportunities(*) in programs.

(*) Not every +t directory is world-writable - some are group-writable,
where the issues aren't directly exposed.

Alexander

Re: [kernel-hardening] symlink/hardlink/FIFO restrictions

[Salvatore Mesoraca]

2017-06-24 16:43 GMT+02:00 Solar Designer <solar@openwall.com>:
> On Thu, Jun 22, 2017 at 09:27:10PM -0700, Kees Cook wrote:
>> Looks reasonable. Do you want to put together a patch for this?
>
> No, I'd prefer that someone else does.  If a new KSPP contributor does,
> then we need to provide some mentoring and be especially careful with
> patch review - perhaps you'd help with that.  I lack time and I'm not up
> to date with latest kernels.

Is someone already working on this?
If not I'll gladly take a look at it.

Salvatore

Re: [kernel-hardening] symlink/hardlink/FIFO restrictions

[Kees Cook]

On Wed, Sep 13, 2017 at 8:32 AM, Salvatore Mesoraca
<s.mesoraca16@gmail.com> wrote:
> 2017-06-24 16:43 GMT+02:00 Solar Designer <solar@openwall.com>:
>> On Thu, Jun 22, 2017 at 09:27:10PM -0700, Kees Cook wrote:
>>> Looks reasonable. Do you want to put together a patch for this?
>>
>> No, I'd prefer that someone else does.  If a new KSPP contributor does,
>> then we need to provide some mentoring and be especially careful with
>> patch review - perhaps you'd help with that.  I lack time and I'm not up
>> to date with latest kernels.
>
> Is someone already working on this?
> If not I'll gladly take a look at it.

I'm not aware of anyone else. Please have at it, and let us know where
we can help! :)

-Kees

-- 
Kees Cook
Pixel Security