From 1ff64391edf9f2e3180238271858698a5a6f30c6 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 19 Oct 2016 15:03:40 +0100 Subject: Fix a buffer bounds check when decoding group messages Fixes a segfault when a group message had exactly the length of the mac + signature. Also tweak skipping of unknown tags to avoid an extra trip around the loop. --- src/message.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/message.cpp b/src/message.cpp index 05fe2c7..1c11a4a 100644 --- a/src/message.cpp +++ b/src/message.cpp @@ -214,11 +214,13 @@ void olm::decode_message( reader.ciphertext = nullptr; reader.ciphertext_length = 0; - if (pos == end) return; if (input_length < mac_length) return; + + if (pos == end) return; reader.version = *(pos++); while (pos != end) { + unknown = pos; pos = decode( pos, end, RATCHET_KEY_TAG, reader.ratchet_key, reader.ratchet_key_length @@ -234,7 +236,6 @@ void olm::decode_message( if (unknown == pos) { pos = skip_unknown(pos, end); } - unknown = pos; } } @@ -303,6 +304,7 @@ void olm::decode_one_time_key_message( reader.version = *(pos++); while (pos != end) { + unknown = pos; pos = decode( pos, end, ONE_TIME_KEY_ID_TAG, reader.one_time_key, reader.one_time_key_length @@ -322,7 +324,6 @@ void olm::decode_one_time_key_message( if (unknown == pos) { pos = skip_unknown(pos, end); } - unknown = pos; } } @@ -377,9 +378,12 @@ void _olm_decode_group_message( results->ciphertext_length = 0; if (input_length < trailer_length) return; + + if (pos == end) return; results->version = *(pos++); while (pos != end) { + unknown = pos; pos = decode( pos, end, GROUP_MESSAGE_INDEX_TAG, results->message_index, has_message_index @@ -391,7 +395,6 @@ void _olm_decode_group_message( if (unknown == pos) { pos = skip_unknown(pos, end); } - unknown = pos; } results->has_message_index = (int)has_message_index; -- cgit v1.2.3-70-g09d2 From 653790eacbf7dcf94cbf181657cdb0c30c890c3f Mon Sep 17 00:00:00 2001 From: Mark Haines Date: Thu, 20 Oct 2016 09:58:55 +0100 Subject: Return the message index when decrypting group messages. Applications can use the index to detect replays of the same message. --- include/olm/inbound_group_session.h | 3 ++- javascript/demo/group_demo.js | 4 ++-- javascript/olm_inbound_group_session.js | 9 +++++++-- python/olm/__main__.py | 2 +- python/olm/inbound_group_session.py | 8 ++++++-- src/inbound_group_session.c | 11 ++++++++--- tests/test_group_session.cpp | 8 +++++--- 7 files changed, 31 insertions(+), 14 deletions(-) (limited to 'src') diff --git a/include/olm/inbound_group_session.h b/include/olm/inbound_group_session.h index 59146c2..f8a0bc3 100644 --- a/include/olm/inbound_group_session.h +++ b/include/olm/inbound_group_session.h @@ -140,7 +140,8 @@ size_t olm_group_decrypt( uint8_t * message, size_t message_length, /* output */ - uint8_t * plaintext, size_t max_plaintext_length + uint8_t * plaintext, size_t max_plaintext_length, + uint32_t * message_index ); diff --git a/javascript/demo/group_demo.js b/javascript/demo/group_demo.js index 1b8f7ab..42a3d84 100644 --- a/javascript/demo/group_demo.js +++ b/javascript/demo/group_demo.js @@ -403,8 +403,8 @@ DemoUser.prototype.decryptGroup = function(jsonpacket, callback) { throw new Error("Unknown session id " + session_id); } - var plaintext = session.decrypt(packet.body); - done(plaintext); + var result = session.decrypt(packet.body); + done(result.plaintext); }, callback); }; diff --git a/javascript/olm_inbound_group_session.js b/javascript/olm_inbound_group_session.js index 6058233..1b7fcfe 100644 --- a/javascript/olm_inbound_group_session.js +++ b/javascript/olm_inbound_group_session.js @@ -73,10 +73,12 @@ InboundGroupSession.prototype['decrypt'] = restore_stack(function( // So we copy the array to a new buffer var message_buffer = stack(message_array); var plaintext_buffer = stack(max_plaintext_length + NULL_BYTE_PADDING_LENGTH); + var message_index = stack(4); var plaintext_length = inbound_group_session_method(Module["_olm_group_decrypt"])( this.ptr, message_buffer, message_array.length, - plaintext_buffer, max_plaintext_length + plaintext_buffer, max_plaintext_length, + message_index ); // Pointer_stringify requires a null-terminated argument (the optional @@ -86,7 +88,10 @@ InboundGroupSession.prototype['decrypt'] = restore_stack(function( 0, "i8" ); - return Pointer_stringify(plaintext_buffer); + return { + "plaintext": Pointer_stringify(plaintext_buffer), + "message_index": Module['getValue'](message_index, "i32") + } }); InboundGroupSession.prototype['session_id'] = restore_stack(function() { diff --git a/python/olm/__main__.py b/python/olm/__main__.py index cf9158d..eb76301 100755 --- a/python/olm/__main__.py +++ b/python/olm/__main__.py @@ -328,7 +328,7 @@ def do_group_decrypt(args): session = InboundGroupSession() session.unpickle(args.key, read_base64_file(args.session_file)) message = args.message_file.read() - plaintext = session.decrypt(message) + plaintext, message_index = session.decrypt(message) with open(args.session_file, "wb") as f: f.write(session.pickle(args.key)) args.plaintext_file.write(plaintext) diff --git a/python/olm/inbound_group_session.py b/python/olm/inbound_group_session.py index d5547fd..27a569c 100644 --- a/python/olm/inbound_group_session.py +++ b/python/olm/inbound_group_session.py @@ -43,6 +43,7 @@ inbound_group_session_function( lib.olm_group_decrypt, c_void_p, c_size_t, # message c_void_p, c_size_t, # plaintext + POINTER(c_uint32), # message_index ) inbound_group_session_function(lib.olm_inbound_group_session_id_length) @@ -82,11 +83,14 @@ class InboundGroupSession(object): ) plaintext_buffer = create_string_buffer(max_plaintext_length) message_buffer = create_string_buffer(message) + + message_index = c_uint32() plaintext_length = lib.olm_group_decrypt( self.ptr, message_buffer, len(message), - plaintext_buffer, max_plaintext_length + plaintext_buffer, max_plaintext_length, + byref(message_index) ) - return plaintext_buffer.raw[:plaintext_length] + return plaintext_buffer.raw[:plaintext_length], message_index def session_id(self): id_length = lib.olm_inbound_group_session_id_length(self.ptr) diff --git a/src/inbound_group_session.c b/src/inbound_group_session.c index bf00008..ed313a0 100644 --- a/src/inbound_group_session.c +++ b/src/inbound_group_session.c @@ -263,7 +263,8 @@ size_t olm_group_decrypt_max_plaintext_length( static size_t _decrypt( OlmInboundGroupSession *session, uint8_t * message, size_t message_length, - uint8_t * plaintext, size_t max_plaintext_length + uint8_t * plaintext, size_t max_plaintext_length, + uint32_t * message_index ) { struct _OlmDecodeGroupMessageResults decoded_results; size_t max_length, r; @@ -286,6 +287,8 @@ static size_t _decrypt( return (size_t)-1; } + *message_index = decoded_results.message_index; + /* verify the signature. We could do this before decoding the message, but * we allow for the possibility of future protocol versions which use a * different signing mechanism; we would rather throw "BAD_MESSAGE_VERSION" @@ -349,7 +352,8 @@ static size_t _decrypt( size_t olm_group_decrypt( OlmInboundGroupSession *session, uint8_t * message, size_t message_length, - uint8_t * plaintext, size_t max_plaintext_length + uint8_t * plaintext, size_t max_plaintext_length, + uint32_t * message_index ) { size_t raw_message_length; @@ -361,7 +365,8 @@ size_t olm_group_decrypt( return _decrypt( session, message, raw_message_length, - plaintext, max_plaintext_length + plaintext, max_plaintext_length, + message_index ); } diff --git a/tests/test_group_session.cpp b/tests/test_group_session.cpp index 9930927..b15875c 100644 --- a/tests/test_group_session.cpp +++ b/tests/test_group_session.cpp @@ -161,8 +161,9 @@ int main() { memcpy(msgcopy, msg, msglen); size = olm_group_decrypt_max_plaintext_length(inbound_session, msgcopy, msglen); uint8_t plaintext_buf[size]; + uint32_t message_index; res = olm_group_decrypt(inbound_session, msg, msglen, - plaintext_buf, size); + plaintext_buf, size, &message_index); assert_equals(plaintext_length, res); assert_equals(plaintext, plaintext_buf, res); } @@ -208,8 +209,9 @@ int main() { memcpy(msgcopy, message, msglen); uint8_t plaintext_buf[size]; + uint32_t message_index; res = olm_group_decrypt( - inbound_session, msgcopy, msglen, plaintext_buf, size + inbound_session, msgcopy, msglen, plaintext_buf, size, &message_index ); assert_equals(plaintext_length, res); assert_equals(plaintext, plaintext_buf, res); @@ -227,7 +229,7 @@ int main() { memcpy(msgcopy, message, msglen); res = olm_group_decrypt( inbound_session, msgcopy, msglen, - plaintext_buf, size + plaintext_buf, size, &message_index ); assert_equals((size_t)-1, res); assert_equals( -- cgit v1.2.3-70-g09d2 From 3091dc2b1d5eab575bfe886bf1d6f2414797373f Mon Sep 17 00:00:00 2001 From: Mark Haines Date: Thu, 20 Oct 2016 11:35:45 +0100 Subject: Add NULL check for message_index pointer --- src/inbound_group_session.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/inbound_group_session.c b/src/inbound_group_session.c index ed313a0..a54e55f 100644 --- a/src/inbound_group_session.c +++ b/src/inbound_group_session.c @@ -287,7 +287,9 @@ static size_t _decrypt( return (size_t)-1; } - *message_index = decoded_results.message_index; + if (message_index != NULL) { + *message_index = decoded_results.message_index; + } /* verify the signature. We could do this before decoding the message, but * we allow for the possibility of future protocol versions which use a -- cgit v1.2.3-70-g09d2 From 21ce3491dd39485eac35ad850257a20fc99f330d Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 21 Oct 2016 17:19:59 +0100 Subject: Clear random buf in olm_init_outbound_group_session All the other methods clear their random inputs. This one needs to do the same, to reduce the risk of the randomness being used elsewhere and leaking key info. --- include/olm/outbound_group_session.h | 2 +- src/outbound_group_session.c | 13 ++++++++----- 2 files changed, 9 insertions(+), 6 deletions(-) (limited to 'src') diff --git a/include/olm/outbound_group_session.h b/include/olm/outbound_group_session.h index 90ccca3..663f1d2 100644 --- a/include/olm/outbound_group_session.h +++ b/include/olm/outbound_group_session.h @@ -96,7 +96,7 @@ size_t olm_init_outbound_group_session_random_length( */ size_t olm_init_outbound_group_session( OlmOutboundGroupSession *session, - uint8_t const * random, size_t random_length + uint8_t *random, size_t random_length ); /** diff --git a/src/outbound_group_session.c b/src/outbound_group_session.c index 4e4561a..ae45694 100644 --- a/src/outbound_group_session.c +++ b/src/outbound_group_session.c @@ -154,20 +154,23 @@ size_t olm_init_outbound_group_session_random_length( size_t olm_init_outbound_group_session( OlmOutboundGroupSession *session, - uint8_t const * random, size_t random_length + uint8_t *random, size_t random_length ) { + const uint8_t *random_ptr = random; + if (random_length < olm_init_outbound_group_session_random_length(session)) { /* Insufficient random data for new session */ session->last_error = OLM_NOT_ENOUGH_RANDOM; return (size_t)-1; } - megolm_init(&(session->ratchet), random, 0); - random += MEGOLM_RATCHET_LENGTH; + megolm_init(&(session->ratchet), random_ptr, 0); + random_ptr += MEGOLM_RATCHET_LENGTH; - _olm_crypto_ed25519_generate_key(random, &(session->signing_key)); - random += ED25519_RANDOM_LENGTH; + _olm_crypto_ed25519_generate_key(random_ptr, &(session->signing_key)); + random_ptr += ED25519_RANDOM_LENGTH; + _olm_unset(random, random_length); return 0; } -- cgit v1.2.3-70-g09d2 From a7310c5821513d5c5b0609ec506dad1ae51603d3 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 24 Oct 2016 10:06:06 +0100 Subject: Return the base64-encoded length of pickles make olm_pickle_* return the lengths of the base64-encoded pickles, rather than the raw pickle. (From the application's POV, the format of the pickle is opaque: it doesn't even know that it is base64-encoded. So returning the length of the raw pickle is particularly unhelpful.) --- src/pickle_encoding.c | 2 +- tests/test_group_session.cpp | 41 +++++++++++++++++++++++------------------ tests/test_olm.cpp | 16 ++++++++++------ 3 files changed, 34 insertions(+), 25 deletions(-) (limited to 'src') diff --git a/src/pickle_encoding.c b/src/pickle_encoding.c index 5d5f8d7..a56e9e3 100644 --- a/src/pickle_encoding.c +++ b/src/pickle_encoding.c @@ -60,7 +60,7 @@ size_t _olm_enc_output( raw_output, length ); _olm_encode_base64(raw_output, length, output); - return raw_length; + return base64_length; } diff --git a/tests/test_group_session.cpp b/tests/test_group_session.cpp index df46f0e..ad67adb 100644 --- a/tests/test_group_session.cpp +++ b/tests/test_group_session.cpp @@ -28,23 +28,26 @@ int main() { size_t pickle_length = olm_pickle_outbound_group_session_length(session); uint8_t pickle1[pickle_length]; - olm_pickle_outbound_group_session(session, - "secret_key", 10, - pickle1, pickle_length); + size_t res = olm_pickle_outbound_group_session( + session, "secret_key", 10, pickle1, pickle_length + ); + assert_equals(pickle_length, res); + uint8_t pickle2[pickle_length]; memcpy(pickle2, pickle1, pickle_length); uint8_t buffer2[size]; OlmOutboundGroupSession *session2 = olm_outbound_group_session(buffer2); - size_t res = olm_unpickle_outbound_group_session(session2, - "secret_key", 10, - pickle2, pickle_length); + res = olm_unpickle_outbound_group_session( + session2, "secret_key", 10, pickle2, pickle_length + ); assert_not_equals((size_t)-1, res); assert_equals(pickle_length, olm_pickle_outbound_group_session_length(session2)); - olm_pickle_outbound_group_session(session2, - "secret_key", 10, - pickle2, pickle_length); + res = olm_pickle_outbound_group_session( + session2, "secret_key", 10, pickle2, pickle_length + ); + assert_equals(pickle_length, res); assert_equals(pickle1, pickle2, pickle_length); } @@ -59,23 +62,25 @@ int main() { size_t pickle_length = olm_pickle_inbound_group_session_length(session); uint8_t pickle1[pickle_length]; - olm_pickle_inbound_group_session(session, - "secret_key", 10, - pickle1, pickle_length); + size_t res = olm_pickle_inbound_group_session( + session, "secret_key", 10, pickle1, pickle_length + ); + assert_equals(pickle_length, res); + uint8_t pickle2[pickle_length]; memcpy(pickle2, pickle1, pickle_length); uint8_t buffer2[size]; OlmInboundGroupSession *session2 = olm_inbound_group_session(buffer2); - size_t res = olm_unpickle_inbound_group_session(session2, - "secret_key", 10, - pickle2, pickle_length); + res = olm_unpickle_inbound_group_session( + session2, "secret_key", 10, pickle2, pickle_length + ); assert_not_equals((size_t)-1, res); assert_equals(pickle_length, olm_pickle_inbound_group_session_length(session2)); - olm_pickle_inbound_group_session(session2, - "secret_key", 10, - pickle2, pickle_length); + res = olm_pickle_inbound_group_session( + session2, "secret_key", 10, pickle2, pickle_length + ); assert_equals(pickle1, pickle2, pickle_length); } diff --git a/tests/test_olm.cpp b/tests/test_olm.cpp index af2c9f7..4619558 100644 --- a/tests/test_olm.cpp +++ b/tests/test_olm.cpp @@ -49,7 +49,9 @@ mock_random(ot_random, sizeof(ot_random)); std::size_t pickle_length = ::olm_pickle_account_length(account); std::uint8_t pickle1[pickle_length]; -::olm_pickle_account(account, "secret_key", 10, pickle1, pickle_length); +std::size_t res = ::olm_pickle_account(account, "secret_key", 10, pickle1, pickle_length); +assert_equals(pickle_length, res); + std::uint8_t pickle2[pickle_length]; std::memcpy(pickle2, pickle1, pickle_length); @@ -59,10 +61,10 @@ assert_not_equals(std::size_t(-1), ::olm_unpickle_account( account2, "secret_key", 10, pickle2, pickle_length )); assert_equals(pickle_length, ::olm_pickle_account_length(account2)); -::olm_pickle_account(account2, "secret_key", 10, pickle2, pickle_length); +res = ::olm_pickle_account(account2, "secret_key", 10, pickle2, pickle_length); +assert_equals(pickle_length, res); assert_equals(pickle1, pickle2, pickle_length); - } @@ -122,7 +124,9 @@ mock_random(random2, sizeof(random2)); std::size_t pickle_length = ::olm_pickle_session_length(session); std::uint8_t pickle1[pickle_length]; -::olm_pickle_session(session, "secret_key", 10, pickle1, pickle_length); +std::size_t res = ::olm_pickle_session(session, "secret_key", 10, pickle1, pickle_length); +assert_equals(pickle_length, res); + std::uint8_t pickle2[pickle_length]; std::memcpy(pickle2, pickle1, pickle_length); @@ -132,10 +136,10 @@ assert_not_equals(std::size_t(-1), ::olm_unpickle_session( session2, "secret_key", 10, pickle2, pickle_length )); assert_equals(pickle_length, ::olm_pickle_session_length(session2)); -::olm_pickle_session(session2, "secret_key", 10, pickle2, pickle_length); +res = ::olm_pickle_session(session2, "secret_key", 10, pickle2, pickle_length); +assert_equals(pickle_length, res); assert_equals(pickle1, pickle2, pickle_length); - } { /** Loopback test */ -- cgit v1.2.3-70-g09d2