[PATCH] fs-create-and-use-seq_show_option-for-escaping-fix2
The buildbots noticed seq_show_option should be using const parameters.
Signed-off-by: Kees Cook
---
include/linux/seq_file.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index ff4c631348dd..d4c7271382cb 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -155,7 +155,8 @@ static inline struct user_namespace *seq_user_ns(struct
seq_file *seq)
* @name: the mount option name
* @value: the mount option name's value, can be NULL
*/
-static inline void seq_show_option(struct seq_file *m, char *name, char *value)
+static inline void seq_show_option(struct seq_file *m, const char *name,
+ const char *value)
{
seq_putc(m, ',');
seq_escape(m, name, ",= \t\n\\");
--
1.9.1
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] fs-create-and-use-seq_show_option-for-escaping-fix2
The buildbots noticed seq_show_option should be using const parameters.
Signed-off-by: Kees Cook keesc...@chromium.org
---
include/linux/seq_file.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index ff4c631348dd..d4c7271382cb 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -155,7 +155,8 @@ static inline struct user_namespace *seq_user_ns(struct
seq_file *seq)
* @name: the mount option name
* @value: the mount option name's value, can be NULL
*/
-static inline void seq_show_option(struct seq_file *m, char *name, char *value)
+static inline void seq_show_option(struct seq_file *m, const char *name,
+ const char *value)
{
seq_putc(m, ',');
seq_escape(m, name, ,= \t\n\\);
--
1.9.1
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fs: create and use seq_show_option for escaping
On Friday, August 07, 2015 04:41:50 PM Kees Cook wrote: > Many file systems that implement the show_options hook fail to correctly > escape their output which could lead to unescaped characters (e.g. new > lines) leaking into /proc/mounts and /proc/[pid]/mountinfo files. This > could lead to confusion, spoofed entries (resulting in things like > systemd issuing false d-bus "mount" notifications), and who knows > what else. This looks like it would only be the root user stepping on > themselves, but it's possible weird things could happen in containers > or in other situations with delegated mount privileges. > > Here's an example using overlay with setuid fusermount trusting the > contents of /proc/mounts (via the /etc/mtab symlink). Imagine the use of > "sudo" is something more sneaky: > > $ BASE="ovl" > $ MNT="$BASE/mnt" > $ LOW="$BASE/lower" > $ UP="$BASE/upper" > $ WORK="$BASE/work/ 0 0 > none /proc fuse.pwn user_id=1000" > $ mkdir -p "$LOW" "$UP" "$WORK" > $ sudo mount -t overlay -o "lowerdir=$LOW,upperdir=$UP,workdir=$WORK" none > /mnt $ cat /proc/mounts > none /root/ovl/mnt overlay > rw,relatime,lowerdir=ovl/lower,upperdir=ovl/upper,workdir=ovl/work/ 0 0 > none /proc fuse.pwn user_id=1000 0 0 > $ fusermount -u /proc > $ cat /proc/mounts > cat: /proc/mounts: No such file or directory > > This fixes the problem by adding new seq_show_option and seq_show_option_n > helpers, and updating the vulnerable show_option handlers to use them as > needed. Some, like SELinux, need to be open coded due to unusual existing > escape mechanisms. > > Signed-off-by: Kees Cook > Cc: sta...@vger.kernel.org > --- > fs/ceph/super.c | 2 +- > fs/cifs/cifsfs.c | 6 +++--- > fs/ext3/super.c | 4 ++-- > fs/ext4/super.c | 4 ++-- > fs/gfs2/super.c | 6 +++--- > fs/hfs/super.c | 4 ++-- > fs/hfsplus/options.c | 4 ++-- > fs/hostfs/hostfs_kern.c | 2 +- > fs/ocfs2/super.c | 4 ++-- > fs/overlayfs/super.c | 6 +++--- > fs/reiserfs/super.c | 8 +--- > fs/xfs/xfs_super.c | 4 ++-- > include/linux/seq_file.h | 34 ++ > kernel/cgroup.c | 7 --- > net/ceph/ceph_common.c | 7 +-- > security/selinux/hooks.c | 2 +- > 16 files changed, 72 insertions(+), 32 deletions(-) The SELinux changes look fine to me. Acked-by: Paul Moore -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fs: create and use seq_show_option for escaping
On Fri 07-08-15 16:41:50, Kees Cook wrote:
> Many file systems that implement the show_options hook fail to correctly
> escape their output which could lead to unescaped characters (e.g. new
> lines) leaking into /proc/mounts and /proc/[pid]/mountinfo files. This
> could lead to confusion, spoofed entries (resulting in things like
> systemd issuing false d-bus "mount" notifications), and who knows
> what else. This looks like it would only be the root user stepping on
> themselves, but it's possible weird things could happen in containers
> or in other situations with delegated mount privileges.
>
> Here's an example using overlay with setuid fusermount trusting the
> contents of /proc/mounts (via the /etc/mtab symlink). Imagine the use of
> "sudo" is something more sneaky:
>
> $ BASE="ovl"
> $ MNT="$BASE/mnt"
> $ LOW="$BASE/lower"
> $ UP="$BASE/upper"
> $ WORK="$BASE/work/ 0 0
> none /proc fuse.pwn user_id=1000"
> $ mkdir -p "$LOW" "$UP" "$WORK"
> $ sudo mount -t overlay -o "lowerdir=$LOW,upperdir=$UP,workdir=$WORK" none
> /mnt
> $ cat /proc/mounts
> none /root/ovl/mnt overlay
> rw,relatime,lowerdir=ovl/lower,upperdir=ovl/upper,workdir=ovl/work/ 0 0
> none /proc fuse.pwn user_id=1000 0 0
> $ fusermount -u /proc
> $ cat /proc/mounts
> cat: /proc/mounts: No such file or directory
>
> This fixes the problem by adding new seq_show_option and seq_show_option_n
> helpers, and updating the vulnerable show_option handlers to use them as
> needed. Some, like SELinux, need to be open coded due to unusual existing
> escape mechanisms.
>
> Signed-off-by: Kees Cook
> Cc: sta...@vger.kernel.org
Looks good to me. You can add:
Acked-by: Jan Kara
Honza
> ---
> fs/ceph/super.c | 2 +-
> fs/cifs/cifsfs.c | 6 +++---
> fs/ext3/super.c | 4 ++--
> fs/ext4/super.c | 4 ++--
> fs/gfs2/super.c | 6 +++---
> fs/hfs/super.c | 4 ++--
> fs/hfsplus/options.c | 4 ++--
> fs/hostfs/hostfs_kern.c | 2 +-
> fs/ocfs2/super.c | 4 ++--
> fs/overlayfs/super.c | 6 +++---
> fs/reiserfs/super.c | 8 +---
> fs/xfs/xfs_super.c | 4 ++--
> include/linux/seq_file.h | 34 ++
> kernel/cgroup.c | 7 ---
> net/ceph/ceph_common.c | 7 +--
> security/selinux/hooks.c | 2 +-
> 16 files changed, 72 insertions(+), 32 deletions(-)
>
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index d1c833c321b9..7b6bfcbf801c 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -479,7 +479,7 @@ static int ceph_show_options(struct seq_file *m, struct
> dentry *root)
> if (fsopt->max_readdir_bytes != CEPH_MAX_READDIR_BYTES_DEFAULT)
> seq_printf(m, ",readdir_max_bytes=%d",
> fsopt->max_readdir_bytes);
> if (strcmp(fsopt->snapdir_name, CEPH_SNAPDIRNAME_DEFAULT))
> - seq_printf(m, ",snapdirname=%s", fsopt->snapdir_name);
> + seq_show_option(m, "snapdirname", fsopt->snapdir_name);
>
> return 0;
> }
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 0a9fb6b53126..6a1119e87fbb 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -394,17 +394,17 @@ cifs_show_options(struct seq_file *s, struct dentry
> *root)
> struct sockaddr *srcaddr;
> srcaddr = (struct sockaddr *)>ses->server->srcaddr;
>
> - seq_printf(s, ",vers=%s", tcon->ses->server->vals->version_string);
> + seq_show_option(s, "vers", tcon->ses->server->vals->version_string);
> cifs_show_security(s, tcon->ses);
> cifs_show_cache_flavor(s, cifs_sb);
>
> if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER)
> seq_puts(s, ",multiuser");
> else if (tcon->ses->user_name)
> - seq_printf(s, ",username=%s", tcon->ses->user_name);
> + seq_show_option(s, "username", tcon->ses->user_name);
>
> if (tcon->ses->domainName)
> - seq_printf(s, ",domain=%s", tcon->ses->domainName);
> + seq_show_option(s, "domain", tcon->ses->domainName);
>
> if (srcaddr->sa_family != AF_UNSPEC) {
> struct sockaddr_in *saddr4;
> diff --git a/fs/ext3/super.c b/fs/ext3/super.c
> index 5ed0044fbb37..e9312494f3ee 100644
> --- a/fs/ext3/super.c
> +++ b/fs/ext3/super.c
> @@ -578,10 +578,10 @@ static inline void ext3_show_quota_options(struct
> seq_file *seq, struct super_bl
> }
>
> if (sbi->s_qf_names[USRQUOTA])
> - seq_printf(seq, ",usrjquota=%s", sbi->s_qf_names[USRQUOTA]);
> + seq_show_option(seq, "usrjquota", sbi->s_qf_names[USRQUOTA]);
>
> if (sbi->s_qf_names[GRPQUOTA])
> - seq_printf(seq, ",grpjquota=%s", sbi->s_qf_names[GRPQUOTA]);
> + seq_show_option(seq, "grpjquota", sbi->s_qf_names[GRPQUOTA]);
>
> if (test_opt(sb, USRQUOTA))
> seq_puts(seq, ",usrquota");
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index
Re: [PATCH] fs: create and use seq_show_option for escaping
On Friday, August 07, 2015 04:41:50 PM Kees Cook wrote: Many file systems that implement the show_options hook fail to correctly escape their output which could lead to unescaped characters (e.g. new lines) leaking into /proc/mounts and /proc/[pid]/mountinfo files. This could lead to confusion, spoofed entries (resulting in things like systemd issuing false d-bus mount notifications), and who knows what else. This looks like it would only be the root user stepping on themselves, but it's possible weird things could happen in containers or in other situations with delegated mount privileges. Here's an example using overlay with setuid fusermount trusting the contents of /proc/mounts (via the /etc/mtab symlink). Imagine the use of sudo is something more sneaky: $ BASE=ovl $ MNT=$BASE/mnt $ LOW=$BASE/lower $ UP=$BASE/upper $ WORK=$BASE/work/ 0 0 none /proc fuse.pwn user_id=1000 $ mkdir -p $LOW $UP $WORK $ sudo mount -t overlay -o lowerdir=$LOW,upperdir=$UP,workdir=$WORK none /mnt $ cat /proc/mounts none /root/ovl/mnt overlay rw,relatime,lowerdir=ovl/lower,upperdir=ovl/upper,workdir=ovl/work/ 0 0 none /proc fuse.pwn user_id=1000 0 0 $ fusermount -u /proc $ cat /proc/mounts cat: /proc/mounts: No such file or directory This fixes the problem by adding new seq_show_option and seq_show_option_n helpers, and updating the vulnerable show_option handlers to use them as needed. Some, like SELinux, need to be open coded due to unusual existing escape mechanisms. Signed-off-by: Kees Cook keesc...@chromium.org Cc: sta...@vger.kernel.org --- fs/ceph/super.c | 2 +- fs/cifs/cifsfs.c | 6 +++--- fs/ext3/super.c | 4 ++-- fs/ext4/super.c | 4 ++-- fs/gfs2/super.c | 6 +++--- fs/hfs/super.c | 4 ++-- fs/hfsplus/options.c | 4 ++-- fs/hostfs/hostfs_kern.c | 2 +- fs/ocfs2/super.c | 4 ++-- fs/overlayfs/super.c | 6 +++--- fs/reiserfs/super.c | 8 +--- fs/xfs/xfs_super.c | 4 ++-- include/linux/seq_file.h | 34 ++ kernel/cgroup.c | 7 --- net/ceph/ceph_common.c | 7 +-- security/selinux/hooks.c | 2 +- 16 files changed, 72 insertions(+), 32 deletions(-) The SELinux changes look fine to me. Acked-by: Paul Moore p...@paul-moore.com -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fs: create and use seq_show_option for escaping
On Fri 07-08-15 16:41:50, Kees Cook wrote:
Many file systems that implement the show_options hook fail to correctly
escape their output which could lead to unescaped characters (e.g. new
lines) leaking into /proc/mounts and /proc/[pid]/mountinfo files. This
could lead to confusion, spoofed entries (resulting in things like
systemd issuing false d-bus mount notifications), and who knows
what else. This looks like it would only be the root user stepping on
themselves, but it's possible weird things could happen in containers
or in other situations with delegated mount privileges.
Here's an example using overlay with setuid fusermount trusting the
contents of /proc/mounts (via the /etc/mtab symlink). Imagine the use of
sudo is something more sneaky:
$ BASE=ovl
$ MNT=$BASE/mnt
$ LOW=$BASE/lower
$ UP=$BASE/upper
$ WORK=$BASE/work/ 0 0
none /proc fuse.pwn user_id=1000
$ mkdir -p $LOW $UP $WORK
$ sudo mount -t overlay -o lowerdir=$LOW,upperdir=$UP,workdir=$WORK none
/mnt
$ cat /proc/mounts
none /root/ovl/mnt overlay
rw,relatime,lowerdir=ovl/lower,upperdir=ovl/upper,workdir=ovl/work/ 0 0
none /proc fuse.pwn user_id=1000 0 0
$ fusermount -u /proc
$ cat /proc/mounts
cat: /proc/mounts: No such file or directory
This fixes the problem by adding new seq_show_option and seq_show_option_n
helpers, and updating the vulnerable show_option handlers to use them as
needed. Some, like SELinux, need to be open coded due to unusual existing
escape mechanisms.
Signed-off-by: Kees Cook keesc...@chromium.org
Cc: sta...@vger.kernel.org
Looks good to me. You can add:
Acked-by: Jan Kara j...@suse.com
Honza
---
fs/ceph/super.c | 2 +-
fs/cifs/cifsfs.c | 6 +++---
fs/ext3/super.c | 4 ++--
fs/ext4/super.c | 4 ++--
fs/gfs2/super.c | 6 +++---
fs/hfs/super.c | 4 ++--
fs/hfsplus/options.c | 4 ++--
fs/hostfs/hostfs_kern.c | 2 +-
fs/ocfs2/super.c | 4 ++--
fs/overlayfs/super.c | 6 +++---
fs/reiserfs/super.c | 8 +---
fs/xfs/xfs_super.c | 4 ++--
include/linux/seq_file.h | 34 ++
kernel/cgroup.c | 7 ---
net/ceph/ceph_common.c | 7 +--
security/selinux/hooks.c | 2 +-
16 files changed, 72 insertions(+), 32 deletions(-)
diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index d1c833c321b9..7b6bfcbf801c 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -479,7 +479,7 @@ static int ceph_show_options(struct seq_file *m, struct
dentry *root)
if (fsopt-max_readdir_bytes != CEPH_MAX_READDIR_BYTES_DEFAULT)
seq_printf(m, ,readdir_max_bytes=%d,
fsopt-max_readdir_bytes);
if (strcmp(fsopt-snapdir_name, CEPH_SNAPDIRNAME_DEFAULT))
- seq_printf(m, ,snapdirname=%s, fsopt-snapdir_name);
+ seq_show_option(m, snapdirname, fsopt-snapdir_name);
return 0;
}
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 0a9fb6b53126..6a1119e87fbb 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -394,17 +394,17 @@ cifs_show_options(struct seq_file *s, struct dentry
*root)
struct sockaddr *srcaddr;
srcaddr = (struct sockaddr *)tcon-ses-server-srcaddr;
- seq_printf(s, ,vers=%s, tcon-ses-server-vals-version_string);
+ seq_show_option(s, vers, tcon-ses-server-vals-version_string);
cifs_show_security(s, tcon-ses);
cifs_show_cache_flavor(s, cifs_sb);
if (cifs_sb-mnt_cifs_flags CIFS_MOUNT_MULTIUSER)
seq_puts(s, ,multiuser);
else if (tcon-ses-user_name)
- seq_printf(s, ,username=%s, tcon-ses-user_name);
+ seq_show_option(s, username, tcon-ses-user_name);
if (tcon-ses-domainName)
- seq_printf(s, ,domain=%s, tcon-ses-domainName);
+ seq_show_option(s, domain, tcon-ses-domainName);
if (srcaddr-sa_family != AF_UNSPEC) {
struct sockaddr_in *saddr4;
diff --git a/fs/ext3/super.c b/fs/ext3/super.c
index 5ed0044fbb37..e9312494f3ee 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -578,10 +578,10 @@ static inline void ext3_show_quota_options(struct
seq_file *seq, struct super_bl
}
if (sbi-s_qf_names[USRQUOTA])
- seq_printf(seq, ,usrjquota=%s, sbi-s_qf_names[USRQUOTA]);
+ seq_show_option(seq, usrjquota, sbi-s_qf_names[USRQUOTA]);
if (sbi-s_qf_names[GRPQUOTA])
- seq_printf(seq, ,grpjquota=%s, sbi-s_qf_names[GRPQUOTA]);
+ seq_show_option(seq, grpjquota, sbi-s_qf_names[GRPQUOTA]);
if (test_opt(sb, USRQUOTA))
seq_puts(seq, ,usrquota);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 58987b5c514b..9981064c4a54 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1763,10 +1763,10 @@ static inline void ext4_show_quota_options(struct
seq_file
Re: [PATCH] fs: create and use seq_show_option for escaping
On Sat, Aug 8, 2015 at 9:41 AM, J. R. Okajima wrote: > > Kees Cook: >> This fixes the problem by adding new seq_show_option and seq_show_option_n >> helpers, and updating the vulnerable show_option handlers to use them as >> needed. Some, like SELinux, need to be open coded due to unusual existing >> escape mechanisms. > > How about other ctrl chars such as CR or FF? > I am using the similar function for many years, and it might be more > generic because it supports all cntrl chars other than "\t\n\\" (see > below). > > Many of other ctrl chars may not be necessary. But some people uses > non-ASCII chars for their pathnames which may contain ESC or other > chars. Any crazy chars can corrupt the output of /proc/mount and > others. So it might be better to consider all ctrl chars. While that's generally true, it's the consumers of mounts and mountinfo that we need to worry about, and for those, it's only the delimiters that we need to be concerned about (space, tab, newline, and escapes). However, since these consumers are expecting to deal with escapes, we could, at any time, add new classes of characters to be escaped. For this change, though, I wanted to keep it minimal for easiest backporting. -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fs: create and use seq_show_option for escaping
Kees Cook:
> This fixes the problem by adding new seq_show_option and seq_show_option_n
> helpers, and updating the vulnerable show_option handlers to use them as
> needed. Some, like SELinux, need to be open coded due to unusual existing
> escape mechanisms.
How about other ctrl chars such as CR or FF?
I am using the similar function for many years, and it might be more
generic because it supports all cntrl chars other than "\t\n\\" (see
below).
Many of other ctrl chars may not be necessary. But some people uses
non-ASCII chars for their pathnames which may contain ESC or other
chars. Any crazy chars can corrupt the output of /proc/mount and
others. So it might be better to consider all ctrl chars.
--
static char au_esc_chars[0x20 + 3]; /* 0x01-0x20, backslash, del, and NULL */
int au_seq_path(struct seq_file *seq, struct path *path)
{
return seq_path(seq, path, au_esc_chars);
}
module_init(void)
{
:::
p = au_esc_chars;
for (i = 1; i <= ' '; i++)
*p++ = i;
*p++ = '\\';
*p++ = '\x7f';
*p = 0;
:::
}
J. R. Okajima
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fs: create and use seq_show_option for escaping
Kees Cook:
This fixes the problem by adding new seq_show_option and seq_show_option_n
helpers, and updating the vulnerable show_option handlers to use them as
needed. Some, like SELinux, need to be open coded due to unusual existing
escape mechanisms.
How about other ctrl chars such as CR or FF?
I am using the similar function for many years, and it might be more
generic because it supports all cntrl chars other than \t\n\\ (see
below).
Many of other ctrl chars may not be necessary. But some people uses
non-ASCII chars for their pathnames which may contain ESC or other
chars. Any crazy chars can corrupt the output of /proc/mount and
others. So it might be better to consider all ctrl chars.
--
static char au_esc_chars[0x20 + 3]; /* 0x01-0x20, backslash, del, and NULL */
int au_seq_path(struct seq_file *seq, struct path *path)
{
return seq_path(seq, path, au_esc_chars);
}
module_init(void)
{
:::
p = au_esc_chars;
for (i = 1; i = ' '; i++)
*p++ = i;
*p++ = '\\';
*p++ = '\x7f';
*p = 0;
:::
}
J. R. Okajima
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fs: create and use seq_show_option for escaping
On Sat, Aug 8, 2015 at 9:41 AM, J. R. Okajima hooanon...@gmail.com wrote: Kees Cook: This fixes the problem by adding new seq_show_option and seq_show_option_n helpers, and updating the vulnerable show_option handlers to use them as needed. Some, like SELinux, need to be open coded due to unusual existing escape mechanisms. How about other ctrl chars such as CR or FF? I am using the similar function for many years, and it might be more generic because it supports all cntrl chars other than \t\n\\ (see below). Many of other ctrl chars may not be necessary. But some people uses non-ASCII chars for their pathnames which may contain ESC or other chars. Any crazy chars can corrupt the output of /proc/mount and others. So it might be better to consider all ctrl chars. While that's generally true, it's the consumers of mounts and mountinfo that we need to worry about, and for those, it's only the delimiters that we need to be concerned about (space, tab, newline, and escapes). However, since these consumers are expecting to deal with escapes, we could, at any time, add new classes of characters to be escaped. For this change, though, I wanted to keep it minimal for easiest backporting. -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fs: create and use seq_show_option for escaping
On Fri, Aug 07, 2015 at 04:41:50PM -0700, Kees Cook wrote:
> Many file systems that implement the show_options hook fail to correctly
> escape their output which could lead to unescaped characters (e.g. new
> lines) leaking into /proc/mounts and /proc/[pid]/mountinfo files. This
> could lead to confusion, spoofed entries (resulting in things like
> systemd issuing false d-bus "mount" notifications), and who knows
> what else. This looks like it would only be the root user stepping on
> themselves, but it's possible weird things could happen in containers
> or in other situations with delegated mount privileges.
>
> Here's an example using overlay with setuid fusermount trusting the
> contents of /proc/mounts (via the /etc/mtab symlink). Imagine the use of
> "sudo" is something more sneaky:
>
> $ BASE="ovl"
> $ MNT="$BASE/mnt"
> $ LOW="$BASE/lower"
> $ UP="$BASE/upper"
> $ WORK="$BASE/work/ 0 0
> none /proc fuse.pwn user_id=1000"
> $ mkdir -p "$LOW" "$UP" "$WORK"
> $ sudo mount -t overlay -o "lowerdir=$LOW,upperdir=$UP,workdir=$WORK" none
> /mnt
> $ cat /proc/mounts
> none /root/ovl/mnt overlay
> rw,relatime,lowerdir=ovl/lower,upperdir=ovl/upper,workdir=ovl/work/ 0 0
> none /proc fuse.pwn user_id=1000 0 0
> $ fusermount -u /proc
> $ cat /proc/mounts
> cat: /proc/mounts: No such file or directory
>
> This fixes the problem by adding new seq_show_option and seq_show_option_n
> helpers, and updating the vulnerable show_option handlers to use them as
> needed. Some, like SELinux, need to be open coded due to unusual existing
> escape mechanisms.
>
> Signed-off-by: Kees Cook
> Cc: sta...@vger.kernel.org
Acked-by: Serge Hallyn
> ---
> fs/ceph/super.c | 2 +-
> fs/cifs/cifsfs.c | 6 +++---
> fs/ext3/super.c | 4 ++--
> fs/ext4/super.c | 4 ++--
> fs/gfs2/super.c | 6 +++---
> fs/hfs/super.c | 4 ++--
> fs/hfsplus/options.c | 4 ++--
> fs/hostfs/hostfs_kern.c | 2 +-
> fs/ocfs2/super.c | 4 ++--
> fs/overlayfs/super.c | 6 +++---
> fs/reiserfs/super.c | 8 +---
> fs/xfs/xfs_super.c | 4 ++--
> include/linux/seq_file.h | 34 ++
> kernel/cgroup.c | 7 ---
> net/ceph/ceph_common.c | 7 +--
> security/selinux/hooks.c | 2 +-
> 16 files changed, 72 insertions(+), 32 deletions(-)
>
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index d1c833c321b9..7b6bfcbf801c 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -479,7 +479,7 @@ static int ceph_show_options(struct seq_file *m, struct
> dentry *root)
> if (fsopt->max_readdir_bytes != CEPH_MAX_READDIR_BYTES_DEFAULT)
> seq_printf(m, ",readdir_max_bytes=%d",
> fsopt->max_readdir_bytes);
> if (strcmp(fsopt->snapdir_name, CEPH_SNAPDIRNAME_DEFAULT))
> - seq_printf(m, ",snapdirname=%s", fsopt->snapdir_name);
> + seq_show_option(m, "snapdirname", fsopt->snapdir_name);
>
> return 0;
> }
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 0a9fb6b53126..6a1119e87fbb 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -394,17 +394,17 @@ cifs_show_options(struct seq_file *s, struct dentry
> *root)
> struct sockaddr *srcaddr;
> srcaddr = (struct sockaddr *)>ses->server->srcaddr;
>
> - seq_printf(s, ",vers=%s", tcon->ses->server->vals->version_string);
> + seq_show_option(s, "vers", tcon->ses->server->vals->version_string);
> cifs_show_security(s, tcon->ses);
> cifs_show_cache_flavor(s, cifs_sb);
>
> if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER)
> seq_puts(s, ",multiuser");
> else if (tcon->ses->user_name)
> - seq_printf(s, ",username=%s", tcon->ses->user_name);
> + seq_show_option(s, "username", tcon->ses->user_name);
>
> if (tcon->ses->domainName)
> - seq_printf(s, ",domain=%s", tcon->ses->domainName);
> + seq_show_option(s, "domain", tcon->ses->domainName);
>
> if (srcaddr->sa_family != AF_UNSPEC) {
> struct sockaddr_in *saddr4;
> diff --git a/fs/ext3/super.c b/fs/ext3/super.c
> index 5ed0044fbb37..e9312494f3ee 100644
> --- a/fs/ext3/super.c
> +++ b/fs/ext3/super.c
> @@ -578,10 +578,10 @@ static inline void ext3_show_quota_options(struct
> seq_file *seq, struct super_bl
> }
>
> if (sbi->s_qf_names[USRQUOTA])
> - seq_printf(seq, ",usrjquota=%s", sbi->s_qf_names[USRQUOTA]);
> + seq_show_option(seq, "usrjquota", sbi->s_qf_names[USRQUOTA]);
>
> if (sbi->s_qf_names[GRPQUOTA])
> - seq_printf(seq, ",grpjquota=%s", sbi->s_qf_names[GRPQUOTA]);
> + seq_show_option(seq, "grpjquota", sbi->s_qf_names[GRPQUOTA]);
>
> if (test_opt(sb, USRQUOTA))
> seq_puts(seq, ",usrquota");
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 58987b5c514b..9981064c4a54 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
>
Re: [PATCH] fs: create and use seq_show_option for escaping
On Fri, Aug 7, 2015 at 4:41 PM, Kees Cook wrote:
> Many file systems that implement the show_options hook fail to correctly
> escape their output which could lead to unescaped characters (e.g. new
> lines) leaking into /proc/mounts and /proc/[pid]/mountinfo files. This
> could lead to confusion, spoofed entries (resulting in things like
> systemd issuing false d-bus "mount" notifications), and who knows
> what else. This looks like it would only be the root user stepping on
> themselves, but it's possible weird things could happen in containers
> or in other situations with delegated mount privileges.
>
> Here's an example using overlay with setuid fusermount trusting the
> contents of /proc/mounts (via the /etc/mtab symlink). Imagine the use of
> "sudo" is something more sneaky:
>
> $ BASE="ovl"
> $ MNT="$BASE/mnt"
> $ LOW="$BASE/lower"
> $ UP="$BASE/upper"
> $ WORK="$BASE/work/ 0 0
> none /proc fuse.pwn user_id=1000"
> $ mkdir -p "$LOW" "$UP" "$WORK"
> $ sudo mount -t overlay -o "lowerdir=$LOW,upperdir=$UP,workdir=$WORK" none
> /mnt
> $ cat /proc/mounts
> none /root/ovl/mnt overlay
> rw,relatime,lowerdir=ovl/lower,upperdir=ovl/upper,workdir=ovl/work/ 0 0
> none /proc fuse.pwn user_id=1000 0 0
> $ fusermount -u /proc
> $ cat /proc/mounts
> cat: /proc/mounts: No such file or directory
>
> This fixes the problem by adding new seq_show_option and seq_show_option_n
> helpers, and updating the vulnerable show_option handlers to use them as
> needed. Some, like SELinux, need to be open coded due to unusual existing
> escape mechanisms.
>
> Signed-off-by: Kees Cook
> Cc: sta...@vger.kernel.org
> ---
> fs/ceph/super.c | 2 +-
> fs/cifs/cifsfs.c | 6 +++---
> fs/ext3/super.c | 4 ++--
> fs/ext4/super.c | 4 ++--
> fs/gfs2/super.c | 6 +++---
> fs/hfs/super.c | 4 ++--
> fs/hfsplus/options.c | 4 ++--
> fs/hostfs/hostfs_kern.c | 2 +-
> fs/ocfs2/super.c | 4 ++--
> fs/overlayfs/super.c | 6 +++---
> fs/reiserfs/super.c | 8 +---
> fs/xfs/xfs_super.c | 4 ++--
> include/linux/seq_file.h | 34 ++
> kernel/cgroup.c | 7 ---
> net/ceph/ceph_common.c | 7 +--
> security/selinux/hooks.c | 2 +-
> 16 files changed, 72 insertions(+), 32 deletions(-)
>
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index d1c833c321b9..7b6bfcbf801c 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -479,7 +479,7 @@ static int ceph_show_options(struct seq_file *m, struct
> dentry *root)
> if (fsopt->max_readdir_bytes != CEPH_MAX_READDIR_BYTES_DEFAULT)
> seq_printf(m, ",readdir_max_bytes=%d",
> fsopt->max_readdir_bytes);
> if (strcmp(fsopt->snapdir_name, CEPH_SNAPDIRNAME_DEFAULT))
> - seq_printf(m, ",snapdirname=%s", fsopt->snapdir_name);
> + seq_show_option(m, "snapdirname", fsopt->snapdir_name);
>
> return 0;
> }
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 0a9fb6b53126..6a1119e87fbb 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -394,17 +394,17 @@ cifs_show_options(struct seq_file *s, struct dentry
> *root)
> struct sockaddr *srcaddr;
> srcaddr = (struct sockaddr *)>ses->server->srcaddr;
>
> - seq_printf(s, ",vers=%s", tcon->ses->server->vals->version_string);
> + seq_show_option(s, "vers", tcon->ses->server->vals->version_string);
> cifs_show_security(s, tcon->ses);
> cifs_show_cache_flavor(s, cifs_sb);
>
> if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER)
> seq_puts(s, ",multiuser");
> else if (tcon->ses->user_name)
> - seq_printf(s, ",username=%s", tcon->ses->user_name);
> + seq_show_option(s, "username", tcon->ses->user_name);
>
> if (tcon->ses->domainName)
> - seq_printf(s, ",domain=%s", tcon->ses->domainName);
> + seq_show_option(s, "domain", tcon->ses->domainName);
>
> if (srcaddr->sa_family != AF_UNSPEC) {
> struct sockaddr_in *saddr4;
> diff --git a/fs/ext3/super.c b/fs/ext3/super.c
> index 5ed0044fbb37..e9312494f3ee 100644
> --- a/fs/ext3/super.c
> +++ b/fs/ext3/super.c
> @@ -578,10 +578,10 @@ static inline void ext3_show_quota_options(struct
> seq_file *seq, struct super_bl
> }
>
> if (sbi->s_qf_names[USRQUOTA])
> - seq_printf(seq, ",usrjquota=%s", sbi->s_qf_names[USRQUOTA]);
> + seq_show_option(seq, "usrjquota", sbi->s_qf_names[USRQUOTA]);
>
> if (sbi->s_qf_names[GRPQUOTA])
> - seq_printf(seq, ",grpjquota=%s", sbi->s_qf_names[GRPQUOTA]);
> + seq_show_option(seq, "grpjquota", sbi->s_qf_names[GRPQUOTA]);
>
> if (test_opt(sb, USRQUOTA))
> seq_puts(seq, ",usrquota");
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 58987b5c514b..9981064c4a54 100644
> --- a/fs/ext4/super.c
> +++
[PATCH] fs: create and use seq_show_option for escaping
Many file systems that implement the show_options hook fail to correctly
escape their output which could lead to unescaped characters (e.g. new
lines) leaking into /proc/mounts and /proc/[pid]/mountinfo files. This
could lead to confusion, spoofed entries (resulting in things like
systemd issuing false d-bus "mount" notifications), and who knows
what else. This looks like it would only be the root user stepping on
themselves, but it's possible weird things could happen in containers
or in other situations with delegated mount privileges.
Here's an example using overlay with setuid fusermount trusting the
contents of /proc/mounts (via the /etc/mtab symlink). Imagine the use of
"sudo" is something more sneaky:
$ BASE="ovl"
$ MNT="$BASE/mnt"
$ LOW="$BASE/lower"
$ UP="$BASE/upper"
$ WORK="$BASE/work/ 0 0
none /proc fuse.pwn user_id=1000"
$ mkdir -p "$LOW" "$UP" "$WORK"
$ sudo mount -t overlay -o "lowerdir=$LOW,upperdir=$UP,workdir=$WORK" none /mnt
$ cat /proc/mounts
none /root/ovl/mnt overlay
rw,relatime,lowerdir=ovl/lower,upperdir=ovl/upper,workdir=ovl/work/ 0 0
none /proc fuse.pwn user_id=1000 0 0
$ fusermount -u /proc
$ cat /proc/mounts
cat: /proc/mounts: No such file or directory
This fixes the problem by adding new seq_show_option and seq_show_option_n
helpers, and updating the vulnerable show_option handlers to use them as
needed. Some, like SELinux, need to be open coded due to unusual existing
escape mechanisms.
Signed-off-by: Kees Cook
Cc: sta...@vger.kernel.org
---
fs/ceph/super.c | 2 +-
fs/cifs/cifsfs.c | 6 +++---
fs/ext3/super.c | 4 ++--
fs/ext4/super.c | 4 ++--
fs/gfs2/super.c | 6 +++---
fs/hfs/super.c | 4 ++--
fs/hfsplus/options.c | 4 ++--
fs/hostfs/hostfs_kern.c | 2 +-
fs/ocfs2/super.c | 4 ++--
fs/overlayfs/super.c | 6 +++---
fs/reiserfs/super.c | 8 +---
fs/xfs/xfs_super.c | 4 ++--
include/linux/seq_file.h | 34 ++
kernel/cgroup.c | 7 ---
net/ceph/ceph_common.c | 7 +--
security/selinux/hooks.c | 2 +-
16 files changed, 72 insertions(+), 32 deletions(-)
diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index d1c833c321b9..7b6bfcbf801c 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -479,7 +479,7 @@ static int ceph_show_options(struct seq_file *m, struct
dentry *root)
if (fsopt->max_readdir_bytes != CEPH_MAX_READDIR_BYTES_DEFAULT)
seq_printf(m, ",readdir_max_bytes=%d",
fsopt->max_readdir_bytes);
if (strcmp(fsopt->snapdir_name, CEPH_SNAPDIRNAME_DEFAULT))
- seq_printf(m, ",snapdirname=%s", fsopt->snapdir_name);
+ seq_show_option(m, "snapdirname", fsopt->snapdir_name);
return 0;
}
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 0a9fb6b53126..6a1119e87fbb 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -394,17 +394,17 @@ cifs_show_options(struct seq_file *s, struct dentry *root)
struct sockaddr *srcaddr;
srcaddr = (struct sockaddr *)>ses->server->srcaddr;
- seq_printf(s, ",vers=%s", tcon->ses->server->vals->version_string);
+ seq_show_option(s, "vers", tcon->ses->server->vals->version_string);
cifs_show_security(s, tcon->ses);
cifs_show_cache_flavor(s, cifs_sb);
if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER)
seq_puts(s, ",multiuser");
else if (tcon->ses->user_name)
- seq_printf(s, ",username=%s", tcon->ses->user_name);
+ seq_show_option(s, "username", tcon->ses->user_name);
if (tcon->ses->domainName)
- seq_printf(s, ",domain=%s", tcon->ses->domainName);
+ seq_show_option(s, "domain", tcon->ses->domainName);
if (srcaddr->sa_family != AF_UNSPEC) {
struct sockaddr_in *saddr4;
diff --git a/fs/ext3/super.c b/fs/ext3/super.c
index 5ed0044fbb37..e9312494f3ee 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -578,10 +578,10 @@ static inline void ext3_show_quota_options(struct
seq_file *seq, struct super_bl
}
if (sbi->s_qf_names[USRQUOTA])
- seq_printf(seq, ",usrjquota=%s", sbi->s_qf_names[USRQUOTA]);
+ seq_show_option(seq, "usrjquota", sbi->s_qf_names[USRQUOTA]);
if (sbi->s_qf_names[GRPQUOTA])
- seq_printf(seq, ",grpjquota=%s", sbi->s_qf_names[GRPQUOTA]);
+ seq_show_option(seq, "grpjquota", sbi->s_qf_names[GRPQUOTA]);
if (test_opt(sb, USRQUOTA))
seq_puts(seq, ",usrquota");
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 58987b5c514b..9981064c4a54 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1763,10 +1763,10 @@ static inline void ext4_show_quota_options(struct
seq_file *seq,
}
if (sbi->s_qf_names[USRQUOTA])
- seq_printf(seq, ",usrjquota=%s", sbi->s_qf_names[USRQUOTA]);
+ seq_show_option(seq,
[PATCH] fs: create and use seq_show_option for escaping
Many file systems that implement the show_options hook fail to correctly
escape their output which could lead to unescaped characters (e.g. new
lines) leaking into /proc/mounts and /proc/[pid]/mountinfo files. This
could lead to confusion, spoofed entries (resulting in things like
systemd issuing false d-bus mount notifications), and who knows
what else. This looks like it would only be the root user stepping on
themselves, but it's possible weird things could happen in containers
or in other situations with delegated mount privileges.
Here's an example using overlay with setuid fusermount trusting the
contents of /proc/mounts (via the /etc/mtab symlink). Imagine the use of
sudo is something more sneaky:
$ BASE=ovl
$ MNT=$BASE/mnt
$ LOW=$BASE/lower
$ UP=$BASE/upper
$ WORK=$BASE/work/ 0 0
none /proc fuse.pwn user_id=1000
$ mkdir -p $LOW $UP $WORK
$ sudo mount -t overlay -o lowerdir=$LOW,upperdir=$UP,workdir=$WORK none /mnt
$ cat /proc/mounts
none /root/ovl/mnt overlay
rw,relatime,lowerdir=ovl/lower,upperdir=ovl/upper,workdir=ovl/work/ 0 0
none /proc fuse.pwn user_id=1000 0 0
$ fusermount -u /proc
$ cat /proc/mounts
cat: /proc/mounts: No such file or directory
This fixes the problem by adding new seq_show_option and seq_show_option_n
helpers, and updating the vulnerable show_option handlers to use them as
needed. Some, like SELinux, need to be open coded due to unusual existing
escape mechanisms.
Signed-off-by: Kees Cook keesc...@chromium.org
Cc: sta...@vger.kernel.org
---
fs/ceph/super.c | 2 +-
fs/cifs/cifsfs.c | 6 +++---
fs/ext3/super.c | 4 ++--
fs/ext4/super.c | 4 ++--
fs/gfs2/super.c | 6 +++---
fs/hfs/super.c | 4 ++--
fs/hfsplus/options.c | 4 ++--
fs/hostfs/hostfs_kern.c | 2 +-
fs/ocfs2/super.c | 4 ++--
fs/overlayfs/super.c | 6 +++---
fs/reiserfs/super.c | 8 +---
fs/xfs/xfs_super.c | 4 ++--
include/linux/seq_file.h | 34 ++
kernel/cgroup.c | 7 ---
net/ceph/ceph_common.c | 7 +--
security/selinux/hooks.c | 2 +-
16 files changed, 72 insertions(+), 32 deletions(-)
diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index d1c833c321b9..7b6bfcbf801c 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -479,7 +479,7 @@ static int ceph_show_options(struct seq_file *m, struct
dentry *root)
if (fsopt-max_readdir_bytes != CEPH_MAX_READDIR_BYTES_DEFAULT)
seq_printf(m, ,readdir_max_bytes=%d,
fsopt-max_readdir_bytes);
if (strcmp(fsopt-snapdir_name, CEPH_SNAPDIRNAME_DEFAULT))
- seq_printf(m, ,snapdirname=%s, fsopt-snapdir_name);
+ seq_show_option(m, snapdirname, fsopt-snapdir_name);
return 0;
}
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 0a9fb6b53126..6a1119e87fbb 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -394,17 +394,17 @@ cifs_show_options(struct seq_file *s, struct dentry *root)
struct sockaddr *srcaddr;
srcaddr = (struct sockaddr *)tcon-ses-server-srcaddr;
- seq_printf(s, ,vers=%s, tcon-ses-server-vals-version_string);
+ seq_show_option(s, vers, tcon-ses-server-vals-version_string);
cifs_show_security(s, tcon-ses);
cifs_show_cache_flavor(s, cifs_sb);
if (cifs_sb-mnt_cifs_flags CIFS_MOUNT_MULTIUSER)
seq_puts(s, ,multiuser);
else if (tcon-ses-user_name)
- seq_printf(s, ,username=%s, tcon-ses-user_name);
+ seq_show_option(s, username, tcon-ses-user_name);
if (tcon-ses-domainName)
- seq_printf(s, ,domain=%s, tcon-ses-domainName);
+ seq_show_option(s, domain, tcon-ses-domainName);
if (srcaddr-sa_family != AF_UNSPEC) {
struct sockaddr_in *saddr4;
diff --git a/fs/ext3/super.c b/fs/ext3/super.c
index 5ed0044fbb37..e9312494f3ee 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -578,10 +578,10 @@ static inline void ext3_show_quota_options(struct
seq_file *seq, struct super_bl
}
if (sbi-s_qf_names[USRQUOTA])
- seq_printf(seq, ,usrjquota=%s, sbi-s_qf_names[USRQUOTA]);
+ seq_show_option(seq, usrjquota, sbi-s_qf_names[USRQUOTA]);
if (sbi-s_qf_names[GRPQUOTA])
- seq_printf(seq, ,grpjquota=%s, sbi-s_qf_names[GRPQUOTA]);
+ seq_show_option(seq, grpjquota, sbi-s_qf_names[GRPQUOTA]);
if (test_opt(sb, USRQUOTA))
seq_puts(seq, ,usrquota);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 58987b5c514b..9981064c4a54 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1763,10 +1763,10 @@ static inline void ext4_show_quota_options(struct
seq_file *seq,
}
if (sbi-s_qf_names[USRQUOTA])
- seq_printf(seq, ,usrjquota=%s, sbi-s_qf_names[USRQUOTA]);
+ seq_show_option(seq, usrjquota, sbi-s_qf_names[USRQUOTA]);
if (sbi-s_qf_names[GRPQUOTA])
Re: [PATCH] fs: create and use seq_show_option for escaping
On Fri, Aug 7, 2015 at 4:41 PM, Kees Cook keesc...@chromium.org wrote:
Many file systems that implement the show_options hook fail to correctly
escape their output which could lead to unescaped characters (e.g. new
lines) leaking into /proc/mounts and /proc/[pid]/mountinfo files. This
could lead to confusion, spoofed entries (resulting in things like
systemd issuing false d-bus mount notifications), and who knows
what else. This looks like it would only be the root user stepping on
themselves, but it's possible weird things could happen in containers
or in other situations with delegated mount privileges.
Here's an example using overlay with setuid fusermount trusting the
contents of /proc/mounts (via the /etc/mtab symlink). Imagine the use of
sudo is something more sneaky:
$ BASE=ovl
$ MNT=$BASE/mnt
$ LOW=$BASE/lower
$ UP=$BASE/upper
$ WORK=$BASE/work/ 0 0
none /proc fuse.pwn user_id=1000
$ mkdir -p $LOW $UP $WORK
$ sudo mount -t overlay -o lowerdir=$LOW,upperdir=$UP,workdir=$WORK none
/mnt
$ cat /proc/mounts
none /root/ovl/mnt overlay
rw,relatime,lowerdir=ovl/lower,upperdir=ovl/upper,workdir=ovl/work/ 0 0
none /proc fuse.pwn user_id=1000 0 0
$ fusermount -u /proc
$ cat /proc/mounts
cat: /proc/mounts: No such file or directory
This fixes the problem by adding new seq_show_option and seq_show_option_n
helpers, and updating the vulnerable show_option handlers to use them as
needed. Some, like SELinux, need to be open coded due to unusual existing
escape mechanisms.
Signed-off-by: Kees Cook keesc...@chromium.org
Cc: sta...@vger.kernel.org
---
fs/ceph/super.c | 2 +-
fs/cifs/cifsfs.c | 6 +++---
fs/ext3/super.c | 4 ++--
fs/ext4/super.c | 4 ++--
fs/gfs2/super.c | 6 +++---
fs/hfs/super.c | 4 ++--
fs/hfsplus/options.c | 4 ++--
fs/hostfs/hostfs_kern.c | 2 +-
fs/ocfs2/super.c | 4 ++--
fs/overlayfs/super.c | 6 +++---
fs/reiserfs/super.c | 8 +---
fs/xfs/xfs_super.c | 4 ++--
include/linux/seq_file.h | 34 ++
kernel/cgroup.c | 7 ---
net/ceph/ceph_common.c | 7 +--
security/selinux/hooks.c | 2 +-
16 files changed, 72 insertions(+), 32 deletions(-)
diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index d1c833c321b9..7b6bfcbf801c 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -479,7 +479,7 @@ static int ceph_show_options(struct seq_file *m, struct
dentry *root)
if (fsopt-max_readdir_bytes != CEPH_MAX_READDIR_BYTES_DEFAULT)
seq_printf(m, ,readdir_max_bytes=%d,
fsopt-max_readdir_bytes);
if (strcmp(fsopt-snapdir_name, CEPH_SNAPDIRNAME_DEFAULT))
- seq_printf(m, ,snapdirname=%s, fsopt-snapdir_name);
+ seq_show_option(m, snapdirname, fsopt-snapdir_name);
return 0;
}
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 0a9fb6b53126..6a1119e87fbb 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -394,17 +394,17 @@ cifs_show_options(struct seq_file *s, struct dentry
*root)
struct sockaddr *srcaddr;
srcaddr = (struct sockaddr *)tcon-ses-server-srcaddr;
- seq_printf(s, ,vers=%s, tcon-ses-server-vals-version_string);
+ seq_show_option(s, vers, tcon-ses-server-vals-version_string);
cifs_show_security(s, tcon-ses);
cifs_show_cache_flavor(s, cifs_sb);
if (cifs_sb-mnt_cifs_flags CIFS_MOUNT_MULTIUSER)
seq_puts(s, ,multiuser);
else if (tcon-ses-user_name)
- seq_printf(s, ,username=%s, tcon-ses-user_name);
+ seq_show_option(s, username, tcon-ses-user_name);
if (tcon-ses-domainName)
- seq_printf(s, ,domain=%s, tcon-ses-domainName);
+ seq_show_option(s, domain, tcon-ses-domainName);
if (srcaddr-sa_family != AF_UNSPEC) {
struct sockaddr_in *saddr4;
diff --git a/fs/ext3/super.c b/fs/ext3/super.c
index 5ed0044fbb37..e9312494f3ee 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -578,10 +578,10 @@ static inline void ext3_show_quota_options(struct
seq_file *seq, struct super_bl
}
if (sbi-s_qf_names[USRQUOTA])
- seq_printf(seq, ,usrjquota=%s, sbi-s_qf_names[USRQUOTA]);
+ seq_show_option(seq, usrjquota, sbi-s_qf_names[USRQUOTA]);
if (sbi-s_qf_names[GRPQUOTA])
- seq_printf(seq, ,grpjquota=%s, sbi-s_qf_names[GRPQUOTA]);
+ seq_show_option(seq, grpjquota, sbi-s_qf_names[GRPQUOTA]);
if (test_opt(sb, USRQUOTA))
seq_puts(seq, ,usrquota);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 58987b5c514b..9981064c4a54 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1763,10 +1763,10 @@ static inline void ext4_show_quota_options(struct
seq_file *seq,
}
if (sbi-s_qf_names[USRQUOTA])
-
Re: [PATCH] fs: create and use seq_show_option for escaping
On Fri, Aug 07, 2015 at 04:41:50PM -0700, Kees Cook wrote:
Many file systems that implement the show_options hook fail to correctly
escape their output which could lead to unescaped characters (e.g. new
lines) leaking into /proc/mounts and /proc/[pid]/mountinfo files. This
could lead to confusion, spoofed entries (resulting in things like
systemd issuing false d-bus mount notifications), and who knows
what else. This looks like it would only be the root user stepping on
themselves, but it's possible weird things could happen in containers
or in other situations with delegated mount privileges.
Here's an example using overlay with setuid fusermount trusting the
contents of /proc/mounts (via the /etc/mtab symlink). Imagine the use of
sudo is something more sneaky:
$ BASE=ovl
$ MNT=$BASE/mnt
$ LOW=$BASE/lower
$ UP=$BASE/upper
$ WORK=$BASE/work/ 0 0
none /proc fuse.pwn user_id=1000
$ mkdir -p $LOW $UP $WORK
$ sudo mount -t overlay -o lowerdir=$LOW,upperdir=$UP,workdir=$WORK none
/mnt
$ cat /proc/mounts
none /root/ovl/mnt overlay
rw,relatime,lowerdir=ovl/lower,upperdir=ovl/upper,workdir=ovl/work/ 0 0
none /proc fuse.pwn user_id=1000 0 0
$ fusermount -u /proc
$ cat /proc/mounts
cat: /proc/mounts: No such file or directory
This fixes the problem by adding new seq_show_option and seq_show_option_n
helpers, and updating the vulnerable show_option handlers to use them as
needed. Some, like SELinux, need to be open coded due to unusual existing
escape mechanisms.
Signed-off-by: Kees Cook keesc...@chromium.org
Cc: sta...@vger.kernel.org
Acked-by: Serge Hallyn serge.hal...@canonical.com
---
fs/ceph/super.c | 2 +-
fs/cifs/cifsfs.c | 6 +++---
fs/ext3/super.c | 4 ++--
fs/ext4/super.c | 4 ++--
fs/gfs2/super.c | 6 +++---
fs/hfs/super.c | 4 ++--
fs/hfsplus/options.c | 4 ++--
fs/hostfs/hostfs_kern.c | 2 +-
fs/ocfs2/super.c | 4 ++--
fs/overlayfs/super.c | 6 +++---
fs/reiserfs/super.c | 8 +---
fs/xfs/xfs_super.c | 4 ++--
include/linux/seq_file.h | 34 ++
kernel/cgroup.c | 7 ---
net/ceph/ceph_common.c | 7 +--
security/selinux/hooks.c | 2 +-
16 files changed, 72 insertions(+), 32 deletions(-)
diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index d1c833c321b9..7b6bfcbf801c 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -479,7 +479,7 @@ static int ceph_show_options(struct seq_file *m, struct
dentry *root)
if (fsopt-max_readdir_bytes != CEPH_MAX_READDIR_BYTES_DEFAULT)
seq_printf(m, ,readdir_max_bytes=%d,
fsopt-max_readdir_bytes);
if (strcmp(fsopt-snapdir_name, CEPH_SNAPDIRNAME_DEFAULT))
- seq_printf(m, ,snapdirname=%s, fsopt-snapdir_name);
+ seq_show_option(m, snapdirname, fsopt-snapdir_name);
return 0;
}
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 0a9fb6b53126..6a1119e87fbb 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -394,17 +394,17 @@ cifs_show_options(struct seq_file *s, struct dentry
*root)
struct sockaddr *srcaddr;
srcaddr = (struct sockaddr *)tcon-ses-server-srcaddr;
- seq_printf(s, ,vers=%s, tcon-ses-server-vals-version_string);
+ seq_show_option(s, vers, tcon-ses-server-vals-version_string);
cifs_show_security(s, tcon-ses);
cifs_show_cache_flavor(s, cifs_sb);
if (cifs_sb-mnt_cifs_flags CIFS_MOUNT_MULTIUSER)
seq_puts(s, ,multiuser);
else if (tcon-ses-user_name)
- seq_printf(s, ,username=%s, tcon-ses-user_name);
+ seq_show_option(s, username, tcon-ses-user_name);
if (tcon-ses-domainName)
- seq_printf(s, ,domain=%s, tcon-ses-domainName);
+ seq_show_option(s, domain, tcon-ses-domainName);
if (srcaddr-sa_family != AF_UNSPEC) {
struct sockaddr_in *saddr4;
diff --git a/fs/ext3/super.c b/fs/ext3/super.c
index 5ed0044fbb37..e9312494f3ee 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -578,10 +578,10 @@ static inline void ext3_show_quota_options(struct
seq_file *seq, struct super_bl
}
if (sbi-s_qf_names[USRQUOTA])
- seq_printf(seq, ,usrjquota=%s, sbi-s_qf_names[USRQUOTA]);
+ seq_show_option(seq, usrjquota, sbi-s_qf_names[USRQUOTA]);
if (sbi-s_qf_names[GRPQUOTA])
- seq_printf(seq, ,grpjquota=%s, sbi-s_qf_names[GRPQUOTA]);
+ seq_show_option(seq, grpjquota, sbi-s_qf_names[GRPQUOTA]);
if (test_opt(sb, USRQUOTA))
seq_puts(seq, ,usrquota);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 58987b5c514b..9981064c4a54 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1763,10 +1763,10 @@ static inline void ext4_show_quota_options(struct
seq_file *seq,
}
if (sbi-s_qf_names[USRQUOTA])
-
