Harden and limit access to volume key internals.

The volume key structure may often be in configuration where 'key' member does not contain real data. Some examples: - volume key acquired by querring device-mapper where key was originaly passed by kernel keyring reference. - volume key allocated by crypt_alloc_volume_key(size, NULL) With this patch access to internal 'uninitialized' data result in failed assert(). For use cases where key data are not needed (keyring reference wrapper, key length info only) we do not have to allocate and lock the safe buffer in memory. Further improvements might to completely hide the volume key internals and access only via setter and getter functions.
2025-12-05 16:00:05 +01:00 · 2025-02-03 16:59:19 +01:00
parent f86ab28ad6
commit 6ee76934fa
12 changed files with 288 additions and 127 deletions
--- a/lib/luks2/luks2_keyslot.c
+++ b/lib/luks2/luks2_keyslot.c
@@ -339,31 +339,43 @@ static int _open_and_verify(struct crypt_device *cd,
 	int keyslot,
 	const char *password,
 	size_t password_len,
-	struct volume_key **vk)
+	struct volume_key **r_vk)
 {
 	int r, key_size = LUKS2_get_keyslot_stored_key_size(hdr, keyslot);
+	struct volume_key *vk = NULL;
+	void *key = NULL;

 	if (key_size < 0)
 		return -EINVAL;

-	*vk = crypt_alloc_volume_key(key_size, NULL);
-	if (!*vk)
+	key = crypt_safe_alloc(key_size);
+	if (!key)
 		return -ENOMEM;

-	r = h->open(cd, keyslot, password, password_len, (*vk)->key, (*vk)->keylength);
-	if (r < 0)
-		log_dbg(cd, "Keyslot %d (%s) open failed with %d.", keyslot, h->name, r);
-	else
-		r = LUKS2_digest_verify(cd, hdr, *vk, keyslot);
-
+	r = h->open(cd, keyslot, password, password_len, key, key_size);
 	if (r < 0) {
-		crypt_free_volume_key(*vk);
-		*vk = NULL;
+		log_dbg(cd, "Keyslot %d (%s) open failed with %d.", keyslot, h->name, r);
+		goto err;
 	}

-	crypt_volume_key_set_id(*vk, r);
+	vk = crypt_alloc_volume_key_by_safe_alloc(&key);
+	if (!vk) {
+		r = -ENOMEM;
+		goto err;
+	}

-	return r < 0 ? r : keyslot;
+	r = LUKS2_digest_verify(cd, hdr, vk, keyslot);
+	if (r < 0)
+		goto err;
+
+	crypt_volume_key_set_id(vk, r);
+	*r_vk = vk;
+	return keyslot;
+err:
+	crypt_safe_free(key);
+	crypt_free_volume_key(vk);
+
+	return r;
 }

 static int LUKS2_open_and_verify(struct crypt_device *cd,
--- a/lib/luks2/luks2_keyslot_luks2.c
+++ b/lib/luks2/luks2_keyslot_luks2.c
@@ -193,7 +193,6 @@ static int luks2_keyslot_set_key(struct crypt_device *cd,
 	const char *password, size_t passwordLen,
 	const char *volume_key, size_t volume_key_len)
 {
-	struct volume_key *derived_key;
 	char *salt = NULL, cipher[MAX_CIPHER_LEN], cipher_mode[MAX_CIPHER_LEN];
 	char *AfKey = NULL;
 	const char *af_hash = NULL;
@@ -202,6 +201,8 @@ static int luks2_keyslot_set_key(struct crypt_device *cd,
 	uint64_t area_offset;
 	struct crypt_pbkdf_type pbkdf;
 	int r;
+	struct volume_key *derived_vk = NULL;
+	void *derived_key = NULL;

 	if (!json_object_object_get_ex(jobj_keyslot, "kdf", &jobj_kdf) ||
 	    !json_object_object_get_ex(jobj_keyslot, "af", &jobj_af) ||
@@ -239,7 +240,7 @@ static int luks2_keyslot_set_key(struct crypt_device *cd,
 	/*
 	 * Allocate derived key storage.
 	 */
-	derived_key = crypt_alloc_volume_key(keyslot_key_len, NULL);
+	derived_key = crypt_safe_alloc(keyslot_key_len);
 	if (!derived_key) {
 		free(salt);
 		return -ENOMEM;
@@ -250,7 +251,7 @@ static int luks2_keyslot_set_key(struct crypt_device *cd,
 	log_dbg(cd, "Running keyslot key derivation.");
 	r = crypt_pbkdf(pbkdf.type, pbkdf.hash, password, passwordLen,
 			salt, LUKS_SALTSIZE,
-			derived_key->key, derived_key->keylength,
+			derived_key, keyslot_key_len,
 			pbkdf.iterations, pbkdf.max_memory_kb,
 			pbkdf.parallel_threads);
 	free(salt);
@@ -260,16 +261,15 @@ static int luks2_keyslot_set_key(struct crypt_device *cd,
 			log_err(cd, _("PBKDF2 iteration value overflow."));
 		if (r == -ENOMEM)
 			log_err(cd, _("Not enough memory for keyslot key derivation."));
-		crypt_free_volume_key(derived_key);
-		return r;
+		goto out;
 	}

 	// FIXME: verity key_size to AFEKSize
 	AFEKSize = AF_split_sectors(volume_key_len, LUKS_STRIPES) * SECTOR_SIZE;
 	AfKey = crypt_safe_alloc(AFEKSize);
 	if (!AfKey) {
-		crypt_free_volume_key(derived_key);
-		return -ENOMEM;
+		r = -ENOMEM;
+		goto out;
 	}

 	r = crypt_hash_size(af_hash);
@@ -278,15 +278,23 @@ static int luks2_keyslot_set_key(struct crypt_device *cd,
 	else
 		r = AF_split(cd, volume_key, AfKey, volume_key_len, LUKS_STRIPES, af_hash);

-	if (r == 0) {
-		log_dbg(cd, "Updating keyslot area [0x%04" PRIx64 "].", area_offset);
-		/* FIXME: sector_offset should be size_t, fix LUKS_encrypt... accordingly */
-		r = luks2_encrypt_to_storage(AfKey, AFEKSize, cipher, cipher_mode,
-				    derived_key, (unsigned)(area_offset / SECTOR_SIZE), cd);
+	if (r < 0)
+		goto out;
+
+	derived_vk = crypt_alloc_volume_key_by_safe_alloc(&derived_key);
+	if (!derived_vk) {
+		r = -ENOMEM;
+		goto out;
 	}

+	log_dbg(cd, "Updating keyslot area [0x%04" PRIx64 "].", area_offset);
+	/* FIXME: sector_offset should be size_t, fix LUKS_encrypt... accordingly */
+	r = luks2_encrypt_to_storage(AfKey, AFEKSize, cipher, cipher_mode,
+			    derived_vk, (unsigned)(area_offset / SECTOR_SIZE), cd);
+out:
 	crypt_safe_free(AfKey);
-	crypt_free_volume_key(derived_key);
+	crypt_safe_free(derived_key);
+	crypt_free_volume_key(derived_vk);
 	if (r < 0)
 		return r;

@@ -298,7 +306,6 @@ static int luks2_keyslot_get_key(struct crypt_device *cd,
 	const char *password, size_t passwordLen,
 	char *volume_key, size_t volume_key_len)
 {
-	struct volume_key *derived_key = NULL;
 	struct crypt_pbkdf_type pbkdf, *cd_pbkdf;
 	char *AfKey = NULL;
 	size_t AFEKSize;
@@ -309,6 +316,8 @@ static int luks2_keyslot_get_key(struct crypt_device *cd,
 	size_t keyslot_key_len;
 	bool try_serialize_lock = false;
 	int r;
+	struct volume_key *derived_vk = NULL;
+	void *derived_key = NULL;

 	if (!json_object_object_get_ex(jobj_keyslot, "af", &jobj_af) ||
 	    !json_object_object_get_ex(jobj_keyslot, "area", &jobj_area))
@@ -339,7 +348,7 @@ static int luks2_keyslot_get_key(struct crypt_device *cd,
 	/*
 	 * Allocate derived key storage space.
 	 */
-	derived_key = crypt_alloc_volume_key(keyslot_key_len, NULL);
+	derived_key = crypt_safe_alloc(keyslot_key_len);
 	if (!derived_key) {
 		r = -ENOMEM;
 		goto out;
@@ -376,20 +385,27 @@ static int luks2_keyslot_get_key(struct crypt_device *cd,
 	log_dbg(cd, "Running keyslot key derivation.");
 	r = crypt_pbkdf(pbkdf.type, pbkdf.hash, password, passwordLen,
 			salt, LUKS_SALTSIZE,
-			derived_key->key, derived_key->keylength,
+			derived_key, keyslot_key_len,
 			pbkdf.iterations, pbkdf.max_memory_kb,
 			pbkdf.parallel_threads);

 	if (try_serialize_lock)
 		crypt_serialize_unlock(cd);

-	if (r == 0) {
-		log_dbg(cd, "Reading keyslot area [0x%04" PRIx64 "].", area_offset);
-		/* FIXME: sector_offset should be size_t, fix LUKS_decrypt... accordingly */
-		r = luks2_decrypt_from_storage(AfKey, AFEKSize, cipher, cipher_mode,
-				      derived_key, (unsigned)(area_offset / SECTOR_SIZE), cd);
+	if (r < 0)
+		goto out;
+
+	derived_vk = crypt_alloc_volume_key_by_safe_alloc(&derived_key);
+	if (!derived_vk) {
+		r = -ENOMEM;
+		goto out;
 	}

+	log_dbg(cd, "Reading keyslot area [0x%04" PRIx64 "].", area_offset);
+	/* FIXME: sector_offset should be size_t, fix LUKS_decrypt... accordingly */
+	r = luks2_decrypt_from_storage(AfKey, AFEKSize, cipher, cipher_mode,
+			      derived_vk, (unsigned)(area_offset / SECTOR_SIZE), cd);
+
 	if (r == 0) {
 		r = crypt_hash_size(af_hash);
 		if (r < 0)
@@ -399,8 +415,9 @@ static int luks2_keyslot_get_key(struct crypt_device *cd,
 	}
 out:
 	free(salt);
-	crypt_free_volume_key(derived_key);
+	crypt_free_volume_key(derived_vk);
 	crypt_safe_free(AfKey);
+	crypt_safe_free(derived_key);

 	return r;
 }
--- a/lib/luks2/luks2_reencrypt_digest.c
+++ b/lib/luks2/luks2_reencrypt_digest.c
@@ -252,12 +252,13 @@ static int reencrypt_assembly_verification_data(struct crypt_device *cd,
 	struct luks2_hdr *hdr,
 	struct volume_key *vks,
 	uint8_t version,
-	struct volume_key **verification_data)
+	struct volume_key **r_verification_data)
 {
 	uint8_t *ptr;
-	int digest_new, digest_old;
-	struct volume_key *data = NULL, *vk_old = NULL, *vk_new = NULL;
+	int digest_new, digest_old, r = -EINVAL;
+	struct volume_key *verification_data = NULL, *vk_old = NULL, *vk_new = NULL;
 	size_t keyslot_data_len, segments_data_len, data_len = 2;
+	void *data = NULL;

 	/*
 	 * This works up to (including) version v207.
@@ -299,11 +300,11 @@ static int reencrypt_assembly_verification_data(struct crypt_device *cd,
 	data_len += segments_data_len;

 	/* Alloc and fill serialization data */
-	data = crypt_alloc_volume_key(data_len, NULL);
+	data = crypt_safe_alloc(data_len);
 	if (!data)
 		return -ENOMEM;

-	ptr = (uint8_t*)data->key;
+	ptr = (uint8_t*)data;

 	*ptr++ = 0x76;
 	*ptr++ = 0x30 + version;
@@ -324,14 +325,20 @@ static int reencrypt_assembly_verification_data(struct crypt_device *cd,
 		goto bad;
 	ptr += segments_data_len;

-	assert((size_t)(ptr - (uint8_t*)data->key) == data_len);
+	assert((size_t)(ptr - (uint8_t*)data) == data_len);

-	*verification_data = data;
+	verification_data = crypt_alloc_volume_key_by_safe_alloc(&data);
+	if (!verification_data) {
+		r = -ENOMEM;
+		goto bad;
+	}
+	*r_verification_data = verification_data;

 	return 0;
 bad:
-	crypt_free_volume_key(data);
-	return -EINVAL;
+	crypt_safe_free(data);
+	crypt_free_volume_key(verification_data);
+	return r;
 }

 int LUKS2_keyslot_reencrypt_digest_create(struct crypt_device *cd,