Re: [RFC] Restrict writes into untrusted FIFOs and regular files

[Solar Designer]

On Mon, Sep 18, 2017 at 02:00:50PM -0700, Kees Cook wrote:
> On Fri, Sep 15, 2017 at 1:43 AM, Salvatore Mesoraca <s.mesoraca16@xxxxxxxxx> wrote:
> > The purpose is to make data spoofing attacks harder.
> 
> Do you have any examples of attacks (CVEs, blog posts, etc) that you
> could link to in this commit?

I doubt there are (m)any examples of attacks and blog posts on this,
because most systems didn't have similar (sym)link restrictions until
recently and those attacks are simpler.  As to CVEs, I expect that a
large subset of CVEs assigned to "improper usage of temporary files" or
"symlink attacks" or such are about vulnerabilities that are also
exploitable into data spoofing, even if perhaps with different (and
often lesser) impact than they are when exploited via (sym)links.

IIRC, the attacks were first pointed out on LKML circa 1997 as an
argument against (sym)link restrictions - that they're not enough on
their own against some attacks anyway, because FIFOs and even regular
files could also be used for those.  My response was to include FIFO
restrictions (on top of (sym)link restrictions) into -ow patches, and
now the same became relevant for mainline (as it has the optional
(sym)link restrictions now).

One thing that has changed since is the addition of inotify, which
probably made use of regular files for similar attacks much more
practical.  This is one reason why we might also protect against
maybe-maliciously pre-created regular files now.

Another reason is that we, or at least some users/sysadmins, may want to
discourage misuse of /tmp, /dev/shm, and similar directories in general.
Those directories are supposed to only be accessed through appropriate
library interfaces, or using specific known-safe procedures.  Trying to
O_CREAT a file in there without O_EXCL is almost never right.  It makes
sense to provide an easy way to disallow that as policy enforcement.

> bike shed: I think "_files" is redundant, since we're already in proc/sys/fs/

+1

> > +protected_regular_files:
> > +
> > +This protection is similar to protected_fifos, but it
> > +avoids writes to an attacker-controlled regular file, where program
> > +expected to create one.
> > +
> > +When set to "0", regular files writing is unrestricted.
> > +
> > +When set to "1" don't allow O_CREAT open on regular files that we
> > +don't own in world writable sticky directories, unless they are
> > +owned by the owner of the directory.
> > +
> > +This protection is based on the restrictions in Openwall.

While symlink/hardlink/FIFO restrictions were in -ow patches, the
regular file restriction was not - we're just inventing it now, even if
upon my suggestion.

Although this is sufficient against attacks (if the kernel's check for
these properties is not racy; I don't know if it is), for the policy
enforcement use case and reason we might want to support a simpler mode
where O_CREAT without O_EXCL would be disallowed in sticky directories
(only world writable? or also writable by anyone other than us? - e.g.,
it'd catch some unsafe uses of mail spools) regardless of whether a
file of that name already exists or not.  Maybe extra settings:

When set to "2" also don't allow O_CREAT open without O_EXCL in
world-writable sticky directories (even if the files don't already
exist, for consistent policy enforcement)

When set to "3" also don't allow O_CREAT open on regular files that we
don't own in sticky directories writable by anyone else, unless the
files are owned by the owner of the directory.

When set to "4" also don't allow O_CREAT open without O_EXCL in
sticky directories writable by anyone else (even if the files don't
already exist, for consistent policy enforcement)

Or maybe "2" and "4" should be a separate knob, so that "3" could be
used without the policy enforcement aspect of "2", although enabling
this separate knob at the highest level would make protected_regular
redundant.

I could envision further levels for non-sticky world-writable and
writable-by-others directories, and even for unsafe writes without
O_CREAT and unsafe reads, but then the protected_regular name would
become even more misleading as without O_CREAT the program could
actually intend to work with a non-regular file.

Let's avoid further scope creep for now, but have this in mind.  As I
had mentioned in another thread on kernel-hardening, policy enforcement
like this implemented in a kernel module helped me find weaknesses in
old Postfix' privsep implementation, which were promptly patched (that
was many years ago).  Having this generally available and easy to enable
could result in more findings like this by more people.

A setting similar to "3" above should probably also exist for
protected_fifos (would be "2" there).

> Otherwise, yeah, looks good! :)

+1

Alexander