2020-05-19 19:05:16

by Ard Biesheuvel

[permalink] [raw]
Subject: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

Stephan reports that the arm64 implementation of cts(cbc(aes)) deviates
from the generic implementation in what it returns as the output IV. So
fix this, and add some test vectors to catch other non-compliant
implementations.

Stephan, could you provide a reference for the NIST validation tool and
how it flags this behaviour as non-compliant? Thanks.

Cc: Stephan Mueller <[email protected]>

Ard Biesheuvel (2):
crypto: arm64/aes - align output IV with generic CBC-CTS driver
crypto: testmgr - add output IVs for AES-CBC with ciphertext stealing

arch/arm64/crypto/aes-modes.S | 2 ++
crypto/testmgr.h | 12 ++++++++++++
2 files changed, 14 insertions(+)

--
2.20.1


2020-05-19 19:05:28

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

(add Gilad for cc-ree)

On Tue, 19 May 2020 at 21:02, Ard Biesheuvel <[email protected]> wrote:
>
> Stephan reports that the arm64 implementation of cts(cbc(aes)) deviates
> from the generic implementation in what it returns as the output IV. So
> fix this, and add some test vectors to catch other non-compliant
> implementations.
>
> Stephan, could you provide a reference for the NIST validation tool and
> how it flags this behaviour as non-compliant? Thanks.
>
> Cc: Stephan Mueller <[email protected]>
>
> Ard Biesheuvel (2):
> crypto: arm64/aes - align output IV with generic CBC-CTS driver
> crypto: testmgr - add output IVs for AES-CBC with ciphertext stealing
>
> arch/arm64/crypto/aes-modes.S | 2 ++
> crypto/testmgr.h | 12 ++++++++++++
> 2 files changed, 14 insertions(+)
>
> --
> 2.20.1
>

2020-05-20 06:04:44

by Stephan Müller

[permalink] [raw]
Subject: Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

Am Dienstag, 19. Mai 2020, 21:02:09 CEST schrieb Ard Biesheuvel:

Hi Ard,

> Stephan reports that the arm64 implementation of cts(cbc(aes)) deviates
> from the generic implementation in what it returns as the output IV. So
> fix this, and add some test vectors to catch other non-compliant
> implementations.
>
> Stephan, could you provide a reference for the NIST validation tool and
> how it flags this behaviour as non-compliant? Thanks.

The test definition that identified the inconsistent behavior is specified
with [1]. Note, this testing is intended to become an RFC standard.

To facilitate that testing, NIST offers an internet service, the ACVP server,
that allows obtaining test vectors and uploading responses. You see the large
number of concluded testing with [2]. A particular completion of the CTS
testing I finished yesterday is given in [3]. That particular testing was also
performed on an ARM system with CE where the issue was detected.

I am performing the testing with [4] that has an extension to test the kernel
crypto API.

[1] https://github.com/usnistgov/ACVP/blob/master/artifacts/draft-celi-acvp-block-ciph-00.txt#L366

[2] https://csrc.nist.gov/projects/cryptographic-algorithm-validation-program/
validation-search?searchMode=validation&family=1&productType=-1&ipp=25

[3] https://csrc.nist.gov/projects/cryptographic-algorithm-validation-program/
details?validation=32608

[4] https://github.com/smuellerDD/acvpparser
>
> Cc: Stephan Mueller <[email protected]>
>
> Ard Biesheuvel (2):
> crypto: arm64/aes - align output IV with generic CBC-CTS driver
> crypto: testmgr - add output IVs for AES-CBC with ciphertext stealing
>
> arch/arm64/crypto/aes-modes.S | 2 ++
> crypto/testmgr.h | 12 ++++++++++++
> 2 files changed, 14 insertions(+)


Ciao
Stephan


2020-05-20 06:41:51

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

On Wed, 20 May 2020 at 08:03, Stephan Mueller <[email protected]> wrote:
>
> Am Dienstag, 19. Mai 2020, 21:02:09 CEST schrieb Ard Biesheuvel:
>
> Hi Ard,
>
> > Stephan reports that the arm64 implementation of cts(cbc(aes)) deviates
> > from the generic implementation in what it returns as the output IV. So
> > fix this, and add some test vectors to catch other non-compliant
> > implementations.
> >
> > Stephan, could you provide a reference for the NIST validation tool and
> > how it flags this behaviour as non-compliant? Thanks.
>
> The test definition that identified the inconsistent behavior is specified
> with [1]. Note, this testing is intended to become an RFC standard.
>

Are you referring to the line

CT[j] = AES_CBC_CS_ENCRYPT(Key[i], PT[j])

where the CTS transform is invoked without an IV altogether? That
simply seems like a bug to me. In an abstract specification like this,
it would be insane for pseudocode functions to be stateful objects,
and there is nothing in the pseudocode that explicitly captures the
'output IV' of that function call.


> To facilitate that testing, NIST offers an internet service, the ACVP server,
> that allows obtaining test vectors and uploading responses. You see the large
> number of concluded testing with [2]. A particular completion of the CTS
> testing I finished yesterday is given in [3]. That particular testing was also
> performed on an ARM system with CE where the issue was detected.
>
> I am performing the testing with [4] that has an extension to test the kernel
> crypto API.
>

OK. So given that that neither the CTS spec nor this document makes
any mention of an output IV or what its value should be, my suggestion
would be to capture the IV directly from the ciphertext, rather than
relying on some unspecified behavior to give you the right data. Note
that we have other implementations of cts(cbc(aes)) in the kernel as
well (h/w accelerated ones) and if there is no specification that
defines this behavior, you really shouldn't be relying on it.


That 'specification' invokes AES_CBC_CS_ENCRYPT() twice using a
different prototype, without any mention whatsoever what the implied
value of IV[] is if it is missing. This is especially problematic
given that it seems to cover all of CS1/2/3, and the relation between
next IV and ciphertext is not even the same between those for inputs
that are a multiple of the blocksize.


> [1] https://github.com/usnistgov/ACVP/blob/master/artifacts/draft-celi-acvp-block-ciph-00.txt#L366
>
> [2] https://csrc.nist.gov/projects/cryptographic-algorithm-validation-program/
> validation-search?searchMode=validation&family=1&productType=-1&ipp=25
>
> [3] https://csrc.nist.gov/projects/cryptographic-algorithm-validation-program/
> details?validation=32608
>
> [4] https://github.com/smuellerDD/acvpparser
> >
> > Cc: Stephan Mueller <[email protected]>
> >
> > Ard Biesheuvel (2):
> > crypto: arm64/aes - align output IV with generic CBC-CTS driver
> > crypto: testmgr - add output IVs for AES-CBC with ciphertext stealing
> >
> > arch/arm64/crypto/aes-modes.S | 2 ++
> > crypto/testmgr.h | 12 ++++++++++++
> > 2 files changed, 14 insertions(+)
>
>
> Ciao
> Stephan
>
>

2020-05-20 06:48:37

by Stephan Müller

[permalink] [raw]
Subject: Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

Am Mittwoch, 20. Mai 2020, 08:40:57 CEST schrieb Ard Biesheuvel:

Hi Ard,

> On Wed, 20 May 2020 at 08:03, Stephan Mueller <[email protected]> wrote:
> > Am Dienstag, 19. Mai 2020, 21:02:09 CEST schrieb Ard Biesheuvel:
> >
> > Hi Ard,
> >
> > > Stephan reports that the arm64 implementation of cts(cbc(aes)) deviates
> > > from the generic implementation in what it returns as the output IV. So
> > > fix this, and add some test vectors to catch other non-compliant
> > > implementations.
> > >
> > > Stephan, could you provide a reference for the NIST validation tool and
> > > how it flags this behaviour as non-compliant? Thanks.
> >
> > The test definition that identified the inconsistent behavior is specified
> > with [1]. Note, this testing is intended to become an RFC standard.
>
> Are you referring to the line
>
> CT[j] = AES_CBC_CS_ENCRYPT(Key[i], PT[j])
>
> where the CTS transform is invoked without an IV altogether?

Precisely.

> That
> simply seems like a bug to me. In an abstract specification like this,
> it would be insane for pseudocode functions to be stateful objects,
> and there is nothing in the pseudocode that explicitly captures the
> 'output IV' of that function call.

I think the description may be updated by simply refer to IV[j-1]. Then you
would not have a stateful operation, but you rest on the IV of the previous
operation.

The state of all block chaining modes we currently have is defined with the
IV. That is the reason why I mentioned it can be implemented stateless when I
am able to get the IV output from the previous operation.

>
> > To facilitate that testing, NIST offers an internet service, the ACVP
> > server, that allows obtaining test vectors and uploading responses. You
> > see the large number of concluded testing with [2]. A particular
> > completion of the CTS testing I finished yesterday is given in [3]. That
> > particular testing was also performed on an ARM system with CE where the
> > issue was detected.
> >
> > I am performing the testing with [4] that has an extension to test the
> > kernel crypto API.
>
> OK. So given that that neither the CTS spec nor this document makes
> any mention of an output IV or what its value should be, my suggestion
> would be to capture the IV directly from the ciphertext, rather than
> relying on some unspecified behavior to give you the right data. Note
> that we have other implementations of cts(cbc(aes)) in the kernel as
> well (h/w accelerated ones) and if there is no specification that
> defines this behavior, you really shouldn't be relying on it.

Agreed, but all I need is the IV from the previous round without relying on
any state.
>
>
> That 'specification' invokes AES_CBC_CS_ENCRYPT() twice using a
> different prototype, without any mention whatsoever what the implied
> value of IV[] is if it is missing. This is especially problematic
> given that it seems to cover all of CS1/2/3, and the relation between
> next IV and ciphertext is not even the same between those for inputs
> that are a multiple of the blocksize.

I will relay that comment back to the authors for update.
>
> > [1]
> > https://github.com/usnistgov/ACVP/blob/master/artifacts/draft-celi-acvp-b
> > lock-ciph-00.txt#L366
> >
> > [2]
> > https://csrc.nist.gov/projects/cryptographic-algorithm-validation-program
> > / validation-search?searchMode=validation&family=1&productType=-1&ipp=25
> >
> > [3]
> > https://csrc.nist.gov/projects/cryptographic-algorithm-validation-program
> > / details?validation=32608
> >
> > [4] https://github.com/smuellerDD/acvpparser
> >
> > > Cc: Stephan Mueller <[email protected]>
> > >
> > > Ard Biesheuvel (2):
> > > crypto: arm64/aes - align output IV with generic CBC-CTS driver
> > > crypto: testmgr - add output IVs for AES-CBC with ciphertext stealing
> > >
> > > arch/arm64/crypto/aes-modes.S | 2 ++
> > > crypto/testmgr.h | 12 ++++++++++++
> > > 2 files changed, 14 insertions(+)
> >
> > Ciao
> > Stephan


Ciao
Stephan


2020-05-20 06:54:51

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

On Wed, 20 May 2020 at 08:47, Stephan Mueller <[email protected]> wrote:
>
> Am Mittwoch, 20. Mai 2020, 08:40:57 CEST schrieb Ard Biesheuvel:
>
> Hi Ard,
>
> > On Wed, 20 May 2020 at 08:03, Stephan Mueller <[email protected]> wrote:
> > > Am Dienstag, 19. Mai 2020, 21:02:09 CEST schrieb Ard Biesheuvel:
> > >
> > > Hi Ard,
> > >
> > > > Stephan reports that the arm64 implementation of cts(cbc(aes)) deviates
> > > > from the generic implementation in what it returns as the output IV. So
> > > > fix this, and add some test vectors to catch other non-compliant
> > > > implementations.
> > > >
> > > > Stephan, could you provide a reference for the NIST validation tool and
> > > > how it flags this behaviour as non-compliant? Thanks.
> > >
> > > The test definition that identified the inconsistent behavior is specified
> > > with [1]. Note, this testing is intended to become an RFC standard.
> >
> > Are you referring to the line
> >
> > CT[j] = AES_CBC_CS_ENCRYPT(Key[i], PT[j])
> >
> > where the CTS transform is invoked without an IV altogether?
>
> Precisely.
>
> > That
> > simply seems like a bug to me. In an abstract specification like this,
> > it would be insane for pseudocode functions to be stateful objects,
> > and there is nothing in the pseudocode that explicitly captures the
> > 'output IV' of that function call.
>
> I think the description may be updated by simply refer to IV[j-1]. Then you
> would not have a stateful operation, but you rest on the IV of the previous
> operation.
>

But that value is not the value you are using now, right? I suspect
that the line

IV[i+1] = MSB(CT[j], IV.length)

needs to be duplicated in the inner loop for j, although that would
require different versions for CS1/2/3


> The state of all block chaining modes we currently have is defined with the
> IV. That is the reason why I mentioned it can be implemented stateless when I
> am able to get the IV output from the previous operation.
>

But it is simply the same as the penultimate block of ciphertext. So
you can simply capture it after encrypt, or before decrypt. There is
really no need to rely on the CTS transformation to pass it back to
you via the buffer that is only specified to provide an input to the
CTS transform.


> >
> > > To facilitate that testing, NIST offers an internet service, the ACVP
> > > server, that allows obtaining test vectors and uploading responses. You
> > > see the large number of concluded testing with [2]. A particular
> > > completion of the CTS testing I finished yesterday is given in [3]. That
> > > particular testing was also performed on an ARM system with CE where the
> > > issue was detected.
> > >
> > > I am performing the testing with [4] that has an extension to test the
> > > kernel crypto API.
> >
> > OK. So given that that neither the CTS spec nor this document makes
> > any mention of an output IV or what its value should be, my suggestion
> > would be to capture the IV directly from the ciphertext, rather than
> > relying on some unspecified behavior to give you the right data. Note
> > that we have other implementations of cts(cbc(aes)) in the kernel as
> > well (h/w accelerated ones) and if there is no specification that
> > defines this behavior, you really shouldn't be relying on it.
>
> Agreed, but all I need is the IV from the previous round without relying on
> any state.

So just grab it from the ciphertext of the previous round.

> >
> >
> > That 'specification' invokes AES_CBC_CS_ENCRYPT() twice using a
> > different prototype, without any mention whatsoever what the implied
> > value of IV[] is if it is missing. This is especially problematic
> > given that it seems to cover all of CS1/2/3, and the relation between
> > next IV and ciphertext is not even the same between those for inputs
> > that are a multiple of the blocksize.
>
> I will relay that comment back to the authors for update.

Thanks.


> >
> > > [1]
> > > https://github.com/usnistgov/ACVP/blob/master/artifacts/draft-celi-acvp-b
> > > lock-ciph-00.txt#L366
> > >
> > > [2]
> > > https://csrc.nist.gov/projects/cryptographic-algorithm-validation-program
> > > / validation-search?searchMode=validation&family=1&productType=-1&ipp=25
> > >
> > > [3]
> > > https://csrc.nist.gov/projects/cryptographic-algorithm-validation-program
> > > / details?validation=32608
> > >
> > > [4] https://github.com/smuellerDD/acvpparser
> > >
> > > > Cc: Stephan Mueller <[email protected]>
> > > >
> > > > Ard Biesheuvel (2):
> > > > crypto: arm64/aes - align output IV with generic CBC-CTS driver
> > > > crypto: testmgr - add output IVs for AES-CBC with ciphertext stealing
> > > >
> > > > arch/arm64/crypto/aes-modes.S | 2 ++
> > > > crypto/testmgr.h | 12 ++++++++++++
> > > > 2 files changed, 14 insertions(+)
> > >
> > > Ciao
> > > Stephan
>
>
> Ciao
> Stephan
>
>

2020-05-20 07:01:41

by Stephan Müller

[permalink] [raw]
Subject: Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

Am Mittwoch, 20. Mai 2020, 08:54:10 CEST schrieb Ard Biesheuvel:

Hi Ard,

> On Wed, 20 May 2020 at 08:47, Stephan Mueller <[email protected]> wrote:
> > Am Mittwoch, 20. Mai 2020, 08:40:57 CEST schrieb Ard Biesheuvel:
> >
> > Hi Ard,
> >
> > > On Wed, 20 May 2020 at 08:03, Stephan Mueller <[email protected]>
wrote:
> > > > Am Dienstag, 19. Mai 2020, 21:02:09 CEST schrieb Ard Biesheuvel:
> > > >
> > > > Hi Ard,
> > > >
> > > > > Stephan reports that the arm64 implementation of cts(cbc(aes))
> > > > > deviates
> > > > > from the generic implementation in what it returns as the output IV.
> > > > > So
> > > > > fix this, and add some test vectors to catch other non-compliant
> > > > > implementations.
> > > > >
> > > > > Stephan, could you provide a reference for the NIST validation tool
> > > > > and
> > > > > how it flags this behaviour as non-compliant? Thanks.
> > > >
> > > > The test definition that identified the inconsistent behavior is
> > > > specified
> > > > with [1]. Note, this testing is intended to become an RFC standard.
> > >
> > > Are you referring to the line
> > >
> > > CT[j] = AES_CBC_CS_ENCRYPT(Key[i], PT[j])
> > >
> > > where the CTS transform is invoked without an IV altogether?
> >
> > Precisely.
> >
> > > That
> > > simply seems like a bug to me. In an abstract specification like this,
> > > it would be insane for pseudocode functions to be stateful objects,
> > > and there is nothing in the pseudocode that explicitly captures the
> > > 'output IV' of that function call.
> >
> > I think the description may be updated by simply refer to IV[j-1]. Then
> > you
> > would not have a stateful operation, but you rest on the IV of the
> > previous
> > operation.
>
> But that value is not the value you are using now, right? I suspect
> that the line
>
> IV[i+1] = MSB(CT[j], IV.length)

Yes, such a line would be needed.
>
> needs to be duplicated in the inner loop for j, although that would
> require different versions for CS1/2/3

Correct in the sense to specify exactly what the IV after a cipher operation
actually is.
>
> > The state of all block chaining modes we currently have is defined with
> > the
> > IV. That is the reason why I mentioned it can be implemented stateless
> > when I am able to get the IV output from the previous operation.
>
> But it is simply the same as the penultimate block of ciphertext. So
> you can simply capture it after encrypt, or before decrypt. There is
> really no need to rely on the CTS transformation to pass it back to
> you via the buffer that is only specified to provide an input to the
> CTS transform.

Let me recheck that as I am not fully sure on that one. But if it can be
handled that way, it would make life easier.
>
> > > > To facilitate that testing, NIST offers an internet service, the ACVP
> > > > server, that allows obtaining test vectors and uploading responses.
> > > > You
> > > > see the large number of concluded testing with [2]. A particular
> > > > completion of the CTS testing I finished yesterday is given in [3].
> > > > That
> > > > particular testing was also performed on an ARM system with CE where
> > > > the
> > > > issue was detected.
> > > >
> > > > I am performing the testing with [4] that has an extension to test the
> > > > kernel crypto API.
> > >
> > > OK. So given that that neither the CTS spec nor this document makes
> > > any mention of an output IV or what its value should be, my suggestion
> > > would be to capture the IV directly from the ciphertext, rather than
> > > relying on some unspecified behavior to give you the right data. Note
> > > that we have other implementations of cts(cbc(aes)) in the kernel as
> > > well (h/w accelerated ones) and if there is no specification that
> > > defines this behavior, you really shouldn't be relying on it.
> >
> > Agreed, but all I need is the IV from the previous round without relying
> > on
> > any state.
>
> So just grab it from the ciphertext of the previous round.
>
> > > That 'specification' invokes AES_CBC_CS_ENCRYPT() twice using a
> > > different prototype, without any mention whatsoever what the implied
> > > value of IV[] is if it is missing. This is especially problematic
> > > given that it seems to cover all of CS1/2/3, and the relation between
> > > next IV and ciphertext is not even the same between those for inputs
> > > that are a multiple of the blocksize.
> >
> > I will relay that comment back to the authors for update.
>
> Thanks.

Thank you for your ideas!
>
> > > > [1]
> > > > https://github.com/usnistgov/ACVP/blob/master/artifacts/draft-celi-acv
> > > > p-b
> > > > lock-ciph-00.txt#L366
> > > >
> > > > [2]
> > > > https://csrc.nist.gov/projects/cryptographic-algorithm-validation-prog
> > > > ram
> > > > /
> > > > validation-search?searchMode=validation&family=1&productType=-1&ipp=2
> > > > 5
> > > >
> > > > [3]
> > > > https://csrc.nist.gov/projects/cryptographic-algorithm-validation-prog
> > > > ram
> > > > / details?validation=32608
> > > >
> > > > [4] https://github.com/smuellerDD/acvpparser
> > > >
> > > > > Cc: Stephan Mueller <[email protected]>
> > > > >
> > > > > Ard Biesheuvel (2):
> > > > > crypto: arm64/aes - align output IV with generic CBC-CTS driver
> > > > > crypto: testmgr - add output IVs for AES-CBC with ciphertext
> > > > > stealing
> > > > >
> > > > > arch/arm64/crypto/aes-modes.S | 2 ++
> > > > > crypto/testmgr.h | 12 ++++++++++++
> > > > > 2 files changed, 14 insertions(+)
> > > >
> > > > Ciao
> > > > Stephan
> >
> > Ciao
> > Stephan


Ciao
Stephan


2020-05-20 07:10:26

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

On Wed, 20 May 2020 at 09:01, Stephan Mueller <[email protected]> wrote:
>
> Am Mittwoch, 20. Mai 2020, 08:54:10 CEST schrieb Ard Biesheuvel:
>
> Hi Ard,
>
> > On Wed, 20 May 2020 at 08:47, Stephan Mueller <[email protected]> wrote:
...
> > > The state of all block chaining modes we currently have is defined with
> > > the
> > > IV. That is the reason why I mentioned it can be implemented stateless
> > > when I am able to get the IV output from the previous operation.
> >
> > But it is simply the same as the penultimate block of ciphertext. So
> > you can simply capture it after encrypt, or before decrypt. There is
> > really no need to rely on the CTS transformation to pass it back to
> > you via the buffer that is only specified to provide an input to the
> > CTS transform.
>
> Let me recheck that as I am not fully sure on that one. But if it can be
> handled that way, it would make life easier.

Please refer to patch 2. The .iv_out test vectors were all simply
copied from the appropriate offset into the associated .ctext member.

2020-05-21 13:02:39

by Gilad Ben-Yossef

[permalink] [raw]
Subject: Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

Hi Ard,

Thank you for looping me in.

On Wed, May 20, 2020 at 10:09 AM Ard Biesheuvel <[email protected]> wrote:
>
> On Wed, 20 May 2020 at 09:01, Stephan Mueller <[email protected]> wrote:
> >
> > Am Mittwoch, 20. Mai 2020, 08:54:10 CEST schrieb Ard Biesheuvel:
> >
> > Hi Ard,
> >
> > > On Wed, 20 May 2020 at 08:47, Stephan Mueller <[email protected]> wrote:
> ...
> > > > The state of all block chaining modes we currently have is defined with
> > > > the
> > > > IV. That is the reason why I mentioned it can be implemented stateless
> > > > when I am able to get the IV output from the previous operation.
> > >
> > > But it is simply the same as the penultimate block of ciphertext. So
> > > you can simply capture it after encrypt, or before decrypt. There is
> > > really no need to rely on the CTS transformation to pass it back to
> > > you via the buffer that is only specified to provide an input to the
> > > CTS transform.
> >
> > Let me recheck that as I am not fully sure on that one. But if it can be
> > handled that way, it would make life easier.
>
> Please refer to patch 2. The .iv_out test vectors were all simply
> copied from the appropriate offset into the associated .ctext member.

Not surprisingly since to the best of my understanding this behaviour
is not strictly specified, ccree currently fails the IV output check
with the 2nd version of the patch.

If I understand you correctly, the expected output IV is simply the
next to last block of the ciphertext?

Thanks,
Gilad



--
Gilad Ben-Yossef
Chief Coffee Drinker

values of β will give rise to dom!

2020-05-21 13:24:44

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

On Thu, 21 May 2020 at 15:01, Gilad Ben-Yossef <[email protected]> wrote:
>
> Hi Ard,
>
> Thank you for looping me in.
>
> On Wed, May 20, 2020 at 10:09 AM Ard Biesheuvel <[email protected]> wrote:
> >
> > On Wed, 20 May 2020 at 09:01, Stephan Mueller <[email protected]> wrote:
> > >
> > > Am Mittwoch, 20. Mai 2020, 08:54:10 CEST schrieb Ard Biesheuvel:
> > >
> > > Hi Ard,
> > >
> > > > On Wed, 20 May 2020 at 08:47, Stephan Mueller <[email protected]> wrote:
> > ...
> > > > > The state of all block chaining modes we currently have is defined with
> > > > > the
> > > > > IV. That is the reason why I mentioned it can be implemented stateless
> > > > > when I am able to get the IV output from the previous operation.
> > > >
> > > > But it is simply the same as the penultimate block of ciphertext. So
> > > > you can simply capture it after encrypt, or before decrypt. There is
> > > > really no need to rely on the CTS transformation to pass it back to
> > > > you via the buffer that is only specified to provide an input to the
> > > > CTS transform.
> > >
> > > Let me recheck that as I am not fully sure on that one. But if it can be
> > > handled that way, it would make life easier.
> >
> > Please refer to patch 2. The .iv_out test vectors were all simply
> > copied from the appropriate offset into the associated .ctext member.
>
> Not surprisingly since to the best of my understanding this behaviour
> is not strictly specified, ccree currently fails the IV output check
> with the 2nd version of the patch.
>

That is what I suspected, hence the cc:

> If I understand you correctly, the expected output IV is simply the
> next to last block of the ciphertext?

Yes. But this happens to work for the generic case because the CTS
driver itself requires the encapsulated CBC mode to return the output
IV, which is simply passed through back to the caller. CTS mode itself
does not specify any kind of output IV, so we should not rely on this
behavior.

2020-05-23 18:53:21

by Stephan Müller

[permalink] [raw]
Subject: Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

Am Donnerstag, 21. Mai 2020, 15:23:41 CEST schrieb Ard Biesheuvel:

Hi Ard,

> On Thu, 21 May 2020 at 15:01, Gilad Ben-Yossef <[email protected]> wrote:
> > Hi Ard,
> >
> > Thank you for looping me in.
> >
> > On Wed, May 20, 2020 at 10:09 AM Ard Biesheuvel <[email protected]> wrote:
> > > On Wed, 20 May 2020 at 09:01, Stephan Mueller <[email protected]>
wrote:
> > > > Am Mittwoch, 20. Mai 2020, 08:54:10 CEST schrieb Ard Biesheuvel:
> > > >
> > > > Hi Ard,
> > > >
> > > > > On Wed, 20 May 2020 at 08:47, Stephan Mueller <[email protected]>
wrote:
> > > ...
> > >
> > > > > > The state of all block chaining modes we currently have is defined
> > > > > > with
> > > > > > the
> > > > > > IV. That is the reason why I mentioned it can be implemented
> > > > > > stateless
> > > > > > when I am able to get the IV output from the previous operation.
> > > > >
> > > > > But it is simply the same as the penultimate block of ciphertext. So
> > > > > you can simply capture it after encrypt, or before decrypt. There is
> > > > > really no need to rely on the CTS transformation to pass it back to
> > > > > you via the buffer that is only specified to provide an input to the
> > > > > CTS transform.
> > > >
> > > > Let me recheck that as I am not fully sure on that one. But if it can
> > > > be
> > > > handled that way, it would make life easier.
> > >
> > > Please refer to patch 2. The .iv_out test vectors were all simply
> > > copied from the appropriate offset into the associated .ctext member.
> >
> > Not surprisingly since to the best of my understanding this behaviour
> > is not strictly specified, ccree currently fails the IV output check
> > with the 2nd version of the patch.
>
> That is what I suspected, hence the cc:
> > If I understand you correctly, the expected output IV is simply the
> > next to last block of the ciphertext?
>
> Yes. But this happens to work for the generic case because the CTS
> driver itself requires the encapsulated CBC mode to return the output
> IV, which is simply passed through back to the caller. CTS mode itself
> does not specify any kind of output IV, so we should not rely on this
> behavior.

Note, the update to the spec based on your suggestion is already in a merge
request:

https://github.com/usnistgov/ACVP/issues/860

Thanks for your input.

Ciao
Stephan


2020-05-23 22:41:12

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

On Sat, 23 May 2020 at 20:52, Stephan Müller <[email protected]> wrote:
>
> Am Donnerstag, 21. Mai 2020, 15:23:41 CEST schrieb Ard Biesheuvel:
>
> Hi Ard,
>
> > On Thu, 21 May 2020 at 15:01, Gilad Ben-Yossef <[email protected]> wrote:
> > > Hi Ard,
> > >
> > > Thank you for looping me in.
> > >
> > > On Wed, May 20, 2020 at 10:09 AM Ard Biesheuvel <[email protected]> wrote:
> > > > On Wed, 20 May 2020 at 09:01, Stephan Mueller <[email protected]>
> wrote:
> > > > > Am Mittwoch, 20. Mai 2020, 08:54:10 CEST schrieb Ard Biesheuvel:
> > > > >
> > > > > Hi Ard,
> > > > >
> > > > > > On Wed, 20 May 2020 at 08:47, Stephan Mueller <[email protected]>
> wrote:
> > > > ...
> > > >
> > > > > > > The state of all block chaining modes we currently have is defined
> > > > > > > with
> > > > > > > the
> > > > > > > IV. That is the reason why I mentioned it can be implemented
> > > > > > > stateless
> > > > > > > when I am able to get the IV output from the previous operation.
> > > > > >
> > > > > > But it is simply the same as the penultimate block of ciphertext. So
> > > > > > you can simply capture it after encrypt, or before decrypt. There is
> > > > > > really no need to rely on the CTS transformation to pass it back to
> > > > > > you via the buffer that is only specified to provide an input to the
> > > > > > CTS transform.
> > > > >
> > > > > Let me recheck that as I am not fully sure on that one. But if it can
> > > > > be
> > > > > handled that way, it would make life easier.
> > > >
> > > > Please refer to patch 2. The .iv_out test vectors were all simply
> > > > copied from the appropriate offset into the associated .ctext member.
> > >
> > > Not surprisingly since to the best of my understanding this behaviour
> > > is not strictly specified, ccree currently fails the IV output check
> > > with the 2nd version of the patch.
> >
> > That is what I suspected, hence the cc:
> > > If I understand you correctly, the expected output IV is simply the
> > > next to last block of the ciphertext?
> >
> > Yes. But this happens to work for the generic case because the CTS
> > driver itself requires the encapsulated CBC mode to return the output
> > IV, which is simply passed through back to the caller. CTS mode itself
> > does not specify any kind of output IV, so we should not rely on this
> > behavior.
>
> Note, the update to the spec based on your suggestion is already in a merge
> request:
>
> https://github.com/usnistgov/ACVP/issues/860
>
> Thanks for your input.
>

Thanks for the head's up. I've left a comment there, as the proposed
change is not equivalent to the unspecified current behavior.

2020-05-28 07:36:33

by Herbert Xu

[permalink] [raw]
Subject: Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

Ard Biesheuvel <[email protected]> wrote:
> Stephan reports that the arm64 implementation of cts(cbc(aes)) deviates
> from the generic implementation in what it returns as the output IV. So
> fix this, and add some test vectors to catch other non-compliant
> implementations.
>
> Stephan, could you provide a reference for the NIST validation tool and
> how it flags this behaviour as non-compliant? Thanks.

I think our CTS and XTS are both broken with respect to af_alg.

The reason we use output IVs in general is to support chaining
which is required by algif_skcipher to break up large requests
into smaller ones.

For CTS and XTS that simply doesn't work. So we should fix this
by changing algif_skcipher to not do chaining (and hence drop
support for large requests like algif_aead) for algorithms like
CTS/XTS.

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2020-05-28 08:34:11

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

On Thu, 28 May 2020 at 09:33, Herbert Xu <[email protected]> wrote:
>
> Ard Biesheuvel <[email protected]> wrote:
> > Stephan reports that the arm64 implementation of cts(cbc(aes)) deviates
> > from the generic implementation in what it returns as the output IV. So
> > fix this, and add some test vectors to catch other non-compliant
> > implementations.
> >
> > Stephan, could you provide a reference for the NIST validation tool and
> > how it flags this behaviour as non-compliant? Thanks.
>
> I think our CTS and XTS are both broken with respect to af_alg.
>
> The reason we use output IVs in general is to support chaining
> which is required by algif_skcipher to break up large requests
> into smaller ones.
>
> For CTS and XTS that simply doesn't work. So we should fix this
> by changing algif_skcipher to not do chaining (and hence drop
> support for large requests like algif_aead) for algorithms like
> CTS/XTS.
>

The reason we return output IVs for CBC is because our generic
implementation of CTS can wrap any CBC implementation, and relies on
this output IV rather than grabbing it from the ciphertext directly
(which may be tricky and is best left up to the CBC code)

For CTS itself, as well as XTS, returning an output IV is meaningless,
given that
a) the implementations rely on the skcipher_walk infrastructure to
present all input except the last bit in chunks that are a multiple of
the block size,
b) neither specification defines how chaining of inputs should work,
regardless of whether the preceding input was a multiple of the block
size or not.

The CS3 mode that we implement for CTS swaps the final two blocks
unconditionally. So even if the input is a whole multiple of the block
size, the preceding ciphertext will turn out differently if any output
happens to follow.

For XTS, the IV is encrypted before processing the first block, so
even if you do return an output IV, the subsequent invocations of the
skcipher need to omit the encryption, which we don't implement
currently.

So if you are saying that we should never split up algif_skcipher
requests into multiple calls into the underlying skcipher, then I
agree with you. Even if the generic CTS seems to work more or less as
expected by, e.g., the NIST validation tool, this is unspecified
behavior, and definitely broken in various other places.

2020-05-29 08:07:10

by Herbert Xu

[permalink] [raw]
Subject: Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

On Thu, May 28, 2020 at 10:33:25AM +0200, Ard Biesheuvel wrote:
>
> The reason we return output IVs for CBC is because our generic
> implementation of CTS can wrap any CBC implementation, and relies on
> this output IV rather than grabbing it from the ciphertext directly
> (which may be tricky and is best left up to the CBC code)

No that's not the main reason. The main user of chaining is
algif_skcipher.

> So if you are saying that we should never split up algif_skcipher
> requests into multiple calls into the underlying skcipher, then I
> agree with you. Even if the generic CTS seems to work more or less as
> expected by, e.g., the NIST validation tool, this is unspecified
> behavior, and definitely broken in various other places.

I was merely suggesting that requests to CTS/XTS shouldn't be
split up. Doing it for others would be a serious regression.

However, having looked at this it would seem that the effort
in marking CTS/XTS is not that different to just adding support
to hold the last two blocks of data so that CTS/XTS can support
chaining.

I'll work on this.

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2020-05-29 08:21:14

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

On Fri, 29 May 2020 at 10:05, Herbert Xu <[email protected]> wrote:
>
> On Thu, May 28, 2020 at 10:33:25AM +0200, Ard Biesheuvel wrote:
> >
> > The reason we return output IVs for CBC is because our generic
> > implementation of CTS can wrap any CBC implementation, and relies on
> > this output IV rather than grabbing it from the ciphertext directly
> > (which may be tricky and is best left up to the CBC code)
>
> No that's not the main reason. The main user of chaining is
> algif_skcipher.
>

But many implementation do not return an output IV at all. The only
mode that requires it (for the selftests to pass) is CBC.

> > So if you are saying that we should never split up algif_skcipher
> > requests into multiple calls into the underlying skcipher, then I
> > agree with you. Even if the generic CTS seems to work more or less as
> > expected by, e.g., the NIST validation tool, this is unspecified
> > behavior, and definitely broken in various other places.
>
> I was merely suggesting that requests to CTS/XTS shouldn't be
> split up. Doing it for others would be a serious regression.
>

Given that in these cases, doing so will give incorrect results even
if the input size is a whole multiple of the block size, I agree that
adding this restriction will not break anything that is working
consistently at the moment.

But could you elaborate on the serious regression for other cases? Do
you have anything particular in mind?

> However, having looked at this it would seem that the effort
> in marking CTS/XTS is not that different to just adding support
> to hold the last two blocks of data so that CTS/XTS can support
> chaining.
>

For XTS, we would have to carry some metadata around that tells you
whether the initial encryption of the IV has occurred or not. In the
CTS case, you need two swap the last two blocks of ciphertext at the
very end.

So does that mean some kind of init/update/final model for skcipher? I
can see how that could address these issues (init() would encrypt the
IV for XTS, and final() would do the final block handling for CTS).
Just holding two blocks of data does not seem sufficient to me to
handle these issues.

2020-05-29 11:52:08

by Herbert Xu

[permalink] [raw]
Subject: Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

On Fri, May 29, 2020 at 10:20:27AM +0200, Ard Biesheuvel wrote:
>
> But many implementation do not return an output IV at all. The only
> mode that requires it (for the selftests to pass) is CBC.

Most modes can be chained, e.g., CBC, PCBC, OFB, CFB and CTR.
As it stands algif_skcipher requres all algorithms to support
chaining.

> For XTS, we would have to carry some metadata around that tells you
> whether the initial encryption of the IV has occurred or not. In the

You're right, XTS in its current form cannot be chained. So we
do need a way to mark that for algif_skcipher.

> CTS case, you need two swap the last two blocks of ciphertext at the
> very end.

CTS can be easily chained. You just need to always keep two blocks
from being processed until you reach the end.

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2020-05-29 12:01:05

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

On Fri, 29 May 2020 at 13:51, Herbert Xu <[email protected]> wrote:
>
> On Fri, May 29, 2020 at 10:20:27AM +0200, Ard Biesheuvel wrote:
> >
> > But many implementation do not return an output IV at all. The only
> > mode that requires it (for the selftests to pass) is CBC.
>
> Most modes can be chained, e.g., CBC, PCBC, OFB, CFB and CTR.
> As it stands algif_skcipher requres all algorithms to support
> chaining.
>

Even if this is the case, it requires that an skcipher implementation
stores an output IV in the buffer that skcipher request's IV field
points to. Currently, we only check whether this is the case for CBC
implementations, and so it is quite likely that lots of h/w
accelerators or arch code don't adhere to this today.


> > For XTS, we would have to carry some metadata around that tells you
> > whether the initial encryption of the IV has occurred or not. In the
>
> You're right, XTS in its current form cannot be chained. So we
> do need a way to mark that for algif_skcipher.
>
> > CTS case, you need two swap the last two blocks of ciphertext at the
> > very end.
>
> CTS can be easily chained. You just need to always keep two blocks
> from being processed until you reach the end.
>

This might be feasible for the generic CTS driver wrapping h/w
accelerated CBC. But how is this supposed to work, e.g., for the two
existing h/w implementations of cts(cbc(aes)) that currently ignore
this?

2020-05-29 12:02:50

by Herbert Xu

[permalink] [raw]
Subject: Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

On Fri, May 29, 2020 at 02:00:14PM +0200, Ard Biesheuvel wrote:
>
> Even if this is the case, it requires that an skcipher implementation
> stores an output IV in the buffer that skcipher request's IV field
> points to. Currently, we only check whether this is the case for CBC
> implementations, and so it is quite likely that lots of h/w
> accelerators or arch code don't adhere to this today.

They are and have always been broken because algif_skcipher has
always relied on this.

> This might be feasible for the generic CTS driver wrapping h/w
> accelerated CBC. But how is this supposed to work, e.g., for the two
> existing h/w implementations of cts(cbc(aes)) that currently ignore
> this?

They'll have to disable chaining.

The way I'm doing this would allow some implementations to allow
chaining while others of the same algorithm can disable chaining
and require the whole request to be presented together.

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2020-05-29 13:11:28

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

On Fri, 29 May 2020 at 14:02, Herbert Xu <[email protected]> wrote:
>
> On Fri, May 29, 2020 at 02:00:14PM +0200, Ard Biesheuvel wrote:
> >
> > Even if this is the case, it requires that an skcipher implementation
> > stores an output IV in the buffer that skcipher request's IV field
> > points to. Currently, we only check whether this is the case for CBC
> > implementations, and so it is quite likely that lots of h/w
> > accelerators or arch code don't adhere to this today.
>
> They are and have always been broken because algif_skcipher has
> always relied on this.
>

OK, so the undocumented assumption is that algif_skcipher requests are
delineated by ALG_SET_IV commands, and that anything that gets sent to
the socket in between should be treated as a single request, right? I
think that makes sense, but do note that this deviates from Stephan's
use case, where the ciphertext stealing block swap was applied after
every call into af_alg, with the IV being inherited from one request
to the next. I think that case was invalid to begin with, I just hope
no other use cases exist where this unspecified behavior is being
relied upon.

> > This might be feasible for the generic CTS driver wrapping h/w
> > accelerated CBC. But how is this supposed to work, e.g., for the two
> > existing h/w implementations of cts(cbc(aes)) that currently ignore
> > this?
>
> They'll have to disable chaining.
>
> The way I'm doing this would allow some implementations to allow
> chaining while others of the same algorithm can disable chaining
> and require the whole request to be presented together.
>

Fair enough.

2020-05-29 13:20:28

by Herbert Xu

[permalink] [raw]
Subject: Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

On Fri, May 29, 2020 at 03:10:43PM +0200, Ard Biesheuvel wrote:
>
> OK, so the undocumented assumption is that algif_skcipher requests are
> delineated by ALG_SET_IV commands, and that anything that gets sent to
> the socket in between should be treated as a single request, right? I

Correct.

> think that makes sense, but do note that this deviates from Stephan's
> use case, where the ciphertext stealing block swap was applied after
> every call into af_alg, with the IV being inherited from one request
> to the next. I think that case was invalid to begin with, I just hope
> no other use cases exist where this unspecified behavior is being
> relied upon.

That does indeed sound broken.

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2020-05-29 13:43:04

by Herbert Xu

[permalink] [raw]
Subject: Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

On Fri, May 29, 2020 at 03:41:08PM +0200, Ard Biesheuvel wrote:
>
> So what about the final request? At which point do you decide to
> return the final chunk of data that you have been holding back in
> order to ensure that you can perform the final processing correctly if
> it is not being followed by a ALG_SET_IV command?

When the MORE flag is unset.

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2020-05-29 13:44:05

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [RFC/RFT PATCH 0/2] crypto: add CTS output IVs for arm64 and testmgr

On Fri, 29 May 2020 at 15:19, Herbert Xu <[email protected]> wrote:
>
> On Fri, May 29, 2020 at 03:10:43PM +0200, Ard Biesheuvel wrote:
> >
> > OK, so the undocumented assumption is that algif_skcipher requests are
> > delineated by ALG_SET_IV commands, and that anything that gets sent to
> > the socket in between should be treated as a single request, right? I
>
> Correct.
>

So what about the final request? At which point do you decide to
return the final chunk of data that you have been holding back in
order to ensure that you can perform the final processing correctly if
it is not being followed by a ALG_SET_IV command?

2020-06-12 12:10:01

by Herbert Xu

[permalink] [raw]
Subject: [PATCH 0/3] crypto: skcipher - Add support for no chaining and partial chaining

This patch-set adds support to the Crypto API and algif_skcipher
for algorithms that cannot be chained, as well as ones that can
be chained if you withhold a certain number of blocks at the end.

It only modifies one algorithm to utilise this, namely cts-generic.
Changing others should be fairly straightforward. In particular,
we should mark all the ones that don't support chaining (e.g., most
stream ciphers).

Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt