Fix a bug in XSalsa20

If you called set_key, then set_iv, then set_iv again without having
previously reset the key, you would end up with a garbled state buffer
that depended on the value of the first IV.

This only affected 192-bit Salsa nonces, not other sizes.

4 files changed, 61 insertions, 25 deletions

diff --git a/src/lib/stream/salsa20/salsa20.cpp b/src/lib/stream/salsa20/salsa20.cpp
index 47123c970..3ee6cf37e 100644
--- a/src/lib/stream/salsa20/salsa20.cpp
+++ b/src/lib/stream/salsa20/salsa20.cpp
@@ -128,10 +128,7 @@ void Salsa20::cipher(const uint8_t in[], uint8_t out[], size_t length)
    m_position += length;
    }
 
-/*
-* Salsa20 Key Schedule
-*/
-void Salsa20::key_schedule(const uint8_t key[], size_t length)
+void Salsa20::initialize_state()
    {
    static const uint32_t TAU[] =
       { 0x61707865, 0x3120646e, 0x79622d36, 0x6b206574 };
@@ -139,32 +136,53 @@ void Salsa20::key_schedule(const uint8_t key[], size_t length)
    static const uint32_t SIGMA[] =
       { 0x61707865, 0x3320646e, 0x79622d32, 0x6b206574 };
 
-   const uint32_t* CONSTANTS = (length == 16) ? TAU : SIGMA;
-
-   m_state.resize(16);
-   m_buffer.resize(64);
+   const uint32_t* CONSTANTS = (m_key.size() == 4) ? TAU : SIGMA;
 
    m_state[0] = CONSTANTS[0];
    m_state[5] = CONSTANTS[1];
    m_state[10] = CONSTANTS[2];
    m_state[15] = CONSTANTS[3];
 
-   m_state[1] = load_le<uint32_t>(key, 0);
-   m_state[2] = load_le<uint32_t>(key, 1);
-   m_state[3] = load_le<uint32_t>(key, 2);
-   m_state[4] = load_le<uint32_t>(key, 3);
+   m_state[1] = m_key[0];
+   m_state[2] = m_key[1];
+   m_state[3] = m_key[2];
+   m_state[4] = m_key[3];
 
-   if(length == 32)
-      key += 16;
+   if(m_key.size() == 4)
+      {
+      m_state[11] = m_key[0];
+      m_state[12] = m_key[1];
+      m_state[13] = m_key[2];
+      m_state[14] = m_key[3];
+      }
+   else
+      {
+      m_state[11] = m_key[4];
+      m_state[12] = m_key[5];
+      m_state[13] = m_key[6];
+      m_state[14] = m_key[7];
+      }
 
-   m_state[11] = load_le<uint32_t>(key, 0);
-   m_state[12] = load_le<uint32_t>(key, 1);
-   m_state[13] = load_le<uint32_t>(key, 2);
-   m_state[14] = load_le<uint32_t>(key, 3);
+   m_state[6] = 0;
+   m_state[7] = 0;
+   m_state[8] = 0;
+   m_state[9] = 0;
 
    m_position = 0;
+   }
+
+/*
+* Salsa20 Key Schedule
+*/
+void Salsa20::key_schedule(const uint8_t key[], size_t length)
+   {
+   m_key.resize(length / 4);
+   load_le<uint32_t>(m_key.data(), key, m_key.size());
+
+   m_state.resize(16);
+   m_buffer.resize(64);
 
-   set_iv(nullptr, 0); // all-zero IV
+   set_iv(nullptr, 0);
    }
 
 /*
@@ -177,6 +195,8 @@ void Salsa20::set_iv(const uint8_t iv[], size_t length)
    if(!valid_iv_length(length))
       throw Invalid_IV_Length(name(), length);
 
+   initialize_state();
+
    if(length == 0)
       {
       // Salsa20 null IV
@@ -235,6 +255,7 @@ std::string Salsa20::name() const
 */
 void Salsa20::clear()
    {
+   zap(m_key);
    zap(m_state);
    zap(m_buffer);
    m_position = 0;
diff --git a/src/lib/stream/salsa20/salsa20.h b/src/lib/stream/salsa20/salsa20.h
index d21ee318e..090f9b18d 100644
--- a/src/lib/stream/salsa20/salsa20.h
+++ b/src/lib/stream/salsa20/salsa20.h
@@ -40,6 +40,9 @@ class BOTAN_PUBLIC_API(2,0) Salsa20 final : public StreamCipher
    private:
       void key_schedule(const uint8_t key[], size_t key_len) override;
 
+      void initialize_state();
+
+      secure_vector<uint32_t> m_key;
       secure_vector<uint32_t> m_state;
       secure_vector<uint8_t> m_buffer;
       size_t m_position = 0;
diff --git a/src/tests/data/stream/salsa20.vec b/src/tests/data/stream/salsa20.vec
index e8276bf32..5523ebd20 100644
--- a/src/tests/data/stream/salsa20.vec
+++ b/src/tests/data/stream/salsa20.vec
@@ -1,6 +1,5 @@
 [Salsa20]
 Key = 000102030405060708090A0B0C0D0E0F
-Nonce = 0000000000000000
 Out = 2DD5C3F7BA2B20F76802410C688688895AD8C1BD4EA6C9B140FB9B90E21049BF583F527970EBC1
 
 Key = 1B1C1D1E1F202122232425262728292A2B2C2D2E2F303132333435363738393A
diff --git a/src/tests/test_stream.cpp b/src/tests/test_stream.cpp
index 63ea9a9bf..5b61518f5 100644
--- a/src/tests/test_stream.cpp
+++ b/src/tests/test_stream.cpp
@@ -88,12 +88,15 @@ class Stream_Cipher_Tests final : public Text_Based_Test
                }
 
             bool accepted_nonce_early = false;
-            try
+            if(nonce.size() > 0)
                {
-               cipher->set_iv(nonce.data(), nonce.size());
-               accepted_nonce_early = true;
+               try
+                  {
+                  cipher->set_iv(nonce.data(), nonce.size());
+                  accepted_nonce_early = true;
+                  }
+               catch(Botan::Invalid_State&) {}
                }
-            catch(Botan::Invalid_State&) {}
 
             cipher->set_key(key);
 
@@ -112,7 +115,7 @@ class Stream_Cipher_Tests final : public Text_Based_Test
             not set. So, don't set the nonce now, to ensure the previous call
             had an effect.
             */
-            if(accepted_nonce_early == false)
+            if(nonce.size() > 0 && accepted_nonce_early == false)
                {
                cipher->set_iv(nonce.data(), nonce.size());
                }
@@ -134,6 +137,16 @@ class Stream_Cipher_Tests final : public Text_Based_Test
             result.test_eq(provider, "encrypt", buf, expected);
             }
 
+            if(nonce.size() > 0)
+               {
+               std::vector<uint8_t> buf = input;
+               cipher->set_iv(nonce.data(), nonce.size());
+               if(seek != 0)
+                  cipher->seek(seek);
+               cipher->encrypt(buf);
+               result.test_eq(provider, "second encrypt", buf, expected);
+               }
+
             cipher->clear();
 
             try