Fail when an unpickle succeeds but has extra junk data at the end.

Also adds tests to ensure this is working.

src/inbound_group_session.c

@@ -271,9 +271,14 @@ size_t olm_unpickle_inbound_group_session(
     } else {
         pos = _olm_unpickle_bool(pos, end, &(session->signing_key_verified));
     }
-
     FAIL_ON_CORRUPTED_PICKLE(pos, session);
 
+    if (pos != end) {
+        /* Input was longer than expected. */
+        session->last_error = OLM_CORRUPTED_PICKLE;
+        return (size_t)-1;
+    }
+
     return pickled_length;
 }
 

src/olm.cpp

@@ -282,20 +282,31 @@ size_t olm_unpickle_account(
     void * pickled, size_t pickled_length
 ) {
     olm::Account & object = *from_c(account);
-    std::uint8_t * const pos = from_c(pickled);
+    std::uint8_t * input = from_c(pickled);
     std::size_t raw_length = _olm_enc_input(
-        from_c(key), key_length, pos, pickled_length, &object.last_error
+        from_c(key), key_length, input, pickled_length, &object.last_error
     );
     if (raw_length == std::size_t(-1)) {
         return std::size_t(-1);
     }
-    std::uint8_t * const end = pos + raw_length;
-    if (!unpickle(pos, end, object)) {
+
+    std::uint8_t const * pos = input;
+    std::uint8_t const * end = pos + raw_length;
+
+    pos = unpickle(pos, end, object);
+
+    if (!pos) {
+        /* Input was corrupted. */
         if (object.last_error == OlmErrorCode::OLM_SUCCESS) {
             object.last_error = OlmErrorCode::OLM_CORRUPTED_PICKLE;
         }
         return std::size_t(-1);
+    } else if (pos != end) {
+        /* Input was longer than expected. */
+        object.last_error = OlmErrorCode::OLM_CORRUPTED_PICKLE;
+        return std::size_t(-1);
     }
+
     return pickled_length;
 }
 
@@ -306,21 +317,31 @@ size_t olm_unpickle_session(
     void * pickled, size_t pickled_length
 ) {
     olm::Session & object = *from_c(session);
-    std::uint8_t * const pos = from_c(pickled);
+    std::uint8_t * input = from_c(pickled);
     std::size_t raw_length = _olm_enc_input(
-        from_c(key), key_length, pos, pickled_length, &object.last_error
+        from_c(key), key_length, input, pickled_length, &object.last_error
     );
     if (raw_length == std::size_t(-1)) {
         return std::size_t(-1);
     }
 
-    std::uint8_t * const end = pos + raw_length;
-    if (!unpickle(pos, end, object)) {
+    std::uint8_t const * pos = input;
+    std::uint8_t const * end = pos + raw_length;
+
+    pos = unpickle(pos, end, object);
+
+    if (!pos) {
+        /* Input was corrupted. */
         if (object.last_error == OlmErrorCode::OLM_SUCCESS) {
             object.last_error = OlmErrorCode::OLM_CORRUPTED_PICKLE;
         }
         return std::size_t(-1);
+    } else if (pos != end) {
+        /* Input was longer than expected. */
+        object.last_error = OlmErrorCode::OLM_CORRUPTED_PICKLE;
+        return std::size_t(-1);
     }
+
     return pickled_length;
 }
 

src/outbound_group_session.c

@@ -158,6 +158,12 @@ size_t olm_unpickle_outbound_group_session(
     pos = _olm_unpickle_ed25519_key_pair(pos, end, &(session->signing_key));
     FAIL_ON_CORRUPTED_PICKLE(pos, session);
 
+    if (pos != end) {
+        /* Input was longer than expected. */
+        session->last_error = OLM_CORRUPTED_PICKLE;
+        return (size_t)-1;
+    }
+
     return pickled_length;
 }
 

src/pk.cpp

@@ -326,22 +326,32 @@ size_t olm_unpickle_pk_decryption(
         object.last_error = OlmErrorCode::OLM_OUTPUT_BUFFER_TOO_SMALL;
         return std::size_t(-1);
     }
-    std::uint8_t * const pos = reinterpret_cast<std::uint8_t *>(pickled);
+    std::uint8_t * const input = reinterpret_cast<std::uint8_t *>(pickled);
     std::size_t raw_length = _olm_enc_input(
         reinterpret_cast<std::uint8_t const *>(key), key_length,
-        pos, pickled_length, &object.last_error
+        input, pickled_length, &object.last_error
     );
     if (raw_length == std::size_t(-1)) {
         return std::size_t(-1);
     }
-    std::uint8_t * const end = pos + raw_length;
 
-    if (!unpickle(pos, end, object)) {
+    std::uint8_t const * pos = input;
+    std::uint8_t const * end = pos + raw_length;
+
+    pos = unpickle(pos, end, object);
+
+    if (!pos) {
+        /* Input was corrupted. */
         if (object.last_error == OlmErrorCode::OLM_SUCCESS) {
             object.last_error = OlmErrorCode::OLM_CORRUPTED_PICKLE;
         }
         return std::size_t(-1);
+    } else if (pos != end) {
+        /* Input was longer than expected. */
+        object.last_error = OlmErrorCode::OLM_CORRUPTED_PICKLE;
+        return std::size_t(-1);
     }
+
     if (pubkey != NULL) {
         olm::encode_base64(
             (const uint8_t *)object.key_pair.public_key.public_key,
@@ -349,6 +359,7 @@ size_t olm_unpickle_pk_decryption(
             (uint8_t *)pubkey
         );
     }
+
     return pickled_length;
 }
 

tests/include/utils.hh

+/* Copyright 2021 The Matrix.org Foundation C.I.C.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <string.h>
+
+#include "olm/pickle_encoding.h"
+
+size_t add_junk_suffix_to_pickle(void const * key, size_t key_length,
+    void * pickled, size_t pickled_length, size_t junk_length)
+{
+    size_t raw_length = _olm_enc_input((uint8_t const *) key, key_length,
+        (uint8_t *) pickled, pickled_length, nullptr);
+
+    /* Insert junk. */
+    size_t new_length = raw_length + junk_length;
+    uint8_t * pos = (uint8_t *) pickled + raw_length;
+    while (junk_length--) {
+        *pos++ = 255;
+    }
+
+    void * dest = _olm_enc_output_pos((uint8_t *) pickled, new_length);
+    memmove(dest, pickled, new_length);
+
+    return _olm_enc_output(
+        (uint8_t const *) key, key_length, (uint8_t *) pickled, new_length);
+}

tests/test_group_session.cpp

@@ -15,6 +15,7 @@
 #include "olm/inbound_group_session.h"
 #include "olm/outbound_group_session.h"
 #include "unittest.hh"
+#include "utils.hh"
 
 #include <vector>
 
@@ -50,8 +51,31 @@ int main() {
     assert_equals(pickle_length, res);
 
     assert_equals(pickle1.data(), pickle2.data(), pickle_length);
-}
 
+    /* Deliberately corrupt the pickled session by supplying a junk suffix and
+    * ensure this is caught as an error. */
+    const size_t junk_length = 1;
+    std::vector<std::uint8_t> junk_pickle(pickle_length + junk_length);
+
+    olm_pickle_outbound_group_session(
+        session, "secret_key", 10, junk_pickle.data(), pickle_length
+    );
+
+    const size_t junk_pickle_length = add_junk_suffix_to_pickle(
+        "secret_key", 10,
+        junk_pickle.data(),
+        pickle_length,
+        junk_length);
+
+    assert_equals(std::size_t(-1),
+        olm_unpickle_outbound_group_session(
+            session,
+            "secret_key", 10,
+            junk_pickle.data(), junk_pickle_length
+        ));
+    assert_equals(OLM_CORRUPTED_PICKLE,
+                  olm_outbound_group_session_last_error_code(session));
+}
 
 {
     TestCase test_case("Pickle inbound group session");
@@ -82,6 +106,30 @@ int main() {
     );
 
     assert_equals(pickle1.data(), pickle2.data(), pickle_length);
+
+    /* Deliberately corrupt the pickled session by supplying a junk suffix and
+    * ensure this is caught as an error. */
+    const size_t junk_length = 1;
+    std::vector<std::uint8_t> junk_pickle(pickle_length + junk_length);
+
+    olm_pickle_inbound_group_session(
+        session, "secret_key", 10, junk_pickle.data(), pickle_length
+    );
+
+    const size_t junk_pickle_length = add_junk_suffix_to_pickle(
+        "secret_key", 10,
+        junk_pickle.data(),
+        pickle_length,
+        junk_length);
+
+    assert_equals(std::size_t(-1),
+        olm_unpickle_inbound_group_session(
+            session,
+            "secret_key", 10,
+            junk_pickle.data(), junk_pickle_length
+        ));
+    assert_equals(OLM_CORRUPTED_PICKLE,
+                  olm_inbound_group_session_last_error_code(session));
 }
 
 {

tests/test_olm.cpp

 #include "olm/olm.h"
+
 #include "unittest.hh"
+#include "utils.hh"
 
 #include <cstddef>
 #include <cstdint>
@@ -65,6 +67,23 @@ res = ::olm_pickle_account(account2, "secret_key", 10, pickle2.data(), pickle_le
 assert_equals(pickle_length, res);
 
 assert_equals(pickle1.data(), pickle2.data(), pickle_length);
+
+/* Deliberately corrupt the pickled account by supplying a junk suffix and
+ * ensure this is caught as an error. */
+const size_t junk_length = 1;
+std::vector<std::uint8_t> junk_pickle(pickle_length + junk_length);
+
+res = ::olm_pickle_account(
+    account, "secret_key", 10, junk_pickle.data(), pickle_length);
+assert_equals(pickle_length, res);
+
+const size_t junk_pickle_length = add_junk_suffix_to_pickle(
+    "secret_key", 10, junk_pickle.data(), pickle_length, junk_length);
+
+assert_equals(std::size_t(-1),
+    ::olm_unpickle_account(account, "secret_key", 10,
+        junk_pickle.data(), junk_pickle_length));
+assert_equals(OLM_CORRUPTED_PICKLE, olm_account_last_error_code(account));
 }
 
 
@@ -139,6 +158,23 @@ res = ::olm_pickle_session(session2, "secret_key", 10, pickle2.data(), pickle_le
 assert_equals(pickle_length, res);
 
 assert_equals(pickle1.data(), pickle2.data(), pickle_length);
+
+/* Deliberately corrupt the pickled session by supplying a junk suffix and
+ * ensure this is caught as an error. */
+const size_t junk_length = 1;
+std::vector<std::uint8_t> junk_pickle(pickle_length + junk_length);
+
+res = ::olm_pickle_session(
+    session, "secret_key", 10, junk_pickle.data(), pickle_length);
+assert_equals(pickle_length, res);
+
+const size_t junk_pickle_length = add_junk_suffix_to_pickle(
+    "secret_key", 10, junk_pickle.data(), pickle_length, junk_length);
+
+assert_equals(std::size_t(-1),
+    ::olm_unpickle_session(session, "secret_key", 10,
+        junk_pickle.data(), junk_pickle_length));
+assert_equals(OLM_CORRUPTED_PICKLE, olm_session_last_error_code(session));
 }
 
 { /** Loopback test */

tests/test_pk.cpp

-#include "olm/pk.h"
 #include "olm/crypto.h"
 #include "olm/olm.h"
+#include "olm/pk.h"
 
 #include "unittest.hh"
+#include "utils.hh"
 
 #include <iostream>
 #include <vector>
@@ -141,6 +142,34 @@ olm_unpickle_pk_decryption(
 
 assert_equals(alice_public, pubkey.data(), olm_pk_key_length());
 
+/* Deliberately corrupt the pickled session by supplying a junk suffix and
+ * ensure this is caught as an error. */
+const size_t junk_length = 1;
+std::size_t pickle_length = olm_pickle_pk_decryption_length(decryption);
+std::vector<std::uint8_t> junk_pickle(pickle_length + junk_length);
+
+olm_pickle_pk_decryption(
+    decryption,
+    PICKLE_KEY, strlen((char *)PICKLE_KEY),
+    junk_pickle.data(), pickle_length
+);
+
+const size_t junk_pickle_length = add_junk_suffix_to_pickle(
+    PICKLE_KEY, strlen((char *)PICKLE_KEY),
+    junk_pickle.data(),
+    pickle_length,
+    junk_length);
+
+assert_equals(std::size_t(-1),
+    olm_unpickle_pk_decryption(
+        decryption,
+        PICKLE_KEY, strlen((char *)PICKLE_KEY),
+        junk_pickle.data(), junk_pickle_length,
+        pubkey.data(), pubkey.size()
+    ));
+assert_equals(OLM_CORRUPTED_PICKLE, olm_pk_decryption_last_error_code(decryption));
+/***/
+
 char *ciphertext = strdup("ntk49j/KozVFtSqJXhCejg");
 const char *mac = "zpzU6BkZcNI";
 const char *ephemeral_key = "3p7bfXt9wbTTW2HC7OQ1Nz+DQ8hbeGdNrfx+FG+IK08";