Re: [PATCH v2] KEYS: Convert KEYCTL_DH_COMPUTE to use the crypto KPP API

Re: [PATCH v2] KEYS: Convert KEYCTL_DH_COMPUTE to use the crypto KPP API

[David Howells]

Mat Martineau <mathew.j.martineau@linux.intel.com> wrote:

> The initial Diffie-Hellman computation made direct use of the MPI
> library because the crypto module did not support DH at the time. Now
> that KPP is implemented, KEYCTL_DH_COMPUTE should use it to get rid of
> duplicate code and leverage possible hardware acceleration.

This doesn't apply to linus/master.  I've pushed the keyrings fix patches I
have, including a bunch from Eric Biggers that fix DH stuff, to:

	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git

branch:

	keys-fixes

though I think there may be a couple of bugs in on of Eric's patches where
he's assumed that he can do:

	memzero_explicit(NULL, 0);

I'm not sure whether it's permissible to assume that memset(NULL, 0, 0) is
guaranteed to work correctly.

Note that I haven't included Eric's DH patch that was obsoleted by Stephan's
patch that was obsoleted by this one.

David

[PATCH v2] KEYS: Convert KEYCTL_DH_COMPUTE to use the crypto KPP API

[David Howells]

Mat Martineau <mathew.j.martineau@linux.intel.com> wrote:

> The initial Diffie-Hellman computation made direct use of the MPI
> library because the crypto module did not support DH at the time. Now
> that KPP is implemented, KEYCTL_DH_COMPUTE should use it to get rid of
> duplicate code and leverage possible hardware acceleration.

This doesn't apply to linus/master.  I've pushed the keyrings fix patches I
have, including a bunch from Eric Biggers that fix DH stuff, to:

	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git

branch:

	keys-fixes

though I think there may be a couple of bugs in on of Eric's patches where
he's assumed that he can do:

	memzero_explicit(NULL, 0);

I'm not sure whether it's permissible to assume that memset(NULL, 0, 0) is
guaranteed to work correctly.

Note that I haven't included Eric's DH patch that was obsoleted by Stephan's
patch that was obsoleted by this one.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] KEYS: Convert KEYCTL_DH_COMPUTE to use the crypto KPP API

[Stephan Müller]

Am Freitag, 2. Juni 2017, 17:58:22 CEST schrieb David Howells:

Hi David,

> Note that I haven't included Eric's DH patch that was obsoleted by Stephan's
> patch that was obsoleted by this one.

Eric's patches 1, 2, 4, and 5 should be pulled as they are unrelated to any 
other patch. Eric's patch 3 should be disregarded, my patch in response to 
Eric's patch 3 should be disregarded, and this patch from Mat should be used.

Thanks a lot.

Ciao
Stephan

[PATCH v2] KEYS: Convert KEYCTL_DH_COMPUTE to use the crypto KPP API

[Stephan Müller]

Am Freitag, 2. Juni 2017, 17:58:22 CEST schrieb David Howells:

Hi David,

> Note that I haven't included Eric's DH patch that was obsoleted by Stephan's
> patch that was obsoleted by this one.

Eric's patches 1, 2, 4, and 5 should be pulled as they are unrelated to any 
other patch. Eric's patch 3 should be disregarded, my patch in response to 
Eric's patch 3 should be disregarded, and this patch from Mat should be used.

Thanks a lot.

Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] KEYS: Convert KEYCTL_DH_COMPUTE to use the crypto KPP API

[David Howells]

Stephan Müller <smueller@chronox.de> wrote:

> > Note that I haven't included Eric's DH patch that was obsoleted by Stephan's
> > patch that was obsoleted by this one.
> 
> Eric's patches 1, 2, 4, and 5 should be pulled as they are unrelated to any 
> other patch. Eric's patch 3 should be disregarded, my patch in response to 
> Eric's patch 3 should be disregarded, and this patch from Mat should be used.

Indeed.  But Mat's patch doesn't apply.

David

[PATCH v2] KEYS: Convert KEYCTL_DH_COMPUTE to use the crypto KPP API

[David Howells]

Stephan M?ller <smueller@chronox.de> wrote:

> > Note that I haven't included Eric's DH patch that was obsoleted by Stephan's
> > patch that was obsoleted by this one.
> 
> Eric's patches 1, 2, 4, and 5 should be pulled as they are unrelated to any 
> other patch. Eric's patch 3 should be disregarded, my patch in response to 
> Eric's patch 3 should be disregarded, and this patch from Mat should be used.

Indeed.  But Mat's patch doesn't apply.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] KEYS: Convert KEYCTL_DH_COMPUTE to use the crypto KPP API

[Eric Biggers]

Hi Mat,

On Wed, May 10, 2017 at 05:35:57PM -0700, Mat Martineau wrote:
> The initial Diffie-Hellman computation made direct use of the MPI
> library because the crypto module did not support DH at the time. Now
> that KPP is implemented, KEYCTL_DH_COMPUTE should use it to get rid of
> duplicate code and leverage possible hardware acceleration.
> 
> Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

Have you verified that this doesn't change the API at all --- order of bytes
within the numbers, whether they are padded with zeroes, any other corner cases
etc.?  There can't be breaking changes to the API after this is released in
mainline, and this is already in v4.12-rc4.

Also, Stephan, what is the purpose of the "__spare" field in keyctl_kdf_params?
How is it ever supposed to be used for anything if the kernel doesn't even
validate that it contains 0's?  Some flag hacked into the 'otherinfolen'
field...?

Eric

Re: [PATCH v2] KEYS: Convert KEYCTL_DH_COMPUTE to use the crypto KPP API

[Stephan Müller]

Am Montag, 5. Juni 2017, 05:41:32 CEST schrieb Eric Biggers:

Hi Eric,
> 
> Also, Stephan, what is the purpose of the "__spare" field in
> keyctl_kdf_params? How is it ever supposed to be used for anything if the
> kernel doesn't even validate that it contains 0's?  Some flag hacked into
> the 'otherinfolen' field...?

The idea in the very first patch set was the presence of a "flags" field. We 
have a proliferation of system calls in the last few years which just add a 
"flags" parameter compared to an identical older system call.

To prevent falling into this trap, I wanted to have a flags field. The core 
reason is that there are multiple options allowed and possible how to derive a 
key. The current approach is the easiest and smallest one, i.e. using the 
SP800-108 counter KDF. Other KDFs are possible too. To allow such openness, we 
decided to leave some room.

See https://patchwork.kernel.org/patch/9224837/ and search for __spare[8] in 
the answer from Mat.

Ciao
Stephan

Re: [PATCH v2] KEYS: Convert KEYCTL_DH_COMPUTE to use the crypto KPP API

[Eric Biggers]

Hi Stephan,

On Mon, Jun 05, 2017 at 11:51:32PM +0200, Stephan Müller wrote:
> Am Montag, 5. Juni 2017, 05:41:32 CEST schrieb Eric Biggers:
> 
> Hi Eric,
> > 
> > Also, Stephan, what is the purpose of the "__spare" field in
> > keyctl_kdf_params? How is it ever supposed to be used for anything if the
> > kernel doesn't even validate that it contains 0's?  Some flag hacked into
> > the 'otherinfolen' field...?
> 
> The idea in the very first patch set was the presence of a "flags" field. We 
> have a proliferation of system calls in the last few years which just add a 
> "flags" parameter compared to an identical older system call.
> 
> To prevent falling into this trap, I wanted to have a flags field. The core 
> reason is that there are multiple options allowed and possible how to derive a 
> key. The current approach is the easiest and smallest one, i.e. using the 
> SP800-108 counter KDF. Other KDFs are possible too. To allow such openness, we 
> decided to leave some room.
> 
> See https://patchwork.kernel.org/patch/9224837/ and search for __spare[8] in 
> the answer from Mat.
> 

Well, regardless of what it's reserved *for* (if anything), this is *not* the
correct way to do a reserved field.  The API needs to verify that it has value 0
and return -EINVAL otherwise, i.e.

	if (memchr_inv(kdfcopy->__spare, 0, sizeof(kdfcopy->__spare)))
		return -EINVAL;

It can't simply be ignored, otherwise users will have no way to know whether any
new fields are recognized or not.  Also if there is no validation, users will
screw up and pass in a nonzero value (perhaps through leaving it uninitialized)
before it's un-reserved, which will then break as soon as the space is actually
used for something.

Eric

Re: [PATCH v2] KEYS: Convert KEYCTL_DH_COMPUTE to use the crypto KPP API

[Mat Martineau]

Hi Eric -

On Sun, 4 Jun 2017, Eric Biggers wrote:

> Hi Mat,
>
> On Wed, May 10, 2017 at 05:35:57PM -0700, Mat Martineau wrote:
>> The initial Diffie-Hellman computation made direct use of the MPI
>> library because the crypto module did not support DH at the time. Now
>> that KPP is implemented, KEYCTL_DH_COMPUTE should use it to get rid of
>> duplicate code and leverage possible hardware acceleration.
>>
>> Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>
> Have you verified that this doesn't change the API at all --- order of bytes
> within the numbers, whether they are padded with zeroes, any other corner cases
> etc.?  There can't be breaking changes to the API after this is released in
> mainline, and this is already in v4.12-rc4.

The available unit tests in keyutils (next branch) and ELL still pass.

Stephan pointed out a difference regarding handling of leading zeros in 
KDF, which is what I addressed in v2 of the patch. There's a gap in the 
unit tests where results with leading zeros are not covered well, and I'm 
working on that. DH results are definitely handled the same with this 
patch, but I have yet to confirm KDF output - I don't have a known-good 
test vector for KDF when the shared secret has leading zeros.

--
Mat Martineau
Intel OTC

Re: [PATCH v2] KEYS: Convert KEYCTL_DH_COMPUTE to use the crypto KPP API

[Mat Martineau]

Hi David -

On Fri, 2 Jun 2017, David Howells wrote:

> Mat Martineau <mathew.j.martineau@linux.intel.com> wrote:
>
>> The initial Diffie-Hellman computation made direct use of the MPI
>> library because the crypto module did not support DH at the time. Now
>> that KPP is implemented, KEYCTL_DH_COMPUTE should use it to get rid of
>> duplicate code and leverage possible hardware acceleration.
>
> This doesn't apply to linus/master.

It was on top of keys-next, for what it's worth.

> I've pushed the keyrings fix patches I
> have, including a bunch from Eric Biggers that fix DH stuff, to:
>
> 	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git
>
> branch:
>
> 	keys-fixes

I'll post a v3 that applies to keys-fixes right after I send this email.

>
> though I think there may be a couple of bugs in on of Eric's patches where
> he's assumed that he can do:
>
> 	memzero_explicit(NULL, 0);
>
> I'm not sure whether it's permissible to assume that memset(NULL, 0, 0) is
> guaranteed to work correctly.

I'm still working on unit test coverage to confirm correct behavior of KDF 
when the DH shared secret has leading zeros. Stephan, have you found any 
such tests (last time I asked you were still looking)? If I see 
inconsistent results when I make up a vector (choosing inputs that result 
in a 0x01 shared secret), I'm not sure if the old or new answer is 
correct.

> Note that I haven't included Eric's DH patch that was obsoleted by Stephan's
> patch that was obsoleted by this one.

Thanks,

--
Mat Martineau
Intel OTC

[PATCH v2] KEYS: Convert KEYCTL_DH_COMPUTE to use the crypto KPP API

[Mat Martineau]

Hi David -

On Fri, 2 Jun 2017, David Howells wrote:

> Mat Martineau <mathew.j.martineau@linux.intel.com> wrote:
>
>> The initial Diffie-Hellman computation made direct use of the MPI
>> library because the crypto module did not support DH at the time. Now
>> that KPP is implemented, KEYCTL_DH_COMPUTE should use it to get rid of
>> duplicate code and leverage possible hardware acceleration.
>
> This doesn't apply to linus/master.

It was on top of keys-next, for what it's worth.

> I've pushed the keyrings fix patches I
> have, including a bunch from Eric Biggers that fix DH stuff, to:
>
> 	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git
>
> branch:
>
> 	keys-fixes

I'll post a v3 that applies to keys-fixes right after I send this email.

>
> though I think there may be a couple of bugs in on of Eric's patches where
> he's assumed that he can do:
>
> 	memzero_explicit(NULL, 0);
>
> I'm not sure whether it's permissible to assume that memset(NULL, 0, 0) is
> guaranteed to work correctly.

I'm still working on unit test coverage to confirm correct behavior of KDF 
when the DH shared secret has leading zeros. Stephan, have you found any 
such tests (last time I asked you were still looking)? If I see 
inconsistent results when I make up a vector (choosing inputs that result 
in a 0x01 shared secret), I'm not sure if the old or new answer is 
correct.

> Note that I haven't included Eric's DH patch that was obsoleted by Stephan's
> patch that was obsoleted by this one.

Thanks,

--
Mat Martineau
Intel OTC
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] KEYS: Convert KEYCTL_DH_COMPUTE to use the crypto KPP API

[Mat Martineau]

On Mon, 5 Jun 2017, Mat Martineau wrote:

>
> Hi Eric -
>
> On Sun, 4 Jun 2017, Eric Biggers wrote:
>
>> Hi Mat,
>> 
>> On Wed, May 10, 2017 at 05:35:57PM -0700, Mat Martineau wrote:
>>> The initial Diffie-Hellman computation made direct use of the MPI
>>> library because the crypto module did not support DH at the time. Now
>>> that KPP is implemented, KEYCTL_DH_COMPUTE should use it to get rid of
>>> duplicate code and leverage possible hardware acceleration.
>>> 
>>> Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>> 
>> Have you verified that this doesn't change the API at all --- order of 
>> bytes within the numbers, whether they are padded with zeroes, any 
>> other corner cases etc.?  There can't be breaking changes to the API 
>> after this is released in mainline, and this is already in v4.12-rc4.
>
> The available unit tests in keyutils (next branch) and ELL still pass.
>
> Stephan pointed out a difference regarding handling of leading zeros in 
> KDF, which is what I addressed in v2 of the patch. There's a gap in the 
> unit tests where results with leading zeros are not covered well, and 
> I'm working on that. DH results are definitely handled the same with 
> this patch, but I have yet to confirm KDF output - I don't have a 
> known-good test vector for KDF when the shared secret has leading zeros.

To close this out, I have confirmed that the leading zeros are handled 
correctly by the "KEYS: Convert KEYCTL_DH_COMPUTE to use the crypto KPP 
API" patch v2 and later. I'm sending the keyutils unit test to this list.

--
Mat Martineau
Intel OTC