New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

secure cell x32-x64 compatibility fix #279

Merged
merged 6 commits into from Jan 17, 2018

Conversation

Projects
None yet
3 participants
@vixentael
Member

vixentael commented Jan 16, 2018

The problem described in #220 appears to be SecureCell incompatibility between x32 and x64 platforms. SecureCell is working correctly on both platforms, however, ciphertext calculations are platform-dependent.

Where is the problem
Message length and context length are used in encryption and decryption in every mode of SecureCell, and they are defined as size_t types (secure_cell.c#L47, secure_cell.c#L29 and below).

During encryption-decryption process sizeof(message_length) is calculated (sym_enc_message.c#L184) which is equal to sizeof(size_t).

Result of sizeof(size_t) is platform dependent: 4 bytes on x32 platforms, 8 bytes on x64 platforms.

This value is used inside soter_kdf function for calculating hmac.

Therefore, resulting ciphertext uses different amount of bytes on different platforms.

Fix
Real fix is to use platform-independent types, like uint32_t, instead of size_t. uint32_t variables are already used among many data structures themis_auth_sym_message_hdr_type and themis_sym_message_hdr_type

Changing sizeof(message_length) to sizeof(uint32_t) results in using 4 bytes on every platform, making encryption-decryption compatible among x32 and x64.

Compatibility fix
Unfortunately using 4 bytes for x64 leads to unability to decrypt messages encrypted by themis 0.9.6 x64.

Therefore on x64 platform if decrypting fails first time, we try to re-decrypt message using 8 bytes as context length for the second time.

Testing
I made manual integration testing among x32 and x64 Ubuntu 17.04 for themis 0.9.6 and 0.9.7. I encrypt data in every Secure Cell mode, write it to the files, and encrypt on another platform.

Results

  • fully compatible (can be encrypted and decrypted both ways)
    0.9.7 х64 <-> 0.9.7 х32
    0.9.7 х64 <-> 0.9.6 х32
    0.9.7 х32 <-> 0.9.6 х32

  • almost fully compatible
    0.9.7 x64 <-> 0.9.6 x64 (messages encrypted with context imprint mode can't be decrypted on 0.9.7)

  • not compatible
    0.9.6 x64 can't decrypt messages encrypted by 0.9.7 x64/x32. Users should update to 0.9.7.
    0.9.7 x32 can't decrypt messages encrypted by 0.9.6 x64. Users should update to 0.9.7.

@vixentael vixentael self-assigned this Jan 16, 2018

@vixentael vixentael requested review from secumod and Lagovas Jan 16, 2018

@secumod

Generally, I would add a COMPAT compile time switch (enabled on compile time by adding -DCOMPAT and replace hardcoded THEMIS_097_SECURE_CELL_X64_COMPATIBILITY_FIX) with something like

#ifdef COMPAT
/* below will likely be evaluated compile time, so for other cases it will be a no-op */
if (sizeof(size_t) == sizeof(uint64_t) {
  /* the compat code */
}
#endif
// since 0.9.7 Secure Cell context size is equal sizeof(uint32_t) for both platforms,
// causing compatibility issues on x64
// this define makes SecureCell
#define THEMIS_097_SECURE_CELL_X64_COMPATIBILITY_FIX

This comment has been minimized.

@secumod

secumod Jan 16, 2018

Contributor

Why do we hardcode this? I would make this a compile time flag instead.

This comment has been minimized.

@vixentael

vixentael Jan 16, 2018

Member

great idea

@@ -27,7 +27,7 @@
#include <stdint.h>
/** @brief return type */
typedef int soter_status_t;
typedef int32_t soter_status_t;

This comment has been minimized.

@secumod

secumod Jan 16, 2018

Contributor

This is just an error code. If we don't depend on its size anywhere, better leave it as int, that is platform natural type.

This comment has been minimized.

@vixentael

vixentael Jan 16, 2018

Member
  1. to be consistent with
    https://github.com/cossacklabs/themis/blob/master/src/themis/themis_error.h#L31

  2. what if we decide to use large numbers as error codes in future? I think platform independent types are better

This comment has been minimized.

@secumod

secumod Jan 16, 2018

Contributor

The point here not the size, but that the types are "native" to the platform. Therefore the resulting code is better optimised. So, for error codes, when they are not used in any serialisation structures (and they shouldn't be), this is the preferred approach.

This comment has been minimized.

@vixentael

vixentael Jan 16, 2018

Member

wouldn't we have platform inconsistency using int, if someone decided to use 0x7FFFFFFFFFFFFFFF as error code?

This comment has been minimized.

@secumod

secumod Jan 17, 2018

Contributor

Most projects do not have any issues with that - including Linux userspace API :)

@@ -181,7 +181,7 @@ themis_status_t themis_auth_sym_encrypt_message(const uint8_t* key,
size_t* encrypted_message_length){
uint8_t key_[THEMIS_AUTH_SYM_KEY_LENGTH/8];
THEMIS_CHECK_PARAM(message!=NULL && message_length!=0);
THEMIS_STATUS_CHECK(themis_sym_kdf(key,key_length, THEMIS_SYM_KDF_KEY_LABEL, (uint8_t*)(&message_length), sizeof(message_length), in_context, in_context_length, key_, sizeof(key_)),THEMIS_SUCCESS);
THEMIS_STATUS_CHECK(themis_sym_kdf(key,key_length, THEMIS_SYM_KDF_KEY_LABEL, (uint8_t*)(&message_length), sizeof(uint32_t), in_context, in_context_length, key_, sizeof(key_)),THEMIS_SUCCESS);

This comment has been minimized.

@secumod

secumod Jan 16, 2018

Contributor

It is not a good practice to do sizeof(type) at this point. Better declare message_length with the type we want and use sizeof(variable): if we want to update the message_length type in the future again, we would have to remember to update this line as well, otherwise we can get a buffer overflow.

If you use sizeof(var) approach - the code "adjusts itself"

This comment has been minimized.

@vixentael

vixentael Jan 16, 2018

Member

agree with you

unfortunately, we can't easily declare message_length as uint32_t because it requires changing function signatures all over the project (changing params from size_t to uint32_t), which requires updating most of the wrappers. (pile of work, many mistakes :doge:)

from another perspective, current code already uses uint32_t cast in some places like
https://github.com/cossacklabs/themis/blob/master/src/themis/sym_enc_message.c#L166 so I would say that changing message_length type in future is not an easy task anyway

This comment has been minimized.

@secumod

secumod Jan 16, 2018

Contributor

This just creates a potential landmine for the future (and maybe even a security incident). If it is possible, better change it now than suffer the consequences in the future.

This comment has been minimized.

@vixentael

vixentael Jan 16, 2018

Member

I'm on your side totally.

However i'm trying to fix 3yrs old mistake of using sizeof(size_t) without need of re-writing half of Themis core and introducing more mistakes. Believe me, first thing I tried was changing a type of message_length, but I faced tons of changed code, including secure message, secure session, tests, wrappers, everywhere.

Can we define uint32_t somewhere in this file and use define instead of actual type? :)

This comment has been minimized.

@secumod

secumod Jan 16, 2018

Contributor

The point is this is used to calculate buffer sizes, therefore enforces buffer safety. Our primary goal is secure code and this type of fix makes it worse in that regard. Even if it is much more work - it is justified IMO. Otherwise we might be breaking a "security promise" for Themis users.

sizeof(type) in buffer length calculation and using an actual value of a different type is just too typical for a potential security issue we should be making in a security oriented project.

This comment has been minimized.

@vixentael

vixentael Jan 17, 2018

Member

Thank you for your comments! You're absolutely right in terms of security and 'best practices' perspective.

We will update Themis core to use uint32_t or even uint64_t for message calculations when we will have automated integration testing between platforms, wrappers and core versions. Until then we cannot guarantee that significant code changes won't break core functionality.

Unfortunately the code already has force casts like this

const size_t message_length
...
hdr->message_length=(uint32_t)message_length;

I don't see how adding

size_t* encrypted_message_length
...
sizeof((uint32_t)(encrypted_message_length))

is better than sizeof(uint32_t).

Taking into account limited resources and severity of the problem we're trying to solve, I believe that this temporary solution works before proper refactoring. We already working on integration test suit.

Can you please explain how using sizeof(uint32_t) decreases current security guarantees, except the point that we will forget to change this code during future refactoring.

This comment has been minimized.

@secumod

secumod Jan 17, 2018

Contributor

Unfortunately the code already has force casts like this

That troubles me as well: casting up is fine, casting down usually creates issues (unless you're sure they are not)

Can you please explain how using sizeof(uint32_t) decreases current security guarantees

The point is in "forgetting to change" - for example you have code which serialises secure cell structure and you usually do it by iterating each variable usually, something like

uint8_t *p = malloc(hdr + len);
memcpy(p, &hdr);
p += sizeof(hdr);
memcpy(p, &len);
p += sizeof(len);
...

there are two potential problems here if you get incorrect size forgetting to update sizeof(len) with proper type:

  • if you update it in serialisation code, but not in the malloc code - it will allocate smaller buffer and you'll write data past the buffer boundary
  • even if you update it in both places, but actual length is 64 bit, but we just use sizeof(uint32_t) as a storage buffer (cast down) - this code will break on big-endian systems (which are still common nowadays), because it will write zeroes at best and garbage at worst

Now these type of errors are very easy to make, they are very hard to spot during code review, they will likely not be caught by automatic test suite.

This comment has been minimized.

@vixentael

vixentael Jan 17, 2018

Member

Thank you, got it!

How about next plan:

  • merge fix as is;
  • document these problems (down-casting and sizeof(uint32_t)) as places of potential risks (add "warning-note-for-future-us" inside code);
  • check code with static analyzers (afaik we are doing this manually time to time, not sure whether analyzers trigger warnings about down-casting);
  • finish automated between-platforms integration tests (we are working on this right now, any ideas how to test on big-endian systems?);
  • create refactoring plan, paying attention on risky places, change size_t variable to uint32_t (uint64_t? whatever we decide is better);
  • refactor core and wrappers, keeping an eye on compatibility.

I believe that your suggestion #270 to update C code style will help us a lot!

What do you think?

This comment has been minimized.

@secumod

secumod Jan 17, 2018

Contributor

Probably adding a TODO in the comment above these "tricky lines" will suffice for now.

This comment has been minimized.

@vixentael
if (decryption_result != THEMIS_SUCCESS && decryption_result != THEMIS_BUFFER_TOO_SMALL) {
// we are on x64, should sizeof(uin64_t) for backwards compatibility with themis 0.9.6 x64
if (sizeof(size_t) == sizeof(uint64_t)) {

This comment has been minimized.

@secumod

secumod Jan 16, 2018

Contributor

This is resolved on compile time, so can be converted to MACROS and added to THEMIS_097_SECURE_CELL_X64_COMPATIBILITY_FIX above

This comment has been minimized.

@vixentael

vixentael Jan 16, 2018

Member

you mean, the MACROS code may look smth like

#ifdef COMPAT

  #define THEMIS_SCELL_COMPAT(func){ \
    /* below will likely be evaluated compile time, so for other cases it will be a no-op */
    if (sizeof(size_t) == sizeof(uint64_t) { \
     // compatibility code
      {func;} \
    } \
  }

#else
  #define THEMIS_SCELL_COMPAT(func) {}
#endif
}

usage code

  if (decryption_result != THEMIS_SUCCESS && decryption_result != THEMIS_BUFFER_TOO_SMALL) {

    THEMIS_SCELL_COMPAT({
        THEMIS_STATUS_CHECK(themis_sym_kdf(key,key_length, THEMIS_SYM_KDF_KEY_LABEL, (uint8_t*)(&encrypted_message_length), sizeof(uint64_t), NULL, 0, key_, sizeof(key_)),THEMIS_SUCCESS);
        decryption_result = themis_sym_decrypt_message_u_(key_,sizeof(key_),context,context_length,encrypted_message,encrypted_message_length,message,message_length);
    });
  }

This comment has been minimized.

@secumod

secumod Jan 16, 2018

Contributor

Well I don't think we actually need #define THEMIS_SCELL_COMPAT(func) at all, just

#ifdef COMPAT
if (sizeof(size_t) == sizeof(uint64_t) { ...
}
#endif

the code will be empty if either COMPAT is not defined, or compiler evaluates sizeof(size_t) == sizeof(uint64_t) to false.

@@ -262,7 +277,7 @@ themis_status_t themis_sym_encrypt_message_u(const uint8_t* key,
uint8_t* encrypted_message,
size_t* encrypted_message_length){
uint8_t key_[THEMIS_SYM_KEY_LENGTH/8];
THEMIS_STATUS_CHECK(themis_sym_kdf(key,key_length, THEMIS_SYM_KDF_KEY_LABEL, (uint8_t*)(&message_length), sizeof(message_length), NULL, 0, key_, sizeof(key_)),THEMIS_SUCCESS);
THEMIS_STATUS_CHECK(themis_sym_kdf(key,key_length, THEMIS_SYM_KDF_KEY_LABEL, (uint8_t*)(&message_length), sizeof(uint32_t), NULL, 0, key_, sizeof(key_)),THEMIS_SUCCESS);

This comment has been minimized.

@secumod

secumod Jan 16, 2018

Contributor

ditto

if (decryption_result != THEMIS_SUCCESS && decryption_result != THEMIS_BUFFER_TOO_SMALL) {
// we are on x64, should sizeof(uin64_t) for backwards compatibility with themis 0.9.6 x64
if (sizeof(size_t) == sizeof(uint64_t)) {

This comment has been minimized.

@secumod

secumod Jan 16, 2018

Contributor

ditto adding to macros

@mobrio

This comment has been minimized.

mobrio commented Jan 16, 2018

I entirely appreciate the desire for a safe and minimal intervention and the fine detail of that is not my competence.

However, my understanding (which may be entirely wrong) from #220 is that x32 primarily (if not solely) occurs for the Android emulator. Similarly I would expect x64 to be the majority case in server or indeed desktop contexts.

So some thoughts/questions:

Should we not then consider x32 to be the exception that is the focus for compatibility fixing and x64 the norm?

For example does constraining x64 platforms to use 4 bytes via sizeof(uint32_t) rather than 8 result in any performance loss?

Similarly if there is a case when our compatibility strategy is to "fail and retry with a different size" would it not be better for that performance hit to occur in a x32 emulator rather than x64 real world devices?

Also, as I recall it the C preprocessor knows all about stuff like SIZEOF_INT so perhaps there's the opportunity for natural adaption rather than some explicit -DCOMPAT switch?

@vixentael

This comment has been minimized.

Member

vixentael commented Jan 17, 2018

@mobrio, actually saying, x64 code should be considered as "broken" implementation, and x32 as "correct".

The encryption-decryption code uses uint32_t structures mostly, except that variables defined as size_t. On x32 platforms sizeof(size_t) equals sizeof(uint32_t) equals 4 bytes. All calculations are respective and correct. On x64 sizeof(size_t) == sizeof(uint64_t) == 8 bytes, making the code a mix of uint_32t structures and 8-bytes long buffers :(

Android emulator was just the trigger to find this issue. It affects any x32<->x64 platforms. We believe that most users are using x64 "broken" themis version. That's why adding "fail and retry with a different size" for x64 is important: we should allows users to decrypt their data, previously encrypted by "broken" code. The "double decryption" strategy should be enabled by default, allowing users to disable it manually.

Changing data types for uint64_t, removing explicit sizeof(uint32_t) and "double decryption" strategy, is our "next step" update. However, in my opinion, we should release quick fix, set up automated integration tests (who knows how many similar issues are still undetected), detect those issues, understand the problem landscape better, and perform proper refactoring only after that.

@vixentael

This comment has been minimized.

Member

vixentael commented Jan 17, 2018

In latest commit I've changed code to

#ifdef SCELL_COMPAT

  // we are on x64, should sizeof(uin64_t) for backwards compatibility with themis 0.9.6 x64
  if (sizeof(size_t) == sizeof(uint64_t)) {
    if (decryption_result != THEMIS_SUCCESS && decryption_result != THEMIS_BUFFER_TOO_SMALL) {
        THEMIS_STATUS_CHECK(themis_sym_kdf(key,key_length, THEMIS_SYM_KDF_KEY_LABEL, (uint8_t*)(&encrypted_message_length), sizeof(uint64_t), NULL, 0, key_, sizeof(key_)),THEMIS_SUCCESS);
        decryption_result = themis_sym_decrypt_message_u_(key_,sizeof(key_),context,context_length,encrypted_message,encrypted_message_length,message,message_length);
    }
  }
#endif

and enable -DSCELL_COMPAT by default in Makefile.

To disable compatibility fix, users should pass NO_SCELL_COMPAT parameter into make, like this

make NO_SCELL_COMPAT=y all install

vixentael added some commits Jan 17, 2018

@vixentael

This comment has been minimized.

Member

vixentael commented Jan 17, 2018

Added warning lines.
Created refactoring issue and describe plan there.
Waiting for tests to complete and merge this issue.

Thank you a lot for this discussion, it was very useful and strategic-minded!

@vixentael vixentael merged commit dd758bd into master Jan 17, 2018

3 checks passed

ci/bitrise/b32b4ea8bffedad7/pr Passed - themis
Details
ci/bitrise/b32b4ea8bffedad7/push Passed - themis
Details
ci/circleci Your tests passed on CircleCI!
Details

@vixentael vixentael deleted the vixentael/new_x32_fix branch Jan 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment