[PATCH] misc: fixed error handling in e4crypt

[PATCH] misc: fixed error handling in e4crypt

[Joe Richey]

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

Due to some interesting behaviour in keyctl (as described in the
comments), we use KEYCTL_GET_KEYRING_ID to translate the special value
of KEY_SPEC_SESSION_KEYRING to a real keyring id. However, how we
currently do this is flawed in two ways.

First, if KEYCTL_GET_KEYRING_ID fails, we don't detect it as it returns
-1 and zero is used for an error value in get_keyring_id. Second, if the
user specifies "-k @s" the translation never runs and the undesireable
behavior occurs.

These are both fixed by doing the translation outside of get_keyring_id.

Signed-off-by: Joe Richey <joerichey@google.com>
---
 misc/e4crypt.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/misc/e4crypt.c b/misc/e4crypt.c
index 04faca3..fd2fc8f 100644
--- a/misc/e4crypt.c
+++ b/misc/e4crypt.c
@@ -507,24 +507,12 @@ static int get_keyring_id(const char *keyring)
 
 	/*
 	 * If no keyring is specified, by default use either the user
-	 * session key ring or the session keyring.  Fetching the
+	 * session keyring or the session keyring.  Fetching the
 	 * session keyring will return the user session keyring if no
 	 * session keyring has been set.
-	 *
-	 * We need to do this instead of simply adding the key to
-	 * KEY_SPEC_SESSION_KEYRING since trying to add a key to a
-	 * session keyring that does not yet exist will cause the
-	 * kernel to create a session keyring --- which wil then get
-	 * garbage collected as soon as e4crypt exits.
-	 *
-	 * The fact that the keyctl system call and the add_key system
-	 * call treats KEY_SPEC_SESSION_KEYRING differently when a
-	 * session keyring does not exist is very unfortunate and
-	 * confusing, but so it goes...
 	 */
 	if (keyring == NULL)
-		return keyctl(KEYCTL_GET_KEYRING_ID,
-			      KEY_SPEC_SESSION_KEYRING, 0);
+		return KEY_SPEC_SESSION_KEYRING;
 	for (x = 0; x < (sizeof(keyrings) / sizeof(keyrings[0])); ++x) {
 		if (strcmp(keyring, keyrings[x].name) == 0) {
 			return keyrings[x].code;
@@ -585,6 +573,24 @@ static void insert_key_into_keyring(const char *keyring, struct salt *salt)
 	key.mode = EXT4_ENCRYPTION_MODE_AES_256_XTS;
 	memcpy(key.raw, salt->key, EXT4_MAX_KEY_SIZE);
 	key.size = EXT4_MAX_KEY_SIZE;
+
+	/*
+	 * We need to do this instead of simply adding the key to
+	 * KEY_SPEC_SESSION_KEYRING since trying to add a key to a
+	 * session keyring that does not yet exist will cause the
+	 * kernel to create a session keyring --- which wil then get
+	 * garbage collected as soon as e4crypt exits.
+	 *
+	 * The fact that the keyctl system call and the add_key system
+	 * call treats KEY_SPEC_SESSION_KEYRING differently when a
+	 * session keyring does not exist is very unfortunate and
+	 * confusing, but so it goes...
+	 */
+	if (keyring_id == KEY_SPEC_SESSION_KEYRING) {
+		keyring_id = keyctl(KEYCTL_GET_KEYRING_ID, keyring_id, 0);
+		if (keyring_id < 0)
+			printf("Could not get session keyring.\n");
+	}
 	rc = add_key(EXT2FS_KEY_TYPE_LOGON, key_ref_full, (void *)&key,
 		     sizeof(key), keyring_id);
 	if (rc == -1) {
-- 
2.7.4


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4849 bytes --]

[PATCH] misc: fixed error handling in e4crypt

[Joe Richey]

From: Joe Richey <joerichey@google.com>

Due to some interesting behaviour in keyctl (as described in the
comments), we use KEYCTL_GET_KEYRING_ID to translate the special value
of KEY_SPEC_SESSION_KEYRING to a real keyring id. However, how we
currently do this is flawed in two ways.

First, if KEYCTL_GET_KEYRING_ID fails, we don't detect it as it returns
-1 and zero is used for an error value in get_keyring_id. Second, if the
user specifies "-k @s" the translation never runs and the undesireable
behavior occurs.

These are both fixed by doing the translation outside of get_keyring_id.

Signed-off-by: Joe Richey <joerichey@google.com>
---
 misc/e4crypt.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/misc/e4crypt.c b/misc/e4crypt.c
index 04faca3..600d0b7 100644
--- a/misc/e4crypt.c
+++ b/misc/e4crypt.c
@@ -507,24 +507,12 @@ static int get_keyring_id(const char *keyring)
 
 	/*
 	 * If no keyring is specified, by default use either the user
-	 * session key ring or the session keyring.  Fetching the
+	 * session keyring or the session keyring.  Fetching the
 	 * session keyring will return the user session keyring if no
 	 * session keyring has been set.
-	 *
-	 * We need to do this instead of simply adding the key to
-	 * KEY_SPEC_SESSION_KEYRING since trying to add a key to a
-	 * session keyring that does not yet exist will cause the
-	 * kernel to create a session keyring --- which wil then get
-	 * garbage collected as soon as e4crypt exits.
-	 *
-	 * The fact that the keyctl system call and the add_key system
-	 * call treats KEY_SPEC_SESSION_KEYRING differently when a
-	 * session keyring does not exist is very unfortunate and
-	 * confusing, but so it goes...
 	 */
 	if (keyring == NULL)
-		return keyctl(KEYCTL_GET_KEYRING_ID,
-			      KEY_SPEC_SESSION_KEYRING, 0);
+		return KEY_SPEC_SESSION_KEYRING;
 	for (x = 0; x < (sizeof(keyrings) / sizeof(keyrings[0])); ++x) {
 		if (strcmp(keyring, keyrings[x].name) == 0) {
 			return keyrings[x].code;
@@ -585,6 +573,27 @@ static void insert_key_into_keyring(const char *keyring, struct salt *salt)
 	key.mode = EXT4_ENCRYPTION_MODE_AES_256_XTS;
 	memcpy(key.raw, salt->key, EXT4_MAX_KEY_SIZE);
 	key.size = EXT4_MAX_KEY_SIZE;
+
+	/*
+	 * We need to do this instead of simply adding the key to
+	 * KEY_SPEC_SESSION_KEYRING since trying to add a key to a
+	 * session keyring that does not yet exist will cause the
+	 * kernel to create a session keyring --- which wil then get
+	 * garbage collected as soon as e4crypt exits.
+	 *
+	 * The fact that the keyctl system call and the add_key system
+	 * call treats KEY_SPEC_SESSION_KEYRING differently when a
+	 * session keyring does not exist is very unfortunate and
+	 * confusing, but so it goes...
+	 */
+	if (keyring_id == KEY_SPEC_SESSION_KEYRING) {
+		keyring_id = keyctl(KEYCTL_GET_KEYRING_ID, keyring_id, 0);
+		if (keyring_id < 0) {
+			printf("Error getting session keyring ID: %s\n",
+			       strerror(errno));
+			exit(1);
+		}
+	}
 	rc = add_key(EXT2FS_KEY_TYPE_LOGON, key_ref_full, (void *)&key,
 		     sizeof(key), keyring_id);
 	if (rc == -1) {
-- 
2.7.4

Re: [PATCH] misc: fixed error handling in e4crypt

[Joseph Richey]

Thanks for catching this! KEYCTL_GET_KEYRING_ID returns -1 on error, so
e4crypt should definitely exit here. Sending the revised path.

Joe

On Sat, Apr 1, 2017 at 9:24 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Mon, Mar 20, 2017 at 08:34:36PM +0000, Joe Richey wrote:
>> +     if (keyring_id == KEY_SPEC_SESSION_KEYRING) {
>> +             keyring_id = keyctl(KEYCTL_GET_KEYRING_ID, keyring_id, 0);
>> +             if (keyring_id < 0)
>> +                     printf("Could not get session keyring.\n");
>> +     }
>>       rc = add_key(EXT2FS_KEY_TYPE_LOGON, key_ref_full, (void *)&key,
>>                    sizeof(key), keyring_id);
>
> Are you sure this is right?  If keyring_id is negative, we'll print
> the warning/error message --- and then pass the negative keyring_id to
> add_key.  Is that going to do the right thing?
>
>                                         - Ted

Re: [PATCH] misc: fixed error handling in e4crypt

[Theodore Ts'o]

On Mon, Mar 20, 2017 at 08:34:36PM +0000, Joe Richey wrote:
> +	if (keyring_id == KEY_SPEC_SESSION_KEYRING) {
> +		keyring_id = keyctl(KEYCTL_GET_KEYRING_ID, keyring_id, 0);
> +		if (keyring_id < 0)
> +			printf("Could not get session keyring.\n");
> +	}
>  	rc = add_key(EXT2FS_KEY_TYPE_LOGON, key_ref_full, (void *)&key,
>  		     sizeof(key), keyring_id);

Are you sure this is right?  If keyring_id is negative, we'll print
the warning/error message --- and then pass the negative keyring_id to
add_key.  Is that going to do the right thing?

	     	  	      	  	- Ted

[PATCH] misc: fixed error handling in e4crypt

[Joe Richey]

From: Joe Richey <joerichey@google.com>

Due to some interesting behaviour in keyctl (as described in the
comments), we use KEYCTL_GET_KEYRING_ID to translate the special value
of KEY_SPEC_SESSION_KEYRING to a real keyring id. However, how we
currently do this is flawed in two ways.

First, if KEYCTL_GET_KEYRING_ID fails, we don't detect it as it returns
-1 and zero is used for an error value in get_keyring_id. Second, if the
user specifies "-k @s" the translation never runs and the undesireable
behavior occurs.

These are both fixed by doing the translation outside of get_keyring_id.

Signed-off-by: Joe Richey <joerichey@google.com>
---
 misc/e4crypt.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/misc/e4crypt.c b/misc/e4crypt.c
index 04faca3..fd2fc8f 100644
--- a/misc/e4crypt.c
+++ b/misc/e4crypt.c
@@ -507,24 +507,12 @@ static int get_keyring_id(const char *keyring)
 
 	/*
 	 * If no keyring is specified, by default use either the user
-	 * session key ring or the session keyring.  Fetching the
+	 * session keyring or the session keyring.  Fetching the
 	 * session keyring will return the user session keyring if no
 	 * session keyring has been set.
-	 *
-	 * We need to do this instead of simply adding the key to
-	 * KEY_SPEC_SESSION_KEYRING since trying to add a key to a
-	 * session keyring that does not yet exist will cause the
-	 * kernel to create a session keyring --- which wil then get
-	 * garbage collected as soon as e4crypt exits.
-	 *
-	 * The fact that the keyctl system call and the add_key system
-	 * call treats KEY_SPEC_SESSION_KEYRING differently when a
-	 * session keyring does not exist is very unfortunate and
-	 * confusing, but so it goes...
 	 */
 	if (keyring == NULL)
-		return keyctl(KEYCTL_GET_KEYRING_ID,
-			      KEY_SPEC_SESSION_KEYRING, 0);
+		return KEY_SPEC_SESSION_KEYRING;
 	for (x = 0; x < (sizeof(keyrings) / sizeof(keyrings[0])); ++x) {
 		if (strcmp(keyring, keyrings[x].name) == 0) {
 			return keyrings[x].code;
@@ -585,6 +573,24 @@ static void insert_key_into_keyring(const char *keyring, struct salt *salt)
 	key.mode = EXT4_ENCRYPTION_MODE_AES_256_XTS;
 	memcpy(key.raw, salt->key, EXT4_MAX_KEY_SIZE);
 	key.size = EXT4_MAX_KEY_SIZE;
+
+	/*
+	 * We need to do this instead of simply adding the key to
+	 * KEY_SPEC_SESSION_KEYRING since trying to add a key to a
+	 * session keyring that does not yet exist will cause the
+	 * kernel to create a session keyring --- which wil then get
+	 * garbage collected as soon as e4crypt exits.
+	 *
+	 * The fact that the keyctl system call and the add_key system
+	 * call treats KEY_SPEC_SESSION_KEYRING differently when a
+	 * session keyring does not exist is very unfortunate and
+	 * confusing, but so it goes...
+	 */
+	if (keyring_id == KEY_SPEC_SESSION_KEYRING) {
+		keyring_id = keyctl(KEYCTL_GET_KEYRING_ID, keyring_id, 0);
+		if (keyring_id < 0)
+			printf("Could not get session keyring.\n");
+	}
 	rc = add_key(EXT2FS_KEY_TYPE_LOGON, key_ref_full, (void *)&key,
 		     sizeof(key), keyring_id);
 	if (rc == -1) {
-- 
2.7.4