Symmetric keygen: JavaThemis

[ilammy]

Implement symmetric key generation utilities described in RFC 1 (not available publicly at the moment). This is new API introduced in #560, now distributed to JavaThemis wrapper.

Now that #563 and #564 are merged we can finally do it.

Language API

Java

Here's how it can be used:

import com.cossacklabs.themis.SymmetricKey;

SymmetricKey key = new SymmetricKey();

// Use IKey interface to access bytes:
byte[] keyBytes = key.toByteArray();

// Can also wrap existing buffers
SymmetricKey sameKey = new SymmetricKey(keyBytes);

Checklist

• Change is covered by automated tests
• Benchmark results are attached (not interesting)
• The coding guidelines are followed
• Public API has proper documentation
• Example projects and code samples are updated (later)
• Changelog is updated

[vixentael]

shall we try to clean up / zero key data before exiting?

[ilammy]

shall we try to clean up / zero key data before exiting?

Now that's a professional paranoid person at work!

My initial reaction to this was: "This BS, how is this exploitable? It's just a random buffer. It has not even been used as a key".

But then I switched on my paranoia gland... and realized that it could be exploitable. If the PRNG leaks some information on failure path, if that could be used to learn something abouth the seed, if that can be triggered by an attacker, then it could be possible, under certain circumstances, to predict PRNG output — that is, if you trick the user to register an account at the right moment then you could recover their data because you have predicted the random key generated for Secure Cell that keeps the user's master key. That's probably one of the gazillion reasons why BoringSSL aborts the process if its PRNG fails to fill the whole buffer.

I have also reflected on how the random data output is used throughout Themis, which also has some implications. For example, we could be leaking some partial computation results on failure paths (like the ephemeral key used in Secure Message).

So... how about the following mitigations?

1. Ensure that we never leak PRNG state.

   Let's fix it for all wrappers at one fell swoop in soter_rand(). If the backend fails to generate random data then soter_wipe() the entire buffer to ensure that nothing gets leaked, in case the backend has partially filled the buffer.

2. Manually review soter_rand() call sites.

   Make sure that we check the error code and if it's not successful then GTFO to the caller as fast as we can, never using the 'random' zeros we got.

3. Ensure that we never leak partial data.

   At all Themis entry points, if we're not returning THEMIS_SUCCESS then soter_wipe() all output buffers, never leaking any partial information that may have been written there.

4. Ensure that we don't leave partial data in memory.

   This is harder. We'd need to make sure that we wipe all sensitive data that we allocate on stack and heap. Just to be sure that we don't leave anything lying around.

   Heap is easier since we can grep for it (34 malloc/calloc/realloc calls in Soter and Themis).

I'm sure that there are more creative exploits of the 'observe Themis while it tries to decrypt data with invalid keys and recover it' sort, but that's too paranoid for me to think about.

---

While we're here, I've remembered another thing about leaks. Remember the padding oracle attack? The one that lets the attacker recover the key if the implementation returns different error codes for "invalid input" and "corrupted data" conditions? Just because you leak one bit of information. It may be not applicable to encryption modes used by Themis, but Themis error codes are unnecessarily detailed.

How about leaving only the following error codes:

• THEMIS_SUCCESS — you input is correct, here's your output
• THEMIS_FAIL — you input is not correct, here's your error
• THEMIS_BUFFER_TOO_SMALL — used to measure the output size
• THEMIS_INVALID_PARAMS — these errors should have been caught by language type system, but C is too unsafe for that so we have it (and it should not be used to indicate invalid input data, like we do now)
• THEMIS_NO_MEMORY — may be useful, but can be replaced with THEMIS_FAIL, so to speak (we should not ever allocate memory based on user input)
• THEMIS_NOT_SUPPORTED — may be useful for debugging configuration issues, so I'd leave it
• those special codes returned by Secure Session and Secure Comparator

and deprecating everything else? Specifically, THEMIS_DATA_CORRUPT, THEMIS_INVALID_SIGNATURE, and SOTER_ENGINE_FAIL (may be returned by Themis, actually).

---

P.S. And here's me thinking: this was supposed to be a new, clean, simple, one-function API to return random bytes, how come it turned out to be this audit trip full of possible exploits that makes you doubt the security of the whole thing?

[vixentael]

1. If the backend fails to generate random data then soter_wipe() the entire buffer to ensure that nothing gets leaked

Yes, please do

2. check the error code and if it's not successful then GTFO

Would be good

3. if we're not returning THEMIS_SUCCESS then soter_wipe() all output buffers

Yes, but keep an eye that some output buffers are input pointers. I think we don't want to damage user vars.

4. Ensure that we don't leave partial data in memory.

This might be a hard one. We tried to audit malloc/alloc calls in Themis/Soter previously, but makes sense to re-read that code again.

---

How about leaving only the following error codes:

Agree with you. I'd deprecate THEMIS_NO_MEMORY in favor of THEMIS_FAIL as well.

---

this was supposed to be a new, clean, simple, one-function API to return random bytes, how come it turned out to be this audit trip full of possible exploits that makes you doubt the security of the whole thing?

We're getting older / smarter / wiser / more paranoid every time we're making large changes. Think about this as about evolution. But let's move code quality changes to the separate PR? :)

[ilammy]

But let's move code quality changes to the separate PR?

Sure. All of this is definitely out of scope for this pull request.

[vixentael]

I kinda want to zero key in case of error, but at the same time, if key is null or 0, i can't think of use case where zeroing would be necessary :D

[vixentael]

do we really want to allow users to create their own 'insecure' keys?

[ilammy]

In the world where we provide strict type interfaces (accepting a concrete SymmetricKey or PublicKey instead of just byte[]) the users will need a way to construct keys from raw bytes that they read from a file, for example. The constructors should ensure that the keys are only constructed from valid bytes.

In the future we may impose additional restrictions to prohibit usage of weak keys. But right now we don’t have any other than keys being non-empty.

Plus, PublicKey and PrivateKey in Java already allow construction from byte arrays. Though, they don’t validate them at all, leaving that task to crypto systems.

[vixentael]

My point is that if we will have two interfaces (read this as illustration):

• encryptUsingKey(SymmetricKey: key, ...)
• encryptUsingPassphrase(String: passphrase, ...)

users might do following:

key = SymmetricKey("qwerty".toBytesArray())
cell.encryptUsingKey(key, data)

thus using low entropy string as input param for creating bad "symmetric key", avoiding KDF call.

I'd suggest separate two interfaces:

• SymmetricKey could be only generated by Soter, returns 32 bytes
• Passphrase could be generated from user input

[ilammy]

They sure can do that, but there is no realistic way to prevent that situation with just API design. We cannot gauge 'randomness' of a key by a single sample. We could introduce a bottom limit on the key length, but that will not prevent the users from using "could you please let me use Secure Cell, mkay?".toByteArray() as a key which is hardly random. Even if it's random, that's a hardcoded, fixed key that can be reverse-engineered or just stolen from the source code.

You're right about the passphrase interface, but we cannot leave the users with only a constructor that returns new instances of SymmetricKey. This would mean that the key can never leave local memory of the JVM instance that created it. The users will not be able to save the key to a persistent storage, then retrieve it later and use to decrypt the data. For that they will need to way to convert arbitrary byte[] array into a key usable by Secure Cell. That's what this constructor provides.

[Lagovas]

lgtm