On Wed Jul 20, 2011 at 11:03:44AM +0200, Roberto Sassu <roberto.sassu@xxxxxxxxx> wrote:
> Hi Tyler
>
> i looked at your patch more in deep and i found
> some issues.
Thanks for the review.
>
> Ok, the key lock should be released before requesting
> the FEK decryption from ecryptfsd. However, the kernel
> and the userspace parts of eCryptfs may see different
> authentication tokens because the key is not locked.
>
> Other comments are inline.
>
>
> On Tuesday, July 19, 2011 07:58:14 AM Tyler Hicks wrote:
> > Fixes a regression caused by b5695d04634fa4ccca7dcbc05bb4a66522f02e0b
> >
> > Kernel keyring keys containing eCryptfs authentication tokens should not
> > be write locked when calling out to ecryptfsd to wrap and unwrap file
> > encryption keys. The eCryptfs kernel code can not hold the key's write
> > lock because ecryptfsd needs to request the key after receiving such a
> > request from the kernel.
> >
> > Without this fix, all file opens and creates will timeout and fail when
> > using the eCryptfs PKI infrastructure. This is not an issue when using
> > passphrase-based mount keys, which is the most widely deployed eCryptfs
> > configuration.
> >
> > Signed-off-by: Tyler Hicks <tyhicks@xxxxxxxxxxxxxxxxxx>
> > Cc: Roberto Sassu <roberto.sassu@xxxxxxxxx>
> > Cc: <da_fox@xxxxxxxxxxxxxxxxx>
> > ---
> > fs/ecryptfs/keystore.c | 55 +++++++++++++++++++++++++++---------------------
> > 1 files changed, 31 insertions(+), 24 deletions(-)
> >
> > diff --git a/fs/ecryptfs/keystore.c b/fs/ecryptfs/keystore.c
> > index 27a7fef..c533ef6 100644
> > --- a/fs/ecryptfs/keystore.c
> > +++ b/fs/ecryptfs/keystore.c
> > @@ -1138,13 +1138,16 @@ ecryptfs_get_auth_tok_sig(char **sig, struct ecryptfs_auth_tok *auth_tok)
> >
> > /**
> > * decrypt_pki_encrypted_session_key - Decrypt the session key with the given auth_tok.
> > + * @auth_tok_key: The authentication token key to unlock and put when done with
> > + * @auth_tok
> > * @auth_tok: The key authentication token used to decrypt the session key
> > * @crypt_stat: The cryptographic context
> > *
> > * Returns zero on success; non-zero error otherwise.
> > */
> > static int
> > -decrypt_pki_encrypted_session_key(struct ecryptfs_auth_tok *auth_tok,
> > +decrypt_pki_encrypted_session_key(struct key *auth_tok_key,
> > + struct ecryptfs_auth_tok *auth_tok,
> > struct ecryptfs_crypt_stat *crypt_stat)
> > {
> > u8 cipher_code = 0;
> > @@ -1159,10 +1162,14 @@ decrypt_pki_encrypted_session_key(struct ecryptfs_auth_tok *auth_tok,
> > if (rc) {
> > printk(KERN_ERR "Unrecognized auth tok type: [%d]\n",
> > auth_tok->token_type);
> > + up_write(&(auth_tok_key->sem));
> > + key_put(auth_tok_key);
> > goto out;
> > }
> > rc = write_tag_64_packet(auth_tok_sig, &(auth_tok->session_key),
> > &payload, &payload_len);
> > + up_write(&(auth_tok_key->sem));
> > + key_put(auth_tok_key);
>
> The key should be locked another time after ecryptfs_wait_for_response()
> because you are modifying the authentication token.
Agreed, I'll make this change and send out the new patch.
>
>
> > if (rc) {
> > ecryptfs_printk(KERN_ERR, "Failed to write tag 64 packet\n");
> > goto out;
> > @@ -1868,11 +1875,6 @@ int ecryptfs_parse_packet_set(struct ecryptfs_crypt_stat *crypt_stat,
> > * just one will be sufficient to decrypt to get the FEK. */
> > find_next_matching_auth_tok:
> > found_auth_tok = 0;
> > - if (auth_tok_key) {
> > - up_write(&(auth_tok_key->sem));
> > - key_put(auth_tok_key);
> > - auth_tok_key = NULL;
> > - }
> > list_for_each_entry(auth_tok_list_item, &auth_tok_list, list) {
> > candidate_auth_tok = &auth_tok_list_item->auth_tok;
> > if (unlikely(ecryptfs_verbosity > 0)) {
> > @@ -1902,6 +1904,8 @@ find_next_matching_auth_tok:
> > ecryptfs_printk(KERN_ERR, "Could not find a usable "
> > "authentication token\n");
> > rc = -EIO;
> > + up_write(&(auth_tok_key->sem));
> > + key_put(auth_tok_key);
>
> You must check whether the 'auth_tok_key' pointer is NULL,
> otherwise a NULL pointer exception arises if a key request
> fails.
Agreed.
>
>
> > goto out_wipe_list;
> > }
> > found_matching_auth_tok:
> > @@ -1909,7 +1913,8 @@ found_matching_auth_tok:
> > memcpy(&(candidate_auth_tok->token.private_key),
> > &(matching_auth_tok->token.private_key),
> > sizeof(struct ecryptfs_private_key));
> > - rc = decrypt_pki_encrypted_session_key(candidate_auth_tok,
> > + rc = decrypt_pki_encrypted_session_key(auth_tok_key,
> > + candidate_auth_tok,
> > crypt_stat);
> > } else if (candidate_auth_tok->token_type == ECRYPTFS_PASSWORD) {
> > memcpy(&(candidate_auth_tok->token.password),
> > @@ -1917,6 +1922,8 @@ found_matching_auth_tok:
> > sizeof(struct ecryptfs_password));
> > rc = decrypt_passphrase_encrypted_session_key(
> > candidate_auth_tok, crypt_stat);
> > + up_write(&(auth_tok_key->sem));
> > + key_put(auth_tok_key);
> > }
> > if (rc) {
> > struct ecryptfs_auth_tok_list_item *auth_tok_list_item_tmp;
> > @@ -1956,15 +1963,12 @@ found_matching_auth_tok:
> > out_wipe_list:
> > wipe_auth_tok_list(&auth_tok_list);
> > out:
> > - if (auth_tok_key) {
> > - up_write(&(auth_tok_key->sem));
> > - key_put(auth_tok_key);
> > - }
> > return rc;
> > }
> >
> > static int
> > -pki_encrypt_session_key(struct ecryptfs_auth_tok *auth_tok,
> > +pki_encrypt_session_key(struct key *auth_tok_key,
> > + struct ecryptfs_auth_tok *auth_tok,
> > struct ecryptfs_crypt_stat *crypt_stat,
> > struct ecryptfs_key_record *key_rec)
> > {
> > @@ -1979,6 +1983,8 @@ pki_encrypt_session_key(struct ecryptfs_auth_tok *auth_tok,
> > crypt_stat->cipher,
> > crypt_stat->key_size),
> > crypt_stat, &payload, &payload_len);
> > + up_write(&(auth_tok_key->sem));
> > + key_put(auth_tok_key);
> > if (rc) {
> > ecryptfs_printk(KERN_ERR, "Error generating tag 66 packet\n");
> > goto out;
> > @@ -2008,6 +2014,8 @@ out:
> > * write_tag_1_packet - Write an RFC2440-compatible tag 1 (public key) packet
> > * @dest: Buffer into which to write the packet
> > * @remaining_bytes: Maximum number of bytes that can be writtn
> > + * @auth_tok_key: The authentication token key to unlock and put when done with
> > + * @auth_tok
> > * @auth_tok: The authentication token used for generating the tag 1 packet
> > * @crypt_stat: The cryptographic context
> > * @key_rec: The key record struct for the tag 1 packet
> > @@ -2018,7 +2026,7 @@ out:
> > */
> > static int
> > write_tag_1_packet(char *dest, size_t *remaining_bytes,
> > - struct ecryptfs_auth_tok *auth_tok,
> > + struct key *auth_tok_key, struct ecryptfs_auth_tok *auth_tok,
> > struct ecryptfs_crypt_stat *crypt_stat,
> > struct ecryptfs_key_record *key_rec, size_t *packet_size)
> > {
> > @@ -2039,12 +2047,15 @@ write_tag_1_packet(char *dest, size_t *remaining_bytes,
> > memcpy(key_rec->enc_key,
> > auth_tok->session_key.encrypted_key,
> > auth_tok->session_key.encrypted_key_size);
> > + up_write(&(auth_tok_key->sem));
> > + key_put(auth_tok_key);
> > goto encrypted_session_key_set;
> > }
> > if (auth_tok->session_key.encrypted_key_size == 0)
> > auth_tok->session_key.encrypted_key_size =
> > auth_tok->token.private_key.key_size;
> > - rc = pki_encrypt_session_key(auth_tok, crypt_stat, key_rec);
> > + rc = pki_encrypt_session_key(auth_tok_key, auth_tok, crypt_stat,
> > + key_rec);
> > if (rc) {
> > printk(KERN_ERR "Failed to encrypt session key via a key "
> > "module; rc = [%d]\n", rc);
> > @@ -2421,6 +2432,8 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> > &max, auth_tok,
> > crypt_stat, key_rec,
> > &written);
> > + up_write(&(auth_tok_key->sem));
> > + key_put(auth_tok_key);
> > if (rc) {
> > ecryptfs_printk(KERN_WARNING, "Error "
> > "writing tag 3 packet\n");
> > @@ -2438,8 +2451,8 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> > }
> > (*len) += written;
> > } else if (auth_tok->token_type == ECRYPTFS_PRIVATE_KEY) {
> > - rc = write_tag_1_packet(dest_base + (*len),
> > - &max, auth_tok,
> > + rc = write_tag_1_packet(dest_base + (*len), &max,
> > + auth_tok_key, auth_tok,
> > crypt_stat, key_rec, &written);
> > if (rc) {
> > ecryptfs_printk(KERN_WARNING, "Error "
> > @@ -2448,14 +2461,13 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> > }
> > (*len) += written;
> > } else {
> > + up_write(&(auth_tok_key->sem));
> > + key_put(auth_tok_key);
> > ecryptfs_printk(KERN_WARNING, "Unsupported "
> > "authentication token type\n");
> > rc = -EINVAL;
> > goto out_free;
> > }
> > - up_write(&(auth_tok_key->sem));
> > - key_put(auth_tok_key);
> > - auth_tok_key = NULL;
> > }
> > if (likely(max > 0)) {
> > dest_base[(*len)] = 0x00;
> > @@ -2468,11 +2480,6 @@ out_free:
> > out:
> > if (rc)
> > (*len) = 0;
> > - if (auth_tok_key) {
> > - up_write(&(auth_tok_key->sem));
> > - key_put(auth_tok_key);
> > - }
> > -
> > mutex_unlock(&crypt_stat->keysig_list_mutex);
> > return rc;
> > }
> >
>
> I think a safer solution may be to pass all the data required
> for the external decryption from the kernel side to ecryptfsd,
> so that the latter can perform the operation without
> requesting the key.
You're right, but that would require some significant changes to the
kernel and userspace code. I want to get this regression fix out and fix
some other higher priority issues before I look at making these changes.
Thanks again for the review.
Tyler
>
> Roberto Sassu
> --
> To unsubscribe from this list: send the line "unsubscribe ecryptfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe ecryptfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
