Skip to content

Conversation

@buggywhip
Copy link
Contributor

This PR addresses several issues found while reviewing the stream ciphers. The current focus is on Salsa20/XSalsa20 and Sosemanuk, but the intent is, if in agreement, to extend the following to all stream ciphers. ...and perhaps block ciphers and hash functions.

1- flow control between the cipher's functions: enforce the calling of setiv() before calling crypt(), enforce the calling of setup() before calling setiv(), etc. The proper sequences vary for Salsa20, XSalsa20, and RC4 for example.
state diagram - chacha salsa sosemanuk rabbit
state diagram - xsalsa

2- "one call" helper functions to support the [frequent] needs to crypt small, in-memory data with a single call. See Salsa20 and Sosemanuk in doc/crypt.tex.

3- an alternate and preferred way to get a cipher's state size. See bottom of Dynamic Language section in doc/crypt.tex.

4- miscellaneous small "corrections"

Checklist

  • [ x ] documentation is added or updated
  • tests will be added or updated

…controls which functions may be a called and in which order.  Unless there is good reason to not, this pattern will be extended to each of the stream ciphers.  See uploaded state diagrams.
…controls which functions may be a called and in which order. See also uploaded state diagrams.
will implement this pattern for the rest of the stream ciphers.
will implement for the remaining stream ciphers.
@buggywhip buggywhip requested review from karel-m and sjaeckel June 25, 2018 02:58
@karel-m
Copy link
Member

karel-m commented Jun 25, 2018

Larry, there are IMO too much things combined in one PR.

1/ Things related to st->status

  • I see the point, looks more or less reasonable

2/ The <something>_onecall stuff

  • here it needs some discussion
  • not that I am against but I am not sure whether there is a demand for this (I do not have a use case but others may have)
  • naming convention - we already use *_memory style for encauth functions of similar kind
  • these functions have to be in a separate .c files otherwise you get a bunch of useless bloat linked to your executable when using static library
  • if we want to support this we should provide it for all stream ciphers

3/ Ad <something>_state_size

  • thing like this IMO does not quite fit into libtomcrypt API
  • having a special functions like salsa20_state_size, xsalsa20_state_size, sosemanuk_state_size for 3 "somehow chosen" structs simply does not look right to me
  • do not we already have crypt_get_size for this?

4/ Renaming sosemanuk_state *ss > sosemanuk_state *st

And my comments to the code ... well, later :)

includes a fix to chacha20poly1305_setiv.c to preserve flow control status.

and a fix to wipe key if an error midstream of the one call.
the plan is to extend the helper function concept to block ciphers and hash functions
@buggywhip
Copy link
Contributor Author

buggywhip commented Jun 27, 2018 via email

@buggywhip
Copy link
Contributor Author

buggywhip commented Jun 27, 2018 via email

@buggywhip
Copy link
Contributor Author

buggywhip commented Jun 27, 2018 via email

@sjaeckel
Copy link
Member

1/ Things related to st->status

LGTM

2/ The _onecall stuff
...
I wasn't aware. I've adopted the *_memory() convention.

Perfect! Please create them in the appropriate folders as single files and not in a throw-in helper c-file

3/ Ad _state_size

please don't do this!

... also I don't see the disadvantage of using your _get_sizes() ... it's 6 lines of boilerplate in your python code vs. 3 lines of code in C + 1 new API function for each size you want to export like that ...

Now please split this PR up into 3 separate ones with the topics as proposed by Karel!

I'd do them in the order 4/ - 2/ - 1/ which is IMO the order of discussion-worthyness and therefore merged the fastest so we can go on.

@buggywhip buggywhip closed this Jul 6, 2018
@buggywhip
Copy link
Contributor Author

#418 would be better if closed, subdivided into 4 smaller, better defined subtopics, and each resubmitted

@buggywhip buggywhip reopened this Jul 6, 2018
@buggywhip
Copy link
Contributor Author

Oops

@buggywhip buggywhip closed this Jul 6, 2018
@sjaeckel
Copy link
Member

sjaeckel commented Jul 6, 2018

#418 would be better if closed, subdivided into 4 smaller, better defined subtopics, and each resubmitted

fine by me, please don't use weird characters in the branch names :D

@buggywhip buggywhip deleted the enforce-call-_setiv()-policy branch July 10, 2018 05:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants