[RFC] eCryptfs: Add mount option for cipher mode

[RFC] eCryptfs: Add mount option for cipher mode

[Will Morrison]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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 <camocrazed@gmail.com>
- ---
diff -uprN -X ecryptfs_base/Documentation/dontdiff ecryptfs_base/fs/ecryptfs/crypto.c ecryptfs/fs/ecryptfs/crypto.c
- --- ecryptfs_base/fs/ecryptfs/crypto.c	2013-06-03 07:49:51.734430193 -0400
+++ ecryptfs/fs/ecryptfs/crypto.c	2013-05-23 17:51:37.814355879 -0400
@@ -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
- --- ecryptfs_base/fs/ecryptfs/ecryptfs_kernel.h	2013-06-03 07:49:51.734430193 -0400
+++ ecryptfs/fs/ecryptfs/ecryptfs_kernel.h	2013-05-23 17:51:37.814355879 -0400
@@ -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
- --- ecryptfs_base/fs/ecryptfs/keystore.c	2013-06-03 07:49:51.734430193 -0400
+++ ecryptfs/fs/ecryptfs/keystore.c	2013-06-03 10:45:56.142519966 -0400
@@ -1467,6 +1467,9 @@ parse_tag_3_packet(struct ecryptfs_crypt
 		crypt_stat->key_size =
 			(*new_auth_tok)->session_key.encrypted_key_size;
 	}
+	strncpy(crypt_stat->cipher_mode,
+		crypt_stat->mount_crypt_stat->global_default_cipher_mode_name,
+		ECRYPTFS_MAX_CIPHER_MODE_NAME_SIZE);
 	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
- --- ecryptfs_base/fs/ecryptfs/main.c	2013-06-03 07:49:51.734430193 -0400
+++ ecryptfs/fs/ecryptfs/main.c	2013-06-03 10:54:26.570524304 -0400
@@ -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"},
@@ -258,10 +261,13 @@ static void ecryptfs_init_mount_crypt_st
 static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
 				  uid_t *check_ruid)
 {
+	const char *mode_white_list[] = {"cbc", "ecb", "gcm"};
 	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;
@@ -317,6 +325,18 @@ static int ecryptfs_parse_options(struct
 			cipher_name_dst[ECRYPTFS_MAX_CIPHER_NAME_SIZE] = '\0';
 			cipher_name_set = 1;
 			break;
+		case ecryptfs_opt_cipher_mode:
+		case ecryptfs_opt_ecryptfs_cipher_mode:
+			cipher_mode_name_src = args[0].from;
+			cipher_mode_name_dst =
+				mount_crypt_stat->
+				global_default_cipher_mode_name;
+			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 =
@@ -412,6 +432,14 @@ static int ecryptfs_parse_options(struct
 		strcpy(mount_crypt_stat->global_default_cipher_name,
 		       ECRYPTFS_DEFAULT_CIPHER);
 	}
+	if (!cipher_mode_name_set) {
+		int cipher_mode_name_len = strlen(ECRYPTFS_DEFAULT_CIPHER_MODE);
+
+		BUG_ON(cipher_mode_name_len >=
+			ECRYPTFS_MAX_CIPHER_MODE_NAME_SIZE);
+		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,
+				"eCryptfs doesn't support cipher mode: %s",
+				mount_crypt_stat->
+					global_default_cipher_mode_name);
+		goto out;
+	}
+
 	mutex_lock(&key_tfm_list_mutex);
 	if (!ecryptfs_tfm_exists(mount_crypt_stat->global_default_cipher_name,
 				 NULL)) {
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iQIcBAEBAgAGBQJRrM7JAAoJEH8zVN2+6bAcNDQP/RqMXatRh93P2C8Cz9b5gT+Q
FwYCuyp8dr+TA/CQ+qTl80XdVUlMzAz6121ENm+WjY5M3KjTu5WuCnKRr1v6JfyT
sFDDd+gc+CGOT4KTF1bwIxCUWxkaFWAD2bC0+Bttfb7jjyTMEjOu0YULiHsAXiBi
hOdstGAeayk7Fbkc6cDnrJVaM6n/D9pLvGS+RjUa3NHXWQHAH4zwGpCnER7ZeFcR
p6FFpLoMjbVEco41juvp8ZcWYmerEvSo/N+kmUb1DNoR0m7Dp7h5dbkK9rFFUV9/
CusszUiZ87//79R7fLN9pCuxGAiQiJsnZv//mAMZi3zPWvgLhjMMO2dWyAaA7rGD
YCDlhl3tkaAnPCwUHYc9WDuJR+8MqwZIfTMJqLP1J3Ne9Fie+OheEMyMpOmqafSG
M1pRGX1ZU2UdZEWcn8d0hGSz23wfyM/B+A9/w+1MHrUeWLGPGMR+6fsPThUqPXDz
8Qp+NYCXmabWKWMqshVgjXd7veCKWB/P0rWe/gNX+irM8oII4hUVe+QisxCulJVh
/sg+cZ00V/R6htJDpQCAztUAFxMAPRi+CyC3gsaoYGGLKUDQ6QQJz8WO4uz4Ue5B
U7l1PA9UQ9eJ+oNkGAxLvVQWUC24QRMVegk5i3PfXUu8nFz3vDbikqTNi+IYJ0lu
4jFJhK37ZMwfHja1bkbm
=NWjy
-----END PGP SIGNATURE-----

Re: [RFC] eCryptfs: Add mount option for cipher mode

[Tyler Hicks]

[-- Attachment #1: Type: text/plain, Size: 13958 bytes --]

On 2013-06-03 13:13:45, Will Morrison wrote:
> 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.

Glad to hear that you all are making some headway.

> 
> 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.

Good to know. I'll be a little picky and point out everything that I
see.

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.)

> 
> Signed-off-by: William Morrison <camocrazed@gmail.com>
> ---
> diff -uprN -X ecryptfs_base/Documentation/dontdiff ecryptfs_base/fs/ecryptfs/crypto.c ecryptfs/fs/ecryptfs/crypto.c

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?

> --- ecryptfs_base/fs/ecryptfs/crypto.c	2013-06-03 07:49:51.734430193 -0400
> +++ ecryptfs/fs/ecryptfs/crypto.c	2013-05-23 17:51:37.814355879 -0400
> @@ -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
> --- ecryptfs_base/fs/ecryptfs/ecryptfs_kernel.h	2013-06-03 07:49:51.734430193 -0400
> +++ ecryptfs/fs/ecryptfs/ecryptfs_kernel.h	2013-05-23 17:51:37.814355879 -0400
> @@ -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
> --- ecryptfs_base/fs/ecryptfs/keystore.c	2013-06-03 07:49:51.734430193 -0400
> +++ ecryptfs/fs/ecryptfs/keystore.c	2013-06-03 10:45:56.142519966 -0400
> @@ -1467,6 +1467,9 @@ parse_tag_3_packet(struct ecryptfs_crypt
>  		crypt_stat->key_size =
>  			(*new_auth_tok)->session_key.encrypted_key_size;
>  	}
> +	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.

>  	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
> --- ecryptfs_base/fs/ecryptfs/main.c	2013-06-03 07:49:51.734430193 -0400
> +++ ecryptfs/fs/ecryptfs/main.c	2013-06-03 10:54:26.570524304 -0400
> @@ -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"},
> @@ -258,10 +261,13 @@ static void ecryptfs_init_mount_crypt_st
>  static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
>  				  uid_t *check_ruid)
>  {
> +	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.

>  	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;
> @@ -317,6 +325,18 @@ static int ecryptfs_parse_options(struct
>  			cipher_name_dst[ECRYPTFS_MAX_CIPHER_NAME_SIZE] = '\0';
>  			cipher_name_set = 1;
>  			break;
> +		case ecryptfs_opt_cipher_mode:
> +		case ecryptfs_opt_ecryptfs_cipher_mode:
> +			cipher_mode_name_src = args[0].from;
> +			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)

> +			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 =
> @@ -412,6 +432,14 @@ static int ecryptfs_parse_options(struct
>  		strcpy(mount_crypt_stat->global_default_cipher_name,
>  		       ECRYPTFS_DEFAULT_CIPHER);
>  	}
> +	if (!cipher_mode_name_set) {
> +		int cipher_mode_name_len = strlen(ECRYPTFS_DEFAULT_CIPHER_MODE);
> +
> +		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().

> +		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,
> +				"eCryptfs doesn't support cipher mode: %s",
> +				mount_crypt_stat->
> +					global_default_cipher_mode_name);
> +		goto out;
> +	}
> +
>  	mutex_lock(&key_tfm_list_mutex);
>  	if (!ecryptfs_tfm_exists(mount_crypt_stat->global_default_cipher_name,
>  				 NULL)) {
> --

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC] eCryptfs: Add mount option for cipher mode

[Will Morrison]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 13-06-03 09:51 PM, Tyler Hicks wrote:

> 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. :)

> 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?

> 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.

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.

> 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 <torvalds@linux-foundation.org>
Date:   Sun Mar 10 16:54:19 2013 -0700

    Linux 3.9-rc2

>> +	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.

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.

>> +	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.

Will have that removed for actual patch submission.

>> +			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?

>> +		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().

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?

> 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
> 
Thanks for being helpful and responsive. :)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iQIcBAEBAgAGBQJRrWMkAAoJEH8zVN2+6bAcjnoP/37JHcXKdvQQ+VCOznBkJz/u
J5p01ulqoXd0KJ+tCJQunmGUmVlY7AYk87xr/pkcbQkZru98G/cTbIVVOrQ2Zbu0
+ENGfW8XitghDos5aVD4I9ny1+5txrGitSecoWzzOGduvdAV35Qsm+bgNPxzczZu
ZOI4hkKlBDOKLrw6SsKHHgzW8k/VCML+zSU4Arq3xuMg0VY6SMykRudrWlO4gcXn
EGibwWmh1v/UFZ5qviNb0XYHbWvcmp7YqrVZUzi5gZzO8oUpjgvza0vApm6d/tj/
1SSjouDbhcie1jbLW32dFjVnmp2gr24uymfIjnQPP99816WKCRMoTlnECNvmMwt3
qJMpsROquQZQDFRPYDKMOch4YcrSeBfptHwwYjiI042W71SWoNIxEv43U101IInd
5GG/DVl2+8WBAJqrBt+rlsN93l4PScEaz/82f5ieyPI6T1AV1MqvIeoCeBnGyXhU
W+vM1jHvSYACjyQIiRi5BcUUjEKOr+GQEtlMB6deL75YMmGcoPSO4R7cJAWnyP9F
3w1yE4EDy+O6vavAYnS/8diTcosLohZcxv64qnCGcDXcJSghvhP3If4OvsUFCOHL
wjExnKGXdSf+HSlYCPL42njUR5RHCcwhbR88Jm6JO55GvsU9v33MdBDQ6oTAkiDH
OL11HkuaBgBzEuoftw//
=QEhN
-----END PGP SIGNATURE-----

Re: [RFC] eCryptfs: Add mount option for cipher mode

[Tyler Hicks]

[-- Attachment #1: Type: text/plain, Size: 8065 bytes --]

On 2013-06-03 23:46:44, Will Morrison wrote:
> On 13-06-03 09:51 PM, Tyler Hicks wrote:
> 
> > 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. :)
> 
> > 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?

git format-patch will generate the patches and save them as files

git send-email will email those patches out. You can also use git
format-patch directly and skip the git format-patch step if you'd like.

> 
> > 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.
> 
> 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.

I'm not sure about PGP/MIME. I know that unsigned works perfectly so
that's probably the easiest option.

> 
> > 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 <torvalds@linux-foundation.org>
> Date:   Sun Mar 10 16:54:19 2013 -0700
> 
>     Linux 3.9-rc2

So a little bit behind. Typically, you'd want to rebase and test your
patches at least against current mainline before you send them out. Even
better would be to do it against my next branch
(http://git.kernel.org/cgit/linux/kernel/git/tyhicks/ecryptfs.git/log/?h=next),
which contains the latest eCryptfs patches, but mainline will probably
offer a more stable kernel for you to work against.

I'm really not picky though. eCryptfs development moves slow enough that
you will normally be fine developing against a slightly older codebase.
If there's a big conflict, I'll just ask you to fix it. I'll handle
small conflicts myself.

> 
> >> +	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.
> 
> 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.

Sounds good.

> 
> >> +	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.
> 
> Will have that removed for actual patch submission.
> 
> >> +			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?

Printed strings are one example for breaking the 80 char rule (eCryptfs
wraps them in most cases, though). But, it mostly comes down to
experience/judgement. 

It is probably best if you stay within the 80 char limit in most
situations. If I have a problem with some line wrapping, I'll fix it up
while merging the patch.

> 
> >> +		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().
> 
> 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?

I haven't actually used BUILD_BUG* before. I stumbled upon it after not liking
that runtime BUG_ON(). However, the comments of BUILD_BUG_ON() are pretty
clear:

/**
 * BUILD_BUG_ON - break compile if a condition is true.
 * @condition: the condition which the compiler should know is false.
 *
 * If you have some code which relies on certain constants being equal, or
 * some other compile-time-evaluated condition, you should use BUILD_BUG_ON to
 * detect if someone changes it.

You'll have to give it a try and make sure it works as you expect.

Tyler

> 
> > 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
> > 
> Thanks for being helpful and responsive. :)

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]