Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1520368 > unrolled thread

dm-crypt accepts '+' in the key

Back to article view | Back to linux.kernel

Contents

  dm-crypt accepts '+' in the key Mikulas Patocka <mpatocka@redhat.com> - 2016-11-12 21:30 +0100
    Re: dm-crypt accepts '+' in the key Milan Broz <gmazyland@gmail.com> - 2016-11-13 15:50 +0100
      Re: dm-crypt accepts '+' in the key Alexey Dobriyan <adobriyan@gmail.com> - 2016-11-13 23:40 +0100
        Re: dm-crypt accepts '+' in the key Mikulas Patocka <mpatocka@redhat.com> - 2016-11-14 22:20 +0100

#1520368 — dm-crypt accepts '+' in the key

Hi

dm-crypt uses the function kstrtou8 to decode the encryption key. kstrtou8 
calls kstrtoull and kstrtoull skips the first character if it is '+'.

Consequently, it is possible to load keys with '+' in it. For example, 
this is possible:

dmsetup create cr --table "0 131072 crypt aes-cbc-essiv:sha256 +0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0 0 /dev/debian/tmptest 0"

Should this be fixed in dm-crypt or in kstrtou8? A fix in kstrtou8 could 
be more appropriate, but we don't know how many other kernel parts depend 
on this "skip plus" behavior...

Mikulas

[toc] | [next] | [standalone]

#1520607

On 11/12/2016 09:20 PM, Mikulas Patocka wrote:
> Hi
> 
> dm-crypt uses the function kstrtou8 to decode the encryption key. kstrtou8 
> calls kstrtoull and kstrtoull skips the first character if it is '+'.
> 
> Consequently, it is possible to load keys with '+' in it. For example, 
> this is possible:
> 
> dmsetup create cr --table "0 131072 crypt aes-cbc-essiv:sha256 +0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0 0 /dev/debian/tmptest 0"
> 
> Should this be fixed in dm-crypt or in kstrtou8? A fix in kstrtou8 could 
> be more appropriate, but we don't know how many other kernel parts depend 
> on this "skip plus" behavior...

I would way it should be checked in both places...
For dmcrypt, it should validate input here and should
not accept anything in key field in dm table that is not in hexa representation.

(Is this regression since code switched from simple_strtoul to  kstrtou8
or this bug was there always?)

Milan

[toc] | [prev] | [next] | [standalone]

#1520715

On Sun, Nov 13, 2016 at 03:45:27PM +0100, Milan Broz wrote:
> On 11/12/2016 09:20 PM, Mikulas Patocka wrote:
> > Hi
> > 
> > dm-crypt uses the function kstrtou8 to decode the encryption key. kstrtou8 
> > calls kstrtoull and kstrtoull skips the first character if it is '+'.
> > 
> > Consequently, it is possible to load keys with '+' in it. For example, 
> > this is possible:
> > 
> > dmsetup create cr --table "0 131072 crypt aes-cbc-essiv:sha256 +0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0 0 /dev/debian/tmptest 0"
> > 
> > Should this be fixed in dm-crypt or in kstrtou8? A fix in kstrtou8 could 
> > be more appropriate, but we don't know how many other kernel parts depend 
> > on this "skip plus" behavior...
> 
> I would way it should be checked in both places...
> For dmcrypt, it should validate input here and should
> not accept anything in key field in dm table that is not in hexa representation.
> 
> (Is this regression since code switched from simple_strtoul to  kstrtou8
> or this bug was there always?)

Well, before kernel would silently parse anything broken as "0".

But since it is base-16, "0[xX]" will be accepted before every byte.

dm-crypt should parse key by hand, frankly.

[toc] | [prev] | [next] | [standalone]

#1522089

On Mon, 14 Nov 2016, Alexey Dobriyan wrote:

> On Sun, Nov 13, 2016 at 03:45:27PM +0100, Milan Broz wrote:
> > On 11/12/2016 09:20 PM, Mikulas Patocka wrote:
> > > Hi
> > > 
> > > dm-crypt uses the function kstrtou8 to decode the encryption key. kstrtou8 
> > > calls kstrtoull and kstrtoull skips the first character if it is '+'.
> > > 
> > > Consequently, it is possible to load keys with '+' in it. For example, 
> > > this is possible:
> > > 
> > > dmsetup create cr --table "0 131072 crypt aes-cbc-essiv:sha256 +0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0+0 0 /dev/debian/tmptest 0"
> > > 
> > > Should this be fixed in dm-crypt or in kstrtou8? A fix in kstrtou8 could 
> > > be more appropriate, but we don't know how many other kernel parts depend 
> > > on this "skip plus" behavior...
> > 
> > I would way it should be checked in both places...
> > For dmcrypt, it should validate input here and should
> > not accept anything in key field in dm table that is not in hexa representation.
> > 
> > (Is this regression since code switched from simple_strtoul to  kstrtou8
> > or this bug was there always?)
> 
> Well, before kernel would silently parse anything broken as "0".

dm-crypt already validates that there are exactly two characters passed to 
kstrtou8 or simple_strtoul.

> But since it is base-16, "0[xX]" will be accepted before every byte.

Yes, the old dm-crypt code that used simple_strtoul accepted "0x" in a key 
(and parsed it as zero byte). It didn't accept "+" or "-".

> dm-crypt should parse key by hand, frankly.

Mikulas

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web