-
Notifications
You must be signed in to change notification settings - Fork 143
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
Test and deprecate Themis 0.9.6 compatibility path #614
Merged
Merged
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
7b500aa
Test for Themis 0.9.6 compatibility
ilammy 459b9e7
Cleanup Context Imprint compatibility code path
ilammy 5adcf33
Cleanup Context Imprint IV computation
ilammy b31cf33
Drop unused typedef themis_sym_message_hdr_t
ilammy 8a1781f
Remove SCELL_COMPAT define and ignore NO_SCELL_COMPAT variable
ilammy dfa33a5
Use compatibility path on 32-bit machines too
ilammy 545e782
Revert "Remove SCELL_COMPAT define and ignore NO_SCELL_COMPAT variable"
ilammy a6de319
Disable SCELL_COMPAT by default
ilammy 9676fda
Describe migration strategy in changelog
ilammy 39a4a1a
Test build WITH_SCELL_COMPAT on CircleCI
ilammy 0846314
Test build WITH_SCELL_COMPAT on GitHub Actions
ilammy ab34fa2
Move "error" label inside SCELL_COMPAT ifdef
ilammy 5631124
Merge branch 'master' into always-compat
ilammy 78ffd13
Merge branch 'master' into always-compat
ilammy 7932990
Reword changelog for clarity
ilammy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -198,7 +198,6 @@ static themis_status_t themis_auth_sym_kdf_context(uint32_t message_length, | |
return THEMIS_SUCCESS; | ||
} | ||
|
||
#ifdef SCELL_COMPAT | ||
/* | ||
* Themis 0.9.6 incorrectly used 64-bit message length for this field. | ||
*/ | ||
|
@@ -214,7 +213,6 @@ static themis_status_t themis_auth_sym_kdf_context_compat(uint32_t message_lengt | |
*kdf_context_length = sizeof(uint64_t); | ||
return THEMIS_SUCCESS; | ||
} | ||
#endif | ||
|
||
static themis_status_t themis_auth_sym_derive_encryption_key(const struct themis_scell_auth_token_key* hdr, | ||
const uint8_t* key, | ||
|
@@ -458,8 +456,7 @@ themis_status_t themis_auth_sym_decrypt_message_(const uint8_t* key, | |
* Themis 0.9.6 used slightly different KDF. If decryption fails, | ||
* maybe it was encrypted with that incorrect key. Try it out. | ||
*/ | ||
#ifdef SCELL_COMPAT | ||
if (res != THEMIS_SUCCESS && res != THEMIS_BUFFER_TOO_SMALL && sizeof(size_t) == sizeof(uint64_t)) { | ||
if (res != THEMIS_SUCCESS && res != THEMIS_BUFFER_TOO_SMALL) { | ||
kdf_context_length = sizeof(kdf_context); | ||
res = themis_auth_sym_kdf_context_compat(hdr.message_length, kdf_context, &kdf_context_length); | ||
if (res != THEMIS_SUCCESS) { | ||
|
@@ -491,7 +488,6 @@ themis_status_t themis_auth_sym_decrypt_message_(const uint8_t* key, | |
hdr.auth_tag, | ||
hdr.auth_tag_length); | ||
} | ||
#endif | ||
|
||
/* Sanity check of resulting message length */ | ||
if (*message_length != encrypted_message_length) { | ||
|
@@ -551,11 +547,66 @@ themis_status_t themis_auth_sym_decrypt_message(const uint8_t* key, | |
message_length); | ||
} | ||
|
||
typedef struct themis_sym_message_hdr_type { | ||
uint32_t alg; | ||
uint32_t iv_length; | ||
uint32_t message_length; | ||
} themis_sym_message_hdr_t; | ||
static themis_status_t themis_sym_derive_encryption_key(const uint8_t* key, | ||
size_t key_length, | ||
size_t message_length, | ||
uint8_t* derived_key, | ||
size_t derived_key_length) | ||
{ | ||
uint8_t kdf_context[sizeof(uint32_t)]; | ||
/* | ||
* Note that we use only the least significant 32 bits of size_t here. | ||
* If message_length is longer that 4 GB then the context value gets | ||
* truncated here. (And on 16-bit platforms it will be zero-padded.) | ||
*/ | ||
stream_write_uint32LE(kdf_context, (uint32_t)message_length); | ||
return themis_sym_kdf(key, | ||
key_length, | ||
THEMIS_SYM_KDF_KEY_LABEL, | ||
kdf_context, | ||
sizeof(kdf_context), | ||
NULL, | ||
0, | ||
derived_key, | ||
derived_key_length); | ||
} | ||
|
||
/* | ||
* Themis 0.9.6 used 64-bit message length in computations, so here we go. | ||
*/ | ||
static themis_status_t themis_sym_derive_encryption_key_compat(const uint8_t* key, | ||
size_t key_length, | ||
size_t message_length, | ||
uint8_t* derived_key, | ||
size_t derived_key_length) | ||
{ | ||
uint8_t kdf_context[sizeof(uint64_t)]; | ||
stream_write_uint64LE(kdf_context, (uint64_t)message_length); | ||
return themis_sym_kdf(key, | ||
key_length, | ||
THEMIS_SYM_KDF_KEY_LABEL, | ||
kdf_context, | ||
sizeof(kdf_context), | ||
NULL, | ||
0, | ||
derived_key, | ||
derived_key_length); | ||
} | ||
|
||
static themis_status_t themis_sym_derive_encryption_iv(const uint8_t* key, | ||
size_t key_length, | ||
const uint8_t* context, | ||
size_t context_length, | ||
uint8_t* iv, | ||
size_t iv_length) | ||
{ | ||
/* | ||
* Yes, you are reading it correct. We do derive IV with a KDF. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice comment |
||
* Context Imprint mode needs to be completely deterministic. | ||
* That's why it has lower security than Seal or Token Protect. | ||
*/ | ||
return themis_sym_kdf(key, key_length, THEMIS_SYM_KDF_IV_LABEL, context, context_length, NULL, 0, iv, iv_length); | ||
} | ||
|
||
themis_status_t themis_sym_encrypt_message_u_(const uint8_t* key, | ||
const size_t key_length, | ||
|
@@ -566,35 +617,34 @@ themis_status_t themis_sym_encrypt_message_u_(const uint8_t* key, | |
uint8_t* encrypted_message, | ||
size_t* encrypted_message_length) | ||
{ | ||
themis_status_t res = THEMIS_FAIL; | ||
uint8_t iv[THEMIS_SYM_IV_LENGTH]; | ||
|
||
THEMIS_CHECK_PARAM(message != NULL && message_length != 0); | ||
THEMIS_CHECK_PARAM(context != NULL && context_length != 0); | ||
|
||
if ((*encrypted_message_length) < message_length) { | ||
(*encrypted_message_length) = message_length; | ||
return THEMIS_BUFFER_TOO_SMALL; | ||
} | ||
(*encrypted_message_length) = message_length; | ||
uint8_t iv[THEMIS_SYM_IV_LENGTH]; | ||
THEMIS_STATUS_CHECK(themis_sym_kdf(key, | ||
key_length, | ||
THEMIS_SYM_KDF_IV_LABEL, | ||
context, | ||
context_length, | ||
NULL, | ||
0, | ||
iv, | ||
THEMIS_SYM_IV_LENGTH), | ||
THEMIS_SUCCESS); | ||
THEMIS_STATUS_CHECK(themis_sym_plain_encrypt(THEMIS_SYM_ALG, | ||
key, | ||
key_length, | ||
iv, | ||
THEMIS_SYM_IV_LENGTH, | ||
message, | ||
message_length, | ||
encrypted_message, | ||
encrypted_message_length), | ||
THEMIS_SUCCESS); | ||
return THEMIS_SUCCESS; | ||
|
||
res = themis_sym_derive_encryption_iv(key, key_length, context, context_length, iv, sizeof(iv)); | ||
if (res != THEMIS_SUCCESS) { | ||
return res; | ||
} | ||
|
||
res = themis_sym_plain_encrypt(THEMIS_SYM_ALG, | ||
key, | ||
key_length, | ||
iv, | ||
sizeof(iv), | ||
message, | ||
message_length, | ||
encrypted_message, | ||
encrypted_message_length); | ||
soter_wipe(iv, sizeof(iv)); | ||
return res; | ||
} | ||
|
||
themis_status_t themis_sym_encrypt_message_u(const uint8_t* key, | ||
|
@@ -606,28 +656,26 @@ 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]; | ||
|
||
// TODO: TYPE WARNING Should update `sizeof(uint32_t)` to `sizeof(message_length)` after | ||
// changing encrypted_message_length type to uint32_t | ||
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); | ||
return themis_sym_encrypt_message_u_(key_, | ||
sizeof(key_), | ||
message, | ||
message_length, | ||
context, | ||
context_length, | ||
encrypted_message, | ||
encrypted_message_length); | ||
themis_status_t res = THEMIS_FAIL; | ||
uint8_t derived_key[THEMIS_SYM_KEY_LENGTH / 8]; | ||
|
||
res = themis_sym_derive_encryption_key(key, key_length, message_length, derived_key, sizeof(derived_key)); | ||
if (res != THEMIS_SUCCESS) { | ||
return res; | ||
} | ||
|
||
res = themis_sym_encrypt_message_u_(derived_key, | ||
sizeof(derived_key), | ||
message, | ||
message_length, | ||
context, | ||
context_length, | ||
encrypted_message, | ||
encrypted_message_length); | ||
|
||
soter_wipe(derived_key, sizeof(derived_key)); | ||
|
||
return res; | ||
} | ||
|
||
themis_status_t themis_sym_decrypt_message_u_(const uint8_t* key, | ||
|
@@ -639,35 +687,34 @@ themis_status_t themis_sym_decrypt_message_u_(const uint8_t* key, | |
uint8_t* message, | ||
size_t* message_length) | ||
{ | ||
themis_status_t res = THEMIS_FAIL; | ||
uint8_t iv[THEMIS_SYM_IV_LENGTH]; | ||
|
||
THEMIS_CHECK_PARAM(encrypted_message != NULL && encrypted_message_length != 0); | ||
THEMIS_CHECK_PARAM(context != NULL && context_length != 0); | ||
|
||
if ((*message_length) < encrypted_message_length) { | ||
(*message_length) = encrypted_message_length; | ||
return THEMIS_BUFFER_TOO_SMALL; | ||
} | ||
(*message_length) = encrypted_message_length; | ||
uint8_t iv[THEMIS_SYM_IV_LENGTH]; | ||
THEMIS_STATUS_CHECK(themis_sym_kdf(key, | ||
key_length, | ||
THEMIS_SYM_KDF_IV_LABEL, | ||
context, | ||
context_length, | ||
NULL, | ||
0, | ||
iv, | ||
THEMIS_SYM_IV_LENGTH), | ||
THEMIS_SUCCESS); | ||
THEMIS_STATUS_CHECK(themis_sym_plain_decrypt(THEMIS_SYM_ALG, | ||
key, | ||
key_length, | ||
iv, | ||
THEMIS_SYM_IV_LENGTH, | ||
encrypted_message, | ||
encrypted_message_length, | ||
message, | ||
message_length), | ||
THEMIS_SUCCESS); | ||
return THEMIS_SUCCESS; | ||
|
||
res = themis_sym_derive_encryption_iv(key, key_length, context, context_length, iv, sizeof(iv)); | ||
if (res != THEMIS_SUCCESS) { | ||
return res; | ||
} | ||
|
||
res = themis_sym_plain_decrypt(THEMIS_SYM_ALG, | ||
key, | ||
key_length, | ||
iv, | ||
sizeof(iv), | ||
encrypted_message, | ||
encrypted_message_length, | ||
message, | ||
message_length); | ||
soter_wipe(iv, sizeof(iv)); | ||
return res; | ||
} | ||
|
||
themis_status_t themis_sym_decrypt_message_u(const uint8_t* key, | ||
|
@@ -679,56 +726,51 @@ themis_status_t themis_sym_decrypt_message_u(const uint8_t* key, | |
uint8_t* message, | ||
size_t* message_length) | ||
{ | ||
uint8_t key_[THEMIS_SYM_KEY_LENGTH / 8]; | ||
|
||
// TODO: TYPE WARNING Should update `sizeof(uint32_t)` to `sizeof(encrypted_message_length)` | ||
// after changing encrypted_message_length type to uint32_t | ||
THEMIS_STATUS_CHECK(themis_sym_kdf(key, | ||
key_length, | ||
THEMIS_SYM_KDF_KEY_LABEL, | ||
(uint8_t*)(&encrypted_message_length), | ||
sizeof(uint32_t), | ||
NULL, | ||
0, | ||
key_, | ||
sizeof(key_)), | ||
THEMIS_SUCCESS); | ||
themis_status_t decryption_result = themis_sym_decrypt_message_u_(key_, | ||
sizeof(key_), | ||
context, | ||
context_length, | ||
encrypted_message, | ||
encrypted_message_length, | ||
message, | ||
message_length); | ||
|
||
#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) { | ||
// TODO: TYPE WARNING `sizeof(uint64_t)`. Fix that on next versions | ||
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); | ||
themis_status_t res = THEMIS_FAIL; | ||
uint8_t derived_key[THEMIS_SYM_KEY_LENGTH / 8]; | ||
|
||
res = themis_sym_derive_encryption_key(key, | ||
key_length, | ||
encrypted_message_length, | ||
derived_key, | ||
sizeof(derived_key)); | ||
if (res != THEMIS_SUCCESS) { | ||
return res; | ||
} | ||
|
||
res = themis_sym_decrypt_message_u_(derived_key, | ||
sizeof(derived_key), | ||
context, | ||
context_length, | ||
encrypted_message, | ||
encrypted_message_length, | ||
message, | ||
message_length); | ||
/* | ||
* Themis 0.9.6 used slightly different KDF. If decryption fails, | ||
* maybe it was encrypted with that incorrect key. Try it out. | ||
*/ | ||
if (res != THEMIS_SUCCESS && res != THEMIS_BUFFER_TOO_SMALL) { | ||
res = themis_sym_derive_encryption_key_compat(key, | ||
key_length, | ||
encrypted_message_length, | ||
derived_key, | ||
sizeof(derived_key)); | ||
if (res != THEMIS_SUCCESS) { | ||
goto error; | ||
} | ||
res = themis_sym_decrypt_message_u_(derived_key, | ||
sizeof(derived_key), | ||
context, | ||
context_length, | ||
encrypted_message, | ||
encrypted_message_length, | ||
message, | ||
message_length); | ||
} | ||
#endif | ||
|
||
return decryption_result; | ||
error: | ||
soter_wipe(derived_key, sizeof(derived_key)); | ||
|
||
return res; | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We used sizeof check to minimize scope and do extra step only when needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but it also disables the compatibility path for 32-bit machines. They will not be able to decrypt data produced earlier by 64-bit machines. We have quite a few 32-bit platforms that we care about: ARM, WebAssembly, simulators/emulators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR 279 has compatibility matrix.
These are the only incompatible pathes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am really suspicious of historical statements like this, based on memory and old writing with no data. So I tried reproducing the results.
“I’m gonna science the heck outta this!”
Works only on 64-bit Linux with 32-bit libraries. On Debian they can be installed like this:
Does not really work on macOS because of multiple reasons. Older versions of Themis are not as good at locating modern OpenSSL on macOS, and modern macOS does not include proper 32-bit support for OpenSSL.
Check out Themis git repo.
Put this as
scell_seal_string_echo.c
in the root:whatever.sh
in the root:Parse results:
Where 0.10.0+ is the version with workaround enabled (the default one) and 0.10.0– is the one with
NO_SCELL_COMPAT=1
.So the statement seems to be correct.
Note how 0.9.6 (64-bit) fails to interoperate with anything but itself, and produces data that cannot be decrypted without the compatibility code.
But also note how removal of sizeof comparison in this PR makes it possible for 32-bit platforms to decrypt data produced by 0.9.6 (64-bit).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are freak :D
Agree. I used it to limit the double decryption surface 🤔