Fix luksFormat to properly use key file with --master-key-file switch.

Fix possible double free when handling master key file. git-svn-id: https://cryptsetup.googlecode.com/svn/trunk@357 36d66b0a-2a48-0410-832c-cd162a569da5
2025-12-12 11:20:10 +01:00 · 2010-10-29 20:04:01 +00:00
parent 2fbf5994f4
commit 791481b9e9
3 changed files with 46 additions and 50 deletions
--- a/2
+++ b/2
@@ -8,6 +8,8 @@
 	* Allow UUID setting in luksFormat and luksUUID (--uuid parameter).
 	* Add --keyfile-size and --new-keyfile-size (in bytes) size and disallow overloading
 	  of --key-size for limiting keyfile reads.
+	* Fix luksFormat to properly use key file with --master-key-file switch.
+	* Fix possible double free when handling master key file.

 2010-10-17  Milan Broz  <mbroz@redhat.com>
 	* Add crypt_get_device_name() to API (get underlying device name).
--- a/src/cryptsetup.c
+++ b/src/cryptsetup.c
@@ -311,16 +311,19 @@ static int _read_mk(const char *file, char **key, int keysize)
 	fd = open(file, O_RDONLY);
 	if (fd == -1) {
 		log_err("Cannot read keyfile %s.\n", file);
-		return -EINVAL;
+		goto fail;
 	}
 	if ((read(fd, *key, keysize) != keysize)) {
 		log_err("Cannot read %d bytes from keyfile %s.\n", keysize, file);
 		close(fd);
-		crypt_safe_free(*key);
-		return -EINVAL;
+		goto fail;
 	}
 	close(fd);
 	return 0;
+fail:
+	crypt_safe_free(*key);
+	*key = NULL;
+	return -EINVAL;
 }

 static int action_luksFormat(int arg)
@@ -336,13 +339,6 @@ static int action_luksFormat(int arg)
 		.data_alignment = opt_align_payload,
 	};

-	/* Avoid overwriting possibly wrong part of device than user requested by rejecting these options */
-	if (opt_offset || opt_skip) {
-		log_err("Options --offset and --skip are not supported for luksFormat.\n");
-		r = -EINVAL;
-		goto out;;
-	}
-
 	if (action_argc > 1) {
 		key_file = action_argv[1];
 		if (opt_key_file)
@@ -355,13 +351,10 @@ static int action_luksFormat(int arg)
 		r = -ENOMEM;
 		goto out;
 	}
-	r = yesDialog(msg);
+	r = yesDialog(msg) ? 0 : -EINVAL;
 	free(msg);
-
-	if (!r) {
-		r = -EINVAL;
+	if (r < 0)
 		goto out;
-	}

 	r = crypt_parse_name_and_mode(opt_cipher ?: DEFAULT_CIPHER(LUKS1),
 				      cipher, cipher_mode);
@@ -387,10 +380,21 @@ static int action_luksFormat(int arg)
 	else if (opt_urandom)
 		crypt_set_rng_type(cd, CRYPT_RNG_URANDOM);

+	r = -EINVAL;
+	crypt_get_key(_("Enter LUKS passphrase: "),
+		      &password, &passwordLen,
+		      opt_keyfile_size, key_file,
+		      opt_timeout,
+		      opt_batch_mode ? 0 : 1, /* always verify */
+		      cd);
+	if(!password)
+		goto out;
+
 	if (opt_master_key_file) {
 		r = _read_mk(opt_master_key_file, &key, keysize);
 		if (r < 0)
 			goto out;
+	}

 	r = crypt_format(cd, CRYPT_LUKS1, cipher, cipher_mode,
 			 opt_uuid, key, keysize, &params);
@@ -398,29 +402,8 @@ static int action_luksFormat(int arg)
 		goto out;

 	r = crypt_keyslot_add_by_volume_key(cd, opt_key_slot,
-						    key, keysize, NULL, 0);
-	} else {
-		crypt_get_key(_("Enter LUKS passphrase: "),
-			      &password, &passwordLen,
-			      opt_keyfile_size, key_file,
-			      opt_timeout,
-			      opt_batch_mode ? 0 : 1, /* always verify */
-			      cd);
-
-		if(!password) {
-			r = -EINVAL;
-			goto out;
-		}
-
-		r = crypt_format(cd, CRYPT_LUKS1, cipher, cipher_mode,
-				 opt_uuid, NULL, keysize, &params);
-		if (r < 0)
-			goto out;
-
-		/* Add keyslot using internally stored volume key generated during format */
-		r = crypt_keyslot_add_by_volume_key(cd, opt_key_slot,
-						    NULL, 0, password, passwordLen);
-	}
+					    key, keysize,
+					    password, passwordLen);
 out:
 	crypt_free(cd);
 	crypt_safe_free(key);
@@ -876,6 +859,10 @@ int main(int argc, char **argv)
 		usage(popt_context, 1, _("Option --uuid is allowed only for luksFormat and luksUUID."),
 		      poptGetInvocationName(popt_context));

+	if ((opt_offset || opt_skip) && strcmp(aname, "create"))
+		usage(popt_context, 1, _("Options --offset and --skip are supported only for create command.\n"),
+		      poptGetInvocationName(popt_context));
+
 	action_argc = 0;
 	action_argv = poptGetArgs(popt_context);
 	/* Make return values of poptGetArgs more consistent in case of remaining argc = 0 */
--- a/tests/compat-test
+++ b/tests/compat-test
@@ -92,7 +92,7 @@ echo "compatkey" | $CRYPTSETUP luksOpen $LOOPDEV $DEV_NAME || fail
 check_exists

 prepare "[2] open - compat image - denial check" new
-echo "wrongkey" | $CRYPTSETUP luksOpen $LOOPDEV $DEV_NAME && fail
+echo "wrongkey" | $CRYPTSETUP luksOpen $LOOPDEV $DEV_NAME 2>/dev/null && fail
 check

 # All headers items and first key material section must change
@@ -116,7 +116,7 @@ echo "key1" | $CRYPTSETUP luksOpen $LOOPDEV $DEV_NAME || fail

 # Unsuccessful Key Delete - nothing may change
 prepare "[7] unsuccessful delete"
-echo "invalid" | $CRYPTSETUP luksKillSlot $LOOPDEV 1 && fail
+echo "invalid" | $CRYPTSETUP luksKillSlot $LOOPDEV 1 2>/dev/null && fail
 check

 # Delete Key Test
@@ -124,7 +124,7 @@ check
 prepare "[8] successful delete"
 $CRYPTSETUP -q luksKillSlot $LOOPDEV 1 || fail
 check "$KEY_SLOT1 $KEY_MATERIAL1_EXT"
-echo "key1" | $CRYPTSETUP luksOpen $LOOPDEV $DEV_NAME && fail
+echo "key1" | $CRYPTSETUP luksOpen $LOOPDEV $DEV_NAME 2> /dev/null && fail
 echo "key0" | $CRYPTSETUP luksOpen $LOOPDEV $DEV_NAME || fail

 # Key Slot 1 and key material section 1 must change, the rest must not
@@ -137,14 +137,14 @@ $CRYPTSETUP -d $KEY1 luksOpen $LOOPDEV $DEV_NAME || fail
 prepare "[10] delete key test with key1 as remaining key"
 $CRYPTSETUP -d $KEY1 luksKillSlot $LOOPDEV 0 || fail
 check "$KEY_SLOT0 $KEY_MATERIAL0_EXT"
-echo "key0" | $CRYPTSETUP luksOpen $LOOPDEV $DEV_NAME && fail
+echo "key0" | $CRYPTSETUP luksOpen $LOOPDEV $DEV_NAME 2>/dev/null && fail
 $CRYPTSETUP luksOpen -d $KEY1 $LOOPDEV $DEV_NAME || fail

 # Delete last slot
 prepare "[11] delete last key" wipe
 echo "key0" | $CRYPTSETUP luksFormat $LOOPDEV || fail
 echo "key0" | $CRYPTSETUP luksKillSlot $LOOPDEV 0 || fail
-echo "key0" | $CRYPTSETUP luksOpen $LOOPDEV $DEV_NAME && fail
+echo "key0" | $CRYPTSETUP luksOpen $LOOPDEV $DEV_NAME 2>/dev/null && fail

 # Format test for ESSIV, and some other parameters.
 prepare "[12] parameter variation test" wipe
@@ -165,7 +165,7 @@ prepare	"[14] format/open - passphrase on stdin & new line" wipe
 echo -n $'foo\nbar' | $CRYPTSETUP -q luksFormat $LOOPDEV - || fail
 echo -n $'foo\nbar' | $CRYPTSETUP -q --key-file=- luksOpen $LOOPDEV $DEV_NAME || fail
 $CRYPTSETUP -q luksClose  $DEV_NAME || fail
-echo -n $'foo\nbar' | $CRYPTSETUP -q luksOpen $LOOPDEV $DEV_NAME && fail
+echo -n $'foo\nbar' | $CRYPTSETUP -q luksOpen $LOOPDEV $DEV_NAME 2>/dev/null && fail
 # now also try --key-file
 echo -n $'foo\nbar' | $CRYPTSETUP -q luksFormat $LOOPDEV --key-file=- || fail
 echo -n $'foo\nbar' | $CRYPTSETUP -q --key-file=- luksOpen $LOOPDEV $DEV_NAME || fail
@@ -185,5 +185,12 @@ $CRYPTSETUP -q luksUUID --uuid $TEST_UUID $LOOPDEV || fail
 tst=$($CRYPTSETUP -q luksUUID $LOOPDEV)
 [ "$tst"x = "$TEST_UUID"x ] || fail

+prepare "[16] luksFormat" wipe
+echo "key0" | $CRYPTSETUP -q luksFormat --master-key-file /dev/urandom $LOOPDEV || fail
+echo "key0" | $CRYPTSETUP -q luksFormat --master-key-file /dev/urandom $LOOPDEV -d $KEY1 || fail
+$CRYPTSETUP -q luksFormat --master-key-file /dev/urandom -s 128 --uuid $TEST_UUID $LOOPDEV $KEY1 || fail
+$CRYPTSETUP luksOpen -d $KEY1 $LOOPDEV $DEV_NAME || fail
+$CRYPTSETUP -q luksClose  $DEV_NAME || fail
+
 remove_mapping
 exit 0