From d704f4bd3c1b16e6a1ac3fbaf12f43164bbba689 Mon Sep 17 00:00:00 2001 From: Denis Kasak Date: Thu, 15 Jul 2021 15:06:14 +0200 Subject: [PATCH] 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 | 7 ++++- src/olm.cpp | 37 ++++++++++++++++++++------ src/outbound_group_session.c | 6 +++++ src/pk.cpp | 19 +++++++++++--- tests/include/utils.hh | 38 +++++++++++++++++++++++++++ tests/test_group_session.cpp | 50 +++++++++++++++++++++++++++++++++++- tests/test_olm.cpp | 36 ++++++++++++++++++++++++++ tests/test_pk.cpp | 31 +++++++++++++++++++++- 8 files changed, 209 insertions(+), 15 deletions(-) create mode 100644 tests/include/utils.hh diff --git a/src/inbound_group_session.c b/src/inbound_group_session.c index f60b0dc..421c17b 100644 --- a/src/inbound_group_session.c +++ b/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; } diff --git a/src/olm.cpp b/src/olm.cpp index d90db9b..d0cf653 100644 --- a/src/olm.cpp +++ b/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; } diff --git a/src/outbound_group_session.c b/src/outbound_group_session.c index 2e2a0da..bc38a5a 100644 --- a/src/outbound_group_session.c +++ b/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; } diff --git a/src/pk.cpp b/src/pk.cpp index fd77a0a..2cb40d2 100644 --- a/src/pk.cpp +++ b/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(pickled); + std::uint8_t * const input = reinterpret_cast(pickled); std::size_t raw_length = _olm_enc_input( reinterpret_cast(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; } diff --git a/tests/include/utils.hh b/tests/include/utils.hh new file mode 100644 index 0000000..01c9f80 --- /dev/null +++ b/tests/include/utils.hh @@ -0,0 +1,38 @@ +/* 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 + +#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); +} diff --git a/tests/test_group_session.cpp b/tests/test_group_session.cpp index 19b7761..9a1e9a3 100644 --- a/tests/test_group_session.cpp +++ b/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 @@ -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 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 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)); } { diff --git a/tests/test_olm.cpp b/tests/test_olm.cpp index ab7d816..0d93c90 100644 --- a/tests/test_olm.cpp +++ b/tests/test_olm.cpp @@ -1,5 +1,7 @@ #include "olm/olm.h" + #include "unittest.hh" +#include "utils.hh" #include #include @@ -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 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 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 */ diff --git a/tests/test_pk.cpp b/tests/test_pk.cpp index b917a2e..2db4eae 100644 --- a/tests/test_pk.cpp +++ b/tests/test_pk.cpp @@ -1,8 +1,9 @@ -#include "olm/pk.h" #include "olm/crypto.h" #include "olm/olm.h" +#include "olm/pk.h" #include "unittest.hh" +#include "utils.hh" #include #include @@ -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 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";