From bdd73c5c327bcf0749eaedc8ba17501d739323e7 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Tue, 6 Jul 2021 12:36:39 +0200 Subject: [PATCH] Fix unpickling error handling. --- fuzzing/fuzzers/fuzz_unpickle_account.cpp | 6 +---- include/olm/pickle.h | 21 +++++++++++++-- include/olm/pickle.hh | 16 ++++++++++++ src/account.cpp | 32 ++++++++++++++--------- src/inbound_group_session.c | 15 +++++++---- src/megolm.c | 4 +++ src/olm.cpp | 12 ++------- src/outbound_group_session.c | 15 ++++++----- src/pickle.cpp | 24 ++++++++++------- src/pk.cpp | 14 +++++----- src/ratchet.cpp | 28 ++++++++++---------- src/session.cpp | 15 ++++++----- 12 files changed, 121 insertions(+), 81 deletions(-) diff --git a/fuzzing/fuzzers/fuzz_unpickle_account.cpp b/fuzzing/fuzzers/fuzz_unpickle_account.cpp index 0d0dbe4..1084b5c 100644 --- a/fuzzing/fuzzers/fuzz_unpickle_account.cpp +++ b/fuzzing/fuzzers/fuzz_unpickle_account.cpp @@ -9,11 +9,7 @@ size_t fuzz_unpickle_account( std::uint8_t * const pos = reinterpret_cast(pickled); std::uint8_t * const end = pos + pickled_length; - /* On success unpickle will return (pos + raw_length). If unpickling - * 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 (!unpickle(pos, end, object)) { if (object.last_error == OlmErrorCode::OLM_SUCCESS) { object.last_error = OlmErrorCode::OLM_CORRUPTED_PICKLE; } diff --git a/include/olm/pickle.h b/include/olm/pickle.h index 0e668bb..d62c371 100644 --- a/include/olm/pickle.h +++ b/include/olm/pickle.h @@ -15,8 +15,25 @@ #ifndef OLM_PICKLE_H_ #define OLM_PICKLE_H_ +#include #include +/* 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 diff --git a/include/olm/pickle.hh b/include/olm/pickle.hh index 4cf3f36..44bb29e 100644 --- a/include/olm/pickle.hh +++ b/include/olm/pickle.hh @@ -21,6 +21,12 @@ #include #include +/* 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 & 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; } diff --git a/src/account.cpp b/src/account.cpp index 7b81dc7..2c660b9 100644 --- a/src/account.cpp +++ b/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(pos, end, value.curve25519_key); + pos = _olm_unpickle_ed25519_key_pair(pos, end, &value.ed25519_key); UNPICKLE_OK(pos); + 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.published); - pos = olm::unpickle(pos, end, value.key); + pos = olm::unpickle(pos, end, value.id); UNPICKLE_OK(pos); + pos = olm::unpickle(pos, end, value.published); UNPICKLE_OK(pos); + 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.prev_fallback_key); + pos = olm::unpickle(pos, end, value.current_fallback_key); UNPICKLE_OK(pos); + 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; } diff --git a/src/inbound_group_session.c b/src/inbound_group_session.c index 4d8e770..f60b0dc 100644 --- a/src/inbound_group_session.c +++ b/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) { - /* We had the wrong number of bytes in the input. */ - session->last_error = OLM_CORRUPTED_PICKLE; - return (size_t)-1; - } + FAIL_ON_CORRUPTED_PICKLE(pos, session); return pickled_length; } diff --git a/src/megolm.c b/src/megolm.c index 3395449..c4d1110 100644 --- a/src/megolm.c +++ b/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; } diff --git a/src/olm.cpp b/src/olm.cpp index 5024874..d90db9b 100644 --- a/src/olm.cpp +++ b/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 - * 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 (!unpickle(pos, end, 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 - * 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 (!unpickle(pos, end, object)) { if (object.last_error == OlmErrorCode::OLM_SUCCESS) { object.last_error = OlmErrorCode::OLM_CORRUPTED_PICKLE; } diff --git a/src/outbound_group_session.c b/src/outbound_group_session.c index 4a251d0..2e2a0da 100644 --- a/src/outbound_group_session.c +++ b/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) { - /* We had the wrong number of bytes in the input. */ - session->last_error = OLM_CORRUPTED_PICKLE; - return (size_t)-1; - } + pos = megolm_unpickle(&(session->ratchet), pos, end); + FAIL_ON_CORRUPTED_PICKLE(pos, session); + + pos = _olm_unpickle_ed25519_key_pair(pos, end, &(session->signing_key)); + FAIL_ON_CORRUPTED_PICKLE(pos, session); return pickled_length; } diff --git a/src/pickle.cpp b/src/pickle.cpp index b6ee4c5..d69fa98 100644 --- a/src/pickle.cpp +++ b/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; } diff --git a/src/pk.cpp b/src/pk.cpp index 70d64a5..fd77a0a 100644 --- a/src/pk.cpp +++ b/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 - * (pos + raw_length). On error unpickle will return (pos + raw_length + 1). - */ - if (end != unpickle(pos, end + 1, object)) { + + if (!unpickle(pos, end, object)) { if (object.last_error == OlmErrorCode::OLM_SUCCESS) { object.last_error = OlmErrorCode::OLM_CORRUPTED_PICKLE; } diff --git a/src/ratchet.cpp b/src/ratchet.cpp index 360a20b..1d284a6 100644 --- a/src/ratchet.cpp +++ b/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.chain_key.key); - pos = olm::unpickle(pos, end, value.chain_key.index); + pos = olm::unpickle(pos, end, value.ratchet_key); UNPICKLE_OK(pos); + pos = olm::unpickle(pos, end, value.chain_key.key); UNPICKLE_OK(pos); + 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.chain_key.key); - pos = olm::unpickle(pos, end, value.chain_key.index); + pos = olm::unpickle(pos, end, value.ratchet_key); UNPICKLE_OK(pos); + pos = olm::unpickle(pos, end, value.chain_key.key); UNPICKLE_OK(pos); + 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.message_key.key); - pos = olm::unpickle(pos, end, value.message_key.index); + pos = olm::unpickle(pos, end, value.ratchet_key); UNPICKLE_OK(pos); + pos = olm::unpickle(pos, end, value.message_key.key); UNPICKLE_OK(pos); + 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.sender_chain); - pos = unpickle(pos, end, value.receiver_chains); - pos = unpickle(pos, end, value.skipped_message_keys); + pos = unpickle(pos, end, value.root_key); UNPICKLE_OK(pos); + pos = unpickle(pos, end, value.sender_chain); UNPICKLE_OK(pos); + pos = unpickle(pos, end, value.receiver_chains); UNPICKLE_OK(pos); + 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; } diff --git a/src/session.cpp b/src/session.cpp index bd622fc..53d34cd 100644 --- a/src/session.cpp +++ b/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.alice_identity_key); - pos = olm::unpickle(pos, end, value.alice_base_key); - pos = olm::unpickle(pos, end, value.bob_one_time_key); - pos = olm::unpickle(pos, end, value.ratchet, includes_chain_index); + pos = olm::unpickle(pos, end, value.received_message); UNPICKLE_OK(pos); + pos = olm::unpickle(pos, end, value.alice_identity_key); UNPICKLE_OK(pos); + pos = olm::unpickle(pos, end, value.alice_base_key); UNPICKLE_OK(pos); + pos = olm::unpickle(pos, end, value.bob_one_time_key); UNPICKLE_OK(pos); + pos = olm::unpickle(pos, end, value.ratchet, includes_chain_index); UNPICKLE_OK(pos); + return pos; }