diff mbox series

[RFC,v2,07/18] lib: lift fscrypt base64 conversion into lib/

Message ID 20200904160537.76663-8-jlayton@kernel.org (mailing list archive)
State New, archived
Headers show
Series ceph+fscrypt: context, filename and symlink support | expand

Commit Message

Jeff Layton Sept. 4, 2020, 4:05 p.m. UTC
Once we allow encrypted filenames on ceph we'll end up with names that
may have illegal characters in them (embedded '\0' or '/'), or
characters that aren't printable.

It will be safer to use strings that are printable. It turns out that the
MDS doesn't really care about the length of filenames, so we can just
base64 encode and decode filenames before writing and reading them.

Lift the base64 implementation that's in fscrypt into lib/. Make fscrypt
select it when it's enabled.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/crypto/Kconfig            |  1 +
 fs/crypto/fname.c            | 64 ++------------------------------
 include/linux/base64_fname.h | 11 ++++++
 lib/Kconfig                  |  3 ++
 lib/Makefile                 |  1 +
 lib/base64_fname.c           | 71 ++++++++++++++++++++++++++++++++++++
 6 files changed, 90 insertions(+), 61 deletions(-)
 create mode 100644 include/linux/base64_fname.h
 create mode 100644 lib/base64_fname.c

Comments

Eric Biggers Sept. 8, 2020, 3:59 a.m. UTC | #1
On Fri, Sep 04, 2020 at 12:05:26PM -0400, Jeff Layton wrote:
> Once we allow encrypted filenames on ceph we'll end up with names that
> may have illegal characters in them (embedded '\0' or '/'), or
> characters that aren't printable.
> 
> It will be safer to use strings that are printable. It turns out that the
> MDS doesn't really care about the length of filenames, so we can just
> base64 encode and decode filenames before writing and reading them.
> 
> Lift the base64 implementation that's in fscrypt into lib/. Make fscrypt
> select it when it's enabled.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/crypto/Kconfig            |  1 +
>  fs/crypto/fname.c            | 64 ++------------------------------
>  include/linux/base64_fname.h | 11 ++++++
>  lib/Kconfig                  |  3 ++
>  lib/Makefile                 |  1 +
>  lib/base64_fname.c           | 71 ++++++++++++++++++++++++++++++++++++
>  6 files changed, 90 insertions(+), 61 deletions(-)
>  create mode 100644 include/linux/base64_fname.h
>  create mode 100644 lib/base64_fname.c
> 

I'm still concerned that this functionality is too specific to belong in lib/ at
the moment, given that it's not the most commonly used variant of base64.  How
about keeping these functions in fs/crypto/ for now?  You can call them
fscrypt_base64_encode() and fscrypt_base64_decode() and export them for ceph to
use.

> diff --git a/lib/base64_fname.c b/lib/base64_fname.c
> new file mode 100644
> index 000000000000..7638c45e4035
> --- /dev/null
> +++ b/lib/base64_fname.c
> @@ -0,0 +1,71 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Modified base64 encode/decode functions, suitable for use as filename components.
> + *
> + * Originally lifted from fs/crypto/fname.c
> + *
> + * Copyright (C) 2015, Jaegeuk Kim
> + * Copyright (C) 2015, Eric Biggers
> + */

Please don't change the copyright statements.  The original file had:

 * Copyright (C) 2015, Google, Inc.
 * Copyright (C) 2015, Motorola Mobility

- Eric
Jeff Layton Sept. 8, 2020, 12:51 p.m. UTC | #2
On Mon, 2020-09-07 at 20:59 -0700, Eric Biggers wrote:
> On Fri, Sep 04, 2020 at 12:05:26PM -0400, Jeff Layton wrote:
> > Once we allow encrypted filenames on ceph we'll end up with names that
> > may have illegal characters in them (embedded '\0' or '/'), or
> > characters that aren't printable.
> > 
> > It will be safer to use strings that are printable. It turns out that the
> > MDS doesn't really care about the length of filenames, so we can just
> > base64 encode and decode filenames before writing and reading them.
> > 
> > Lift the base64 implementation that's in fscrypt into lib/. Make fscrypt
> > select it when it's enabled.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/crypto/Kconfig            |  1 +
> >  fs/crypto/fname.c            | 64 ++------------------------------
> >  include/linux/base64_fname.h | 11 ++++++
> >  lib/Kconfig                  |  3 ++
> >  lib/Makefile                 |  1 +
> >  lib/base64_fname.c           | 71 ++++++++++++++++++++++++++++++++++++
> >  6 files changed, 90 insertions(+), 61 deletions(-)
> >  create mode 100644 include/linux/base64_fname.h
> >  create mode 100644 lib/base64_fname.c
> > 
> 
> I'm still concerned that this functionality is too specific to belong in lib/ at
> the moment, given that it's not the most commonly used variant of base64.  How
> about keeping these functions in fs/crypto/ for now?  You can call them
> fscrypt_base64_encode() and fscrypt_base64_decode() and export them for ceph to
> use.
> 

Ok, will do.

> > diff --git a/lib/base64_fname.c b/lib/base64_fname.c
> > new file mode 100644
> > index 000000000000..7638c45e4035
> > --- /dev/null
> > +++ b/lib/base64_fname.c
> > @@ -0,0 +1,71 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Modified base64 encode/decode functions, suitable for use as filename components.
> > + *
> > + * Originally lifted from fs/crypto/fname.c
> > + *
> > + * Copyright (C) 2015, Jaegeuk Kim
> > + * Copyright (C) 2015, Eric Biggers
> > + */
> 
> Please don't change the copyright statements.  The original file had:
> 
>  * Copyright (C) 2015, Google, Inc.
>  * Copyright (C) 2015, Motorola Mobility
diff mbox series

Patch

diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
index a5f5c30368a2..6b27d105420c 100644
--- a/fs/crypto/Kconfig
+++ b/fs/crypto/Kconfig
@@ -6,6 +6,7 @@  config FS_ENCRYPTION
 	select CRYPTO_SKCIPHER
 	select CRYPTO_LIB_SHA256
 	select KEYS
+	select BASE64_FNAME
 	help
 	  Enable encryption of files and directories.  This
 	  feature is similar to ecryptfs, but it is more memory
diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 09f09def87fc..89e26e923547 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -11,6 +11,7 @@ 
  * This has not yet undergone a rigorous security audit.
  */
 
+#include <linux/base64_fname.h>
 #include <linux/namei.h>
 #include <linux/scatterlist.h>
 #include <crypto/hash.h>
@@ -184,64 +185,6 @@  static int fname_decrypt(const struct inode *inode,
 	return 0;
 }
 
-static const char lookup_table[65] =
-	"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+,";
-
-#define BASE64_CHARS(nbytes)	DIV_ROUND_UP((nbytes) * 4, 3)
-
-/**
- * base64_encode() - base64-encode some bytes
- * @src: the bytes to encode
- * @len: number of bytes to encode
- * @dst: (output) the base64-encoded string.  Not NUL-terminated.
- *
- * Encodes the input string using characters from the set [A-Za-z0-9+,].
- * The encoded string is roughly 4/3 times the size of the input string.
- *
- * Return: length of the encoded string
- */
-static int base64_encode(const u8 *src, int len, char *dst)
-{
-	int i, bits = 0, ac = 0;
-	char *cp = dst;
-
-	for (i = 0; i < len; i++) {
-		ac += src[i] << bits;
-		bits += 8;
-		do {
-			*cp++ = lookup_table[ac & 0x3f];
-			ac >>= 6;
-			bits -= 6;
-		} while (bits >= 6);
-	}
-	if (bits)
-		*cp++ = lookup_table[ac & 0x3f];
-	return cp - dst;
-}
-
-static int base64_decode(const char *src, int len, u8 *dst)
-{
-	int i, bits = 0, ac = 0;
-	const char *p;
-	u8 *cp = dst;
-
-	for (i = 0; i < len; i++) {
-		p = strchr(lookup_table, src[i]);
-		if (p == NULL || src[i] == 0)
-			return -2;
-		ac += (p - lookup_table) << bits;
-		bits += 6;
-		if (bits >= 8) {
-			*cp++ = ac & 0xff;
-			ac >>= 8;
-			bits -= 8;
-		}
-	}
-	if (ac)
-		return -1;
-	return cp - dst;
-}
-
 bool fscrypt_fname_encrypted_size(const struct inode *inode, u32 orig_len,
 				  u32 max_len, u32 *encrypted_len_ret)
 {
@@ -335,7 +278,7 @@  void fscrypt_encode_nokey_name(u32 hash, u32 minor_hash,
 				  nokey_name.sha256);
 		size = FSCRYPT_NOKEY_NAME_MAX;
 	}
-	oname->len = base64_encode((const u8 *)&nokey_name, size, oname->name);
+	oname->len = base64_encode_fname((const u8 *)&nokey_name, size, oname->name);
 }
 EXPORT_SYMBOL(fscrypt_encode_nokey_name);
 
@@ -380,7 +323,6 @@  int fscrypt_fname_disk_to_usr(const struct inode *inode,
 	if (fscrypt_has_encryption_key(inode))
 		return fname_decrypt(inode, iname, oname);
 
-	fscrypt_encode_nokey_name(hash, minor_hash, iname, oname);
 	return 0;
 }
 EXPORT_SYMBOL(fscrypt_fname_disk_to_usr);
@@ -460,7 +402,7 @@  int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname,
 	if (fname->crypto_buf.name == NULL)
 		return -ENOMEM;
 
-	ret = base64_decode(iname->name, iname->len, fname->crypto_buf.name);
+	ret = base64_decode_fname(iname->name, iname->len, fname->crypto_buf.name);
 	if (ret < (int)offsetof(struct fscrypt_nokey_name, bytes[1]) ||
 	    (ret > offsetof(struct fscrypt_nokey_name, sha256) &&
 	     ret != FSCRYPT_NOKEY_NAME_MAX)) {
diff --git a/include/linux/base64_fname.h b/include/linux/base64_fname.h
new file mode 100644
index 000000000000..d98d79b4c95c
--- /dev/null
+++ b/include/linux/base64_fname.h
@@ -0,0 +1,11 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+#ifndef _LIB_BASE64_FNAME_H
+#define _LIB_BASE64_FNAME_H
+#include <linux/types.h>
+
+#define BASE64_CHARS(nbytes)	DIV_ROUND_UP((nbytes) * 4, 3)
+
+int base64_encode_fname(const u8 *src, int len, char *dst);
+int base64_decode_fname(const char *src, int len, u8 *dst);
+#endif /* _LIB_BASE64_FNAME_H */
diff --git a/lib/Kconfig b/lib/Kconfig
index b4b98a03ff98..94f3939c4cfa 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -684,3 +684,6 @@  config GENERIC_LIB_UCMPDI2
 config PLDMFW
 	bool
 	default n
+
+config BASE64_FNAME
+	tristate
diff --git a/lib/Makefile b/lib/Makefile
index a4a4c6864f51..4e77167d6252 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -171,6 +171,7 @@  obj-$(CONFIG_LIBCRC32C)	+= libcrc32c.o
 obj-$(CONFIG_CRC8)	+= crc8.o
 obj-$(CONFIG_XXHASH)	+= xxhash.o
 obj-$(CONFIG_GENERIC_ALLOCATOR) += genalloc.o
+obj-$(CONFIG_BASE64_FNAME) += base64_fname.o
 
 obj-$(CONFIG_842_COMPRESS) += 842/
 obj-$(CONFIG_842_DECOMPRESS) += 842/
diff --git a/lib/base64_fname.c b/lib/base64_fname.c
new file mode 100644
index 000000000000..7638c45e4035
--- /dev/null
+++ b/lib/base64_fname.c
@@ -0,0 +1,71 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Modified base64 encode/decode functions, suitable for use as filename components.
+ *
+ * Originally lifted from fs/crypto/fname.c
+ *
+ * Copyright (C) 2015, Jaegeuk Kim
+ * Copyright (C) 2015, Eric Biggers
+ */
+
+#include <linux/types.h>
+#include <linux/export.h>
+#include <linux/string.h>
+
+static const char lookup_table[65] =
+	"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+,";
+
+/**
+ * base64_encode() - base64-encode some bytes
+ * @src: the bytes to encode
+ * @len: number of bytes to encode
+ * @dst: (output) the base64-encoded string.  Not NUL-terminated.
+ *
+ * Encodes the input string using characters from the set [A-Za-z0-9+,].
+ * The encoded string is roughly 4/3 times the size of the input string.
+ *
+ * Return: length of the encoded string
+ */
+int base64_encode_fname(const u8 *src, int len, char *dst)
+{
+	int i, bits = 0, ac = 0;
+	char *cp = dst;
+
+	for (i = 0; i < len; i++) {
+		ac += src[i] << bits;
+		bits += 8;
+		do {
+			*cp++ = lookup_table[ac & 0x3f];
+			ac >>= 6;
+			bits -= 6;
+		} while (bits >= 6);
+	}
+	if (bits)
+		*cp++ = lookup_table[ac & 0x3f];
+	return cp - dst;
+}
+EXPORT_SYMBOL(base64_encode_fname);
+
+int base64_decode_fname(const char *src, int len, u8 *dst)
+{
+	int i, bits = 0, ac = 0;
+	const char *p;
+	u8 *cp = dst;
+
+	for (i = 0; i < len; i++) {
+		p = strchr(lookup_table, src[i]);
+		if (p == NULL || src[i] == 0)
+			return -2;
+		ac += (p - lookup_table) << bits;
+		bits += 6;
+		if (bits >= 8) {
+			*cp++ = ac & 0xff;
+			ac >>= 8;
+			bits -= 8;
+		}
+	}
+	if (ac)
+		return -1;
+	return cp - dst;
+}
+EXPORT_SYMBOL(base64_decode_fname);