Issue 130950043: code review 130950043: go.crypto/sha3: update to sync with draft FIPS-202
|
Created:
10 years, 7 months ago by coruus Modified:
10 years, 6 months ago Reviewers:
CC:
golang-codereviews, agl2, agl1 Visibility:
Public. |
Descriptiongo.crypto/sha3: update to sync with draft FIPS-202
1. API:
This exposes a minimal API: the SHA-3 functions implement hash.Hash. The
SHAKE functions implement a new "ShakeHash" interface that implements
io.Reader, io.Writer, Reset(), and a Clone() function.
(The previous Barrier() function has been removed.)
In addition, go.crypto/sha3 exposes a KeccakF1600 function, to allow
users building other primitives on top of KeccakF-1600 to benefit from
(future) optimized implementations of the permutation.
2. Tests
Added the complete set of ShortMsgKATs from
https://github.com/gvanas/KeccakCodePackage
3. Correctness
In sync with draft FIPS-202.
4. Documentation
A summary of the security properties of the SHA-3 and SHAKE functions is
provided in doc.go; some concrete recommendations as well.
Fixes 8563.
Patch Set 1 #Patch Set 2 : diff -r f3cebac2bd11c2bf03c15bfa4c66688ad02a0a40 https://code.google.com/p/go.crypto #
Total comments: 10
Patch Set 3 : diff -r f3cebac2bd11c2bf03c15bfa4c66688ad02a0a40 https://code.google.com/p/go.crypto #
Total comments: 1
Patch Set 4 : diff -r f3cebac2bd11c2bf03c15bfa4c66688ad02a0a40 https://code.google.com/p/go.crypto #Patch Set 5 : diff -r f3cebac2bd11c2bf03c15bfa4c66688ad02a0a40 https://code.google.com/p/go.crypto #Patch Set 6 : diff -r f3cebac2bd11c2bf03c15bfa4c66688ad02a0a40 https://code.google.com/p/go.crypto #
Total comments: 43
Patch Set 7 : diff -r 00a7d3b31bbab5795b4a51933c04fc2768242970 https://code.google.com/p/go.crypto #
Total comments: 2
MessagesTotal messages: 30
Hello golang-codereviews@googlegroups.com (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go.crypto
Sign in to reply to this message.
As a note, the JSON file with the processed Keccak Team KATs is available at: https://github.com/coruus/keccakc/blob/refactor-pad/keccak/tests/keccak_kats/... (It seems to be too big for codereview.)
Sign in to reply to this message.
R=agl
Sign in to reply to this message.
I can't download the JSON file at the moment because of a poor connection. How large is it? If huge, then it might be worth checking in a gzipped or bzipped version and decompressing on the fly in the tests. https://codereview.appspot.com/130950043/diff/40001/sha3/sha3.go File sha3/sha3.go (left): https://codereview.appspot.com/130950043/diff/40001/sha3/sha3.go#oldcode1 sha3/sha3.go:1: // Copyright 2013 The Go Authors. All rights reserved. Copyright header still needed. https://codereview.appspot.com/130950043/diff/40001/sha3/sha3.go#oldcode5 sha3/sha3.go:5: // Package sha3 implements the SHA3 hash algorithm (formerly called Keccak) chosen by NIST in 2012. Package comment probably still needed. https://codereview.appspot.com/130950043/diff/40001/sha3/sponge.go File sha3/sponge.go (right): https://codereview.appspot.com/130950043/diff/40001/sha3/sponge.go#newcode1 sha3/sponge.go:1: package sha3 Needs copyright header. https://codereview.appspot.com/130950043/diff/40001/sha3/sponge.go#newcode7 sha3/sponge.go:7: // Sponge defines the interface to cryptographic sponges. A sponge is This comment needs to be a lot more understandable to non-cryptographers :) https://codereview.appspot.com/130950043/diff/40001/sha3/sponge.go#newcode14 sha3/sponge.go:14: SpongeSize() int Each method should have a comment that starts with the name of the method. What about implementing io.Writer and io.Reader?
Sign in to reply to this message.
I can't download the JSON file at the moment because of a poor connection. How large is it? If huge, then it might be worth checking in a gzipped or bzipped version and decompressing on the fly in the tests. https://codereview.appspot.com/130950043/diff/40001/sha3/sha3.go File sha3/sha3.go (left): https://codereview.appspot.com/130950043/diff/40001/sha3/sha3.go#oldcode1 sha3/sha3.go:1: // Copyright 2013 The Go Authors. All rights reserved. Copyright header still needed. https://codereview.appspot.com/130950043/diff/40001/sha3/sha3.go#oldcode5 sha3/sha3.go:5: // Package sha3 implements the SHA3 hash algorithm (formerly called Keccak) chosen by NIST in 2012. Package comment probably still needed. https://codereview.appspot.com/130950043/diff/40001/sha3/sponge.go File sha3/sponge.go (right): https://codereview.appspot.com/130950043/diff/40001/sha3/sponge.go#newcode1 sha3/sponge.go:1: package sha3 Needs copyright header. https://codereview.appspot.com/130950043/diff/40001/sha3/sponge.go#newcode7 sha3/sponge.go:7: // Sponge defines the interface to cryptographic sponges. A sponge is This comment needs to be a lot more understandable to non-cryptographers :) https://codereview.appspot.com/130950043/diff/40001/sha3/sponge.go#newcode14 sha3/sponge.go:14: SpongeSize() int Each method should have a comment that starts with the name of the method. What about implementing io.Writer and io.Reader?
Sign in to reply to this message.
p.s. add "Fixes 8563." to the end of the description.
Sign in to reply to this message.
p.s. add "Fixes 8563." to the end of the description.
Sign in to reply to this message.
On 2014/08/21 17:23:15, agl1 wrote: > I can't download the JSON file at the moment because of a poor connection. How > large is it? If huge, then it might be worth checking in a gzipped or bzipped > version and decompressing on the fly in the tests. 1.1 MiB. Will do; would you prefer serialized via Gob?
Sign in to reply to this message.
On Thu, Aug 21, 2014 at 10:29 AM, <coruus@gmail.com> wrote: > 1.1 MiB. Will do; would you prefer serialized via Gob? It's nice to have something that's closely related to the upstream test file. But it's not essential if it's crushing the JSON parser. (Although keep in mind that the full test suite would probably not be run in -short mode so it's ok if the full tests take a few seconds.) Cheers AGL
Sign in to reply to this message.
On 2014/08/21 17:23:14, agl1 wrote: > I can't download the JSON file at the moment because of a poor connection. How > large is it? If huge, then it might be worth checking in a gzipped or bzipped > version and decompressing on the fly in the tests. > > What about implementing io.Writer and io.Reader? It already implements io.Writer, I think (via hash.Hash). Will implement io.Reader.
Sign in to reply to this message.
https://codereview.appspot.com/130950043/diff/40001/sha3/sha3.go File sha3/sha3.go (left): https://codereview.appspot.com/130950043/diff/40001/sha3/sha3.go#oldcode1 sha3/sha3.go:1: // Copyright 2013 The Go Authors. All rights reserved. On 2014/08/21 17:23:14, agl1 wrote: > Copyright header still needed. Acknowledged. https://codereview.appspot.com/130950043/diff/40001/sha3/sha3.go#oldcode5 sha3/sha3.go:5: // Package sha3 implements the SHA3 hash algorithm (formerly called Keccak) chosen by NIST in 2012. On 2014/08/21 17:23:14, agl1 wrote: > Package comment probably still needed. Acknowledged. https://codereview.appspot.com/130950043/diff/40001/sha3/sponge.go File sha3/sponge.go (right): https://codereview.appspot.com/130950043/diff/40001/sha3/sponge.go#newcode1 sha3/sponge.go:1: package sha3 On 2014/08/21 17:23:14, agl1 wrote: > Needs copyright header. Acknowledged. https://codereview.appspot.com/130950043/diff/40001/sha3/sponge.go#newcode7 sha3/sponge.go:7: // Sponge defines the interface to cryptographic sponges. A sponge is On 2014/08/21 17:23:14, agl1 wrote: > This comment needs to be a lot more understandable to non-cryptographers :) Acknowledged. https://codereview.appspot.com/130950043/diff/40001/sha3/sponge.go#newcode14 sha3/sponge.go:14: SpongeSize() int On 2014/08/21 17:23:14, agl1 wrote: > Each method should have a comment that starts with the name of the method. > > What about implementing io.Writer and io.Reader? io.Writer is implemented (via hash.Hash, unless I'm misunderstanding how interface embedding works -- which I may well be). Will implement io.Reader.
Sign in to reply to this message.
On 2014/08/21 18:09:07, agl2 wrote: > On Thu, Aug 21, 2014 at 10:29 AM, <mailto:coruus@gmail.com> wrote: > > 1.1 MiB. Will do; would you prefer serialized via Gob? > > It's nice to have something that's closely related to the upstream > test file. But it's not essential if it's crushing the JSON parser. > (Although keep in mind that the full test suite would probably not be > run in -short mode so it's ok if the full tests take a few seconds.) > > > Cheers > > AGL The JSON parser seems to have issues with one variant of the file; the deflated set included (zopfli --deflate --splitlast -i100) is based on one that doesn't cause the parser to eat itself. Full set of tests take 0.044s; probably fine even in short mode?
Sign in to reply to this message.
Higher level comments: The interface is very broad and non-standard. Interface is something that we have to commit to, even if we don't guarantee it in the subrepos as we do in the main repo. For example, although "absorb" and "squeeze" are standard terms for sponges, I think they should be Write and Read in Go, with the usual signature. (I.e. Read can return io.EOF if trying to read too much.) Do we really need to expose State, SecurityStrength, SpongeSize and Rate? If so, what's the concrete use of these? (For example, BlockSize is a bit of a wart, but exists for HMAC.) (I'm also unclear about the need to expose Pad.) MakeOneWay seems like a bad name because it's transient, no? I might want to call it again on a sponge that is already "one way". Maybe Barrier()? Does it make sense to fit the Shake functions into a hash.Hash? Will anyone use them with a fixed size output? https://codereview.appspot.com/130950043/diff/70001/sha3/keccak_sponge.go File sha3/keccak_sponge.go (right): https://codereview.appspot.com/130950043/diff/70001/sha3/keccak_sponge.go#new... sha3/keccak_sponge.go:5: // Package sha3 implements the SHA-3 fixed-output-length hash functions and The package comment would generally go in sha3.go or doc.go if it's large.
Sign in to reply to this message.
> Higher level comments: > Does it make sense to fit the Shake functions into a hash.Hash? Will anyone use > them with a fixed size output? Probably not. No idea; probably yes -- e.g., EC signatures on specific curves. > For example, although "absorb" and "squeeze" are standard terms for sponges, I > think they should be Write and Read in Go, with the usual signature. (I.e. Read > can return io.EOF if trying to read too much.) My inclination is, having reflected on your comments, now rather towards the minimal. It's probably premature to add a Sponge interface to a part of go.crypto/ that will, presumedly, be merged into go/ at some (nearish) point in the future. I've changed the interface type to "VariableHash", which exposes a minimal set of methods. > Do we really need to expose State, SecurityStrength, SpongeSize and Rate? Not needed; it wasn't clear to me if there were any consumers of this sort of information. Probably for VariableHash should expose something though? > MakeOneWay seems like a bad name because it's transient, no? I might want to > call it again on a sponge that is already "one way". Maybe Barrier()? That is a lovely analogy. But see supra; it is a fairly compelling feature of sponges -- but perhaps it's premature to include anything in the go.crypto interface. (Though it can be used for so many nice applications -- forgetting passwords, forward-secure logging -- perhaps it merits inclusion.) And a couple of random notes: - A lot of implementations of VOFs expose output via the Stream interface. This seems rather undesirable. - Are any other variable-output-length hash functions going to be used? (I.e., perhaps the interface should be specific to Shake for the moment.) It is presently defined so that it is compatible, I think, with any of the variable-output-length hash functions that have been proposed.
Sign in to reply to this message.
(p.s. please ping when there's a code update pending to review.) On the the problems that I have with reviewing this is that I don't have a firm grasp of what people are going to be doing with it. I know the Keccak folks have ideas about making ciphers and all sorts of things from it, but I'm not sure whether anyone else actually wants to do that. There is a common pattern where, at first, the interface to primitives ends up being too wide because people don't know what the uses will be and so try to support everything. Then we suffer from over general interfaces for a long time. I'm unclear in this case whether we risk repeating that mistake, or whether it's just a fundamental part of the world and that "over general" is one something that we can see with hindsight. Cheers AGL On Fri, Aug 22, 2014 at 2:36 PM, <coruus@gmail.com> wrote: >> Higher level comments: >> >> Does it make sense to fit the Shake functions into a hash.Hash? Will > > anyone use >> >> them with a fixed size output? > > > Probably not. No idea; probably yes -- e.g., EC signatures on specific > curves. > > >> For example, although "absorb" and "squeeze" are standard terms for > > sponges, I >> >> think they should be Write and Read in Go, with the usual signature. > > (I.e. Read >> >> can return io.EOF if trying to read too much.) > > > My inclination is, having reflected on your comments, now rather towards > the minimal. > > It's probably premature to add a Sponge interface to a part of > go.crypto/ that will, > presumedly, be merged into go/ at some (nearish) point in the future. > I've changed the > interface type to "VariableHash", which exposes a minimal set of > methods. > > >> Do we really need to expose State, SecurityStrength, SpongeSize and > > Rate? > > Not needed; it wasn't clear to me if there were any consumers of this > sort of information. > Probably for VariableHash should expose something though? > > >> MakeOneWay seems like a bad name because it's transient, no? I might > > want to >> >> call it again on a sponge that is already "one way". Maybe Barrier()? > > > That is a lovely analogy. But see supra; it is a fairly compelling > feature of sponges > -- but perhaps it's premature to include anything in the go.crypto > interface. (Though > it can be used for so many nice applications -- forgetting passwords, > forward-secure > logging -- perhaps it merits inclusion.) > > And a couple of random notes: > > - A lot of implementations of VOFs expose output via the Stream > interface. This > seems rather undesirable. > > - Are any other variable-output-length hash functions going to be used? > (I.e., > perhaps the interface should be specific to Shake for the moment.) It is > presently > defined so that it is compatible, I think, with any of the > variable-output-length > hash functions that have been proposed. > > https://codereview.appspot.com/130950043/
Sign in to reply to this message.
On 2014/08/27 18:50:18, agl2 wrote: > (p.s. please ping when there's a code update pending to review.) Sorry; still getting used to "hg codereview". Must not have "hg > On the the problems that I have with reviewing this is that I don't > have a firm grasp of what people are going to be doing with it. Same problem writing this. (I really appreciate your feedback on design; I would have preferred to submit a patch with a perfectly-ironed-out API, but that seemed infeasible without a second set of eyes.) The only things I know I'll use: d := sha3.NewShake256() d.Write(message) d.Write(nonce) s := make([]byte, 2*curveLen) d.Read(s) And: h := make([]bytes, 64) sha3.ShakeSum256(h, data) > There is a common pattern where, at first, the interface to primitives > ends up being too wide because people don't know what the uses will be > and so try to support everything. Then we suffer from over general > interfaces for a long time. I'm unclear in this case whether we risk > repeating that mistake, or whether it's just a fundamental part of the > world and that "over general" is one something that we can see with > hindsight. I suspect it's mainly the latter. No one can do anything with a primitive until there's an interface to it; and, once there's an interface, no one can do more than the interface permits. So, we tend to see interfaces as being too wide. Perhaps there's another issue lurking, which is genericity: OpenSSL takes this to an extreme with the EVP system, but even Go has "hash.Hash", "cipher.Stream", and "hmac". (E.g. (and I see no way around this), with SHA-3, we have the situation that the security proof for using it as a pre/postfix-MAC is "better"[*] than the security proof for HMAC-SHA-3. But hmac.New takes a hash.Hash, so some folks will wind up using that construction with sha3.New256.) [*] For other readers: HMAC's security proofs make an assumption about one of the input's lengths which is never(?) enforced by implementations. There's no such restriction for SHA-3.
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, agl@google.com, agl@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
On 2014/08/28 18:56:46, coruus wrote: > Hello mailto:golang-codereviews@googlegroups.com, mailto:agl@google.com, mailto:agl@golang.org (cc: > mailto:golang-codereviews@googlegroups.com), > > Please take another look. A quick note: Some dogfooding at https://github.com/coruus/go-sha3 There's a shake256sum command, and a quick implementation of a generic-composition-based authenticated encryption scheme. Something that I've noticed: I consistently get Shake.Read and Shake.Write confused.
Sign in to reply to this message.
https://codereview.appspot.com/130950043/diff/250001/sha3/doc.go File sha3/doc.go (right): https://codereview.appspot.com/130950043/diff/250001/sha3/doc.go#newcode15 sha3/doc.go:15: // If you aren't sure what function you need, use SHAKE256 with at least I think this documentation is fine, but you should expect it to be presented in a variable width font so the ASCII tables and art need to be explained in words, if at all, I'm afraid. https://codereview.appspot.com/130950043/diff/250001/sha3/doc.go#newcode96 sha3/doc.go:96: // remove blank line. https://codereview.appspot.com/130950043/diff/250001/sha3/keccak/keccakf.go File sha3/keccak/keccakf.go (right): https://codereview.appspot.com/130950043/diff/250001/sha3/keccak/keccakf.go#n... sha3/keccak/keccakf.go:5: // Package keccak implements the KeccakF-1600 permutation used by SHA-3. delete this line. https://codereview.appspot.com/130950043/diff/250001/sha3/keccak/keccakf.go#n... sha3/keccak/keccakf.go:8: // delete this line. https://codereview.appspot.com/130950043/diff/250001/sha3/keccak/keccakf.go#n... sha3/keccak/keccakf.go:9: // This is implemented in a separate file to allow for replacement by using a separate file is fine, but I don't think this should be a separate directory (and thus package). https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go File sha3/sha3.go (right): https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode16 sha3/sha3.go:16: MaxKeccakHashRate = 176 No need for "Keccak" in the names of things in a sha3 package. Maybe just MaxRate? https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode19 sha3/sha3.go:19: // SpongeDirection indicates the direction bytes are flowing through the sponge. lower case s now. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode30 sha3/sha3.go:30: bufferLen = 176 // the length of the input buffer; determines maximum rate is this just MaxHashRate? https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode87 sha3/sha3.go:87: // xorBytesInto xors a buffer into the state, byte-swapping to comment says xorBytesInfo but function name is xorBytesIn. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode92 sha3/sha3.go:92: func (d *state) xorBytesIn(buf []byte) int { Is the return value of this function used anywhere? https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode109 sha3/sha3.go:109: // copyBytesOut copies ulint64s to a byte buffer. is the return value of this function used anywhere? https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode146 sha3/sha3.go:146: switch d.state { this is just an if. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode148 sha3/sha3.go:148: // QQQQ: Should this be an error? (This is how CSF and the Keccak I think this should be an error. Perhaps in the future we can add a function to enable MAC-and-continue, but let's skip it for now. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode165 sha3/sha3.go:165: // (willWrite == d.rate) <==> (d.position == 0) ==> (d.inputBufer[:] == 0) I think you need to drop "==>" and everything after. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode166 sha3/sha3.go:166: d.xorBytesIn(p[written : written+willWrite]) it's more Go-like to do p = p[willWrite:] rather than keeping track of written. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode173 sha3/sha3.go:173: // (0 < d.rate == d.position) // 0 < d.rate <==> d.position != 0 perhaps. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode192 sha3/sha3.go:192: switch d.state { this is just an if. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode199 sha3/sha3.go:199: offset := 0 ditto about reslicing out rather than tracking offset. Also, length can be similarly removed. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode206 sha3/sha3.go:206: copy(out[offset:offset+willSqueeze], p.s. you don't need to break lines like this. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode232 sha3/sha3.go:232: // NewKeccakHash creates a new Keccak-based ShakeHash with do we even want to export this function? Are there real uses for this level of control? (Functions can always be added later.) https://codereview.appspot.com/130950043/diff/250001/sha3/sha3f202.go File sha3/sha3f202.go (right): https://codereview.appspot.com/130950043/diff/250001/sha3/sha3f202.go#newcode21 sha3/sha3f202.go:21: // New224 creates a new SHA3-224 hash period at the end of the line, also for the other New* functions.
Sign in to reply to this message.
https://codereview.appspot.com/130950043/diff/250001/sha3/doc.go File sha3/doc.go (right): https://codereview.appspot.com/130950043/diff/250001/sha3/doc.go#newcode15 sha3/doc.go:15: // If you aren't sure what function you need, use SHAKE256 with at least On 2014/09/02 23:40:27, agl1 wrote: > I think this documentation is fine, but you should expect it to be presented in > a variable width font so the ASCII tables and art need to be explained in words, > if at all, I'm afraid. It's possible to put them in a pre by indenting; but this seems an unusual use of that format option. So: deleted. https://codereview.appspot.com/130950043/diff/250001/sha3/doc.go#newcode96 sha3/doc.go:96: // On 2014/09/02 23:40:27, agl1 wrote: > remove blank line. Done. https://codereview.appspot.com/130950043/diff/250001/sha3/keccak/keccakf.go File sha3/keccak/keccakf.go (right): https://codereview.appspot.com/130950043/diff/250001/sha3/keccak/keccakf.go#n... sha3/keccak/keccakf.go:5: // Package keccak implements the KeccakF-1600 permutation used by SHA-3. On 2014/09/02 23:40:27, agl1 wrote: > delete this line. Done. https://codereview.appspot.com/130950043/diff/250001/sha3/keccak/keccakf.go#n... sha3/keccak/keccakf.go:8: // On 2014/09/02 23:40:27, agl1 wrote: > delete this line. Acknowledged. https://codereview.appspot.com/130950043/diff/250001/sha3/keccak/keccakf.go#n... sha3/keccak/keccakf.go:9: // This is implemented in a separate file to allow for replacement by On 2014/09/02 23:40:27, agl1 wrote: > using a separate file is fine, but I don't think this should be a separate > directory (and thus package). Acknowledged. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go File sha3/sha3.go (right): https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode16 sha3/sha3.go:16: MaxKeccakHashRate = 176 On 2014/09/02 23:40:27, agl1 wrote: > No need for "Keccak" in the names of things in a sha3 package. > > Maybe just MaxRate? No need for it, really (having deleted NewShakeHash; thus deleted. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode19 sha3/sha3.go:19: // SpongeDirection indicates the direction bytes are flowing through the sponge. On 2014/09/02 23:40:28, agl1 wrote: > lower case s now. Done. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode30 sha3/sha3.go:30: bufferLen = 176 // the length of the input buffer; determines maximum rate On 2014/09/02 23:40:28, agl1 wrote: > is this just MaxHashRate? Acknowledged. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode87 sha3/sha3.go:87: // xorBytesInto xors a buffer into the state, byte-swapping to On 2014/09/02 23:40:28, agl1 wrote: > comment says xorBytesInfo but function name is xorBytesIn. Done. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode92 sha3/sha3.go:92: func (d *state) xorBytesIn(buf []byte) int { On 2014/09/02 23:40:28, agl1 wrote: > Is the return value of this function used anywhere? Was once; not anymore; removed. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode109 sha3/sha3.go:109: // copyBytesOut copies ulint64s to a byte buffer. On 2014/09/02 23:40:28, agl1 wrote: > is the return value of this function used anywhere? Done. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode146 sha3/sha3.go:146: switch d.state { On 2014/09/02 23:40:28, agl1 wrote: > this is just an if. Done. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode148 sha3/sha3.go:148: // QQQQ: Should this be an error? (This is how CSF and the Keccak On 2014/09/02 23:40:27, agl1 wrote: > I think this should be an error. Perhaps in the future we can add a function to > enable MAC-and-continue, but let's skip it for now. My inclination previously as well. But: Because hash.Sum clones the hash's state before reading output, this means that h := sha3.New512() h.Write(fixedPrefix) h1 := h.Sum(nil) h.Write(message) h2 := h.Sum(nil) will succeed, but h := NewShake128() h.Write(fixedPrefix) h1 := make([[]byte, hlen) h.Read(h1) h.Write(message) h2 := make([]byte, hlen) h.Read(h2) will result in h2 always being the same, no matter what message is. This might be undesirable/ disastrous. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode165 sha3/sha3.go:165: // (willWrite == d.rate) <==> (d.position == 0) ==> (d.inputBufer[:] == 0) On 2014/09/02 23:40:28, agl1 wrote: > I think you need to drop "==>" and everything after. Done. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode166 sha3/sha3.go:166: d.xorBytesIn(p[written : written+willWrite]) On 2014/09/02 23:40:27, agl1 wrote: > it's more Go-like to do p = p[willWrite:] rather than keeping track of written. Done. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode173 sha3/sha3.go:173: // (0 < d.rate == d.position) On 2014/09/02 23:40:28, agl1 wrote: > // 0 < d.rate <==> d.position != 0 > perhaps. (0 < d.rate) iff (d.position != 0) -> !(0 < d.rate) iff !(d.position != 0) -> !(0 < d.rate) iff (d.position == 0) -> (0 >= d.rate) iff (d.position == 0) -> (d.rate <= 0) iff (d.position == 0) --> or perhaps not. :) deleted. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode192 sha3/sha3.go:192: switch d.state { On 2014/09/02 23:40:28, agl1 wrote: > this is just an if. Done. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode199 sha3/sha3.go:199: offset := 0 On 2014/09/02 23:40:28, agl1 wrote: > ditto about reslicing out rather than tracking offset. Also, length can be > similarly removed. Done. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode206 sha3/sha3.go:206: copy(out[offset:offset+willSqueeze], On 2014/09/02 23:40:28, agl1 wrote: > p.s. you don't need to break lines like this. Acknowledged. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode232 sha3/sha3.go:232: // NewKeccakHash creates a new Keccak-based ShakeHash with On 2014/09/02 23:40:28, agl1 wrote: > do we even want to export this function? Are there real uses for this level of > control? (Functions can always be added later.) Deleted. https://codereview.appspot.com/130950043/diff/250001/sha3/sha3f202.go File sha3/sha3f202.go (right): https://codereview.appspot.com/130950043/diff/250001/sha3/sha3f202.go#newcode21 sha3/sha3f202.go:21: // New224 creates a new SHA3-224 hash On 2014/09/02 23:40:28, agl1 wrote: > period at the end of the line, also for the other New* functions. Done.
Sign in to reply to this message.
I made some more changes prior to landing but discovered that I didn't understand the padding. https://codereview.appspot.com/130950043/diff/270001/sha3/sha3.go File sha3/sha3.go (right): https://codereview.appspot.com/130950043/diff/270001/sha3/sha3.go#newcode135 sha3/sha3.go:135: d.in[d.rate-1] ^= 0x80 I'm lost here. We know that d.i < d.rate here, so isn't ^= dsbyte just setting it to dsbyte? (Because d.in[d.i] is known to be zero.) From http://csrc.nist.gov/publications/drafts/fips-202/fips_202_draft.pdf section 5.1, looking at the padding rule I don't see why L135 is correct, although it does pass the test vectors. If d.i == d.rate - 1, doesn't this collide with dsbyte? Also, given the pad10*1 rule, is the 0x80 the last '1'? (I.e. are the bits counted from LSB to MSB?) If so, where's the initial '1'? I also don't see how the "|| 01" in the spec maps to a dsbyte of 0x06 for the SHA3 hashes.
Sign in to reply to this message.
On 2014/09/03 02:50:09, agl1 wrote: > I made some more changes prior to landing but discovered that I didn't > understand the padding. Which is understandable; when I was trying to implement a C version from the spec, this confounded me for a bit. I found keeping this in mind helpful: 10*1 padding requires *at most* three bits (<1 byte) of overhead. > https://codereview.appspot.com/130950043/diff/270001/sha3/sha3.go > File sha3/sha3.go (right): > > https://codereview.appspot.com/130950043/diff/270001/sha3/sha3.go#newcode135 > sha3/sha3.go:135: d.in[d.rate-1] ^= 0x80 > We know that d.i < d.rate here, so isn't ^= dsbyte just setting it to dsbyte? > (Because d.in[d.i] is known to be zero.) No, not necessarily. It isn't zero, because d.rate is 1 greater than the last "used" location in the "rate" bytes. (I.e., an index of rate-1 is the last byte in the rate.) It may have already been set by d.in[d.i] ^= dsbyte; that line could safely be replaced by d.in[d.i] = dsbyte. > From http://csrc.nist.gov/publications/drafts/fips-202/fips_202_draft.pdf > section 5.1, looking at the padding rule I don't see why L135 is correct, > although it does pass the test vectors. > > If d.i == d.rate - 1, doesn't this collide with dsbyte? Indeed; but it doesn't collide. It overlaps. (At least one of the two must be an XOR for this to be correct.) > I also don't see how the "|| 01" in the spec maps to a dsbyte of 0x06 for the > SHA3 hashes. It's extremely confusing because of the bit-ordering conventions used. So, in ordinary binary notation, 0x06 == 0b00000110. However: use a little-endian bit-ordering convention (like, e.g., Intel's ISA docs) and then: 2^01234567 0x06 = 01100000 and 2^01234567 0x80 = 00000001 which turns out to be the 10*1 padding rule with 01 appended to the message. (See Table 4 of draft FIPS-202 for the particular bit-ordering convention used.) See, e.g., https://github.com/gvanas/KeccakCodePackage/blob/master/Tests/genKAT.c for the 0x06 if this argument doesn't convince you.
Sign in to reply to this message.
In fact, I just remembered where I had a C function to print a little-endian
bit-ordered state in binary:
static inline void fprintbits_le(FILE* const restrict file,
const uint8_t* const restrict in,
const size_t inlen) {
for (size_t i = 0; i < inlen; i++) {
ifnotmod(i, 4, printf("\n"));
for (int mask = 1; mask < 256; mask <<= 1) {
fprintf(file, "%x", (in[i] & mask) && 1);
}
printf(" ");
}
}
(From
https://github.com/coruus/keccakc/blob/refactor-pad/keccak/utils/print-impl.h )
Sign in to reply to this message.
https://codereview.appspot.com/130950043/diff/270001/sha3/shake.go File sha3/shake.go (right): https://codereview.appspot.com/130950043/diff/270001/sha3/shake.go#newcode19 sha3/shake.go:19: // error is input is written to it after output has been read from *if
Sign in to reply to this message.
https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go File sha3/sha3.go (right): https://codereview.appspot.com/130950043/diff/250001/sha3/sha3.go#newcode148 sha3/sha3.go:148: // QQQQ: Should this be an error? (This is how CSF and the Keccak On 2014/09/03 01:17:27, coruus wrote: > will succeed, but > > h := NewShake128() > h.Write(fixedPrefix) > h1 := make([[]byte, hlen) > h.Read(h1) > h.Write(message) > h2 := make([]byte, hlen) > h.Read(h2) > > will result in h2 always being the same, no matter what message is. This might > be undesirable/ > disastrous. Agreed. I've made this a panic.
Sign in to reply to this message.
Thanks for the explanation, that makes sense once I realise that the "| 01" notation from the spec is appending two bits, not a byte with value 0x01. Unfortunately, codereview doesn't let me upload suggested changes so we'll have to communication via the repository. I'm going to submit by changed version of the code, please see whether I've added any bugs. The changes are mostly merging the in and out buffers.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=2ce75e338fde&repo=crypto *** go.crypto/sha3: update to sync with draft FIPS-202 1. API: This exposes a minimal API: the SHA-3 functions implement hash.Hash. The SHAKE functions implement a new "ShakeHash" interface that implements io.Reader, io.Writer, and Reset(). (The previous Barrier() function has been removed.) (Alternative proposal: Don't implement io.Reader, but instead provide a "Digest(d []byte) error" function that performs a hash.Hash style copy. Somewhat more minimal, but very easy to use incorrectly.) 2. Tests Added the complete set of ShortMsgKATs from https://github.com/gvanas/KeccakCodePackage 3. Correctness In sync with draft FIPS-202. 4. Documentation A summary of the security properties of the SHA-3 and SHAKE functions is provided in doc.go; some concrete recommendations as well. Fixes 8563. R=golang-codereviews, agl CC=golang-codereviews https://codereview.appspot.com/130950043 Committer: Adam Langley <agl@golang.org>
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
On 2014/09/09 21:36:06, coruus wrote: > LGTM Just a quick note: The committed version has about a 5% worst-case performance regression on the set of benchmarks in sha3_test.go. (I'll open a new issue with some optimizations when I have time.) The included benchmarks aren't particularly good; in particular, they don't test either repeated updates, or the ShakeHash-specific interface. Using the SHAKE instances is much faster, because it avoids the allocation/state-copy mandated by the Sum interface. (I'm writing this because it increases the probability that I'll actually follow-up on this quickly.)
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
