[dm-crypt] Secret data from stdin

[dm-crypt] Secret data from stdin

[Kent Yoder]

Hi,

I recently came across some code that did this:

  cat binary_secret | cryptsetup luksFormat /dev/loop0

This appears to work (no message printed, exit status 0).

What might not be obvious is that if binary_secret contains a '\n'
character, input gets truncated at this point. This is different
than

  cat binary_secret | cryptsetup luksFormat --key-file=- /dev/loop0

which will read all of binary_secret, regardless of whether there's
a \n in it or not.

This difference seems subtle and could lead to truncation of the
secret.  This should probably be clearer in the man page at a
minimum (see patch), but I think a warning is appropriate too.
Secret processing that stops at \n isn't appropriate for binary
data.

Thanks,
Kent

--
IBM LTC Security


diff --git a/man/cryptsetup.8 b/man/cryptsetup.8
index b9298a5..f8d7abb 100644
--- a/man/cryptsetup.8
+++ b/man/cryptsetup.8
@@ -476,7 +476,8 @@ will quit with an error.

 If \-\-key-file=- is used for reading the key from stdin, no
 trailing newline is stripped from the input. Without that option,
-cryptsetup strips trailing newlines from stdin input.
+cryptsetup stops reading from stdin when it encounters a newline,
+even if found in your binary key data!
 .SH NOTES ON PASSWORD PROCESSING FOR LUKS
 LUKS uses PBKDF2 to protect against dictionary attacks (see RFC 2898).

Re: [dm-crypt] Secret data from stdin

[Arno Wagner]

On Tue, Aug 14, 2012 at 05:19:43PM -0500, Kent Yoder wrote:
> Hi,
> 
> I recently came across some code that did this:
> 
>   cat binary_secret | cryptsetup luksFormat /dev/loop0
> 
> This appears to work (no message printed, exit status 0).
> 
> What might not be obvious is that if binary_secret contains a '\n'
> character, input gets truncated at this point. 

This is documented in the man-page of the current release under
"NOTES ON PASSPHRASE PROCESSING FOR LUKS".

> This is different
> than
> 
>   cat binary_secret | cryptsetup luksFormat --key-file=- /dev/loop0
> 
> which will read all of binary_secret, regardless of whether there's
> a \n in it or not.

Indeed. Documented under the entry for "--key-file" in the current
release man-page.
 
> This difference seems subtle and could lead to truncation of the
> secret.  

I agree. I tried to make very sure it is clear what is done 
on my man-page revision.

> This should probably be clearer in the man page at a
> minimum (see patch), but I think a warning is appropriate too.
> Secret processing that stops at \n isn't appropriate for binary
> data.

And that is the thing here. A passphrase is _not_ binary data!
Doing 

   "cat binary_secret | cryptsetup luksFormat /dev/loop0"

is inherently wrong. What you need to do is

  "cat file_with_passphrase_that_could_also_be_entered_interactively
  | cryptsetup luksFormat /dev/loop0"


As to your patch, I am unable to match your patch to the 
current version of the man-page. Did you do a "git pull" 
before? May also be a problem on my side, please verify:

> md5sum cryptsetup.8
4fd70bbd1018f95818902144499c2234  cryptsetup.8

Arno





> Thanks,
> Kent
> 
> --
> IBM LTC Security
> 
> 
> diff --git a/man/cryptsetup.8 b/man/cryptsetup.8
> index b9298a5..f8d7abb 100644
> --- a/man/cryptsetup.8
> +++ b/man/cryptsetup.8
> @@ -476,7 +476,8 @@ will quit with an error.
> 
>  If \-\-key-file=- is used for reading the key from stdin, no
>  trailing newline is stripped from the input. Without that option,
> -cryptsetup strips trailing newlines from stdin input.
> +cryptsetup stops reading from stdin when it encounters a newline,
> +even if found in your binary key data!
>  .SH NOTES ON PASSWORD PROCESSING FOR LUKS
>  LUKS uses PBKDF2 to protect against dictionary attacks (see RFC 2898).
> _______________________________________________
> 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: 1E25338F  FP: 0C30 5782 9D93 F785 E79C  0296 797F 6B50 1E25 338F
----
One of the painful things about our time is that those who feel certainty 
are stupid, and those with any imagination and understanding are filled 
with doubt and indecision. -- Bertrand Russell 

Re: [dm-crypt] Secret data from stdin

[Kent Yoder]

Hi Arno,

>>
>> This appears to work (no message printed, exit status 0).
>>
>> What might not be obvious is that if binary_secret contains a '\n'
>> character, input gets truncated at this point.
>
> This is documented in the man-page of the current release under
> "NOTES ON PASSPHRASE PROCESSING FOR LUKS".

  You were right - I was looking at an old git version. The new
version is clearer IMO.

>> This should probably be clearer in the man page at a
>> minimum (see patch), but I think a warning is appropriate too.
>> Secret processing that stops at \n isn't appropriate for binary
>> data.
>
> And that is the thing here. A passphrase is _not_ binary data!
> Doing
>
>    "cat binary_secret | cryptsetup luksFormat /dev/loop0"
>
> is inherently wrong. What you need to do is
>
>   "cat file_with_passphrase_that_could_also_be_entered_interactively
>   | cryptsetup luksFormat /dev/loop0"

  I agree - just seeing a script that did the first one made me wonder
if it even worked.

> As to your patch, I am unable to match your patch to the
> current version of the man-page. Did you do a "git pull"
> before? May also be a problem on my side, please verify:
>
>> md5sum cryptsetup.8
> 4fd70bbd1018f95818902144499c2234  cryptsetup.8

  Yep, I am out of date here.  What do you think about a code change
that woudl print a big fat warning if non-ascii bytes are detected on
stdin?  Not changing the behavior (we don't want to break people who
might be already doing this), but just a warning.

Kent

> Arno
>
>
>
>
>
>> Thanks,
>> Kent
>>
>> --
>> IBM LTC Security
>>
>>
>> diff --git a/man/cryptsetup.8 b/man/cryptsetup.8
>> index b9298a5..f8d7abb 100644
>> --- a/man/cryptsetup.8
>> +++ b/man/cryptsetup.8
>> @@ -476,7 +476,8 @@ will quit with an error.
>>
>>  If \-\-key-file=- is used for reading the key from stdin, no
>>  trailing newline is stripped from the input. Without that option,
>> -cryptsetup strips trailing newlines from stdin input.
>> +cryptsetup stops reading from stdin when it encounters a newline,
>> +even if found in your binary key data!
>>  .SH NOTES ON PASSWORD PROCESSING FOR LUKS
>>  LUKS uses PBKDF2 to protect against dictionary attacks (see RFC 2898).
>> _______________________________________________
>> 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: 1E25338F  FP: 0C30 5782 9D93 F785 E79C  0296 797F 6B50 1E25 338F
> ----
> One of the painful things about our time is that those who feel certainty
> are stupid, and those with any imagination and understanding are filled
> with doubt and indecision. -- Bertrand Russell
> _______________________________________________
> dm-crypt mailing list
> dm-crypt@saout.de
> http://www.saout.de/mailman/listinfo/dm-crypt



-- 
IBM LTC Security

Re: [dm-crypt] Secret data from stdin

[Milan Broz]

On 08/15/2012 05:12 PM, Kent Yoder wrote:

>   Yep, I am out of date here.  What do you think about a code change
> that woudl print a big fat warning if non-ascii bytes are detected on
> stdin?  Not changing the behavior (we don't want to break people who
> might be already doing this), but just a warning.

No please.
Anything what prints any information about passphrase to screen, log etc.
is not acceptable, it provides info which should not be seen anywhere.

I know that \n handling is problematic, but there is huge amount of
scripts using this redirection. We cannot simply change it.
(At least not for current CLI commands.)

Milan

Re: [dm-crypt] Secret data from stdin

[Arno Wagner]

On Wed, Aug 15, 2012 at 05:44:01PM +0200, Milan Broz wrote:
> On 08/15/2012 05:12 PM, Kent Yoder wrote:
> 
> >   Yep, I am out of date here.  What do you think about a code change
> > that woudl print a big fat warning if non-ascii bytes are detected on
> > stdin?  Not changing the behavior (we don't want to break people who
> > might be already doing this), but just a warning.
> 
> No please.
> Anything what prints any information about passphrase to screen, log etc.
> is not acceptable, it provides info which should not be seen anywhere.

I agree. And in addition what non-ascii is has become fuzzy with
Unicode. The non-ascii and even potential newlines may be just what
the user wanted to be in there.
 
> I know that \n handling is problematic, but there is huge amount of
> scripts using this redirection. We cannot simply change it.
> (At least not for current CLI commands.)

I think it works reasonably well at this time and there is
ample warning in the man-page. If somebody insists on
shooting themselves in the foot, they will always find a 
way to do so.

Maybe we could think about a '--give-lots-of-paranoid-warnings'
option some time in the future. 

Arno
-- 
Arno Wagner,    Dr. sc. techn., Dipl. Inform.,   Email: arno@wagner.name 
GnuPG:  ID: 1E25338F  FP: 0C30 5782 9D93 F785 E79C  0296 797F 6B50 1E25 338F
----
One of the painful things about our time is that those who feel certainty 
are stupid, and those with any imagination and understanding are filled 
with doubt and indecision. -- Bertrand Russell 

Re: [dm-crypt] Secret data from stdin

[Arno Wagner]

On Wed, Aug 15, 2012 at 10:12:27AM -0500, Kent Yoder wrote:
> Hi Arno,
> 
> >>
> >> This appears to work (no message printed, exit status 0).
> >>
> >> What might not be obvious is that if binary_secret contains a '\n'
> >> character, input gets truncated at this point.
> >
> > This is documented in the man-page of the current release under
> > "NOTES ON PASSPHRASE PROCESSING FOR LUKS".
> 
>   You were right - I was looking at an old git version. The new
> version is clearer IMO.

Ok.
 
> >> This should probably be clearer in the man page at a
> >> minimum (see patch), but I think a warning is appropriate too.
> >> Secret processing that stops at \n isn't appropriate for binary
> >> data.
> >
> > And that is the thing here. A passphrase is _not_ binary data!
> > Doing
> >
> >    "cat binary_secret | cryptsetup luksFormat /dev/loop0"
> >
> > is inherently wrong. What you need to do is
> >
> >   "cat file_with_passphrase_that_could_also_be_entered_interactively
> >   | cryptsetup luksFormat /dev/loop0"
> 
>   I agree - just seeing a script that did the first one made me wonder
> if it even worked.

Well, people have to read the documentation if they want it
to work right. It is really not that much for cryptsetup,
just the man-page plus the FAQ. It is not like it has a 500 page
documentation. If people ignore the documentation, they basically
get what they deserve. This problem is worse with crypto, as
lots of problems are non-visible (it works but is insecure), 
but anybody working with crypto needs to understand that.
Those that do not _will_ make fatal mistalkes, no matter 
how much warning is given. Crypto is not a beginners game.

> > As to your patch, I am unable to match your patch to the
> > current version of the man-page. Did you do a "git pull"
> > before? May also be a problem on my side, please verify:
> >
> >> md5sum cryptsetup.8
> > 4fd70bbd1018f95818902144499c2234  cryptsetup.8
> 
>   Yep, I am out of date here.  What do you think about a code change
> that woudl print a big fat warning if non-ascii bytes are detected on
> stdin?  Not changing the behavior (we don't want to break people who
> might be already doing this), but just a warning.

See my reply to Milan.

Arno
-- 
Arno Wagner,    Dr. sc. techn., Dipl. Inform.,   Email: arno@wagner.name 
GnuPG:  ID: 1E25338F  FP: 0C30 5782 9D93 F785 E79C  0296 797F 6B50 1E25 338F
----
One of the painful things about our time is that those who feel certainty 
are stupid, and those with any imagination and understanding are filled 
with doubt and indecision. -- Bertrand Russell