Skip to content
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

Fuzz passphrase API of Secure Cell #583

Merged
merged 6 commits into from
Mar 5, 2020
Merged

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Jan 29, 2020

This PR adds fuzzers for new Secure Cell API added in #577, and fixes implementation bugs found by those fuzzers.

Currently passphrase API supports only Seal mode so we add tools for that only (just like master key API is right now). In fact, the tools are more or less copypasta of master key tools.

Additionally, I have found that using the default 200,000 PBKDF2 iterations provides very unfulfilling fuzzing experience of around 7 executions per second. Lowering iteration count to 10 allows AFL to run 800 attempts every second which results in much more reasonable time until interesting crashes. Thus I have added a compile-time switch to adjust the iteration count. It is used only for fuzzing builds and is not documented.

Now, on to the bugs...

Validate key length on decryption

This is the first crash found by AFL. Decryption code fails to validate key length encoded in algorithm field of Secure Cell header and just proceeds to use it, assuming that it’s one of 128, 192, 256 bits.

Maximum value allowed by data format is 4096 bits (512 bytes), but the code allocates only 32 bytes for the derived key. Longer keys will cause a buffer overflow which allows an attacker to overwrite return address stored on the stack and to theoretically execute arbitrary code. Practically, this can be used for denial of service by exploiting the much more likely outcome – crash due to a segmentation fault.

Fix the issue by validating the key length and allowing only values produced by Themis: 128, 192, or 256 bits.

Avoid unsigned overflow in length computations

This is much more insidious one. Turns out that code like this:

length += sizeof(hdr->auth_tag_length) + hdr->auth_tag_length;

can cause an unsigned overflow on 32-bit systems which later leads to buffer overflow caused by incorrect pointer computations. To handle that we ensure that length never overflows by using a wider type for it (uint64_t).

Another fun fact is that code like this

length += hdr->iv_length + hdr->auth_tag_length + hdr->kdf_context_length;

can also overflow of all those fields are unsigned 32-bit (and they are). And on 32-bit platforms

length += sizeof(hdr->auth_tag_length) + hdr->auth_tag_length;

can overflow too because size_t is 32-bit.

We avoid intermediate overflows by spelling out additions:

length += hdr->iv_length;
length += hdr->auth_tag_length;
length += hdr->kdf_context_length;
// u64 += u32

which causes the compiler to generate correct code.

Bad news here is that unsigned integer overflow is fully defined in C and this issue is normally not caught by sanitizers, static analyzers, etc. Well... let's hope that at least this piece of code is now free from such bugs.

Checklist

  • Change is covered by automated tests
  • The coding guidelines are followed
  • Example projects and code samples are updated

@ilammy ilammy added core Themis Core written in C, its packages infrastructure Automated building and packaging tests Themis test suite labels Jan 29, 2020
Currently passphrase API supports only Seal mode so we add tools for
that only (just like master key API is right now). In fact, the tools
are more or less copypasta of master key tools:

- Encryption code path is verified by "roundtrip" tool. It encrypts
  arbitrary counts of zeros and sees whether it fails to encrypt, or
  decrypt later, or decrypts incorrectly. Fuzzed values here are the
  parameter lengths used.

- Decryption code path is much more interesting and is verified by
  "decrypt" tool. Here we accept fuzzable input and try to decrypt it.
  This is probably the main attack surface of the new API, and boy
  are we vulnerable! AFL immediately found one crash and two more
  came after the first one was fixed. Stay tuned...

This commit also includes test data for the tools and a meta-tool used
to generate that data. Since GoThemis does not support passphrase API
yet, we use CGo directly here.
This is the first crash found by AFL. Decryption code fails to validate
key length encoded in "algorithm" field of Secure Cell header and just
proceeds to use it, assuming that it's one of 128, 192, 256 bits.

Maximum value allowed by data format is 4096 bits (512 bytes), but the
code allocates only 32 bytes for the derived key. Longer keys will cause
a buffer overflow which allows an attacker to overwrite return address
stored on the stack and to theoretically execute arbitrary code.
Practically, this can be used for denial of service by exploiting the
much more likely outcome -- crash due to a segmentation fault.

Fix the issue by validating the key length and allowing only values
produced by Themis: 128, 192, or 256 bits.
Additions like

    hdr->iv_length + hdr->auth_tag_length + hdr->kdf_context_length

and

    length += sizeof(hdr->auth_tag_length) + hdr->auth_tag_length

can cause unsigned overflow during itermediate computations. This can
lead to incorrect buffer lengths being computed, causing further pointer
computations to use invalid memory addresses, resuling in arbitrary
memory writes controlled by an attacker -- since these lengths come
from message data structure that we parse.

This is further complicated by the fact that "size_t" is typically
32-bit on 32-bit systems, causing more possible oveflows.

Bad news here is that unsigned integer overflow is *not* undefined
behavior in C -- it is defined to be two's complement. Therefore most
tools like UBSan and static analyzers don't catch issues due to them
not being "undefined behavior", so they are not reported. This issue
has been caught only due to its 'side effect' in parsing.

The only way to avoid issues is to either check for overflows or avoid
them. Both options require careful coding of each arithmetic operation,
so we opt for avoiding overflows without significant runtime cost. We
update the code to use "uint64_t" where the maximum size can exceed
uint32_t range (we know that it won't overflow uint64_t because there
are only a handful of values added), and similarly use "uint32_t" for
cases where the value cannot overflow "uint32_t".

Hopefully this is it... By the way, this is *the* reason why Rust
does not have implicit casts between integer types.
KDF makes computations slow but fuzzing requires a lot of encryption and
decryption. Let's make the default iteration count to be configurable at
compile time and use a lower number for fuzzer builds. This drastically
increases efficiency of AFL search.

This feature is intended to be used mostly for debugging, and maybe to
provide custom builds on demand, so it's not documented anywhere except
for the code.
@ilammy ilammy changed the base branch from ilammy/kdf+fuzz to master March 5, 2020 12:44
Some sanitizers are built in 32-bit mode because of AFL's limitations.
We should avoid attempts to allocate over 2 GB on 32-bit systems with
Address Sanitizer enabled because this usually causes it to abort.
@ilammy ilammy merged commit 7edf2df into cossacklabs:master Mar 5, 2020
@ilammy ilammy deleted the kdf/fuzz branch March 5, 2020 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Themis Core written in C, its packages infrastructure Automated building and packaging tests Themis test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants