Test and deprecate Themis 0.9.6 compatibility path

[ilammy]

I've been meaning to do this for some time. Now that other parts of Secure Cell has been updated, I guess it's due.

Test for Themis 0.9.6 compatibility

One historical version of Themis has been using incorrect KDF which produced data that cannot be decrypted with the correct function. In order to work around this issue, the code contains a fallback path which tries the incorrect compatibility KDF if decryption fails with the derived key produced by the correct KDF. See #279 for the background on this story.

Unfortunately, this code path is not tested whatsoever and this is not good. Add tests that verify that Themis is able decrypt data produced by buggy Themis 0.9.6.

^{P.S. clang-format surely has some weirdly twisted idea about how byte arrays should be formatted *sigh* whatever...}

Cleanup Context Imprint compatibility code path

Extract key derivation into functions like this is done for Seal and Token Protect mode. This makes the code more readable and highlights the compatibility issue that we are dealing with.

Update the usage sites too, remove magic macros, make sure that the derived keys are wiped after processing.

Cleanup Context Imprint IV computation

It's really not obvious why Context Imprint's IV computation is performed with a key derivation function. At least extract this bit into a separate function that can be commented.

Cleanup the usage sites, replace macros with plain C code, make sure that IV is wiped after processing since it is derived from the key.

Drop unused typedef themis_sym_message_hdr_t

It has never been used since Context Imprint mode does not produce any authentication token that needs a header.

Remove SCELL_COMPAT define and ignore NO_SCELL_COMPAT variable

UPD: We decided to keep the compatibility code for now, but have it disabled by default instead of current approach of keeping it enabled. Now, WITH_SCELL_COMPAT variable has to be used to enable the compatiblity.

It does not really make much sense for the users to not enable the compatibility code path that allows to decrypt historical data. In fact, not a single build that we distribute disables this code path.

Remove all SCELL_COMPAT ifdefs and ignore NO_SCELL_COMPAT setting, making the compatibility always enabled.

This has a side effect of slightly slowing down decryption of really corrupted messages since we will be always trying the fallback path. However, this is not that significant loss as originally envisioned, and normal usage of Themis should not be affected.

Use compatibility path on 32-bit machines too

First of all, 64-bit machines are more common so this code path will be used in almost all cases. Second, 32-bit machines may need to deal with data encrypted by Themis 0.9.6 on 64-bit machines, so disabling it for 32-bit machines does not make much sense.

Checklist

• Change is covered by automated tests
• Benchmark results are attached (too lazy to benchmark failure path)
• The coding guidelines are followed
• Changelog is updated

[vixentael]

I’m kinda happy that we removing old workarounds, at the same time i’m concerned that with this PR, we can get significant slow down when lib tries to decrypt data.

We used SCELL_COMPAT flag and x64 checks to be able to limit slow down on platforms where developers are sure that they didn’t used 0.9.6, thus don’t need compatibility mode.

[vixentael]

We used sizeof check to minimize scope and do extra step only when needed.

[ilammy]

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.

[vixentael]

PR 279 has compatibility matrix.

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.

These are the only incompatible pathes.

[ilammy]

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:

sudo apt install gcc-multilib libssl-dev:amd64 libssl-dev:i386

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.

1. Check out Themis git repo.

2. Put this as scell_seal_string_echo.c in the root:

#include <stdio.h>
#include <string.h>
#include <stdint.h>

#include <themis/themis.h>

static int do_it(const char *command,
                 const uint8_t *key, size_t key_length,
                 const uint8_t *context, size_t context_length,
                 const uint8_t *message, size_t message_length)
{
    themis_status_t res = THEMIS_FAIL;
    uint8_t *output = NULL;
    size_t output_length = 0;

    if (strcmp(command, "enc") == 0) {
        res = themis_secure_cell_encrypt_seal(key,
                                              key_length,
                                              context,
                                              context_length,
                                              message,
                                              message_length,
                                              NULL,
                                              &output_length);
        if (res != THEMIS_BUFFER_TOO_SMALL) {
            fprintf(stderr, "failed to measure output size: %d\n", res);
            return 1;
        }

        output = malloc(output_length);
        if (!output) {
            fprintf(stderr, "cannot allocate %zu bytes\n", output_length);
            return 1;
        }

        res = themis_secure_cell_encrypt_seal(key,
                                              key_length,
                                              context,
                                              context_length,
                                              message,
                                              message_length,
                                              output,
                                              &output_length);
        if (res != THEMIS_SUCCESS) {
            fprintf(stderr, "failed to encrypt: %d\n", res);
            return 1;
        }
    } else if (strcmp(command, "dec") == 0) {
        res = themis_secure_cell_decrypt_seal(key,
                                              key_length,
                                              context,
                                              context_length,
                                              message,
                                              message_length,
                                              NULL,
                                              &output_length);
        if (res != THEMIS_BUFFER_TOO_SMALL) {
            fprintf(stderr, "failed to measure output size: %d\n", res);
            return 1;
        }

        output = malloc(output_length);
        if (!output) {
            fprintf(stderr, "cannot allocate %zu bytes\n", output_length);
            return 1;
        }

        res = themis_secure_cell_decrypt_seal(key,
                                              key_length,
                                              context,
                                              context_length,
                                              message,
                                              message_length,
                                              output,
                                              &output_length);
        if (res != THEMIS_SUCCESS) {
            fprintf(stderr, "failed to encrypt: %d\n", res);
            return 1;
        }
    } else {
        fprintf(stderr, "unknown command: \"%s\", expected \"enc\" or \"dec\"\n",
                command);
        return 1;
    }

    fwrite(output, 1, output_length, stdout);
    free(output);

    return 0;
}

#define MAX_MESSAGE 4096

int main(int argc, char *argv[])
{
    uint8_t message[MAX_MESSAGE] = {0};
    size_t message_length = 0;

    if (argc != 4) {
        fprintf(stderr, "usage:\n\t%s {enc|dec} <key> <context>\n",
                argv[0]);
        return 1;
    }

    message_length = fread(message, 1, sizeof(message), stdin);

    return do_it(argv[1],
                 (const uint8_t*)argv[2], strlen(argv[2]),
                 (const uint8_t*)argv[3], strlen(argv[3]),
                 message, message_length);
}

3. Put this as whatever.sh in the root:

#!/bin/sh

set -e

build_themis() {
    local bin_name=$1
    local version=$2
    local arch=$3
    local extra=$4

    echo "Building tool with Themis $version $arch $extra..."
    git checkout --detach $version
    make clean
    CFLAGS="$arch" make themis_static soter_static $extra
    cc -o $bin_name $arch scell_seal_string_echo.c -Isrc \
        build/libthemis.a build/libsoter.a -lcrypto
    echo "Complete: $bin_name"
    echo
}

build_binaries() {
    build_themis scell_test_0.9.6_i386       0.9.6  -m32
    build_themis scell_test_0.9.6_x86_64     0.9.6  -m64
    build_themis scell_test_0.10.0_i386      0.10.0 -m32
    build_themis scell_test_0.10.0_x86_64    0.10.0 -m64
    build_themis scell_test_0.10.0_i386_nc   0.10.0 -m32 NO_SCELL_COMPAT=1
    build_themis scell_test_0.10.0_x86_64_nc 0.10.0 -m64 NO_SCELL_COMPAT=1
    build_themis scell_test_THIS-PR_i386     always-compat -m32
    build_themis scell_test_THIS-PR_x86_64   always-compat -m64
}

test_binaries() {
    for encrypt_with in scell_test_*
    do
        for decrypt_with in scell_test_*
        do
            echo -n "$encrypt_with,$decrypt_with,"
            echo "ok" | ./$encrypt_with enc "key" "context" | ./$decrypt_with dec "key" "context" || true
        done
    done
}

build_binaries
test_binaries

4. Make it executable and run:

chmod +x whatever.sh
./whatever.sh

Parse results:

[ build output skipped ]

scell_test_0.10.0_i386,scell_test_0.10.0_i386,ok
scell_test_0.10.0_i386,scell_test_0.10.0_i386_nc,ok
scell_test_0.10.0_i386,scell_test_0.10.0_x86_64,ok
scell_test_0.10.0_i386,scell_test_0.10.0_x86_64_nc,ok
scell_test_0.10.0_i386,scell_test_0.9.6_i386,ok
scell_test_0.10.0_i386,scell_test_0.9.6_x86_64,failed to encrypt: 11
scell_test_0.10.0_i386,scell_test_THIS-PR_i386,ok
scell_test_0.10.0_i386,scell_test_THIS-PR_x86_64,ok
scell_test_0.10.0_i386_nc,scell_test_0.10.0_i386,ok
scell_test_0.10.0_i386_nc,scell_test_0.10.0_i386_nc,ok
scell_test_0.10.0_i386_nc,scell_test_0.10.0_x86_64,ok
scell_test_0.10.0_i386_nc,scell_test_0.10.0_x86_64_nc,ok
scell_test_0.10.0_i386_nc,scell_test_0.9.6_i386,ok
scell_test_0.10.0_i386_nc,scell_test_0.9.6_x86_64,failed to encrypt: 11
scell_test_0.10.0_i386_nc,scell_test_THIS-PR_i386,ok
scell_test_0.10.0_i386_nc,scell_test_THIS-PR_x86_64,ok
scell_test_0.10.0_x86_64,scell_test_0.10.0_i386,ok
scell_test_0.10.0_x86_64,scell_test_0.10.0_i386_nc,ok
scell_test_0.10.0_x86_64,scell_test_0.10.0_x86_64,ok
scell_test_0.10.0_x86_64,scell_test_0.10.0_x86_64_nc,ok
scell_test_0.10.0_x86_64,scell_test_0.9.6_i386,ok
scell_test_0.10.0_x86_64,scell_test_0.9.6_x86_64,failed to encrypt: 11
scell_test_0.10.0_x86_64,scell_test_THIS-PR_i386,ok
scell_test_0.10.0_x86_64,scell_test_THIS-PR_x86_64,ok
scell_test_0.10.0_x86_64_nc,scell_test_0.10.0_i386,ok
scell_test_0.10.0_x86_64_nc,scell_test_0.10.0_i386_nc,ok
scell_test_0.10.0_x86_64_nc,scell_test_0.10.0_x86_64,ok
scell_test_0.10.0_x86_64_nc,scell_test_0.10.0_x86_64_nc,ok
scell_test_0.10.0_x86_64_nc,scell_test_0.9.6_i386,ok
scell_test_0.10.0_x86_64_nc,scell_test_0.9.6_x86_64,failed to encrypt: 11
scell_test_0.10.0_x86_64_nc,scell_test_THIS-PR_i386,ok
scell_test_0.10.0_x86_64_nc,scell_test_THIS-PR_x86_64,ok
scell_test_0.9.6_i386,scell_test_0.10.0_i386,ok
scell_test_0.9.6_i386,scell_test_0.10.0_i386_nc,ok
scell_test_0.9.6_i386,scell_test_0.10.0_x86_64,ok
scell_test_0.9.6_i386,scell_test_0.10.0_x86_64_nc,ok
scell_test_0.9.6_i386,scell_test_0.9.6_i386,ok
scell_test_0.9.6_i386,scell_test_0.9.6_x86_64,failed to encrypt: 11
scell_test_0.9.6_i386,scell_test_THIS-PR_i386,ok
scell_test_0.9.6_i386,scell_test_THIS-PR_x86_64,ok
scell_test_0.9.6_x86_64,scell_test_0.10.0_i386,failed to encrypt: 11
scell_test_0.9.6_x86_64,scell_test_0.10.0_i386_nc,failed to encrypt: 11
scell_test_0.9.6_x86_64,scell_test_0.10.0_x86_64,ok
scell_test_0.9.6_x86_64,scell_test_0.10.0_x86_64_nc,failed to encrypt: 11
scell_test_0.9.6_x86_64,scell_test_0.9.6_i386,failed to encrypt: 11
scell_test_0.9.6_x86_64,scell_test_0.9.6_x86_64,ok
scell_test_0.9.6_x86_64,scell_test_THIS-PR_i386,ok
scell_test_0.9.6_x86_64,scell_test_THIS-PR_x86_64,ok
scell_test_THIS-PR_i386,scell_test_0.10.0_i386,ok
scell_test_THIS-PR_i386,scell_test_0.10.0_i386_nc,ok
scell_test_THIS-PR_i386,scell_test_0.10.0_x86_64,ok
scell_test_THIS-PR_i386,scell_test_0.10.0_x86_64_nc,ok
scell_test_THIS-PR_i386,scell_test_0.9.6_i386,ok
scell_test_THIS-PR_i386,scell_test_0.9.6_x86_64,failed to encrypt: 11
scell_test_THIS-PR_i386,scell_test_THIS-PR_i386,ok
scell_test_THIS-PR_i386,scell_test_THIS-PR_x86_64,ok
scell_test_THIS-PR_x86_64,scell_test_0.10.0_i386,ok
scell_test_THIS-PR_x86_64,scell_test_0.10.0_i386_nc,ok
scell_test_THIS-PR_x86_64,scell_test_0.10.0_x86_64,ok
scell_test_THIS-PR_x86_64,scell_test_0.10.0_x86_64_nc,ok
scell_test_THIS-PR_x86_64,scell_test_0.9.6_i386,ok
scell_test_THIS-PR_x86_64,scell_test_0.9.6_x86_64,failed to encrypt: 11
scell_test_THIS-PR_x86_64,scell_test_THIS-PR_i386,ok
scell_test_THIS-PR_x86_64,scell_test_THIS-PR_x86_64,ok

Encrypted with	Decrypted by
	0.9.6 (32-bit)	0.9.6 (64-bit)	0.10.0+ (32-bit)	0.10.0+ (64-bit)	0.10.0– (32-bit)	0.10.0– (64-bit)	this PR (32-bit)	this PR (64-bit)
0.9.6 (32-bit)	✅	❌	✅	✅	✅	✅	✅	✅
0.9.6 (64-bit)	❌	✅	❌	✅	❌	❌	✅	✅
0.10.0+ (32-bit)	✅	❌	✅	✅	✅	✅	✅	✅
0.10.0+ (64-bit)	✅	❌	✅	✅	✅	✅	✅	✅
0.10.0– (32-bit)	✅	❌	✅	✅	✅	✅	✅	✅
0.10.0– (64-bit)	✅	❌	✅	✅	✅	✅	✅	✅
this PR (32-bit)	✅	❌	✅	✅	✅	✅	✅	✅
this PR (64-bit)	✅	❌	✅	✅	✅	✅	✅	✅

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).

[vixentael]

You are freak :D

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).

Agree. I used it to limit the double decryption surface 🤔

[ilammy]

removing old workarounds

Quite the opposite: we remove the way to remove the workaround, carving it in stone. Well, until the moment we decide that no one ever should have data encrypted by faulty 0.9.6 and earlier and remove ability to read it entirely.

we can get significant slow down when lib tries to decrypt data

This code is already enabled in virtually every 64-bit build that we distribute even without this PR. It does not slow down the happy path when valid data is decrypted. However, attempts to decrypt corrupted data are two times slower than necessary, and of course compatibility data takes twice as much time to decrypt successfully.

We used SCELL_COMPAT flag and x64 checks to be able to limit slow down on platforms where developers are sure that they didn’t used 0.9.6, thus don’t need compatibility mode.

It's a valid feature but I believe that probably no one is going to bother with disabling this mode since that involves a) realizing that you need to disable it since you decrypt a lot of corrupted data and an x2 boost will significantly improve your life, b) reading the source code to realize that you actually can disable it, since this flag is not documented anywhere else, c) recompiling Themis Core binaries because we do not ship builds with disabled compatibility path.

Edit: I believe that anyone competent enough to do all of that will surely be able to remove the workaround in the source code themselves. I don't really recall any requests for Themis builds without this flag so keeping it does not make our life easier as well. However, its existence is a constant reminder that we do not check builds with NO_SCELL_COMPAT automatically.

The most questionable thing is the use case. By leaving the compatibility code path we enable Themis to handle strictly more possible valid cases of decryption. By removing it we get a speedup in a questionable use case while making it impossible to decrypt some valid data. It's hard to image a case where you need to constantly deal with corrupted data and that x2 speedup will significantly improve your application.

[vixentael]

Initially #279 PR introduced a temporary workaround to be able to decrypt messages encrypted by buggy version of Themis (0.9.7). This bug affected only narrow case when messages are encrypted on one architecture (let’s say x32) and decrypted on another one (let’s say x64).

It affected only users who used 0.9.6 and updated Themis to 0.9.7 and higher. It was planned to remove double decryption with development of new versions, not to make double decryption a standard behavior :D

I think that this PR makes double decryption a standard behavior in many cases when Themis can’t decrypt messages due to various reasons. As Themis does NOT distinguish if data is corrupted because it was encrypted by 0.9.6 or because any other reasons, it will attempt to double decrypt it anyway. That’s why I think that we are seeing performance degradation in all cases when Themis can’t decrypt data from first time, not only in case that this data was produced by 0.9.6.

(If you show me performance benchmarks that says that double decryption happens ONLY if data was produced by 0.9.6, and not because key is invalid, data is invalid, other reasons, I’ll change my mind.)

My belief is that we should NOT turn “temporary workaround” into a default behavior that will affect all Themis users from now to foreva.

I think we either should leave it AS IS until some new version, or DISABLE the workaround itself (because 0.9.6 was 3 years ago, chances are high that many users have already migrated to newer versions and don’t handle with corrupted data, but even if they do, current version still supports fix).

[ilammy]

It was planned to remove double decryption with development of new versions, not to make double decryption a standard behavior :D

IMO, it depends on our stability guarantees. Bug-for-bug compatibility is totally a thing. I think that changing application code is relatively easier than hunting down all contaminated data, so we should be even more averse to removing data processing capabilities than removing deprecated API features.

My belief is that we should NOT turn “temporary workaround” into a default behavior that will affect all Themis users from now to foreva.

New users should not be generally affected since new versions use the new (correct) KDF during encryption and that's the one that we try first during decryption. It's a nice property of your workaround.

For them to be completely unaffected their need to not use data encrypted by 0.9.6 and eithe wait for us to remove the workaround, or make a custom build of Themis without it for themselves.

because 0.9.6 was 3 years ago, chances are high that many users have already migrated to newer versions and don’t handle with corrupted data

True, but there is no reliable way to tell other than try decrypting everything in the world with the workaround disabled.

but even if they do, current version still supports fix

If we remove the workaround, it won't support it. The release notes for 0.10 did note that you should upgrade, reencrypting the data was described as a last resort approach, not something that needs to be done after upgrading.

---

Given our commitment to a 3-year support cycle, and the fact that 0.9.6 will go EOL on 2020-12-13, it seems a reasonable to take the following apporach:

• leave the NO_SCELL_COMPAT as is for the next release
• issue an announcement that Themis will stop maintaining this workaround in 2021
• emphasize that users should migrate their encrypted data while they still can, if they suspect they might be affected (that is, if they used Themis 0.9.6 or earlier)
• schedule the removal of 0.9.6 compatibility in 2021

As for our own products, it seems that only the initial Acra 0.75 used old enough Themis, so we're kinda probably safe 🤞

[ilammy]

Or rather than simply leaving the NO_SCELL_COMPAT, I'd enable it in this release, removing the compatibility code from general builds that we publish. So the compatibility will be gone, but if someone comes to us with reports that their are much enterprise and cannot migrate but still want the new version, we can at least provide them with instructions on how to build it with the compatibility feature. And then we can completely remove the code in 2021 (so in year, after a couple of releases without the compatibility shim).

[vixentael]

Or rather than simply leaving the NO_SCELL_COMPAT, I'd enable it in this release, removing the compatibility code from general builds that we publish.
..
And then we can completely remove the code in 2021 (so in year, after a couple of releases without the compatibility shim).

Agree with this scenario, let do this way.

[ilammy]

Okay... I've pushed commits that

• bring back SCELL_COMPAT define
• replace NO_SCELL_COMPAT variable with WITH_SCELL_COMPAT
• make the compatibility code disabled by default, unless built WITH_SCELL_COMPAT
• describe the circumstances in CHANGELOG, warn about removal
• add Themis Core tests that verify builds with and without compatibilty

[vixentael]

Suggested change

	Old versions of Themis on 64-bit platforms have been using incorrect encryption algorithm with resulted in Secure Cells that cannot be decrypted on 32-bit machines (see [#279](https://github.com/cossacklabs/themis/pull/279) for details).
	Old versions of Themis platforms have been using incorrect calculation of encrypted data length, which resulted in Secure Cells encrypted on 64-machine cannot be decrypted on 32-bit machines (see [#279](https://github.com/cossacklabs/themis/pull/279) for details).

I disagree with "incorrect encryption algorithm" sentence. The issue was not about encryption algorithm, or params, it was about using wrong data type of storing length. We shouldn't make users think that encryption was bad/broken.

[ilammy]

Technically if the algorithm says “use u32” and you're using u64, that's incorrect, but I agree with you that it can make an impression as if we'd been using DES by accident.

[vixentael]

the algorithm says “use u32”

the issue was not about AES algorithm, but in determining hmac length. That's not an algorithm issue, that's implementation issue :)

[ilammy]

I meant Secure Cell algorithm, as in whatever happens inside themis_secure_cell_mode_encrypt(). Well, given that the implementation is the spec and there is no paper that describes the entire algorithm in some language-agonstic way...

[ilammy]

Anyways, I've reworded that paragraph a bit to talk about “encrypted length computation” rather than “incorrect algorithm” and fixed some typos.

[Lagovas]

nice comment