eCryptfs: Add mount option for cipher mode

This patch removes the hardcoding of CBC mode in eCryptfs and adds a mount option allowing one to specify the desired mode of operation. Currently this defaults to cbc, but should would with any plain block cipher mode supported by the kernel. We've been testing with ecb to ensure we've found any implicit assumptions about cbc.

Future work will involve supporting AEAD modes (GCM) and adding in integrity checking code.

As this is our first kernel patch, we'd like feedback on what we've done right and what we've done wrong, not only in code, but also anything related to the patch submission itself.

Signed-off-by William Morrison <[email protected]>

---
diff -uprN -X ecryptfs_base/Documentation/dontdiff ecryptfs_base/fs/ecryptfs/crypto.c ecryptfs/fs/ecryptfs/crypto.c

fs/ecryptfs/crypto.c +13 -3

@@ -743,7 +743,8 @@ int ecryptfs_init_crypt_ctx(struct ecryp
 	}
 	mutex_lock(&crypt_stat->cs_tfm_mutex);
 	rc = ecryptfs_crypto_api_algify_cipher_name(&full_alg_name,
-						    crypt_stat->cipher, "cbc");
+						crypt_stat->cipher,
+						crypt_stat->cipher_mode);
 	if (rc)
 		goto out_unlock;
 	crypt_stat->tfm = crypto_alloc_blkcipher(full_alg_name, 0,

@@ -753,8 +754,9 @@ int ecryptfs_init_crypt_ctx(struct ecryp
 		rc = PTR_ERR(crypt_stat->tfm);
 		crypt_stat->tfm = NULL;
 		ecryptfs_printk(KERN_ERR, "cryptfs: init_crypt_ctx(): "
-				"Error initializing cipher [%s]\n",
-				crypt_stat->cipher);
+			"Error initializing cipher [%s] and mode [%s]\n",
+			crypt_stat->cipher,
+			crypt_stat->cipher_mode);
 		goto out_unlock;
 	}
 	crypto_blkcipher_set_flags(crypt_stat->tfm, CRYPTO_TFM_REQ_WEAK_KEY);

@@ -916,6 +918,7 @@ static void ecryptfs_set_default_crypt_s
 						      mount_crypt_stat);
 	ecryptfs_set_default_sizes(crypt_stat);
 	strcpy(crypt_stat->cipher, ECRYPTFS_DEFAULT_CIPHER);
+	strcpy(crypt_stat->cipher_mode, ECRYPTFS_DEFAULT_CIPHER_MODE);
 	crypt_stat->key_size = ECRYPTFS_DEFAULT_KEY_BYTES;
 	crypt_stat->flags &= ~(ECRYPTFS_KEY_VALID);
 	crypt_stat->file_version = ECRYPTFS_FILE_VERSION;

@@ -949,6 +952,7 @@ int ecryptfs_new_file_context(struct ino
 	    &ecryptfs_superblock_to_private(
 		    ecryptfs_inode->i_sb)->mount_crypt_stat;
 	int cipher_name_len;
+	int cipher_mode_name_len;
 	int rc = 0;
 
 	ecryptfs_set_default_crypt_stat_vals(crypt_stat, mount_crypt_stat);

@@ -968,6 +972,12 @@ int ecryptfs_new_file_context(struct ino
 	       mount_crypt_stat->global_default_cipher_name,
 	       cipher_name_len);
 	crypt_stat->cipher[cipher_name_len] = '\0';
+	cipher_mode_name_len =
+		strlen(mount_crypt_stat->global_default_cipher_mode_name);
+	memcpy(crypt_stat->cipher_mode,
+		mount_crypt_stat->global_default_cipher_mode_name,
+		cipher_mode_name_len);
+	crypt_stat->cipher_mode[cipher_mode_name_len] = '\0';
 	crypt_stat->key_size =
 		mount_crypt_stat->global_default_cipher_key_size;
 	ecryptfs_generate_new_key(crypt_stat);

diff -uprN -X ecryptfs_base/Documentation/dontdiff ecryptfs_base/fs/ecryptfs/ecryptfs_kernel.h ecryptfs/fs/ecryptfs/ecryptfs_kernel.h

fs/ecryptfs/ecryptfs_kernel.h +5 -0

@@ -124,6 +124,7 @@ ecryptfs_get_key_payload_data(struct key
 
 #define ECRYPTFS_MAX_KEYSET_SIZE 1024
 #define ECRYPTFS_MAX_CIPHER_NAME_SIZE 32
+#define ECRYPTFS_MAX_CIPHER_MODE_NAME_SIZE 32
 #define ECRYPTFS_MAX_NUM_ENC_KEYS 64
 #define ECRYPTFS_MAX_IV_BYTES 16	/* 128 bits */
 #define ECRYPTFS_SALT_BYTES 2

@@ -133,6 +134,7 @@ ecryptfs_get_key_payload_data(struct key
 #define ECRYPTFS_SIZE_AND_MARKER_BYTES (ECRYPTFS_FILE_SIZE_BYTES \
 					+ MAGIC_ECRYPTFS_MARKER_SIZE_BYTES)
 #define ECRYPTFS_DEFAULT_CIPHER "aes"
+#define ECRYPTFS_DEFAULT_CIPHER_MODE "cbc"
 #define ECRYPTFS_DEFAULT_KEY_BYTES 16
 #define ECRYPTFS_DEFAULT_HASH "md5"
 #define ECRYPTFS_TAG_70_DIGEST ECRYPTFS_DEFAULT_HASH

@@ -237,6 +239,7 @@ struct ecryptfs_crypt_stat {
 	struct crypto_hash *hash_tfm; /* Crypto context for generating
 				       * the initialization vectors */
 	unsigned char cipher[ECRYPTFS_MAX_CIPHER_NAME_SIZE];
+	unsigned char cipher_mode[ECRYPTFS_MAX_CIPHER_MODE_NAME_SIZE];
 	unsigned char key[ECRYPTFS_MAX_KEY_BYTES];
 	unsigned char root_iv[ECRYPTFS_MAX_IV_BYTES];
 	struct list_head keysig_list;

@@ -338,6 +341,8 @@ struct ecryptfs_mount_crypt_stat {
 	size_t global_default_fn_cipher_key_bytes;
 	unsigned char global_default_cipher_name[ECRYPTFS_MAX_CIPHER_NAME_SIZE
 						 + 1];
+	unsigned char global_default_cipher_mode_name[
+		ECRYPTFS_MAX_CIPHER_MODE_NAME_SIZE + 1];
 	unsigned char global_default_fn_cipher_name[
 		ECRYPTFS_MAX_CIPHER_NAME_SIZE + 1];
 	char global_default_fnek_sig[ECRYPTFS_SIG_SIZE_HEX + 1];

diff -uprN -X ecryptfs_base/Documentation/dontdiff ecryptfs_base/fs/ecryptfs/keystore.c ecryptfs/fs/ecryptfs/keystore.c

fs/ecryptfs/keystore.c +0 -0

 	rc = ecryptfs_init_crypt_ctx(crypt_stat);
 	if (rc)
 		goto out_free;

diff -uprN -X ecryptfs_base/Documentation/dontdiff ecryptfs_base/fs/ecryptfs/main.c ecryptfs/fs/ecryptfs/main.c

fs/ecryptfs/main.c +3 -0

@@ -171,6 +171,7 @@ void ecryptfs_put_lower_file(struct inod
 
 enum { ecryptfs_opt_sig, ecryptfs_opt_ecryptfs_sig,
        ecryptfs_opt_cipher, ecryptfs_opt_ecryptfs_cipher,
+       ecryptfs_opt_cipher_mode, ecryptfs_opt_ecryptfs_cipher_mode,
        ecryptfs_opt_ecryptfs_key_bytes,
        ecryptfs_opt_passthrough, ecryptfs_opt_xattr_metadata,
        ecryptfs_opt_encrypted_view, ecryptfs_opt_fnek_sig,

@@ -183,7 +184,9 @@ static const match_table_t tokens = {
 	{ecryptfs_opt_sig, "sig=%s"},
 	{ecryptfs_opt_ecryptfs_sig, "ecryptfs_sig=%s"},
 	{ecryptfs_opt_cipher, "cipher=%s"},
+	{ecryptfs_opt_cipher_mode, "cipher_mode=%s"},
 	{ecryptfs_opt_ecryptfs_cipher, "ecryptfs_cipher=%s"},
+	{ecryptfs_opt_ecryptfs_cipher_mode, "ecryptfs_cipher_mode=%s"},
 	{ecryptfs_opt_ecryptfs_key_bytes, "ecryptfs_key_bytes=%u"},
 	{ecryptfs_opt_passthrough, "ecryptfs_passthrough"},
 	{ecryptfs_opt_xattr_metadata, "ecryptfs_xattr_metadata"},

fs/ecryptfs/main.c +4 -0

 	char *p;
+	int i = 0;
 	int rc = 0;
 	int sig_set = 0;
 	int cipher_name_set = 0;
+	int cipher_mode_name_set = 0;
 	int fn_cipher_name_set = 0;
 	int cipher_key_bytes;
 	int cipher_key_bytes_set = 0;

@@ -274,6 +280,8 @@ static int ecryptfs_parse_options(struct
 	char *sig_src;
 	char *cipher_name_dst;
 	char *cipher_name_src;
+	char *cipher_mode_name_dst;
+	char *cipher_mode_name_src;
 	char *fn_cipher_name_dst;
 	char *fn_cipher_name_src;
 	char *fnek_dst;

fs/ecryptfs/main.c +6 -0

+			strncpy(cipher_mode_name_dst, cipher_mode_name_src,
+				ECRYPTFS_MAX_CIPHER_MODE_NAME_SIZE);
+			cipher_mode_name_dst[
+				ECRYPTFS_MAX_CIPHER_MODE_NAME_SIZE] = '\0';
+			cipher_mode_name_set = 1;
+			break;
 		case ecryptfs_opt_ecryptfs_key_bytes:
 			cipher_key_bytes_src = args[0].from;
 			cipher_key_bytes =

fs/ecryptfs/main.c +13 -0

+		strcpy(mount_crypt_stat->global_default_cipher_mode_name,
+		       ECRYPTFS_DEFAULT_CIPHER_MODE);
+	}
 	if ((mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES)
 	    && !fn_cipher_name_set)
 		strcpy(mount_crypt_stat->global_default_fn_cipher_name,

@@ -434,6 +462,22 @@ static int ecryptfs_parse_options(struct
 		goto out;
 	}
 
+	rc = -EINVAL;
+	for (i = 0; i < ARRAY_SIZE(mode_white_list); i++) {
+		if (strcmp(mount_crypt_stat->global_default_cipher_mode_name,
+				mode_white_list[i]) == 0) {
+			rc = 0;
+			break;
+		}
+	}
+	if (rc) {
+		ecryptfs_printk(KERN_ERR,

To start things off... when you have a side note, such as your paragraph above mentioning that this is your first kernel patch, you don't want something like that to end up in the commit log history forever. A good place for notes like that is below the --- line and above the diff command. Most patch merging tools should ignore that content and keep it from being recorded forever in the commit history. (I know that you didn't intend for this patch to be upstreamed at this time, but I thought I'd mention this tip.)

I'll assume that this means that you're not using git. If you plan to continue doing kernel development (or really any open source development) after this feature, I'd recommend that you work on acquiring basic git skills. You previously asked what can help speed up your development workflow and understanding git is certainly something that will help.

In fact, this patch won't apply for me using git am -is /path/to/mbox. I'm not immediately sure why, but I do see some extra '-' characters in the areas that git complains about.

It might be because you GPG-signed the email. GPG signing patches would seem like a good thing to do, but Documentation/email-clients.txt claims that it causes some problems and recommends that you don't sign patches.

Running the following search and replace in vim:

:%s/^- -/-/g

and stripping off the GPG header/footer, I'm kind of able to get the patch to apply. There's offset and fuzz issues, so git am won't apply it (but patch will, of course). Which kernel tree did you develop the patch against?

Quoted 3 lines

(quoted diff) +1 -0

+	strncpy(crypt_stat->cipher_mode, +

crypt_stat->mount_crypt_stat->global_default_cipher_mode_name, + ECRYPTFS_MAX_CIPHER_MODE_NAME_SIZE);

This is a problem. eCryptfs files are atomic in that they contain all of the information needed to decrypt the file (except for the FEKEK) in the header. So a single eCryptfs mount may contain files encrypted by AES-128, AES-256, blowfish, the FEK can be wrapped with a public key, multiple FEKEKs can be used, etc., and eCryptfs can seamlessly handle it because of the various metadata stored in the file header.

At first glance, it doesn't look like RFC2440 defines a field for different cipher modes. eCryptfs has always just made the assumption that it should use its tweaked CBC mode and that has always been sufficient.

We will need a place to store the cipher mode in the header and that field will need to be read in somewhere around here.

Also, when writing out the header information, we'll need to store the cipher mode.

Review

Quoted 1 line

(quoted diff) +1 -0

+	const char *mode_white_list[] = {"cbc", "ecb", "gcm"};

When you submit a patch, it should be able to stand on its own. This patch doesn't stand on its own because ecb and gcm aren't supported but this patch happily accepts them. This white list should only contain "cbc" at this point.

The idea you had with testing with ECB was a good idea, but it isn't something that we'd want to ever support in eCryptfs since our default is more secure. So the "ecb" element should of just been a local change that you made on top of this patch, only for testing.

Quoted 2 lines

(quoted diff) +1 -0

+			cipher_mode_name_dst = +				mount_crypt_stat-> +

global_default_cipher_mode_name;

Yuck. I don't know if this is better or worse than breaking the 80 char rule. eCryptfs uses variable names that are too long in most cases. (This isn't something that you need to worry about)

Quoted 2 lines

(quoted diff) +1 -0

+		BUG_ON(cipher_mode_name_len >= +

ECRYPTFS_MAX_CIPHER_MODE_NAME_SIZE);

I see that you copied and pasted this logic from the !cipher_name_set conditional. So it isn't your fault, but this is terrible.

This is something that should be caught at build time rather than at runtime (why force this BUG() on our users?). These BUG_ON()'s should probably be replaced with a BUILD_BUG_ON().

This was a nice submission for your first shot at it. The biggest problem to fix will be figuring out where to store the mode in the header. Take a look through RFC2440 (and 4880 and 5581) to see if there's already a field we can use for this. If not, we'll have to give it some more thought. I don't want to rely on the user specifying the correct ecryptfs_cipher_mode mount option.

Thanks again for working on this!

Tyler

Quoted 8 lines · Tyler Hicks

To start things off... when you have a side note, such as your paragraph above mentioning that this is your first kernel patch, you don't want something like that to end up in the commit log history forever. A good place for notes like that is below the --- line and above the diff command. Most patch merging tools should ignore that content and keep it from being recorded forever in the commit history. (I know that you didn't intend for this patch to be upstreamed at this time, but I thought I'd mention this tip.)

Didn't know that, thanks for the tip. :)

Quoted 6 lines

I'll assume that this means that you're not using git. If you plan to continue doing kernel development (or really any open source development) after this feature, I'd recommend that you work on acquiring basic git skills. You previously asked what can help speed up your development workflow and understanding git is certainly something that will help.

We are actually using git for development. I used diff to generate the patch because I was following Documentation/SubmittingPatches, which gives diff commands.

What should we be using to generate the patches? git diff?

Quoted 5 lines

In fact, this patch won't apply for me using git am -is /path/to/mbox. I'm not immediately sure why, but I do see some extra '-' characters in the areas that git complains about.

Quoted 4 lines

It might be because you GPG-signed the email. GPG signing patches would seem like a good thing to do, but Documentation/email-clients.txt claims that it causes some problems and recommends that you don't sign patches.

I can disable GPG signing for patches easily enough, or switch to using PGP/MIME. Would one or the other be preferable? I don't know if git can deal with PGP/MIME emails.

Quoted 9 lines

Running the following search and replace in vim:

:%s/^- -/-/g

and stripping off the GPG header/footer, I'm kind of able to get the patch to apply. There's offset and fuzz issues, so git am won't apply it (but patch will, of course). Which kernel tree did you develop the patch against?

will@Aldor ~/FYDP/ecryptfs_base $ git log -1
commit f6161aa153581da4a3867a2d1a7caf4be19b6ec9

Author: Linus Torvalds <[email protected]>
Date:   Sun Mar 10 16:54:19 2013 -0700

    Linux 3.9-rc2

Quoted 21 lines

Quoted 4 lines

(quoted diff) +1 -0

+	strncpy(crypt_stat->cipher_mode, +

crypt_stat->mount_crypt_stat->global_default_cipher_mode_name, + ECRYPTFS_MAX_CIPHER_MODE_NAME_SIZE);

This is a problem. eCryptfs files are atomic in that they contain all of the information needed to decrypt the file (except for the FEKEK) in the header. So a single eCryptfs mount may contain files encrypted by AES-128, AES-256, blowfish, the FEK can be wrapped with a public key, multiple FEKEKs can be used, etc., and eCryptfs can seamlessly handle it because of the various metadata stored in the file header.

At first glance, it doesn't look like RFC2440 defines a field for different cipher modes. eCryptfs has always just made the assumption that it should use its tweaked CBC mode and that has always been sufficient.

We will need a place to store the cipher mode in the header and that field will need to be read in somewhere around here.

Quoted 2 lines

Also, when writing out the header information, we'll need to store the cipher mode.

This is something we've been discussing amongst ourselves, and were planning on addressing that in another patch. This was mainly a stop-gap until then. However, since I don't see anything based on a quick read through the RFCs you mentioned, I'll send an email sometime tomorrow regarding this.

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +1 -0

+	const char *mode_white_list[] = {"cbc", "ecb", "gcm"};

When you submit a patch, it should be able to stand on its own. This patch doesn't stand on its own because ecb and gcm aren't supported but this patch happily accepts them. This white list should only contain "cbc" at this point.

Quoted 5 lines

The idea you had with testing with ECB was a good idea, but it isn't something that we'd want to ever support in eCryptfs since our default is more secure. So the "ecb" element should of just been a local change that you made on top of this patch, only for testing.

Will have that removed for actual patch submission.

Quoted 6 lines

Quoted 2 lines

(quoted diff) +1 -0

+			cipher_mode_name_dst = +				mount_crypt_stat-> +

global_default_cipher_mode_name;

Yuck. I don't know if this is better or worse than breaking the 80 char rule. eCryptfs uses variable names that are too long in most cases. (This isn't something that you need to worry about)

Are there specific exceptions to the 80 char rule, or is this an area where it's down to experience and judgment?

Quoted 8 lines

Quoted 3 lines

(quoted diff) +1 -0

+		BUG_ON(cipher_mode_name_len >= +

ECRYPTFS_MAX_CIPHER_MODE_NAME_SIZE);

I see that you copied and pasted this logic from the !cipher_name_set conditional. So it isn't your fault, but this is terrible.

Quoted 3 lines

This is something that should be caught at build time rather than at runtime (why force this BUG() on our users?). These BUG_ON()'s should probably be replaced with a BUILD_BUG_ON().

I'm not too familiar with kernel macros, but my assumption based on a quick google search is that this is something like an assert?

Quoted 8 lines

This was a nice submission for your first shot at it. The biggest problem to fix will be figuring out where to store the mode in the header. Take a look through RFC2440 (and 4880 and 5581) to see if there's already a field we can use for this. If not, we'll have to give it some more thought. I don't want to rely on the user specifying the correct ecryptfs_cipher_mode mount option.

Quoted 4 lines

Thanks again for working on this!

Tyler

Thanks for being helpful and responsive. :)