linux-kernel - [PATCH] vfs: hard-ban creating files with control characters in the name

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20171003005042.16470-1-kilobyte@angband.pl>
Date:   Tue,  3 Oct 2017 02:50:42 +0200
From:   Adam Borowski <kilobyte@...band.pl>
To:     Alexander Viro <viro@...iv.linux.org.uk>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     Adam Borowski <kilobyte@...band.pl>
Subject: [PATCH] vfs: hard-ban creating files with control characters in the name

Anything with bytes 1-31,127 will get -EACCES.

Especially \n is bad: instead of natural file-per-line, you need an
user-unfriendly feature of -print0 added to every producer and consumer;
a good part of users either don't know or don't feel the need to bother
with escaping this snowflake, thus introducing security holes.

The rest of control characters, while not as harmful, don't have a
legitimate use nor have any real chance of coming from a tarball (malice
and fooling around excluded).  No character set ever supported as a system
locale by glibc, and, TTBMK, by other popular Unices, includes them, thus
it can be assumed no foreign files have such names other than artificially.

This goes in stark contrast with other characters proposed to be banned:
non-UTF8 is common, and even on my desktop's disk I found examples of all
of: [ ], < >, initial -, initial and final space, ?, *, .., ', ", |, &.
Somehow no \ anywhere.  I think I have an idea why no / .

Another debatable point is whether to -EACCES or to silently rename to an
escaped form such as %0A.  I believe the former is better because:
* programs can be confused if a directory has files they didn't just write
* many filesystems already disallow certain characters (like invalid
  Unicode), thus returning an error is consistent

An example of a write-up of this issue can be found at:
https://www.dwheeler.com/essays/fixing-unix-linux-filenames.html

Signed-off-by: Adam Borowski <kilobyte@...band.pl>
---
I really care mostly about \n but went bold and submitted a version with
all ASCII control characters.  If you guys disagree, please consider
banning at least \n.

 fs/namei.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/fs/namei.c b/fs/namei.c
index c75ea03ca147..8c6ed785fd49 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2817,6 +2817,18 @@ static int may_delete(struct inode *dir, struct dentry *victim, bool isdir)
 	return 0;
 }

+/*	Check for banned characters in file name.  Called only on creation, as
+ * we need to allow removal/renaming/reading of existing stuff.
+ */
+static int is_bad_name(const char *name)
+{
+	const char *c;
+	for (c = name; *c; c++)
+		if ((1 <= *c && *c <= 31) || *c == 127)
+			return 1;
+	return 0;
+}
+
 /*	Check whether we can create an object with dentry child in directory
  *  dir.
  *  1. We can't do it if child already exists (open has special treatment for
@@ -2834,6 +2846,8 @@ static inline int may_create(struct inode *dir, struct dentry *child)
 		return -EEXIST;
 	if (IS_DEADDIR(dir))
 		return -ENOENT;
+	if (is_bad_name(child->d_name.name))
+		return -EACCES;
 	s_user_ns = dir->i_sb->s_user_ns;
 	if (!kuid_has_mapping(s_user_ns, current_fsuid()) ||
 	    !kgid_has_mapping(s_user_ns, current_fsgid()))
@@ -2996,6 +3010,9 @@ static int may_o_create(const struct path *dir, struct dentry *dentry, umode_t m
 	if (error)
 		return error;

+	if (is_bad_name(dentry->d_name.name))
+		return -EACCES;
+
 	s_user_ns = dir->dentry->d_sb->s_user_ns;
 	if (!kuid_has_mapping(s_user_ns, current_fsuid()) ||
 	    !kgid_has_mapping(s_user_ns, current_fsgid()))
-- 
2.14.2