On Tue Jul 26, 2011 at 03:53:32PM +0200, Roberto Sassu <roberto.sassu@xxxxxxxxx> wrote:
> Hi Tyler
>
> i have some new comments about this patch.
>
>
> On Tuesday, July 26, 2011 06:19:33 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: Alexis Hafner1 <haf@xxxxxxxxxxxxxx>
> > Cc: <da_fox@xxxxxxxxxxxxxxxxx>
> > ---
> > fs/ecryptfs/keystore.c | 65
> > +++++++++++++++++++++++++++-------------------- 1 files changed, 37
> > insertions(+), 28 deletions(-)
> >
> > diff --git a/fs/ecryptfs/keystore.c b/fs/ecryptfs/keystore.c
> > index 27a7fef..1e6e199 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,14 +1162,15 @@ 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);
> > - goto out;
> > + goto out_unlock;
> > }
> > rc = write_tag_64_packet(auth_tok_sig, &(auth_tok->session_key),
> > &payload, &payload_len);
> > if (rc) {
> > ecryptfs_printk(KERN_ERR, "Failed to write tag 64 packet\n");
> > - goto out;
> > + goto out_unlock;
> > }
> > + up_write(&(auth_tok_key->sem));
> > rc = ecryptfs_send_message(payload, payload_len, &msg_ctx);
> > if (rc) {
> > ecryptfs_printk(KERN_ERR, "Error sending message to "
> > @@ -1180,12 +1184,13 @@ decrypt_pki_encrypted_session_key(struct
> > ecryptfs_auth_tok *auth_tok, rc = -EIO;
> > goto out;
> > }
> > + down_write(&(auth_tok_key->sem));
> > rc = parse_tag_65_packet(&(auth_tok->session_key),
> > &cipher_code, msg);
> > if (rc) {
> > printk(KERN_ERR "Failed to parse tag 65 packet; rc = [%d]\n",
> > rc);
> > - goto out;
> > + goto out_unlock;
> > }
> > auth_tok->session_key.flags |= ECRYPTFS_CONTAINS_DECRYPTED_KEY;
> > memcpy(crypt_stat->key, auth_tok->session_key.decrypted_key,
> > @@ -1195,7 +1200,7 @@ decrypt_pki_encrypted_session_key(struct
> > ecryptfs_auth_tok *auth_tok, if (rc) {
> > ecryptfs_printk(KERN_ERR, "Cipher code [%d] is invalid\n",
> > cipher_code)
> > - goto out;
> > + goto out_unlock;
> > }
> > crypt_stat->flags |= ECRYPTFS_KEY_VALID;
> > if (ecryptfs_verbosity > 0) {
> > @@ -1203,6 +1208,8 @@ decrypt_pki_encrypted_session_key(struct
> > ecryptfs_auth_tok *auth_tok, ecryptfs_dump_hex(crypt_stat->key,
> > crypt_stat->key_size);
> > }
> > +out_unlock:
> > + up_write(&(auth_tok_key->sem));
> > out:
> > if (msg)
> > kfree(msg);
> > @@ -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,10 @@ find_next_matching_auth_tok:
> > ecryptfs_printk(KERN_ERR, "Could not find a usable "
> > "authentication token\n");
> > rc = -EIO;
> > + if (auth_tok_key) {
> > + up_write(&(auth_tok_key->sem));
> > + key_put(auth_tok_key);
> > + }
> > goto out_wipe_list;
> > }
>
> I think that unlocking the key here is not necessary,
> since it can be done, in the code below, each time a
> valid authentication token is found in the keyring.
> It is only needed to add the code for unlocking the key
> if the authentication token type is not ECRYPTFS_PRIVATE_KEY
> nor ECRYPTFS_PASSWORD.
I'll make that change.
>
>
> > found_matching_auth_tok:
> > @@ -1909,7 +1915,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 +1924,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 i understand correctly, 'candidate_auth_tok' is allocated
> during the packets parsing, while the element which we
> want to protect is 'matching_auth_tok'. So, in this case, it
> seems that the key unlock can be performed just after the
> memcpy().
Nice catch. I think that it is the same for the ECRYPTFS_PRIVATE_KEY
auth tok block, too. I may not need to pass the auth_tok_key down into
decrypt_pki_encrypted_session_key().
I'll look into that a bit more and then send out v3.
Tyler
>
> Roberto Sassu
>
>
> > }
> > if (rc) {
> > struct ecryptfs_auth_tok_list_item *auth_tok_list_item_tmp;
> > @@ -1956,15 +1965,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 +1985,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 +2016,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 +2028,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 +2049,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 +2434,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 +2453,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 +2463,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 +2482,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;
> > }
> --
> 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
