Disallows writing into FIFOs or regular files not owned by the user
in world writable sticky directories, unless the owner is the same as
that of the directory or the file is opened without the O_CREAT flag.
The purpose is to make data spoofing attacks harder.
This protection can be turned on and off separately for FIFOs and regular
files via sysctl, just like the symlinks/hardlinks protection.
This patch is based on Openwall's "HARDEN_FIFO" feature by Solar
Designer <solar at openwall.com>.
Suggested-by: Solar Designer <[email protected]>
Suggested-by: Kees Cook <[email protected]>
Signed-off-by: Salvatore Mesoraca <[email protected]>
---
Documentation/sysctl/fs.txt | 34 ++++++++++++++++++++++++++++++++++
fs/namei.c | 23 +++++++++++++++++++++++
include/linux/fs.h | 2 ++
kernel/sysctl.c | 18 ++++++++++++++++++
4 files changed, 77 insertions(+)
diff --git a/Documentation/sysctl/fs.txt b/Documentation/sysctl/fs.txt
index 35e17f7..18e16b7 100644
--- a/Documentation/sysctl/fs.txt
+++ b/Documentation/sysctl/fs.txt
@@ -36,6 +36,8 @@ Currently, these files are in /proc/sys/fs:
- pipe-user-pages-soft
- protected_hardlinks
- protected_symlinks
+- protected_fifos
+- protected_regular_files
- suid_dumpable
- super-max
- super-nr
@@ -222,6 +224,38 @@ This protection is based on the restrictions in Openwall and grsecurity.
==============================================================
+protected_fifos:
+
+The intent of this protection is to avoid unintentional writes to
+an attacker-controlled FIFO, where program expected to create a regular
+file.
+
+When set to "0", FIFOs writing is unrestricted.
+
+When set to "1" don't allow O_CREAT open on FIFOs 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.
+
+==============================================================
+
+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.
+
+==============================================================
+
suid_dumpable:
This value can be used to query and set the core dump mode for setuid
diff --git a/fs/namei.c b/fs/namei.c
index c75ea03..5459dbc 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3225,6 +3225,9 @@ static int lookup_open(struct nameidata *nd, struct path *path,
return error;
}
+int sysctl_protected_fifos __read_mostly;
+int sysctl_protected_regular_files __read_mostly;
+
/*
* Handle the last step of open()
*/
@@ -3354,6 +3357,26 @@ static int do_last(struct nameidata *nd,
seq = 0; /* out of RCU mode, so the value doesn't matter */
inode = d_backing_inode(path.dentry);
+ /*
+ * When this protection is turned on via sysctl:
+ * don't allow "O_CREAT" open on FIFOs or regular files that we
+ * don't own in world writable sticky directories, unless they
+ * are owned by the owner of the directory.
+ */
+ if (((sysctl_protected_fifos && S_ISFIFO(inode->i_mode)) ||
+ (sysctl_protected_regular_files && S_ISREG(inode->i_mode))) &&
+ (dir->d_inode->i_mode & (S_ISVTX|S_IWOTH) == (S_ISVTX|S_IWOTH)) &&
+ !uid_eq(inode->i_uid, dir->d_inode->i_uid) &&
+ !uid_eq(current_fsuid(), inode->i_uid)) {
+ pr_notice_ratelimited("denied writing in file of %d.%d in a sticky directory by UID %d, EUID %d, process %s:%d.",
+ from_kuid(&init_user_ns, inode->i_uid),
+ from_kgid(&init_user_ns, inode->i_gid),
+ from_kuid(&init_user_ns, current_uid()),
+ from_kuid(&init_user_ns, current_euid()),
+ current->comm, current->pid);
+ path_to_nameidata(&path, nd);
+ return -EACCES;
+ }
finish_lookup:
error = step_into(nd, &path, 0, inode, seq);
if (unlikely(error))
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 339e737..591ae87 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -71,6 +71,8 @@
extern int leases_enable, lease_break_time;
extern int sysctl_protected_symlinks;
extern int sysctl_protected_hardlinks;
+extern int sysctl_protected_fifos;
+extern int sysctl_protected_regular_files;
typedef __kernel_rwf_t rwf_t;
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 6648fbb..b18d500 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1807,6 +1807,24 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write,
.extra2 = &one,
},
{
+ .procname = "protected_fifos",
+ .data = &sysctl_protected_fifos,
+ .maxlen = sizeof(int),
+ .mode = 0600,
+ .proc_handler = proc_dointvec_minmax,
+ .extra1 = &zero,
+ .extra2 = &one,
+ },
+ {
+ .procname = "protected_regular_files",
+ .data = &sysctl_protected_regular_files,
+ .maxlen = sizeof(int),
+ .mode = 0600,
+ .proc_handler = proc_dointvec_minmax,
+ .extra1 = &zero,
+ .extra2 = &one,
+ },
+ {
.procname = "suid_dumpable",
.data = &suid_dumpable,
.maxlen = sizeof(int),
--
1.9.1
On Fri, Sep 15, 2017 at 1:43 AM, Salvatore Mesoraca
<[email protected]> wrote:
> Disallows writing into FIFOs or regular files not owned by the user
> in world writable sticky directories, unless the owner is the same as
> that of the directory or the file is opened without the O_CREAT flag.
Thanks for working on this!
> 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?
> This protection can be turned on and off separately for FIFOs and regular
> files via sysctl, just like the symlinks/hardlinks protection.
> This patch is based on Openwall's "HARDEN_FIFO" feature by Solar
> Designer <solar at openwall.com>.
The email address obfuscation can be dropped here, it's already listed
in the "Suggested-by". :)
>
> Suggested-by: Solar Designer <[email protected]>
> Suggested-by: Kees Cook <[email protected]>
> Signed-off-by: Salvatore Mesoraca <[email protected]>
> ---
> Documentation/sysctl/fs.txt | 34 ++++++++++++++++++++++++++++++++++
> fs/namei.c | 23 +++++++++++++++++++++++
> include/linux/fs.h | 2 ++
> kernel/sysctl.c | 18 ++++++++++++++++++
> 4 files changed, 77 insertions(+)
>
> diff --git a/Documentation/sysctl/fs.txt b/Documentation/sysctl/fs.txt
> index 35e17f7..18e16b7 100644
> --- a/Documentation/sysctl/fs.txt
> +++ b/Documentation/sysctl/fs.txt
> @@ -36,6 +36,8 @@ Currently, these files are in /proc/sys/fs:
> - pipe-user-pages-soft
> - protected_hardlinks
> - protected_symlinks
> +- protected_fifos
> +- protected_regular_files
bike shed: I think "_files" is redundant, since we're already in proc/sys/fs/
Also, here and later, keep this list alphabetized (fifos, hardlinks,
regular_files, symlinks).
> - suid_dumpable
> - super-max
> - super-nr
> @@ -222,6 +224,38 @@ This protection is based on the restrictions in Openwall and grsecurity.
>
> ==============================================================
>
> +protected_fifos:
> +
> +The intent of this protection is to avoid unintentional writes to
> +an attacker-controlled FIFO, where program expected to create a regular
> +file.
Typo: "a program" or "programs", instead of "program".
> +
> +When set to "0", FIFOs writing is unrestricted.
> +
> +When set to "1" don't allow O_CREAT open on FIFOs 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.
> +
> +==============================================================
> +
> +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.
> +
> +==============================================================
> +
> suid_dumpable:
>
> This value can be used to query and set the core dump mode for setuid
> diff --git a/fs/namei.c b/fs/namei.c
> index c75ea03..5459dbc 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3225,6 +3225,9 @@ static int lookup_open(struct nameidata *nd, struct path *path,
> return error;
> }
>
> +int sysctl_protected_fifos __read_mostly;
> +int sysctl_protected_regular_files __read_mostly;
> +
> /*
> * Handle the last step of open()
> */
> @@ -3354,6 +3357,26 @@ static int do_last(struct nameidata *nd,
>
> seq = 0; /* out of RCU mode, so the value doesn't matter */
> inode = d_backing_inode(path.dentry);
> + /*
> + * When this protection is turned on via sysctl:
> + * don't allow "O_CREAT" open on FIFOs or regular files that we
> + * don't own in world writable sticky directories, unless they
> + * are owned by the owner of the directory.
> + */
> + if (((sysctl_protected_fifos && S_ISFIFO(inode->i_mode)) ||
> + (sysctl_protected_regular_files && S_ISREG(inode->i_mode))) &&
> + (dir->d_inode->i_mode & (S_ISVTX|S_IWOTH) == (S_ISVTX|S_IWOTH)) &&
> + !uid_eq(inode->i_uid, dir->d_inode->i_uid) &&
> + !uid_eq(current_fsuid(), inode->i_uid)) {
> + pr_notice_ratelimited("denied writing in file of %d.%d in a sticky directory by UID %d, EUID %d, process %s:%d.",
> + from_kuid(&init_user_ns, inode->i_uid),
> + from_kgid(&init_user_ns, inode->i_gid),
> + from_kuid(&init_user_ns, current_uid()),
> + from_kuid(&init_user_ns, current_euid()),
> + current->comm, current->pid);
> + path_to_nameidata(&path, nd);
> + return -EACCES;
> + }
I think instead of putting this inline in do_last(), you can make a
helper function (e.g. may_creat()), as done for the hardlink and
symlink restrictions. Also, I think you'll need to do this check later
and merge with with the existing O_CREAT flag sanity check:
if (open_flag & O_CREAT) {
error = -EISDIR;
if (d_is_dir(nd->path.dentry))
goto out;
error = may_creat(dir, inode);
if (unlikely(error))
goto out;
}
> finish_lookup:
> error = step_into(nd, &path, 0, inode, seq);
> if (unlikely(error))
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 339e737..591ae87 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -71,6 +71,8 @@
> extern int leases_enable, lease_break_time;
> extern int sysctl_protected_symlinks;
> extern int sysctl_protected_hardlinks;
> +extern int sysctl_protected_fifos;
> +extern int sysctl_protected_regular_files;
>
> typedef __kernel_rwf_t rwf_t;
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 6648fbb..b18d500 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1807,6 +1807,24 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write,
> .extra2 = &one,
> },
> {
> + .procname = "protected_fifos",
> + .data = &sysctl_protected_fifos,
> + .maxlen = sizeof(int),
> + .mode = 0600,
> + .proc_handler = proc_dointvec_minmax,
> + .extra1 = &zero,
> + .extra2 = &one,
> + },
> + {
> + .procname = "protected_regular_files",
> + .data = &sysctl_protected_regular_files,
> + .maxlen = sizeof(int),
> + .mode = 0600,
> + .proc_handler = proc_dointvec_minmax,
> + .extra1 = &zero,
> + .extra2 = &one,
> + },
> + {
> .procname = "suid_dumpable",
> .data = &suid_dumpable,
> .maxlen = sizeof(int),
> --
> 1.9.1
>
Otherwise, yeah, looks good! :)
-Kees
--
Kees Cook
Pixel Security
On Mon, Sep 18, 2017 at 02:00:50PM -0700, Kees Cook wrote:
> On Fri, Sep 15, 2017 at 1:43 AM, Salvatore Mesoraca <[email protected]> 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
2017-09-18 23:00 GMT+02:00 Kees Cook <[email protected]>:
> On Fri, Sep 15, 2017 at 1:43 AM, Salvatore Mesoraca
> <[email protected]> wrote:
>> Disallows writing into FIFOs or regular files not owned by the user
>> in world writable sticky directories, unless the owner is the same as
>> that of the directory or the file is opened without the O_CREAT flag.
>
> Thanks for working on this!
Thank you for taking the time to review it!
>> 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'll try to find some examples of CVEs partially fixed by the symlinks/hardlinks
restriction that could be still exploited via FIFOs or regular files.
>> This protection can be turned on and off separately for FIFOs and regular
>> files via sysctl, just like the symlinks/hardlinks protection.
>> This patch is based on Openwall's "HARDEN_FIFO" feature by Solar
>> Designer <solar at openwall.com>.
>
> The email address obfuscation can be dropped here, it's already listed
> in the "Suggested-by". :)
Oops :)
>> Suggested-by: Solar Designer <[email protected]>
>> Suggested-by: Kees Cook <[email protected]>
>> Signed-off-by: Salvatore Mesoraca <[email protected]>
>> ---
>> Documentation/sysctl/fs.txt | 34 ++++++++++++++++++++++++++++++++++
>> fs/namei.c | 23 +++++++++++++++++++++++
>> include/linux/fs.h | 2 ++
>> kernel/sysctl.c | 18 ++++++++++++++++++
>> 4 files changed, 77 insertions(+)
>>
>> diff --git a/Documentation/sysctl/fs.txt b/Documentation/sysctl/fs.txt
>> index 35e17f7..18e16b7 100644
>> --- a/Documentation/sysctl/fs.txt
>> +++ b/Documentation/sysctl/fs.txt
>> @@ -36,6 +36,8 @@ Currently, these files are in /proc/sys/fs:
>> - pipe-user-pages-soft
>> - protected_hardlinks
>> - protected_symlinks
>> +- protected_fifos
>> +- protected_regular_files
>
> bike shed: I think "_files" is redundant, since we're already in proc/sys/fs/
>
> Also, here and later, keep this list alphabetized (fifos, hardlinks,
> regular_files, symlinks).
OK, I'll fix this.
>> - suid_dumpable
>> - super-max
>> - super-nr
>> @@ -222,6 +224,38 @@ This protection is based on the restrictions in Openwall and grsecurity.
>>
>> ==============================================================
>>
>> +protected_fifos:
>> +
>> +The intent of this protection is to avoid unintentional writes to
>> +an attacker-controlled FIFO, where program expected to create a regular
>> +file.
>
> Typo: "a program" or "programs", instead of "program".
Oops 2 :)
>> +
>> +When set to "0", FIFOs writing is unrestricted.
>> +
>> +When set to "1" don't allow O_CREAT open on FIFOs 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.
>> +
>> +==============================================================
>> +
>> +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.
>> +
>> +==============================================================
>> +
>> suid_dumpable:
>>
>> This value can be used to query and set the core dump mode for setuid
>> diff --git a/fs/namei.c b/fs/namei.c
>> index c75ea03..5459dbc 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -3225,6 +3225,9 @@ static int lookup_open(struct nameidata *nd, struct path *path,
>> return error;
>> }
>>
>> +int sysctl_protected_fifos __read_mostly;
>> +int sysctl_protected_regular_files __read_mostly;
>> +
>> /*
>> * Handle the last step of open()
>> */
>> @@ -3354,6 +3357,26 @@ static int do_last(struct nameidata *nd,
>>
>> seq = 0; /* out of RCU mode, so the value doesn't matter */
>> inode = d_backing_inode(path.dentry);
>> + /*
>> + * When this protection is turned on via sysctl:
>> + * don't allow "O_CREAT" open on FIFOs or regular files that we
>> + * don't own in world writable sticky directories, unless they
>> + * are owned by the owner of the directory.
>> + */
>> + if (((sysctl_protected_fifos && S_ISFIFO(inode->i_mode)) ||
>> + (sysctl_protected_regular_files && S_ISREG(inode->i_mode))) &&
>> + (dir->d_inode->i_mode & (S_ISVTX|S_IWOTH) == (S_ISVTX|S_IWOTH)) &&
>> + !uid_eq(inode->i_uid, dir->d_inode->i_uid) &&
>> + !uid_eq(current_fsuid(), inode->i_uid)) {
>> + pr_notice_ratelimited("denied writing in file of %d.%d in a sticky directory by UID %d, EUID %d, process %s:%d.",
>> + from_kuid(&init_user_ns, inode->i_uid),
>> + from_kgid(&init_user_ns, inode->i_gid),
>> + from_kuid(&init_user_ns, current_uid()),
>> + from_kuid(&init_user_ns, current_euid()),
>> + current->comm, current->pid);
>> + path_to_nameidata(&path, nd);
>> + return -EACCES;
>> + }
>
> I think instead of putting this inline in do_last(), you can make a
> helper function (e.g. may_creat()), as done for the hardlink and
> symlink restrictions. Also, I think you'll need to do this check later
> and merge with with the existing O_CREAT flag sanity check:
>
> if (open_flag & O_CREAT) {
> error = -EISDIR;
> if (d_is_dir(nd->path.dentry))
> goto out;
> error = may_creat(dir, inode);
> if (unlikely(error))
> goto out;
> }
>
OK, I'll do this in the next revision.
>> finish_lookup:
>> error = step_into(nd, &path, 0, inode, seq);
>> if (unlikely(error))
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index 339e737..591ae87 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -71,6 +71,8 @@
>> extern int leases_enable, lease_break_time;
>> extern int sysctl_protected_symlinks;
>> extern int sysctl_protected_hardlinks;
>> +extern int sysctl_protected_fifos;
>> +extern int sysctl_protected_regular_files;
>>
>> typedef __kernel_rwf_t rwf_t;
>>
>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> index 6648fbb..b18d500 100644
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -1807,6 +1807,24 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write,
>> .extra2 = &one,
>> },
>> {
>> + .procname = "protected_fifos",
>> + .data = &sysctl_protected_fifos,
>> + .maxlen = sizeof(int),
>> + .mode = 0600,
>> + .proc_handler = proc_dointvec_minmax,
>> + .extra1 = &zero,
>> + .extra2 = &one,
>> + },
>> + {
>> + .procname = "protected_regular_files",
>> + .data = &sysctl_protected_regular_files,
>> + .maxlen = sizeof(int),
>> + .mode = 0600,
>> + .proc_handler = proc_dointvec_minmax,
>> + .extra1 = &zero,
>> + .extra2 = &one,
>> + },
>> + {
>> .procname = "suid_dumpable",
>> .data = &suid_dumpable,
>> .maxlen = sizeof(int),
>> --
>> 1.9.1
>>
>
> Otherwise, yeah, looks good! :)
Great, thank you!
Salvatore
2017-09-19 2:37 GMT+02:00 Solar Designer <[email protected]>:
> On Mon, Sep 18, 2017 at 02:00:50PM -0700, Kees Cook wrote:
>> On Fri, Sep 15, 2017 at 1:43 AM, Salvatore Mesoraca <[email protected]> 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.
Oh OK, I thought it was fair to add that line because the code of the regular
file part is practically identical to the code of the FIFO part, but I'll remove
that line in the next version.
> 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).
I think I could add "3" to both protected_fifos and protected_regulars
actually using "2" for both. And then add another sysctl for modes
"2" and "4" for both fifos and regular files.
>> Otherwise, yeah, looks good! :)
>
> +1
Thank you very much for your time!
Salvatore
On Tue, Sep 19, 2017 at 06:06:15PM +0200, Salvatore Mesoraca wrote:
> 2017-09-19 2:37 GMT+02:00 Solar Designer <[email protected]>:
> > On Mon, Sep 18, 2017 at 02:00:50PM -0700, Kees Cook wrote:
> >> On Fri, Sep 15, 2017 at 1:43 AM, Salvatore Mesoraca <[email protected]> wrote:
> >> > +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.
[...]
> > 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).
>
> I think I could add "3" to both protected_fifos and protected_regulars
> actually using "2" for both. And then add another sysctl for modes
> "2" and "4" for both fifos and regular files.
Sounds good to me. The third sysctl (or several) could be introduced
with a separate patch, focusing on file access safety policy warnings
and enforcement in general rather than on any specific file types.
Alexander