[dm-crypt] Bugs/Undoucmented features

[dm-crypt] Bugs/Undoucmented features

[Ahmed, Safayet (GE Global Research)]

I've been looking into the cryptsetup tool and I just wanted to mention some 
bugs/undocumented features I came across.

1 detached headers
------------------

For a Luks setup that uses a detached header, a lot of the cryptsetup commands
don't work in an intuitive way. Specifically, the "--header" option is ignored 
and cryptsetup throws an error that the specified device is not a Luks device. 
The cryptsetup commands work when the path to the actual luks device is 
replaced with the path to the header file. This was the case for the following 
commands:

    luksDump
    luksAddKey
    luksKillSlot

I didn't check for the other commands. If this is the intended behavior, I 
think it should be mentioned in the doucmentation that the header file should 
be used in place of the <device> argument in the case of detached LUKS headers.


2 master-key-file with new-key-file
-----------------------------------

For the luksAddKey command, when the "--master-key-file" argument is specified,
the new-key-file positional argument is ignored. Whether or not a file is
provided for the new pass phrase, the user is prompted to enter a pass phrase.

The problem is in the implementation of the function "action_luksAddKey" in 
"src/cryptsetup.c". Consider the following lines (916 - 927):

	if (opt_master_key_file) {
		r = _read_mk(opt_master_key_file, &key, keysize);
		if (r < 0)
			goto out;
		//FIXME: process keyfile arg
		r = crypt_keyslot_add_by_volume_key(cd, opt_key_slot,
						    key, keysize, NULL, 0);
	} else if (opt_key_file || opt_new_key_file) {
		r = crypt_keyslot_add_by_keyfile_offset(cd, opt_key_slot,
			opt_key_file, opt_keyfile_size, opt_keyfile_offset,
			opt_new_key_file, opt_new_keyfile_size, opt_new_keyfile_offset);
	} else {

When the master key is present, the code doesn't check "opt_new_key_file" and 
assumes that the user should be prompted for the pass phrase.

Is this the intended behavior of luksAddKey?

regards,

Safayet Ahmed

Re: [dm-crypt] Bugs/Undoucmented features

[Sven Eschenberg]

On Fri, January 23, 2015 22:33, Ahmed, Safayet (GE Global Research) wrote:
> I've been looking into the cryptsetup tool and I just wanted to mention
> some
> bugs/undocumented features I came across.
>
> 1 detached headers
> ------------------
>
> For a Luks setup that uses a detached header, a lot of the cryptsetup
> commands
> don't work in an intuitive way. Specifically, the "--header" option is
> ignored
> and cryptsetup throws an error that the specified device is not a Luks
> device.
> The cryptsetup commands work when the path to the actual luks device is
> replaced with the path to the header file. This was the case for the
> following
> commands:
>
>     luksDump
>     luksAddKey
>     luksKillSlot
>
> I didn't check for the other commands. If this is the intended behavior, I
> think it should be mentioned in the doucmentation that the header file
> should
> be used in place of the <device> argument in the case of detached LUKS
> headers.

Operations on the HEADER naturally require the header and not the actual
payload. It would be redundant to pass the payload (device) and naturally
the header is the only (direct) parameter. Should this be mentioned more
clearly in the docs/manpage? Possibly yes.

>
>
> 2 master-key-file with new-key-file
> -----------------------------------
>
> For the luksAddKey command, when the "--master-key-file" argument is
> specified,
> the new-key-file positional argument is ignored. Whether or not a file is
> provided for the new pass phrase, the user is prompted to enter a pass
> phrase.

I think I reported this quite a while back and it was supposedly fixed
some while ago (2010 IIRC).

>
> The problem is in the implementation of the function "action_luksAddKey"
> in
> "src/cryptsetup.c". Consider the following lines (916 - 927):
>
> 	if (opt_master_key_file) {
> 		r = _read_mk(opt_master_key_file, &key, keysize);
> 		if (r < 0)
> 			goto out;
> 		//FIXME: process keyfile arg
> 		r = crypt_keyslot_add_by_volume_key(cd, opt_key_slot,
> 						    key, keysize, NULL, 0);
> 	} else if (opt_key_file || opt_new_key_file) {
> 		r = crypt_keyslot_add_by_keyfile_offset(cd, opt_key_slot,
> 			opt_key_file, opt_keyfile_size, opt_keyfile_offset,
> 			opt_new_key_file, opt_new_keyfile_size, opt_new_keyfile_offset);
> 	} else {
>
> When the master key is present, the code doesn't check "opt_new_key_file"
> and
> assumes that the user should be prompted for the pass phrase.
>
> Is this the intended behavior of luksAddKey?
>
> regards,
>
> Safayet Ahmed
> _______________________________________________
> dm-crypt mailing list
> dm-crypt@saout.de
> http://www.saout.de/mailman/listinfo/dm-crypt
>

Re: [dm-crypt] Bugs/Undoucmented features

[Arno Wagner]

On Fri, Jan 23, 2015 at 23:23:16 CET, Sven Eschenberg wrote:
> On Fri, January 23, 2015 22:33, Ahmed, Safayet (GE Global Research) wrote:
> > I've been looking into the cryptsetup tool and I just wanted to mention
> > some
> > bugs/undocumented features I came across.
> >
> > 1 detached headers
> > ------------------
> >
> > For a Luks setup that uses a detached header, a lot of the cryptsetup
> > commands
> > don't work in an intuitive way. Specifically, the "--header" option is
> > ignored
> > and cryptsetup throws an error that the specified device is not a Luks
> > device.
> > The cryptsetup commands work when the path to the actual luks device is
> > replaced with the path to the header file. This was the case for the
> > following
> > commands:
> >
> >     luksDump
> >     luksAddKey
> >     luksKillSlot
> >
> > I didn't check for the other commands. If this is the intended behavior, I
> > think it should be mentioned in the doucmentation that the header file
> > should
> > be used in place of the <device> argument in the case of detached LUKS
> > headers.
> 
> Operations on the HEADER naturally require the header and not the actual
> payload. It would be redundant to pass the payload (device) and naturally
> the header is the only (direct) parameter. Should this be mentioned more
> clearly in the docs/manpage? Possibly yes.

While this is obvious behavior if you have the LUKS data model
in mind, I think it is a valid criticism of the documentation.

I will make this more obvious in the man-page and maybe add an 
FAQ-item. May take a week or two though, I currently have a lot 
of work.

> >
> >
> > 2 master-key-file with new-key-file
> > -----------------------------------
> >
> > For the luksAddKey command, when the "--master-key-file" argument is
> > specified,
> > the new-key-file positional argument is ignored. Whether or not a file is
> > provided for the new pass phrase, the user is prompted to enter a pass
> > phrase.
> 
> I think I reported this quite a while back and it was supposedly fixed
> some while ago (2010 IIRC).

This code seems to still be in 1.6.5, but starting at line 955. 

> >
> > The problem is in the implementation of the function "action_luksAddKey"
> > in
> > "src/cryptsetup.c". Consider the following lines (916 - 927):
> >
> > 	if (opt_master_key_file) {
> > 		r = _read_mk(opt_master_key_file, &key, keysize);
> > 		if (r < 0)
> > 			goto out;
> > 		//FIXME: process keyfile arg
> > 		r = crypt_keyslot_add_by_volume_key(cd, opt_key_slot,
> > 						    key, keysize, NULL, 0);
> > 	} else if (opt_key_file || opt_new_key_file) {
> > 		r = crypt_keyslot_add_by_keyfile_offset(cd, opt_key_slot,
> > 			opt_key_file, opt_keyfile_size, opt_keyfile_offset,
> > 			opt_new_key_file, opt_new_keyfile_size, opt_new_keyfile_offset);
> > 	} else {
> >
> > When the master key is present, the code doesn't check "opt_new_key_file"
> > and
> > assumes that the user should be prompted for the pass phrase.
> >
> > Is this the intended behavior of luksAddKey?

I think this is actually a defect. It is a minor one, as using
the master-key is an emergency-only operation and hence likely
done manyally anyways. But if I am right and it is a defect, it
should probably get an item on the issues-list.

Gr"usse,
Arno

> > regards,
> >
> > Safayet Ahmed
> > _______________________________________________
> > dm-crypt mailing list
> > dm-crypt@saout.de
> > http://www.saout.de/mailman/listinfo/dm-crypt
> >
> 
> 
> _______________________________________________
> dm-crypt mailing list
> dm-crypt@saout.de
> http://www.saout.de/mailman/listinfo/dm-crypt

-- 
Arno Wagner,     Dr. sc. techn., Dipl. Inform.,    Email: arno@wagner.name
GnuPG: ID: CB5D9718  FP: 12D6 C03B 1B30 33BB 13CF  B774 E35C 5FA1 CB5D 9718
----
A good decision is based on knowledge and not on numbers. -- Plato

If it's in the news, don't worry about it.  The very definition of 
"news" is "something that hardly ever happens." -- Bruce Schneier

Re: [dm-crypt] Bugs/Undoucmented features

[Milan Broz]

Hi,

On 01/23/2015 10:33 PM, Ahmed, Safayet (GE Global Research) wrote:
> I've been looking into the cryptsetup tool and I just wanted to mention some 
> bugs/undocumented features I came across.
> 
> 1 detached headers
> ------------------
> 
> For a Luks setup that uses a detached header, a lot of the cryptsetup commands
> don't work in an intuitive way. Specifically, the "--header" option is ignored 
> and cryptsetup throws an error that the specified device is not a Luks device. 
> The cryptsetup commands work when the path to the actual luks device is 
> replaced with the path to the header file. This was the case for the following 
> commands:
> 
>     luksDump
>     luksAddKey
>     luksKillSlot
> 
> I didn't check for the other commands. If this is the intended behavior, I 
> think it should be mentioned in the doucmentation that the header file should 
> be used in place of the <device> argument in the case of detached LUKS headers.

The detached header was added later and only the commands that require
the --header were initially modified; commands above operates only with header device.

But they should support --header as well and simply use it if specified, should be
easy to fix.
(The libcryptsetup has universal crypt_set_data_device() call.)

I do not think we should document it as an exception, the --header should be simply supported
everywhere (despite the data device would be completely ignored, this seems to me like
a simpler way for the user - once you specify --header, it is used.
If not, code will try to use default arg.)


> 2 master-key-file with new-key-file
> -----------------------------------
> 
> For the luksAddKey command, when the "--master-key-file" argument is specified,
> the new-key-file positional argument is ignored. Whether or not a file is
> provided for the new pass phrase, the user is prompted to enter a pass phrase.
> 
> The problem is in the implementation of the function "action_luksAddKey" in 
> "src/cryptsetup.c". Consider the following lines (916 - 927):
> 
> 	if (opt_master_key_file) {
> 		r = _read_mk(opt_master_key_file, &key, keysize);
> 		if (r < 0)
> 			goto out;
> 		//FIXME: process keyfile arg
> 		r = crypt_keyslot_add_by_volume_key(cd, opt_key_slot,
> 						    key, keysize, NULL, 0);
> 	} else if (opt_key_file || opt_new_key_file) {
> 		r = crypt_keyslot_add_by_keyfile_offset(cd, opt_key_slot,
> 			opt_key_file, opt_keyfile_size, opt_keyfile_offset,
> 			opt_new_key_file, opt_new_keyfile_size, opt_new_keyfile_offset);
> 	} else {
> 
> When the master key is present, the code doesn't check "opt_new_key_file" and 
> assumes that the user should be prompted for the pass phrase.
> 
> Is this the intended behavior of luksAddKey?

No, I think it is just missing code here. Only opt_new_keyfile_size should be parsed
here because master-key is provided (opt_key_file would be redundant).

Milan

Re: [dm-crypt] Bugs/Undoucmented features

[Arno Wagner]

On Sat, Jan 24, 2015 at 10:44:55 CET, Milan Broz wrote:
> Hi,
> 
> On 01/23/2015 10:33 PM, Ahmed, Safayet (GE Global Research) wrote:
> > I've been looking into the cryptsetup tool and I just wanted to mention some 
> > bugs/undocumented features I came across.
> > 
> > 1 detached headers
> > ------------------
> > 
> > For a Luks setup that uses a detached header, a lot of the cryptsetup commands
> > don't work in an intuitive way. Specifically, the "--header" option is ignored 
> > and cryptsetup throws an error that the specified device is not a Luks device. 
> > The cryptsetup commands work when the path to the actual luks device is 
> > replaced with the path to the header file. This was the case for the following 
> > commands:
> > 
> >     luksDump
> >     luksAddKey
> >     luksKillSlot
> > 
> > I didn't check for the other commands. If this is the intended behavior, I 
> > think it should be mentioned in the doucmentation that the header file should 
> > be used in place of the <device> argument in the case of detached LUKS headers.
> 
> The detached header was added later and only the commands that require
> the --header were initially modified; commands above operates only with header device.
> 
> But they should support --header as well and simply use it if specified, should be
> easy to fix.
> (The libcryptsetup has universal crypt_set_data_device() call.)
> 
> I do not think we should document it as an exception, the --header should be simply supported
> everywhere (despite the data device would be completely ignored, this seems to me like
> a simpler way for the user - once you specify --header, it is used.
> If not, code will try to use default arg.)

Yes, I think that would be better as it does not silently switch 
between two different semantics of the positional argument.
So no changes to the documentation.

Arno
 
> 
> > 2 master-key-file with new-key-file
> > -----------------------------------
> > 
> > For the luksAddKey command, when the "--master-key-file" argument is specified,
> > the new-key-file positional argument is ignored. Whether or not a file is
> > provided for the new pass phrase, the user is prompted to enter a pass phrase.
> > 
> > The problem is in the implementation of the function "action_luksAddKey" in 
> > "src/cryptsetup.c". Consider the following lines (916 - 927):
> > 
> > 	if (opt_master_key_file) {
> > 		r = _read_mk(opt_master_key_file, &key, keysize);
> > 		if (r < 0)
> > 			goto out;
> > 		//FIXME: process keyfile arg
> > 		r = crypt_keyslot_add_by_volume_key(cd, opt_key_slot,
> > 						    key, keysize, NULL, 0);
> > 	} else if (opt_key_file || opt_new_key_file) {
> > 		r = crypt_keyslot_add_by_keyfile_offset(cd, opt_key_slot,
> > 			opt_key_file, opt_keyfile_size, opt_keyfile_offset,
> > 			opt_new_key_file, opt_new_keyfile_size, opt_new_keyfile_offset);
> > 	} else {
> > 
> > When the master key is present, the code doesn't check "opt_new_key_file" and 
> > assumes that the user should be prompted for the pass phrase.
> > 
> > Is this the intended behavior of luksAddKey?
> 
> No, I think it is just missing code here. Only opt_new_keyfile_size should be parsed
> here because master-key is provided (opt_key_file would be redundant).
> 
> Milan
> _______________________________________________
> dm-crypt mailing list
> dm-crypt@saout.de
> http://www.saout.de/mailman/listinfo/dm-crypt

-- 
Arno Wagner,     Dr. sc. techn., Dipl. Inform.,    Email: arno@wagner.name
GnuPG: ID: CB5D9718  FP: 12D6 C03B 1B30 33BB 13CF  B774 E35C 5FA1 CB5D 9718
----
A good decision is based on knowledge and not on numbers. -- Plato

If it's in the news, don't worry about it.  The very definition of 
"news" is "something that hardly ever happens." -- Bruce Schneier

Re: [dm-crypt] Bugs/Undoucmented features

[Milan Broz]

On 01/24/2015 10:44 AM, Milan Broz wrote:

>> 1 detached headers
>> 2 master-key-file with new-key-file

Both these problems are fixed in the devel git tree (so it will be in the next release).

(If there is anything more, let me know please, the best is to use a new issue or this list.)

Thanks,
Milan

Re: [dm-crypt] Bugs/Undoucmented features

[Arno Wagner]

Excellent!

Arno


On Mon, Jan 26, 2015 at 14:55:16 CET, Milan Broz wrote:
> On 01/24/2015 10:44 AM, Milan Broz wrote:
> 
> >> 1 detached headers
> >> 2 master-key-file with new-key-file
> 
> Both these problems are fixed in the devel git tree (so it will be in the next release).
> 
> (If there is anything more, let me know please, the best is to use a new issue or this list.)
> 
> Thanks,
> Milan
> _______________________________________________
> dm-crypt mailing list
> dm-crypt@saout.de
> http://www.saout.de/mailman/listinfo/dm-crypt

-- 
Arno Wagner,     Dr. sc. techn., Dipl. Inform.,    Email: arno@wagner.name
GnuPG: ID: CB5D9718  FP: 12D6 C03B 1B30 33BB 13CF  B774 E35C 5FA1 CB5D 9718
----
A good decision is based on knowledge and not on numbers. -- Plato

If it's in the news, don't worry about it.  The very definition of 
"news" is "something that hardly ever happens." -- Bruce Schneier