Fix unpickling error handling.

fuzzing/fuzzers/fuzz_unpickle_account.cpp

@@ -9,11 +9,7 @@ size_t fuzz_unpickle_account(
     std::uint8_t * const pos = reinterpret_cast<std::uint8_t *>(pickled);
     std::uint8_t * const end = pos + pickled_length;
 
-    /* On success unpickle will return (pos + raw_length). If unpickling
+    if (!unpickle(pos, end, object)) {
-     * terminates too soon then it will return a pointer before
-     * (pos + raw_length). On error unpickle will return (pos + raw_length + 1).
-     */
-    if (end != unpickle(pos, end + 1, object)) {
         if (object.last_error == OlmErrorCode::OLM_SUCCESS) {
             object.last_error = OlmErrorCode::OLM_CORRUPTED_PICKLE;
         }

include/olm/pickle.h

@@ -15,8 +15,25 @@
 #ifndef OLM_PICKLE_H_
 #define OLM_PICKLE_H_
 
+#include <stddef.h>
 #include <stdint.h>
 
+/* Convenience macro for checking the return value of internal unpickling
+ * functions and returning early on failure. */
+#ifndef UNPICKLE_OK
+#define UNPICKLE_OK(x) do { if (!(x)) return NULL; } while(0)
+#endif
+
+/* Convenience macro for failing on corrupted pickles from public
+ * API unpickling functions. */
+#define FAIL_ON_CORRUPTED_PICKLE(pos, session) \
+    do { \
+        if (!pos) { \
+          session->last_error = OLM_CORRUPTED_PICKLE;  \
+          return (size_t)-1; \
+        } \
+    } while(0)
+
 #ifdef __cplusplus
 extern "C" {
 #endif
@@ -59,7 +76,7 @@ uint8_t * _olm_pickle_ed25519_public_key(
 );
 
 /** Unpickle the ed25519 public key. Returns a pointer to the next item in the
- * buffer. */
+ * buffer on success, NULL on error. */
 const uint8_t * _olm_unpickle_ed25519_public_key(
     const uint8_t *pos, const uint8_t *end,
     struct _olm_ed25519_public_key * value
@@ -77,7 +94,7 @@ uint8_t * _olm_pickle_ed25519_key_pair(
 );
 
 /** Unpickle the ed25519 key pair. Returns a pointer to the next item in the
- * buffer. */
+ * buffer on success, NULL on error. */
 const uint8_t * _olm_unpickle_ed25519_key_pair(
     const uint8_t *pos, const uint8_t *end,
     struct _olm_ed25519_key_pair * value

include/olm/pickle.hh

@@ -21,6 +21,12 @@
 #include <cstring>
 #include <cstdint>
 
+/* Convenience macro for checking the return value of internal unpickling
+ * functions and returning early on failure. */
+#ifndef UNPICKLE_OK
+#define UNPICKLE_OK(x) do { if (!(x)) return nullptr; } while(0)
+#endif
+
 namespace olm {
 
 inline std::size_t pickle_length(
@@ -88,11 +94,21 @@ std::uint8_t const * unpickle(
     olm::List<T, max_size> & list
 ) {
     std::uint32_t size;
+
     pos = unpickle(pos, end, size);
+    if (!pos) {
+        return nullptr;
+    }
+
     while (size-- && pos != end) {
         T * value = list.insert(list.end());
         pos = unpickle(pos, end, *value);
+
+        if (!pos) {
+            return nullptr;
         }
+    }
+
     return pos;
 }
 

src/account.cpp

@@ -382,8 +382,8 @@ static std::uint8_t const * unpickle(
     std::uint8_t const * pos, std::uint8_t const * end,
     olm::IdentityKeys & value
 ) {
-    pos = _olm_unpickle_ed25519_key_pair(pos, end, &value.ed25519_key);
+    pos = _olm_unpickle_ed25519_key_pair(pos, end, &value.ed25519_key); UNPICKLE_OK(pos);
-    pos = olm::unpickle(pos, end, value.curve25519_key);
+    pos = olm::unpickle(pos, end, value.curve25519_key); UNPICKLE_OK(pos);
     return pos;
 }
 
@@ -414,9 +414,9 @@ static std::uint8_t const * unpickle(
     std::uint8_t const * pos, std::uint8_t const * end,
     olm::OneTimeKey & value
 ) {
-    pos = olm::unpickle(pos, end, value.id);
+    pos = olm::unpickle(pos, end, value.id); UNPICKLE_OK(pos);
-    pos = olm::unpickle(pos, end, value.published);
+    pos = olm::unpickle(pos, end, value.published); UNPICKLE_OK(pos);
-    pos = olm::unpickle(pos, end, value.key);
+    pos = olm::unpickle(pos, end, value.key); UNPICKLE_OK(pos);
     return pos;
 }
 
@@ -463,28 +463,34 @@ std::uint8_t const * olm::unpickle(
     olm::Account & value
 ) {
     uint32_t pickle_version;
-    pos = olm::unpickle(pos, end, pickle_version);
+
+    pos = olm::unpickle(pos, end, pickle_version); UNPICKLE_OK(pos);
+
     switch (pickle_version) {
         case ACCOUNT_PICKLE_VERSION:
         case 2:
             break;
         case 1:
             value.last_error = OlmErrorCode::OLM_BAD_LEGACY_ACCOUNT_PICKLE;
-            return end;
+            return nullptr;
         default:
             value.last_error = OlmErrorCode::OLM_UNKNOWN_PICKLE_VERSION;
-            return end;
+            return nullptr;
     }
-    pos = olm::unpickle(pos, end, value.identity_keys);
+
-    pos = olm::unpickle(pos, end, value.one_time_keys);
+    pos = olm::unpickle(pos, end, value.identity_keys); UNPICKLE_OK(pos);
+    pos = olm::unpickle(pos, end, value.one_time_keys); UNPICKLE_OK(pos);
+
     if (pickle_version == 2) {
         // version 2 did not have fallback keys
         value.current_fallback_key.published = false;
         value.prev_fallback_key.published = false;
     } else {
-        pos = olm::unpickle(pos, end, value.current_fallback_key);
+        pos = olm::unpickle(pos, end, value.current_fallback_key); UNPICKLE_OK(pos);
-        pos = olm::unpickle(pos, end, value.prev_fallback_key);
+        pos = olm::unpickle(pos, end, value.prev_fallback_key); UNPICKLE_OK(pos);
     }
-    pos = olm::unpickle(pos, end, value.next_one_time_key_id);
+
+    pos = olm::unpickle(pos, end, value.next_one_time_key_id); UNPICKLE_OK(pos);
+
     return pos;
 }

src/inbound_group_session.c

@@ -246,14 +246,23 @@ size_t olm_unpickle_inbound_group_session(
 
     pos = pickled;
     end = pos + raw_length;
+
     pos = _olm_unpickle_uint32(pos, end, &pickle_version);
+    FAIL_ON_CORRUPTED_PICKLE(pos, session);
+
     if (pickle_version < 1 || pickle_version > PICKLE_VERSION) {
         session->last_error = OLM_UNKNOWN_PICKLE_VERSION;
         return (size_t)-1;
     }
+
     pos = megolm_unpickle(&session->initial_ratchet, pos, end);
+    FAIL_ON_CORRUPTED_PICKLE(pos, session);
+
     pos = megolm_unpickle(&session->latest_ratchet, pos, end);
+    FAIL_ON_CORRUPTED_PICKLE(pos, session);
+
     pos = _olm_unpickle_ed25519_public_key(pos, end, &session->signing_key);
+    FAIL_ON_CORRUPTED_PICKLE(pos, session);
 
     if (pickle_version == 1) {
         /* pickle v1 had no signing_key_verified field (all keyshares were
@@ -263,11 +272,7 @@ size_t olm_unpickle_inbound_group_session(
         pos = _olm_unpickle_bool(pos, end, &(session->signing_key_verified));
     }
 
-    if (end != pos) {
+    FAIL_ON_CORRUPTED_PICKLE(pos, session);
-        /* We had the wrong number of bytes in the input. */
-        session->last_error = OLM_CORRUPTED_PICKLE;
-        return (size_t)-1;
-    }
 
     return pickled_length;
 }

src/megolm.c

@@ -73,7 +73,11 @@ const uint8_t * megolm_unpickle(Megolm *megolm, const uint8_t *pos,
                                 const uint8_t *end) {
     pos = _olm_unpickle_bytes(pos, end, (uint8_t *)(megolm->data),
                              MEGOLM_RATCHET_LENGTH);
+    UNPICKLE_OK(pos);
+
     pos = _olm_unpickle_uint32(pos, end, &megolm->counter);
+    UNPICKLE_OK(pos);
+
     return pos;
 }
 

src/olm.cpp

@@ -290,11 +290,7 @@ size_t olm_unpickle_account(
         return std::size_t(-1);
     }
     std::uint8_t * const end = pos + raw_length;
-    /* On success unpickle will return (pos + raw_length). If unpickling
+    if (!unpickle(pos, end, object)) {
-     * terminates too soon then it will return a pointer before
-     * (pos + raw_length). On error unpickle will return (pos + raw_length + 1).
-     */
-    if (end != unpickle(pos, end + 1, object)) {
         if (object.last_error == OlmErrorCode::OLM_SUCCESS) {
             object.last_error = OlmErrorCode::OLM_CORRUPTED_PICKLE;
         }
@@ -319,11 +315,7 @@ size_t olm_unpickle_session(
     }
 
     std::uint8_t * const end = pos + raw_length;
-    /* On success unpickle will return (pos + raw_length). If unpickling
+    if (!unpickle(pos, end, object)) {
-     * terminates too soon then it will return a pointer before
-     * (pos + raw_length). On error unpickle will return (pos + raw_length + 1).
-     */
-    if (end != unpickle(pos, end + 1, object)) {
         if (object.last_error == OlmErrorCode::OLM_SUCCESS) {
             object.last_error = OlmErrorCode::OLM_CORRUPTED_PICKLE;
         }

src/outbound_group_session.c

@@ -143,19 +143,20 @@ size_t olm_unpickle_outbound_group_session(
 
     pos = pickled;
     end = pos + raw_length;
+
     pos = _olm_unpickle_uint32(pos, end, &pickle_version);
+    FAIL_ON_CORRUPTED_PICKLE(pos, session);
+
     if (pickle_version != PICKLE_VERSION) {
         session->last_error = OLM_UNKNOWN_PICKLE_VERSION;
         return (size_t)-1;
     }
-    pos = megolm_unpickle(&(session->ratchet), pos, end);
-    pos = _olm_unpickle_ed25519_key_pair(pos, end, &(session->signing_key));
 
-    if (end != pos) {
+    pos = megolm_unpickle(&(session->ratchet), pos, end);
-        /* We had the wrong number of bytes in the input. */
+    FAIL_ON_CORRUPTED_PICKLE(pos, session);
-        session->last_error = OLM_CORRUPTED_PICKLE;
+
-        return (size_t)-1;
+    pos = _olm_unpickle_ed25519_key_pair(pos, end, &(session->signing_key));
-    }
+    FAIL_ON_CORRUPTED_PICKLE(pos, session);
 
     return pickled_length;
 }

src/pickle.cpp

@@ -30,7 +30,7 @@ std::uint8_t const * olm::unpickle(
     std::uint32_t & value
 ) {
     value = 0;
-    if (end < pos + 4) return end;
+    if (!pos || end <= pos + 4) return nullptr;
     for (unsigned i = 4; i--;) { value <<= 8; value |= *(pos++); }
     return pos;
 }
@@ -47,7 +47,7 @@ std::uint8_t const * olm::unpickle(
     std::uint8_t const * pos, std::uint8_t const * end,
     bool & value
 ) {
-    if (pos == end) return end;
+    if (!pos || pos == end) return nullptr;
     value = *(pos++);
     return pos;
 }
@@ -64,7 +64,7 @@ std::uint8_t const * olm::unpickle_bytes(
     std::uint8_t const * pos, std::uint8_t const * end,
     std::uint8_t * bytes, std::size_t bytes_length
 ) {
-    if (end < pos + bytes_length) return end;
+    if (!pos || end < pos + bytes_length) return nullptr;
     std::memcpy(bytes, pos, bytes_length);
     return pos + bytes_length;
 }
@@ -92,11 +92,9 @@ std::uint8_t const * olm::unpickle(
     std::uint8_t const * pos, std::uint8_t const * end,
     _olm_curve25519_public_key & value
 ) {
-    pos = olm::unpickle_bytes(
+    return olm::unpickle_bytes(
         pos, end, value.public_key, sizeof(value.public_key)
     );
-    return pos;
-
 }
 
 
@@ -132,10 +130,14 @@ std::uint8_t const * olm::unpickle(
         pos, end, value.public_key.public_key,
         sizeof(value.public_key.public_key)
     );
+    if (!pos) return nullptr;
+
     pos = olm::unpickle_bytes(
         pos, end, value.private_key.private_key,
         sizeof(value.private_key.private_key)
     );
+    if (!pos) return nullptr;
+
     return pos;
 }
 
@@ -152,10 +154,9 @@ std::uint8_t * _olm_pickle_ed25519_public_key(
     std::uint8_t * pos,
     const _olm_ed25519_public_key *value
 ) {
-    pos = olm::pickle_bytes(
+    return olm::pickle_bytes(
         pos, value->public_key, sizeof(value->public_key)
     );
-    return pos;
 }
 
 
@@ -163,10 +164,9 @@ std::uint8_t const * _olm_unpickle_ed25519_public_key(
     std::uint8_t const * pos, std::uint8_t const * end,
     _olm_ed25519_public_key * value
 ) {
-    pos = olm::unpickle_bytes(
+    return olm::unpickle_bytes(
         pos, end, value->public_key, sizeof(value->public_key)
     );
-    return pos;
 }
 
 
@@ -202,10 +202,14 @@ std::uint8_t const * _olm_unpickle_ed25519_key_pair(
         pos, end, value->public_key.public_key,
         sizeof(value->public_key.public_key)
     );
+    if (!pos) return nullptr;
+
     pos = olm::unpickle_bytes(
         pos, end, value->private_key.private_key,
         sizeof(value->private_key.private_key)
     );
+    if (!pos) return nullptr;
+
     return pos;
 }
 

src/pk.cpp

@@ -274,7 +274,7 @@ namespace {
         OlmPkDecryption & value
     ) {
         uint32_t pickle_version;
-        pos = olm::unpickle(pos, end, pickle_version);
+        pos = olm::unpickle(pos, end, pickle_version); UNPICKLE_OK(pos);
 
         switch (pickle_version) {
         case 1:
@@ -282,10 +282,11 @@ namespace {
 
         default:
             value.last_error = OlmErrorCode::OLM_UNKNOWN_PICKLE_VERSION;
-            return end;
+            return nullptr;
         }
 
-        pos = olm::unpickle(pos, end, value.key_pair);
+        pos = olm::unpickle(pos, end, value.key_pair); UNPICKLE_OK(pos);
+
         return pos;
     }
 }
@@ -334,11 +335,8 @@ size_t olm_unpickle_pk_decryption(
         return std::size_t(-1);
     }
     std::uint8_t * const end = pos + raw_length;
-    /* On success unpickle will return (pos + raw_length). If unpickling
+
-     * terminates too soon then it will return a pointer before
+    if (!unpickle(pos, end, object)) {
-     * (pos + raw_length). On error unpickle will return (pos + raw_length + 1).
-     */
-    if (end != unpickle(pos, end + 1, object)) {
         if (object.last_error == OlmErrorCode::OLM_SUCCESS) {
             object.last_error = OlmErrorCode::OLM_CORRUPTED_PICKLE;
         }

src/ratchet.cpp

@@ -280,9 +280,9 @@ static std::uint8_t const * unpickle(
     std::uint8_t const * pos, std::uint8_t const * end,
     olm::SenderChain & value
 ) {
-    pos = olm::unpickle(pos, end, value.ratchet_key);
+    pos = olm::unpickle(pos, end, value.ratchet_key); UNPICKLE_OK(pos);
-    pos = olm::unpickle(pos, end, value.chain_key.key);
+    pos = olm::unpickle(pos, end, value.chain_key.key); UNPICKLE_OK(pos);
-    pos = olm::unpickle(pos, end, value.chain_key.index);
+    pos = olm::unpickle(pos, end, value.chain_key.index); UNPICKLE_OK(pos);
     return pos;
 }
 
@@ -312,9 +312,9 @@ static std::uint8_t const * unpickle(
     std::uint8_t const * pos, std::uint8_t const * end,
     olm::ReceiverChain & value
 ) {
-    pos = olm::unpickle(pos, end, value.ratchet_key);
+    pos = olm::unpickle(pos, end, value.ratchet_key); UNPICKLE_OK(pos);
-    pos = olm::unpickle(pos, end, value.chain_key.key);
+    pos = olm::unpickle(pos, end, value.chain_key.key); UNPICKLE_OK(pos);
-    pos = olm::unpickle(pos, end, value.chain_key.index);
+    pos = olm::unpickle(pos, end, value.chain_key.index); UNPICKLE_OK(pos);
     return pos;
 }
 
@@ -345,9 +345,9 @@ static std::uint8_t const * unpickle(
     std::uint8_t const * pos, std::uint8_t const * end,
     olm::SkippedMessageKey & value
 ) {
-    pos = olm::unpickle(pos, end, value.ratchet_key);
+    pos = olm::unpickle(pos, end, value.ratchet_key); UNPICKLE_OK(pos);
-    pos = olm::unpickle(pos, end, value.message_key.key);
+    pos = olm::unpickle(pos, end, value.message_key.key); UNPICKLE_OK(pos);
-    pos = olm::unpickle(pos, end, value.message_key.index);
+    pos = olm::unpickle(pos, end, value.message_key.index); UNPICKLE_OK(pos);
     return pos;
 }
 
@@ -383,15 +383,15 @@ std::uint8_t const * olm::unpickle(
     olm::Ratchet & value,
     bool includes_chain_index
 ) {
-    pos = unpickle(pos, end, value.root_key);
+    pos = unpickle(pos, end, value.root_key); UNPICKLE_OK(pos);
-    pos = unpickle(pos, end, value.sender_chain);
+    pos = unpickle(pos, end, value.sender_chain); UNPICKLE_OK(pos);
-    pos = unpickle(pos, end, value.receiver_chains);
+    pos = unpickle(pos, end, value.receiver_chains); UNPICKLE_OK(pos);
-    pos = unpickle(pos, end, value.skipped_message_keys);
+    pos = unpickle(pos, end, value.skipped_message_keys); UNPICKLE_OK(pos);
 
     // pickle v 0x80000001 includes a chain index; pickle v1 does not.
     if (includes_chain_index) {
         std::uint32_t dummy;
-        pos = unpickle(pos, end, dummy);
+        pos = unpickle(pos, end, dummy); UNPICKLE_OK(pos);
     }
     return pos;
 }

src/session.cpp

@@ -472,7 +472,7 @@ std::uint8_t const * olm::unpickle(
     Session & value
 ) {
     uint32_t pickle_version;
-    pos = olm::unpickle(pos, end, pickle_version);
+    pos = olm::unpickle(pos, end, pickle_version); UNPICKLE_OK(pos);
 
     bool includes_chain_index;
     switch (pickle_version) {
@@ -486,13 +486,14 @@ std::uint8_t const * olm::unpickle(
 
         default:
             value.last_error = OlmErrorCode::OLM_UNKNOWN_PICKLE_VERSION;
-            return end;
+            return nullptr;
     }
 
-    pos = olm::unpickle(pos, end, value.received_message);
+    pos = olm::unpickle(pos, end, value.received_message); UNPICKLE_OK(pos);
-    pos = olm::unpickle(pos, end, value.alice_identity_key);
+    pos = olm::unpickle(pos, end, value.alice_identity_key); UNPICKLE_OK(pos);
-    pos = olm::unpickle(pos, end, value.alice_base_key);
+    pos = olm::unpickle(pos, end, value.alice_base_key); UNPICKLE_OK(pos);
-    pos = olm::unpickle(pos, end, value.bob_one_time_key);
+    pos = olm::unpickle(pos, end, value.bob_one_time_key); UNPICKLE_OK(pos);
-    pos = olm::unpickle(pos, end, value.ratchet, includes_chain_index);
+    pos = olm::unpickle(pos, end, value.ratchet, includes_chain_index); UNPICKLE_OK(pos);
+
     return pos;
 }