From 791481b9e9ccdcc87f1ec3120bbf54b8bbf2b34c Mon Sep 17 00:00:00 2001 From: Milan Broz Date: Fri, 29 Oct 2010 20:04:01 +0000 Subject: [PATCH] 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 --- ChangeLog | 2 ++ src/cryptsetup.c | 73 +++++++++++++++++++++++-------------------------------- tests/compat-test | 19 ++++++++++----- 3 files changed, 45 insertions(+), 49 deletions(-) diff --git a/ChangeLog b/ChangeLog index be218e5..8719d4e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -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 * Add crypt_get_device_name() to API (get underlying device name). diff --git a/src/cryptsetup.c b/src/cryptsetup.c index 839ead0..3a4a2c9 100644 --- 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,40 +380,30 @@ 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, ¶ms); - if (r < 0) - 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, ¶ms); - if (r < 0) - goto out; + r = crypt_format(cd, CRYPT_LUKS1, cipher, cipher_mode, + opt_uuid, key, keysize, ¶ms); + 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); - } + r = crypt_keyslot_add_by_volume_key(cd, opt_key_slot, + 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 */ diff --git a/tests/compat-test b/tests/compat-test index 953f746..8c580a2 100755 --- 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 -- 2.7.4