Skip to content

Commit

Permalink
Fix Secure Session serialization format (#658)
Browse files Browse the repository at this point in the history
secure_session_save() saves short serialized Secure Session state.

That is, it writes "is_client" as 4-byte value while its size in
SESSION_CTX_SERIZALIZED_SIZE is computed as sizeof(bool) which is 1 byte
on most platforms. This causes 3 least significant bytes of "in_seq"
being missing from the serialized data.

Correct size of Secure Session state is 60 bytes, we report only 57.

This issue is not detected by unit tests of JavaThemis -- the only
high-level wrapper supporting this interface -- which works only because
the allocated memory for the output array is slightly bigger than
requested 57 bytes, extra 3 bytes written past-the-end remain there
in RAM and the unit test generally works, if the data stays where it is.
However, sometimes garbage collection occurs at the right moment and the
issue manifests itself as a failing Secure Session test.

Now the output size is reported correctly and all Secure Session data is
written within the allocated bounds.
  • Loading branch information
ilammy authored Jun 16, 2020
1 parent 9f32f8d commit e83ec23
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 8 deletions.
9 changes: 9 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ _Code:_

provide Seal mode API that is safe to use with passphrases ([#577](https://github.com/cossacklabs/themis/pull/577)).

- **Secure Session**

- Fixed serialization issue in `secure_session_save()` and `secure_session_load()` methods
([#658](https://github.com/cossacklabs/themis/pull/658).

- **Breaking changes**

- <a id="0.13.0-drop-0.9.6-compat">Secure Cell compatibility with Themis 0.9.6 is now disabled by default ([#614](https://github.com/cossacklabs/themis/pull/614)).
Expand Down Expand Up @@ -70,6 +75,8 @@ _Code:_
([#639](https://github.com/cossacklabs/themis/pull/639)).
- Updated embedded BoringSSL to the latest version
([#643](https://github.com/cossacklabs/themis/pull/643)).
- Fixed broken `SecureSession#save` and `SecureSession#restore` methods
([#658](https://github.com/cossacklabs/themis/pull/658).

- **Breaking changes**

Expand Down Expand Up @@ -390,6 +397,8 @@ _Code:_
([#633](https://github.com/cossacklabs/themis/pull/633)).
- Kotlin is now officially supported language for JavaThemis
([#637](https://github.com/cossacklabs/themis/pull/637).
- Fixed broken `SecureSession#save` and `SecureSession#restore` methods
([#658](https://github.com/cossacklabs/themis/pull/658).

- Secure Cell API updates:

Expand Down
41 changes: 33 additions & 8 deletions src/themis/secure_session_serialize.c
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,35 @@

#define THEMIS_SESSION_CONTEXT_TAG "TSSC"

#define SESSION_CTX_SERIZALIZED_SIZE(ctx) \
(sizeof((ctx)->session_id) + sizeof((ctx)->is_client) + sizeof((ctx)->session_master_key) \
+ sizeof((ctx)->out_seq) + sizeof((ctx)->in_seq))
/*
* Data layout of serialized Secure Session state looks like this:
*
* 0 1 2 3 4 5 6 7
* +--------+--------+--------+--------+--------+--------+--------+--------+
* | 'T' | 'S' | 'S' | 'C' | size | Soter Container header
* +--------+--------+--------+--------+--------+--------+--------+--------+
* | CRC |
* +--------+--------+--------+--------+
*
* +--------+--------+--------+--------+--------+--------+--------+--------+
* | session ID | is_client | Secure Session context
* +--------+--------+--------+--------+--------+--------+--------+--------+
* | Secure Session master key |
* + - - - -+ - - - -+ - - - -+ - - - -+ - - - -+ - - - -+ - - - -+ - - - -+
* | |
* + - - - -+ - - - -+ - - - -+ - - - -+ - - - -+ - - - -+ - - - -+ - - - -+
* | |
* + - - - -+ - - - -+ - - - -+ - - - -+ - - - -+ - - - -+ - - - -+ - - - -+
* | |
* +--------+--------+--------+--------+--------+--------+--------+--------+
* | out seqnum | in seqnum |
* +--------+--------+--------+--------+--------+--------+--------+--------+
*
* All values are unsigned and encoded as big-endian.
*/

#define SESSION_CTX_SERIALIZED_SIZE \
(2 * sizeof(uint32_t) + SESSION_MASTER_KEY_LENGTH + 2 * sizeof(uint32_t))

themis_status_t secure_session_save(const secure_session_t* session_ctx, void* out, size_t* out_length)
{
Expand All @@ -45,15 +71,14 @@ themis_status_t secure_session_save(const secure_session_t* session_ctx, void* o

/* | session_id | is_client | master_key | out_seq | in_seq | */

if ((!out)
|| (*out_length < (sizeof(soter_container_hdr_t) + SESSION_CTX_SERIZALIZED_SIZE(session_ctx)))) {
*out_length = (sizeof(soter_container_hdr_t) + SESSION_CTX_SERIZALIZED_SIZE(session_ctx));
if ((!out) || (*out_length < (sizeof(soter_container_hdr_t) + SESSION_CTX_SERIALIZED_SIZE))) {
*out_length = (sizeof(soter_container_hdr_t) + SESSION_CTX_SERIALIZED_SIZE);
return THEMIS_BUFFER_TOO_SMALL;
}

*out_length = (sizeof(soter_container_hdr_t) + SESSION_CTX_SERIZALIZED_SIZE(session_ctx));
*out_length = (sizeof(soter_container_hdr_t) + SESSION_CTX_SERIALIZED_SIZE);

soter_container_set_data_size(hdr, SESSION_CTX_SERIZALIZED_SIZE(session_ctx));
soter_container_set_data_size(hdr, SESSION_CTX_SERIALIZED_SIZE);
memcpy(hdr->tag, THEMIS_SESSION_CONTEXT_TAG, SOTER_CONTAINER_TAG_LENGTH);

curr = (uint32_t*)soter_container_data(hdr);
Expand Down

0 comments on commit e83ec23

Please sign in to comment.