Add XSalsa20 #387

buggywhip · 2018-05-03T23:40:50Z

Add XSalsa20 to the suite of stream ciphers.

Checklist

documentation is added or updated
tests are added or updated

karel-m · 2018-05-04T08:00:08Z

Hi Larry,

some cosmetics - this:

/* ref:         HEAD -> develop */
/* git commit:  af67321bf3cde1a470c679e459ebb8189e38c9bd */
/* commit time: 2018-04-13 09:42:47 +0200 *

should be replaced with:

/* ref:         $Format:%D$ */
/* git commit:  $Format:%H$ */
/* commit time: $Format:%ai$ */

The xsalsa20_ivctr64 looks to me like a candidate for misuse (with potential security consequences). I'd leave it out till somebody comes with a reasonable use case.

I am not sure whether to introduce xsalsa20_state; I think using salsa20_state with xsalsa20_* functions is ok.

The last doubt I have (for now) is related to unsigned char *subkey - is it necessary?

buggywhip · 2018-05-04T09:07:57Z

should be replaced with: /* ref: $Format:%D$ */ /* git commit: $Format:%H$ */ /* commit time: $Format:%ai$ */

will do

The xsalsa20_ivctr64 looks to me like a candidate for misuse (with potential security consequences). I'd leave it out till somebody comes with a reasonable use case.

I too struggled with the security implications. It was that or try to keep parallel APIs. I won't feel bad if we yank it.

I am not sure whether to introduce xsalsa20_state; I think using salsa20_state with xsalsa20_*functions is ok.

And you can because xsalsa20_state is defined to salsa20_state in the same way we map xsalsa20_crypt(), _keystream(), and _done() to their salsa20 counterparts. I suggest, however, keeping xsalsa20_state to reduce confusion.

The last doubt I have (for now) is related to unsigned char *subkey - is it necessary?

Not really so long as the user doesn't want a hybrid xsalsa/salsa where the subkey is used more like an ephemeral key. sha256(key+nonce) can accomplish that but the doubleround function is lightweight and more attractive for smaller devices. I can argue this both ways; it comes down to what is our objective? I will wait a few days. Others might want to chime in. I have one additional concern. The "#define xsalsa20_state salsa20_state" appears to kill getting xsalsa20_state_size fm misc/crypt/crypt_sizes.c. I still want to play with it some more but python users might just have to remember to use salsa20_state. ...but all the xsalsa mappings work just fine elsewhere. Confusing?

karel-m · 2018-05-04T11:49:56Z

Ad crypt_sizes.c maybe

#ifdef LTC_SALSA20
    _SZ_STRINGIFY_T(salsa20_state),
#endif
#ifdef LTC_XSALSA20
    _SZ_STRINGIFY_T(xsalsa20_state),
#endif

sjaeckel

Looks good 👍

sjaeckel · 2018-05-05T15:11:37Z

src/stream/xsalsa20/xsalsa20_setup.c

+
+/* ref:         $Format:%D$ */
+/* git commit:  $Format:%H$ */
+/* commit time: $Format:%ai$ */


can you please add a newline on all newly added files

sjaeckel · 2018-05-05T15:23:32Z

src/headers/tomcrypt_cipher.h

+int xsalsa20_test(void);
+
+#define xsalsa20_state     salsa20_state
+#define xsalsa20_ivctr64   salsa20_ivctr64


Do we really have to provide this?
IIUC you could use it, but it's dangerous and therefor: will it even be used?

sjaeckel · 2018-05-05T15:24:12Z

src/headers/tomcrypt_cipher.h

+
+int xsalsa20_setup(salsa20_state *st, const unsigned char *key,   unsigned long keylen,
+                                      const unsigned char *nonce, unsigned long noncelen,
+                                      int rounds, unsigned char *subkey);


shouldn't there be a unsigned long *subkeylen? :)

sjaeckel · 2018-05-05T15:26:34Z

src/stream/xsalsa20/xsalsa20_setup.c

+
+   /* copy out subkey if not NULL */
+   if (subkey != NULL) XMEMCPY(subkey, secondkey, sizeof(secondkey));
+   return CRYPT_OK;


this is missing a

#ifdef LTC_CLEAN_STACK zeromem(secondkey, ... zeromem(x, ... ...

implemented

sjaeckel · 2018-05-05T15:30:28Z

src/stream/xsalsa20/xsalsa20_setup.c

+   st->ivlen = 8;           /* set switch to say nonce/IV has been loaded */
+
+   /* copy out subkey if not NULL */
+   if (subkey != NULL) XMEMCPY(subkey, secondkey, sizeof(secondkey));


Instead of copying why not work with the provided buffer if given?

unsigned char secondkey_buf[32], *secondkey = secondkey_buf; ... if (subkey != NULL) secondkey = subkey; ... #ifdef LTC_CLEAN_STACK zeromem(secondkey_buf, ...

implemented

sjaeckel · 2018-05-05T15:33:34Z

src/stream/xsalsa20/xsalsa20_test.c

+     */
+    {
+        unsigned char key[]   = {0x1b,0x27,0x55,0x64,0x73,0xe9,0x85,0xd4,0x62,0xcd,0x51,0x19,0x7a,0x9a,0x46,0xc7,0x60,0x09,0x54,0x9e,0xac,0x64,0x74,0xf2,0x06,0xc4,0xee,0x08,0x44,0xf6,0x83,0x89};
+        unsigned char nonce[] = {0x69,0x69,0x6e,0xe9,0x55,0xb6,0x2b,0x73,0xcd,0x62,0xbd,0xa8,0x75,0xfc,0x73,0xd6,0x82,0x19,0xe0,0x03,0x6b,0x7a,0x0b,0x37};


sjaeckel · 2018-05-05T15:34:02Z

src/stream/xsalsa20/xsalsa20_test.c

+      ref: stream3.c/out in nacl-20110221/tests
+   */
+   {
+       unsigned char key[]      = {0x1b,0x27,0x55,0x64,0x73,0xe9,0x85,0xd4,0x62,0xcd,0x51,0x19,0x7a,0x9a,0x46,0xc7,0x60,0x09,0x54,0x9e,0xac,0x64,0x74,0xf2,0x06,0xc4,0xee,0x08,0x44,0xf6,0x83,0x89};


sjaeckel · 2018-05-05T15:34:11Z

src/stream/xsalsa20/xsalsa20_test.c

+   */
+   {
+       unsigned char key[]      = {0x1b,0x27,0x55,0x64,0x73,0xe9,0x85,0xd4,0x62,0xcd,0x51,0x19,0x7a,0x9a,0x46,0xc7,0x60,0x09,0x54,0x9e,0xac,0x64,0x74,0xf2,0x06,0xc4,0xee,0x08,0x44,0xf6,0x83,0x89};
+       unsigned char nonce[]    = {0x69,0x69,0x6e,0xe9,0x55,0xb6,0x2b,0x73,0xcd,0x62,0xbd,0xa8,0x75,0xfc,0x73,0xd6,0x82,0x19,0xe0,0x03,0x6b,0x7a,0x0b,0x37};


sjaeckel · 2018-05-05T15:34:20Z

src/stream/xsalsa20/xsalsa20_test.c

+       unsigned char key[]      = {0x1b,0x27,0x55,0x64,0x73,0xe9,0x85,0xd4,0x62,0xcd,0x51,0x19,0x7a,0x9a,0x46,0xc7,0x60,0x09,0x54,0x9e,0xac,0x64,0x74,0xf2,0x06,0xc4,0xee,0x08,0x44,0xf6,0x83,0x89};
+       unsigned char nonce[]    = {0x69,0x69,0x6e,0xe9,0x55,0xb6,0x2b,0x73,0xcd,0x62,0xbd,0xa8,0x75,0xfc,0x73,0xd6,0x82,0x19,0xe0,0x03,0x6b,0x7a,0x0b,0x37};
+       unsigned char subkey[32] = {0};
+       unsigned char expect[]   = {0xdc,0x90,0x8d,0xda,0x0b,0x93,0x44,0xa9,0x53,0x62,0x9b,0x73,0x38,0x20,0x77,0x88,0x80,0xf3,0xce,0xb4,0x21,0xbb,0x61,0xb9,0x1c,0xbd,0x4c,0x3e,0x66,0x25,0x6c,0xe4};


sjaeckel · 2018-05-05T15:34:37Z

src/stream/xsalsa20/xsalsa20_test.c

+      verify correct generation of a keystream
+   */
+   {
+       unsigned char key[]        = {0x1b,0x27,0x55,0x64,0x73,0xe9,0x85,0xd4,0x62,0xcd,0x51,0x19,0x7a,0x9a,0x46,0xc7,0x60,0x09,0x54,0x9e,0xac,0x64,0x74,0xf2,0x06,0xc4,0xee,0x08,0x44,0xf6,0x83,0x89};


const ... and all the others :D

karel-m · 2018-05-06T15:06:53Z

src/headers/tomcrypt_cipher.h


+#ifdef LTC_XSALSA20
+
+int xsalsa20_setup(salsa20_state *st, const unsigned char *key,   unsigned long keylen,


shouldn't we use xsalsa20_state here?

see my comment below

karel-m · 2018-05-06T15:07:53Z

src/stream/salsa20/salsa20_ivctr64.c

 #include "tomcrypt.h"

-#ifdef LTC_SALSA20
+#if defined(LTC_SALSA20) || defined(LTC_XSALSA20)


this can be IMO reverted to #ifdef LTC_SALSA20

I'd also prefer to add something like the following in the "cleanup" section of tomcrypt_custom.h where all the other similar cases are handled

#if defined(LTC_XSALSA20) && !defined(LTC_SALSA20) #error LTC_XSALSA20 requires LTC_SALSA20 #endif

then all the changes to src/stream/salsa20/salsa20_*.c could be reverted

karel-m · 2018-05-06T15:09:04Z

src/stream/xsalsa20/xsalsa20_setup.c

+   @param subkeylen [out] number of bytes copied, NULL if not want a copy
+   @return CRYPT_OK if successful
+*/
+int xsalsa20_setup(salsa20_state *st, const unsigned char *key, unsigned long keylen,


salsa20_state > xsalsa20_state

not sure though if we should remove the xsalsa20_state define and go with only salsa20_state* arguments... it'd be cleaner I guess...

karel-m · 2018-05-06T15:13:20Z

I still do not see a use case for subkey. How can one reuse the value?

I am for: xsalsa20_setup(&st, key, key_len, nonce, nonce_len, rounds)

If there is a demand for subkey we can introduce xsalsa20_setup_ex(&st, key, key_len, nonce, nonce_len, rounds, &subkey, &subkeylen)

sjaeckel · 2018-05-06T19:44:41Z

src/stream/xsalsa20/xsalsa20_setup.c

+   LTC_ARGCHK(keylen    == 32);
+   LTC_ARGCHK(nonce     != NULL);
+   LTC_ARGCHK(noncelen  == 24);
+   LTC_ARGCHK((subkey != NULL && subkeylen != NULL)  ||


LTC_ARGCHK(((subkey == NULL) && (subkeylen == NULL)) || ((subkey != NULL) && (subkeylen != NULL) && (*subkeylen >= 32)));

sjaeckel · 2018-05-06T19:46:29Z

src/stream/xsalsa20/xsalsa20_setup.c

+   @param subkeylen [out] number of bytes copied, NULL if not want a copy
+   @return CRYPT_OK if successful
+*/
+int xsalsa20_setup(salsa20_state *st, const unsigned char *key, unsigned long keylen,


not sure though if we should remove the xsalsa20_state define and go with only salsa20_state* arguments... it'd be cleaner I guess...

sjaeckel · 2018-05-06T19:47:24Z

src/headers/tomcrypt_cipher.h


+#ifdef LTC_XSALSA20
+
+int xsalsa20_setup(salsa20_state *st, const unsigned char *key,   unsigned long keylen,


see my comment below

sjaeckel · 2018-05-06T19:47:48Z

src/stream/salsa20/salsa20_ivctr64.c

 #include "tomcrypt.h"

-#ifdef LTC_SALSA20
+#if defined(LTC_SALSA20) || defined(LTC_XSALSA20)


sjaeckel · 2018-05-06T19:56:39Z

I still do not see a use case for subkey. How can one reuse the value?

very good point!

I am for: xsalsa20_setup(&st, key, key_len, nonce, nonce_len, rounds)

👍

buggywhip · 2018-05-06T22:31:52Z

On 6May, 2018, at 12:56 PM, Steffen Jaeckel ***@***.***> wrote: I still do not see a use case for subkey. How can one reuse the value? very good point!

I can see using the "hashed" subkey in various situations, none of which couldn't be satisfied with another hash function. The only redeeming value might be the doubleround's space and time. I doubt we would ever see a request for two reasons: XSalsa's low adoption and library "shoppers" will simply move on. (My usage was predominately prototyping; I'd carve out the pieces I needed and augment with custom code.) An alternative would be to implement Karel's xsalsa20_setup_ex(&st, key, key_len, nonce, nonce_len, rounds, &subkey, &subkeylen) now. ...but as a "shopper" I'd be much more interested in XChaCha20 with all the bells and whistles.

I am for: xsalsa20_setup(&st, key, key_len, nonce, nonce_len, rounds) 👍

All that said... Clearly this will be the most frequent use case and is *much* cleaner. Done. (offline I'll keep the existing code should we change our mind)

sjaeckel

Should be ready after my last comments were worked in! 👍

sjaeckel · 2018-05-20T11:28:36Z

doc/crypt.tex

+
+xsalsa20_setup() is the only call needed to completely initialize the state.
+The key size must be 32 bytes (256 bits) and nonce size must be 24 bytes (192
+bits).  Rounds must be an even number and if set to 0 it will be changed to 20.


and if set to 0 it will be changed to 20.

and if set to 0 the default number of rounds, 20, will be used.

sjaeckel · 2018-05-20T11:36:00Z

src/stream/salsa20/salsa20_ivctr64.c

 #include "tomcrypt.h"

-#ifdef LTC_SALSA20
+#if defined(LTC_SALSA20) || defined(LTC_XSALSA20)


I'd also prefer to add something like the following in the "cleanup" section of tomcrypt_custom.h where all the other similar cases are handled

#if defined(LTC_XSALSA20) && !defined(LTC_SALSA20) #error LTC_XSALSA20 requires LTC_SALSA20 #endif

then all the changes to src/stream/salsa20/salsa20_*.c could be reverted

karel-m · 2018-05-23T08:41:57Z

src/stream/salsa20/salsa20_keystream.c

 int salsa20_keystream(salsa20_state *st, unsigned char *out, unsigned long outlen)
 {
   if (outlen == 0) return CRYPT_OK; /* nothing to do */
+   LTC_ARGCHK(st->ivlen == 8 || st->ivlen == 24);


there is IMO no need for this check here

karel-m · 2018-05-28T08:05:45Z

Providing the check with #error LTC_XSALSA20 requires LTC_SALSA20 in tomcrypt_custom.h I think we do not need any of these:

-#ifdef LTC_SALSA20
+#if defined(LTC_SALSA20) || defined(LTC_XSALSA20)

karel-m · 2018-05-28T08:06:58Z

src/stream/salsa20/salsa20_ivctr64.c

   LTC_ARGCHK(iv != NULL);
   /* Salsa20: 64-bit IV (nonce) + 64-bit counter */
-   LTC_ARGCHK(ivlen == 8);
+   LTC_ARGCHK(ivlen != 24);


Are you sure here? What's the idea behind this change?

I want to prevent invoking salsa20_ivctr64() after calling xsalsa20_setup(). ...unnecessary.

karel-m · 2018-05-28T21:14:15Z

src/headers/tomcrypt_cipher.h

 #endif /* LTC_CHACHA */

-#ifdef LTC_SALSA20
+#if defined(LTC_SALSA20) || defined(LTC_XSALSA20)


This can IMO stay as it used to be: #ifdef LTC_SALSA20

karel-m · 2018-05-28T21:14:27Z

src/stream/salsa20/salsa20_crypt.c

 #include "tomcrypt.h"

-#ifdef LTC_SALSA20
+#if defined(LTC_SALSA20) || defined(LTC_XSALSA20)


This can IMO stay as it used to be: #ifdef LTC_SALSA20

karel-m · 2018-05-28T21:14:41Z

src/stream/salsa20/salsa20_done.c

 #include "tomcrypt.h"

-#ifdef LTC_SALSA20
+#if defined(LTC_SALSA20) || defined(LTC_XSALSA20)


This can IMO stay as it used to be: #ifdef LTC_SALSA20

karel-m · 2018-05-28T21:14:51Z

src/stream/salsa20/salsa20_keystream.c

 #include "tomcrypt.h"

-#ifdef LTC_SALSA20
+#if defined(LTC_SALSA20) || defined(LTC_XSALSA20)


This can IMO stay as it used to be: #ifdef LTC_SALSA20

karel-m · 2018-05-29T05:44:12Z

Larry, there is one more #if defined(LTC_SALSA20) || defined(LTC_XSALSA20) in salsa20_ivctr64.c that can be IMO reverted.

karel-m

Looks good to me. Just please squash it into 2 commits: 1/ update makefiles + 2/ xsalsa20 (and of course rebase on current develop)

buggywhip · 2018-05-29T06:54:41Z

On 28May, 2018, at 11:49 PM, karel-m ***@***.***> wrote: @karel-m approved this pull request. Looks good to me. Just please squash it into 2 commits: 1/ update makefiles + 2/ xsalsa20

Thanks, will do after one more doc update to add one sentence: "If you define \textit{LTC_XSalsa20} to include \textit{XSalsa20} in a minimal \textit{libtomcrypt} library build, you must also define \textit{LTC_Salsa20}." I'll push it tomorrow.

sjaeckel

👍 LGTM
Please merge after Karel's comments are taken into account (and you finished the docs :) )

Re-review by request

sjaeckel

Perfect after these minor changes

sjaeckel · 2018-05-31T08:21:40Z

doc/crypt.tex

+and if set to 0 the default number of rounds, 20, will be used.
+\vspace{1mm}
+
+If you define \textit{LTC_XSalsa20} to include \textit{XSalsa20} in a minimal


LTC_XSALSA20

sjaeckel · 2018-05-31T08:22:05Z

doc/crypt.tex

+\vspace{1mm}
+
+If you define \textit{LTC_XSalsa20} to include \textit{XSalsa20} in a minimal
+\textit{libtomcrypt} library build, you must also define \textit{LTC_Salsa20}.


LTC_SALSA20

sjaeckel · 2018-05-31T08:36:03Z

src/stream/salsa20/xsalsa20_test.c

+
+int _sha256(unsigned char *hash, const unsigned char *data, const int datalen) {
+   hash_state md;
+   sha256_init(&md);


pretty paranoid, but this would fail to build if someone wants to build w/o sha256

I think it'd be better to use the hash_memory() function instead of this _sha256() ... but I just realized then again we'd have a dependency on the LTC_HASH_HELPERS ... btw. how's this handled in all the other tests? :D

Simply put this function and the second test-case in a #ifdef LTC_SHA256 please

buggywhip changed the title ~~initial load~~ Add XSalsa20 May 4, 2018

buggywhip requested review from karel-m and sjaeckel May 4, 2018 07:59

sjaeckel requested changes May 5, 2018

View reviewed changes

karel-m reviewed May 6, 2018

View reviewed changes

sjaeckel reviewed May 6, 2018

View reviewed changes

sjaeckel requested changes May 20, 2018

View reviewed changes

karel-m reviewed May 23, 2018

View reviewed changes

karel-m reviewed May 28, 2018

View reviewed changes

karel-m approved these changes May 29, 2018

View reviewed changes

sjaeckel previously approved these changes May 29, 2018

View reviewed changes

buggywhip force-pushed the add_xsalsa20 branch from 588b5dd to c8b5761 Compare May 30, 2018 05:18

sjaeckel approved these changes May 31, 2018

View reviewed changes

buggywhip force-pushed the add_xsalsa20 branch from e952563 to 1ee6105 Compare May 31, 2018 17:15

buggywhip added 2 commits May 31, 2018 11:58

update makefiles

2ffcd17

add XSalsa20

8144209

buggywhip force-pushed the add_xsalsa20 branch from 5547370 to 8144209 Compare May 31, 2018 18:58

buggywhip merged commit 82ac9a2 into develop May 31, 2018

buggywhip deleted the add_xsalsa20 branch May 31, 2018 23:42

fperrad mentioned this pull request Jun 1, 2018

xsalsa20: some linting #397

Merged


		#ifdef LTC_XSALSA20

		int xsalsa20_setup(salsa20_state st, const unsigned char key, unsigned long keylen,

Add XSalsa20 #387

Add XSalsa20 #387

Uh oh!

Conversation

buggywhip commented May 3, 2018

Checklist

Uh oh!

karel-m commented May 4, 2018

Uh oh!

buggywhip commented May 4, 2018 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karel-m commented May 4, 2018

Uh oh!

sjaeckel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karel-m commented May 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjaeckel commented May 6, 2018

buggywhip commented May 4, 2018 via email •

edited

Loading

buggywhip commented May 6, 2018 via email •

edited

Loading

buggywhip May 28, 2018 •

edited

Loading