Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ChaCha inputs #2171

Closed
ekr opened this issue Dec 14, 2018 · 10 comments · Fixed by #2990
Closed

ChaCha inputs #2171

ekr opened this issue Dec 14, 2018 · 10 comments · Fixed by #2990
Labels
-tls design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.

Comments

@ekr
Copy link
Collaborator

ekr commented Dec 14, 2018

Here's what 5.4.4 says

in little-endian order and are used as the block count.  The remaining 12 bytes
are interpreted as three concatenated 32-bit numbers in little-endian order and
used as the nonce.

However, RFC 8439 says:

   o  A 96-bit nonce, treated as a concatenation of three 32-bit little-
      endian integers.

   o  A 32-bit block count parameter, treated as a 32-bit little-endian
      integer.

This is a little confusing, but I think the correct way to read this is that the ChaCha function takes the nonce as a 96-bit opaque quantity, and the fact that it's little-endian is an internal detail irrelevant to our purposes, so we should just replace the second sentence with "The remaining 12 bytes are used as the 96-bit ChaCha nonce". This is reinforced by the fact that the test vectors have the nonce as a byte block.

The situation with the counter is more complicated because we are told it is both a counter and 32-bits (which might make you think it's an integer parameter), but it is also treated as a little-endian integer, which makes one think it's a bitstring. And then the test vectors treat it as a value, rather than as a bitstring. If you look at code it typically thinks of this as an integer:

See, for instance:
https://searchfox.org/nss/source/lib/freebl/verified/Hacl_Chacha20.c#147

So, I think the right answer here is to think of ChaCha as having the following API:

ChaCha20(k [32]byte{}, nonce [12]byte{}, nonce uint32, []byte{})

Note that because the value on the wire is in big-endian format, this would invert the nonce, but I think it's the right answer anyway. Otherwise, you're having to pass in the counter in the wrong format on big-endian platforms.

@mikkelfj
Copy link
Contributor

As a completely general note, I don't think it is right to change endianness of crypto wire formats. AES-GCM has a completely backwards neither little nor big endian encoding (byte and bit swapped so no platform wins) this remains so on the wire. Especially for HW processing this is significant. But little endian is also more performant on nearly all relevant platforms today so there is no need to go over board forcing big endian if it can be avoided without insulting RFC's.

@martinthomson
Copy link
Member

Yeah, this should simplify that text some.

@martinthomson martinthomson added editorial An issue that does not affect the design of the protocol; does not require consensus. -tls labels Dec 14, 2018
@DavidSchinazi
Copy link
Contributor

I like the proposed change, the text had somewhat confused me when implementing this in a previous life.

@mnot mnot added this to Editorial Issues in Late Stage Processing Feb 27, 2019
@martinthomson
Copy link
Member

I don't think that the reinterpretation of the little-endian thing is safe, as per @mikkelfj's comment.

The PKCS#11 interface takes a byte buffer as input rather than a uint32_t. That is to allow for different block counter sizes. I'll make the editorial change, but leave the suggestion there alone.

@martinthomson
Copy link
Member

I have a question out to the editor of the PKCS#11 spec. It appears like there are no solid rules in the spec to support any particular position here. I'm going with my best guess at WWDJBD here, but I'm happy to make a change if there is a strong case made for something else.

martinthomson added a commit that referenced this issue Aug 26, 2019
The nonce can just be opaque bytes.

The block counter is tricky, as noted in the issue.  The "obvious"
choice is little-endian, as that is consistent with the philosophy of
the designer (as I understand it), but that is not anything more than a
guess.

Absent strong evidence that big-endian is a better choice, I'm going to
err on the side of not making a substantive change here.  But I think
that we need a consensus call to support that viewpoint.

Closes #2171.
@martinthomson martinthomson removed the editorial An issue that does not affect the design of the protocol; does not require consensus. label Aug 26, 2019
martinthomson added a commit that referenced this issue Aug 26, 2019
The nonce can just be opaque bytes.

The block counter is tricky, as noted in the issue.  The "obvious"
choice is little-endian, as that is consistent with the philosophy of
the designer (as I understand it), but that is not anything more than a
guess.

Absent strong evidence that big-endian is a better choice, I'm going to
err on the side of not making a substantive change here.  But I think
that we need a consensus call to support that viewpoint.

Closes #2171.
@martinthomson
Copy link
Member

@mnot, @larseggert, can we flip this to design please? I don't think that we need to change the substance here, but I want to confirm the little-endian/big-endian choice here.

I hope to have more information to share on the question soon, but that might not be definitive. We might just have to make a call ourselves.

Absent better information, I'm going to suggest that we continue with little-endian, but I this is far from an obvious conclusion.

@martinthomson martinthomson moved this from Editorial Issues to Triage in Late Stage Processing Aug 26, 2019
@martinthomson martinthomson added the proposal-ready An issue which has a proposal that is believed to be ready for a consensus call. label Aug 27, 2019
@mnot mnot added the design An issue that affects the design of the protocol; resolution requires consensus. label Aug 30, 2019
@mnot mnot moved this from Triage to Consensus Emerging in Late Stage Processing Aug 30, 2019
@mnot mnot moved this from Consensus Emerging to Consensus Call issued in Late Stage Processing Sep 3, 2019
@nharper
Copy link
Contributor

nharper commented Sep 5, 2019

The API I'm using for ChaCha20 takes a const uint8_t nonce[12], and I currently pass in a pointer (offset by 4 bytes) to the sample as the nonce, without doing any endianness conversions. I'm in favor of clarifying the language for this input. (Given that this has the design label instead of the editorial label, my design opinion is to keep with the simple thing of passing the bytes directly from the sample to the nonce input of ChaCha20 and not do any endianness conversions. I'm assuming that other crypto libraries have similar APIs though.)

@martinthomson
Copy link
Member

The API I have takes const uint8_t nonce[12], uint32_t counter. PKCS#11 takes pointers just like the API @nharper refers to (but it fails to specify the endianness, which is a critical oversight, compounded by the fact that the size of the counter can vary).

I suspect that - as @kazuho says - the right idea is to note that ChaCha20 will interpret the nonce as three little-endian integers and that the counter might be expressed as a byte sequence that is then (probably) interpreted as a little-endian integer.

Late Stage Processing automation moved this from Consensus Call issued to Text Incorporated Sep 10, 2019
@martinthomson martinthomson reopened this Sep 11, 2019
Late Stage Processing automation moved this from Text Incorporated to Triage Sep 11, 2019
@kazuho
Copy link
Member

kazuho commented Sep 11, 2019

I understand and appreciate this being marked as “design”, though I consider the text that landed in master as editorial, as it (in my view) ended as adding practical advice while retaining the definition we have had.

@martinthomson
Copy link
Member

Yes, but as a design issue, we want to confirm that "no action" is the correct outcome. It's cheap enough to do so.

@mnot mnot moved this from Triage to Consensus Emerging in Late Stage Processing Sep 13, 2019
@mnot mnot moved this from Consensus Emerging to Consensus Call issued in Late Stage Processing Sep 13, 2019
@mnot mnot added has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. and removed proposal-ready An issue which has a proposal that is believed to be ready for a consensus call. labels Sep 13, 2019
@mnot mnot moved this from Consensus Call issued to Consensus Declared in Late Stage Processing Sep 13, 2019
Late Stage Processing automation moved this from Consensus Declared to Text Incorporated Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-tls design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.
Projects
Late Stage Processing
  
Issue Handled
Development

Successfully merging a pull request may close this issue.

7 participants