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

Symmetric keygen: JavaThemis #565

Merged
merged 1 commit into from
Dec 11, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ _Code:_
- Fixed a NullPointerException bug in `SecureSocket` initialisation ([#557](https://github.com/cossacklabs/themis/pull/557)).
- Some Themis exceptions have been converted from checked `Exception` to _unchecked_ `RuntimeException`, relaxing requirements for `throws` specifiers ([#563](https://github.com/cossacklabs/themis/pull/563)).
- Introduced `IKey` interface with accessors to raw key data ([#564](https://github.com/cossacklabs/themis/pull/564)).
- New class `SymmetricKey` can be used to generate symmetric keys for Secure Cell ([#565](https://github.com/cossacklabs/themis/pull/565)).

- **Node.js**

Expand Down
41 changes: 41 additions & 0 deletions jni/themis_keygen.c
Original file line number Diff line number Diff line change
Expand Up @@ -113,3 +113,44 @@ JNIEXPORT jobjectArray JNICALL Java_com_cossacklabs_themis_KeypairGenerator_gene

return keys;
}

JNIEXPORT jbyteArray JNICALL Java_com_cossacklabs_themis_SymmetricKey_generateSymmetricKey(JNIEnv* env,
jobject thiz)
{
themis_status_t res;
jbyteArray key = NULL;
jbyte* key_buffer = NULL;
size_t key_length = 0;

UNUSED(thiz);

res = themis_gen_sym_key(NULL, &key_length);
if (res != THEMIS_BUFFER_TOO_SMALL) {
goto error;
}

key = (*env)->NewByteArray(env, key_length);
if (!key) {
goto error;
}

key_buffer = (*env)->GetByteArrayElements(env, key, NULL);
if (!key_buffer) {
goto error;
}

res = themis_gen_sym_key((uint8_t*)key_buffer, &key_length);
if (res != THEMIS_SUCCESS) {
goto error_release_key;
}

(*env)->ReleaseByteArrayElements(env, key, key_buffer, 0);

return key;

error_release_key:
(*env)->ReleaseByteArrayElements(env, key, key_buffer, JNI_ABORT);

error:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Yes, please do

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

Would be good

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

  1. 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? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

return NULL;
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,12 @@

public class KeyGenerationException extends RuntimeException {

KeyGenerationException(String message) {
super(message);
}

KeyGenerationException(String message, Throwable cause) {
super(message, cause);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public static Keypair generateKeypair() {
try {
return generateKeypair(AsymmetricKey.KEYTYPE_EC);
} catch (InvalidArgumentException e) {
throw new KeyGenerationException();
throw new KeyGenerationException("failed to generate keypair", e);
}
}

Expand All @@ -60,9 +60,9 @@ public static Keypair generateKeypair(int keyType) {
byte[][] keys = generateKeys(keyType);

if (null == keys) {
throw new KeyGenerationException();
throw new KeyGenerationException("failed to generate keypair");
}

return new Keypair(new PrivateKey(keys[0]), new PublicKey(keys[1]));
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
* Copyright (c) 2019 Cossack Labs Limited
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.cossacklabs.themis;

/**
* Symmetric encryption key.
*
* These keys are used with Secure Cell cryptosystem.
*/
public class SymmetricKey extends KeyBytes {

/**
* Generates a new symmetric key.
*/
public SymmetricKey() {
super(newSymmetricKey());
}

/**
* Creates a symmetric key from byte array.
*
* @param key byte array
*
* @throws NullArgumentException if `key` is null.
* @throws InvalidArgumentException if `key` is empty.
*/
public SymmetricKey(byte[] key) {
super(key);
}

private static native byte[] generateSymmetricKey();

private static byte[] newSymmetricKey() {
byte[] key = generateSymmetricKey();
if (key == null || key.length == 0) {
throw new KeyGenerationException("failed to generate symmetric key");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
return key;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/*
* Copyright (c) 2019 Cossack Labs Limited
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.cossacklabs.themis.test;

import com.cossacklabs.themis.SymmetricKey;
import com.cossacklabs.themis.InvalidArgumentException;
import com.cossacklabs.themis.NullArgumentException;

import static org.junit.Assert.*;
import org.junit.Test;

public class SymmetricKeyTest {

private static final int defaultLength = 32;

@Test
public void generateNewKey() {
SymmetricKey key = new SymmetricKey();

byte[] keyBytes = key.toByteArray();
assertNotNull(keyBytes);
assertEquals(keyBytes.length, defaultLength);
}

@Test
public void restoreKeyFromBytes() {
byte[] buffer = { 3, 14, 15, 92, 6 };
SymmetricKey key = new SymmetricKey(buffer);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@ilammy ilammy Dec 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

assertArrayEquals(buffer, key.toByteArray());
}

@Test(expected = NullArgumentException.class)
public void restoreKeyFromNull() {
SymmetricKey key = new SymmetricKey(null);
}

@Test(expected = InvalidArgumentException.class)
public void restoreKeyFromEmpty() {
SymmetricKey key = new SymmetricKey(new byte[0]);
}
}