Re: [PATCH v2] eCryptfs: Unlock keys needed by ecryptfsd

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


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


[LARTC]     [Bugtraq]     [Yosemite Forum]     [Photo]

Powered by Linux