Raw rsa keyring decrypt #81

ttjsu-aws · 2018-09-06T00:41:36Z

Issue #, if available:

Description of changes:

This PR contains the implementation of RSA keyring. The PR also contains a full working implementation of the decrypt_data_key method and it contains a bunch of tests corresponding to the decryption of EDKs and some malformed test cases. It also contains test vectors for the three supported algorithms: RSA with PKCS1 Padding, RSA with OAEP/SHA-1/MGF1 Padding, RSA with OAEP/SHA-256/MGF1 Padding for the 128, 192, and 256 bit AES data keys. The test vectors used in this PR were generated by the Python SDK with a KeySize of 4096. The generate_data_key and encrypt_data_key methods for this keyring will be sent in another PR shortly.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…d other changes

tautschnig

In case this project becomes open-source with the full commit history maintained, commits really need to be tidy.

CMakeLists.txt

tautschnig · 2018-09-06T09:14:53Z

include/aws/cryptosdk/header.h

+    RSA_PKCS1, 
+    RSA_OAEP_SHA1_MGF1,
+    RSA_OAEP_SHA256_MGF1,
+    NO_PADDING,


In absence of any comments: why those, where is the list coming from, and may it need to be expanded at some future date?

This is obtained from here Asymmetric Wrapping Algorithms discussion .

We will need some public-facing documentation for this. (And I do note that you removed NO_PADDING in a later commit.)

include/aws/cryptosdk/hkdf.h

source/cipher.c

source/raw_rsa_keyring.c

tautschnig · 2018-09-06T09:28:15Z

source/raw_rsa_keyring.c

+            aws_reset_error();
+        } else {
+            goto success;
+        }


Do we need to worry about timing sidechannels here? The paths are certainly not balanced.

Good question, let me talk to my team and get back to you.

source/raw_rsa_keyring.c

include/aws/cryptosdk/header.h

include/aws/cryptosdk/private/raw_rsa_keyring.h

david-koenig

In addition to all of the specific comments I've given, I've noticed that you mixed into this a naming change of the enums for the hash function of the HKDF stuff. That is a good change that should be made, but there's no reason to do it within this PR. You should make a separate PR for that, which we should be able to push out quickly.

include/aws/cryptosdk/header.h

include/aws/cryptosdk/private/cipher.h

david-koenig · 2018-09-10T18:19:38Z

include/aws/cryptosdk/private/raw_rsa_keyring.h

+    struct aws_allocator *alloc;
+    const struct aws_string *master_key_id;
+    const struct aws_string *provider_id;
+    const uint8_t *raw_key;


Based on our discussions, I think that this argument is supposed to point to the contents of a PEM file of the private key. I would rename the variable to something that indicates this better, such as private_key_pem. Also, does it make sense to hold the PEM internally as a const struct aws_string, similarly to how we are doing for the other items? You can access the raw bytes of the string at any time with aws_string_bytes(private_key_pem) and the length with private_key_pem->len in order to work with the OpenSSL interface, but that way you will only have one pointer to pass between your functions.

We could make it a const struct aws_string, it would be a minor change.

include/aws/cryptosdk/raw_rsa_keyring.h

david-koenig · 2018-09-10T18:42:45Z

source/cipher.c

+    if (EVP_PKEY_decrypt(ctx, NULL, &outlen, cipher.ptr, cipher.len) <= 0) goto err;
+    if (outlen == 0) goto err;
+    if (!plain->buffer) goto err;
+    if (EVP_PKEY_decrypt(ctx, plain->buffer, &outlen, cipher.ptr, cipher.len) <= 0) {


Am I understanding correctly that everything else will succeed but this function will fail if you use the wrong private key for decryption? Have you tested this? Is this a case where the function fails without generating an OpenSSL error code? If so, have you verified this?

Yes this function fails if we have the wrong private key for decryption. This function return 1 for success and 0 or a negative value (-2 indicates the operation is not supported) for failure in the decryption process. I shall add additional test cases to cover all the possible cases here.

david-koenig · 2018-09-10T18:49:50Z

source/cipher.c

+        case RSA_OAEP_SHA256_MGF1: padding = RSA_PKCS1_OAEP_PADDING; break;
+        default: return aws_raise_error(AWS_CRYPTOSDK_ERR_UNSUPPORTED_FORMAT);
+    }
+    bool openssl_err = true;


Firstly, answer the questions below about the error cases and error codes to make sure you are doing the right thing.

If you are doing the right thing, there is no need to make this a bool. Instead you can do int err_code = AWS_CRYPTOSDK_ERR_CRYPTO_UNKNOWN;. And in the case that you need to change this, you can just reassign err_code = AWS_CRYPTOSDK_ERR_BAD_CIPHERTEXT;. Then in your exit code at the bottom of the function, you don't need a conditional and can just do return aws_raise_error(err_code);. Note that there is nothing wrong with calling flush_openssl_errors(); when OpenSSL has not generated any errors.

david-koenig · 2018-09-10T18:53:24Z

source/raw_rsa_keyring.c

+             */
+            aws_reset_error();
+        } else {
+            goto success;


If there is no other cleanup needed at your success label, then you don't need the goto here. Just return AWS_OP_SUCCESS;

source/raw_rsa_keyring.c

david-koenig · 2018-09-12T21:54:17Z

include/aws/cryptosdk/private/cipher.h

@@ -162,5 +162,31 @@ int aws_cryptosdk_aes_gcm_decrypt(struct aws_byte_buf * plain,
                                  const struct aws_byte_cursor aad,
                                  const struct aws_string * key);

+/**
+ * Does RSA decryption of an encrypted data key to a 128/192/256 bit AES data key.


Currently all of the algorithm suites we use for the Encryption SDK use AES algorithms for the encryption of the plaintext. Therefore, our data keys are always 128/192/256 bit AES data keys. However, it is possible that we could add other algorithm suites in the future that use something other than AES for the plaintext encryption. Therefore, we have no guarantees that all of our data keys will take this form.

The keyring should be agnostic to this. It doesn't care what the length of the data key is, and the fact that the data key is an AES data key is irrelevant. Its job is simply to encrypt whatever data key is provided to it, of whatever length. Therefore, you should not indicate that it is a 128/192/256 bit AES data key here.

I shall go ahead and replace 128/192/256 bit AES data key to unencrypted data key in the comments.

david-koenig · 2018-09-12T22:14:14Z

include/aws/cryptosdk/private/cipher.h

+ * AWS_CRYPTOSDK_ERR_BAD_CIPHERTEXT : unable to decrypt or authenticate cipher text
+ * AWS_CRYPTOSDK_ERR_CRYPTO_UNKNOWN : OpenSSL error
+ *
+ * On either of the last two errors, the plain buffer will be set to all zero bytes, and its


So the reason the AES-GCM decryption function had this kind of behavior was that data could be dumped into the plain buffer partway through, even on an unsuccessful decryption. So we needed to scrub the data out of the buffer when this did not succeed. Is that the case with this decryption algorithm?

I zeroed out the plain buffer to be on a safer side so that we can be sure of no random secret bytes leaking through the plain buffer in the case of errors. Isn't it recommended to zero out the plain->buffer in case of errors anyway?

What I am asking is: Are there any bytes being output to the plain buffer when you hit an error condition?

I just verified that there isn't any bytes being outputted to the plain buffer when I hit an error condition. I don't think we need to set the plain buffer to all zero bytes.

I thought that was likely the case. The reason that it was an issue in the other functions was particular to the way AES-GCM encryption works, as well as the OpenSSL interface to it.

david-koenig · 2018-09-12T22:45:13Z

source/cipher.c

+    enum aws_cryptosdk_rsa_padding_mode rsa_padding_mode) {
+    if (!plain->buffer) return aws_raise_error(AWS_ERROR_INVALID_BUFFER_SIZE);
+    int padding = -1;
+    switch (rsa_padding_mode) {


You're probably going to want to separate this switch statement into its own function, as I think you'll need to reuse it for the encryption function. If you want, you can wait on that change for the encryption PR.

I've done exactly this, will be putting it up with the PR for encrypt.

david-koenig · 2018-09-12T22:47:30Z

source/raw_rsa_keyring.c

+    kr->provider_id = aws_string_new_from_array(alloc, provider_id, provider_id_len);
+    if (!kr->provider_id) goto err;
+
+    kr->rsa_key_pem = aws_string_new_from_array(alloc, rsa_key_pem, strlen((const char *)rsa_key_pem));


Use aws_string_new_from_c_str instead

The reason I didn't use aws_string_new_from_c_str is because the parameters to it is const char *c_str and the rsa_key_pem string is unsigned of type const uint8_t*. The way the aws_string_new_from_c_str API works is it takes in const char *c_str and then makes a call to aws_string_new_from_array under the hood , this loop is what I wanted to avoid.

That being said, it would be useful to have an API called aws_string_from_array similar to the way the aws_byte_buf_from_array works. I shall put in a request to aws-c-commons library for this API.

You have it backwards. You should be using char * for the input argument for something where you are expecting a null terminated C-string as the input type. Once you use the aws_string_new_from_c_str function, it will switch to using the unsigned 8 bit integer data type for its internal array.

david-koenig · 2018-09-12T22:56:11Z

source/raw_rsa_keyring.c

+            aws_reset_error();
+        } else {
+            assert(dec_mat->unencrypted_data_key.len == props->data_key_len);
+            goto success;


I thought I already commented on this before. Did my comment get lost? There's no point in making a goto which just jumps to a return statement. You can just put a return statement here.

The reason I retained the goto success is that, if not for this I would have to add return AWS_OP_SUCCESS on line 62 and line 68 (end of the function after the clean up). Note: If none of the EDKs worked, we clean up unencrypted data key buffer and return success as per materials.h.

It is okay to have more than one return statement in the function.

If you have more than one line of cleanup code that needs to be run in either case, then it is okay to jump to the label.

If you are jumping to the label just to execute a return statement, the goto statement is an extra cycle that is unnecessary. It saves you no lines of code, and forces the programmer reading this to jump to a different place in the code to see what is happening.

david-koenig · 2018-09-12T23:00:07Z

tests/lib/raw_rsa_keyring_test_vectors.c

+struct aws_cryptosdk_keyring *raw_rsa_keyring_tv_new(
+    struct aws_allocator *alloc, enum aws_cryptosdk_rsa_padding_mode rsa_padding_mode) {
+    return aws_cryptosdk_raw_rsa_keyring_new(
+        alloc, raw_rsa_keyring_tv_master_key_id, sizeof(raw_rsa_keyring_tv_master_key_id),


This should be strlen(raw_rsa_keyring_tv_master_key_id) and strlen(raw_rsa_keyring_tv_provider_id). The way you did it, the null byte is part of the contents of the string, which is not what you want.

david-koenig

Sorry, accidentally clicked approve before. Undoing that.

bdonlan · 2018-09-14T17:53:58Z

include/aws/cryptosdk/private/cipher.h

+ * Does RSA decryption of an encrypted data key to a 128/192/256 bit AES data key.
+ * RSA with PKCS1, OAEP_SHA1_MGF1 and OAEP_SHA256_MGF1 padding modes is supported.
+ *
+ * Assumes plain is an already allocated byte buffer. Does NOT assume that length of plain


What should the capacity of 'plain' be? What happens if the encrypted plaintext is larger than the normal key size of the algorithm suite in use?

bdonlan · 2018-09-14T17:58:14Z

include/aws/cryptosdk/raw_rsa_keyring.h

+    size_t master_key_id_len,
+    const uint8_t *provider_id,
+    size_t provider_id_len,
+    const uint8_t *rsa_key_pem,


If we are using this as a C string, use const char *.

Exactly what @bdonlan said here. Once you make this change you won't get the compiler complaint about using aws_string_new_from_c_str that you talked about in the other file.

bdonlan · 2018-09-14T18:00:12Z

source/cipher.c

+    }
+    size_t outlen = 0;
+    if (EVP_PKEY_decrypt(ctx, NULL, &outlen, cipher.ptr, cipher.len) <= 0) goto err;
+    if (outlen <= 0) goto err;


Verify that outlen <= plain->capacity

Also, outlen is an unsigned integer, so it is always non-negative. The test you wrote is equivalent to outlen == 0. From the machine's point of view, there's nothing wrong with writing it either way, but I want to make sure you understand that. Writing the test with == might help avoid creating misunderstandings from other programmers in the future.

bdonlan · 2018-09-14T18:00:27Z

source/cipher.c

+    size_t outlen = 0;
+    if (EVP_PKEY_decrypt(ctx, NULL, &outlen, cipher.ptr, cipher.len) <= 0) goto err;
+    if (outlen <= 0) goto err;
+    if (EVP_PKEY_decrypt(ctx, plain->buffer, &outlen, cipher.ptr, cipher.len) <= 0) {


You can pass &plain->len directly here

bdonlan · 2018-09-14T18:03:38Z

source/cipher.c

+        err_code = AWS_CRYPTOSDK_ERR_BAD_CIPHERTEXT;
+        goto err;
+    }
+    EVP_PKEY_CTX_free(ctx);


Can we merge the cleanup between the success and error paths?

bdonlan · 2018-09-14T18:06:01Z

tests/lib/raw_rsa_keyring_test_vectors.c

+    0x7c, 0xd5, 0x8d, 0x30, 0x8f, 0x44, 0x2e, 0x2a, 0x56, 0x98, 0xbf, 0x2a, 0x05, 0x33, 0x62, 0xd4, 0x6c, 0x11, 0x4e,
+    0x90, 0x43, 0x59, 0x8f, 0x64, 0x56, 0x8a, 0x1d, 0x0b, 0xee, 0x82, 0x81, 0x0e, 0x79, 0x78, 0xad, 0x85, 0xb3
+};
+


Add some malformed test vectors:

Data key too small

Data key too large

Bad padding

…ion for rsa_padding lookup

…unction

…the size of RSA key in PEM format

bdonlan · 2018-09-20T18:01:54Z

source/cipher.c

    int err_code = AWS_CRYPTOSDK_ERR_CRYPTO_UNKNOWN;
    EVP_PKEY *pkey = EVP_PKEY_new();
-    if (!pkey) goto err;
+    if (!pkey) goto cleanup;
    bio = BIO_new_mem_buf(aws_string_bytes(rsa_private_key_pem), -1);


it's a little bit cleaner to use rsa_private_key_pem->len for the length (not a blocker)

bdonlan · 2018-09-20T18:04:12Z

tests/lib/raw_rsa_keyring_test_vectors.c

@@ -72,20 +72,39 @@ static const uint8_t raw_rsa_keyring_tv_wrapping_key[] =
    "r1kmH86LkDARuxu2Vm1EoHP9L/wk\n"
    "-----END PRIVATE KEY-----";

+static const char wrong_raw_rsa_keyring_tv_private_key[] =


Add a comment describing how this key is "wrong".

bdonlan · 2018-09-20T18:05:41Z

source/raw_rsa_keyring.c

+    /* maximum capacity for the unencrypted data key buffer is approximated
+     * to be two-thirds the size of the RSA key in PEM format
+     */
+    if (aws_byte_buf_init(request->alloc, &dec_mat->unencrypted_data_key, self->rsa_key_pem->len * 2 / 3)) {


Do not approximate this. EVP_PKEY_decrypt gives you the precise size bound on the output when called with a NULL output buffer.

Yes, which is what is done in line 592 here : https://github.com/ttjsu-aws/aws-encryption-sdk-c/blob/raw_rsa_kr/source/cipher.c#L592. Ideally, the max capactiy of the dec_mat->unencrypted_data_key should be equal to the RSA Keysize and from my discusison with Greg, I understand that there is no harm in taking 2/3 rd of the RSA key in PEM format as the size in our case. If we want to set the capacity to the return value from EVP_Decrypt we will have to refrain from initializing the dec_mat->unencrypted_data_key buffer in raw_rsa_keyring.c and move the initialization into to cipher.c which may be the best thing to do in this case. Is moving the init to cipher.c okay?

I would suggest allocating it in aws_cryptosdk_rsa_decrypt once you know the required size.

If you do go with an approximation approach, the correct ratio for base64 to unencoded sizes is 6/8 (or 3/4), not 2/3.

I'm okay with doing an approximation so that we don't have to allocate the buffer inside the loop. Note that this was @SalusaSecondus's suggestion.

… raw_rsa_kr

david-koenig · 2018-09-20T23:10:17Z

tests/unit/t_raw_rsa_keyring_decrypt.c

+    return edk;
+}
+
+typedef struct aws_cryptosdk_edk (*edk_generator)();


When you cut and paste code from the other tests like this, you should probably be looking to refactor the common functionality out into files in tests/lib. This will reduce duplication and make all of the tests easier to read. I'm not blocking on this for now. We can make it a later sprint task to go back and refactor these tests.

I created #99 to remind us to address this later.

Sure, you can assign this to me. I'll make it a priority to take this task at a later stage once I have my encrypt and generate function in.

ttjsu-aws added 4 commits September 4, 2018 16:22

Adding raw_rsa_keyring files

0d45c66

Added test vectors for RSA_OAEP_SHA1_MGF1 and RSA_OAEP_SHA256_MGF1 an…

887e8b2

…d other changes

Fixed cipher.c

9ab8b92

Added comments

4d6996c

ttjsu-aws requested review from bdonlan and david-koenig September 6, 2018 00:41

tautschnig suggested changes Sep 6, 2018

View reviewed changes

ttjsu-aws and others added 2 commits September 6, 2018 13:59

Fix PR#81 as per suggestions from review

20bd932

Merge branch 'master' into raw_rsa_kr

9a8bf6f

ttjsu-aws force-pushed the raw_rsa_kr branch from e649e4e to 9a8bf6f Compare September 10, 2018 18:22

david-koenig reviewed Sep 10, 2018

View reviewed changes

ttjsu-aws added 4 commits September 10, 2018 16:37

enum name change and addition of AWS_CRYPTOSDK_ prefix

1ddc073

Added doc comments for the APIs

c918085

Uses struct aws_string instead of uint8_t

8be93c0

Changed variable names and doc comments to indicate PEM format

f42a2c4

david-koenig approved these changes Sep 12, 2018

View reviewed changes

david-koenig suggested changes Sep 12, 2018

View reviewed changes

bdonlan suggested changes Sep 14, 2018

View reviewed changes

ttjsu-aws added 6 commits September 17, 2018 13:44

Merge branch 'master' into raw_rsa_kr

3a88d5a

Merged the cleanup between the success and error paths, created funct…

790f88c

…ion for rsa_padding lookup

Changed const uint8_t * to const char * for C string

57313cf

Added additional malformed test vectors

398014b

Fixed memory leak in EVP_PKEY while using PEM_read_bio_PrivateKey() f…

0fe78b2

…unction

max capacity for the unencrypted datakey buffer approx. to two-third …

f2ab39d

…the size of RSA key in PEM format

mattsb42-aws added the release blocker Necessary feature for a release label Sep 20, 2018

bdonlan suggested changes Sep 20, 2018

View reviewed changes

ttjsu-aws added 4 commits September 20, 2018 15:20

Added comment to describe wrong_raw_rsa_keyring_tv_private_key

987a2b6

Moving init of plain buffer from raw_rsa_keyring.c to cipher.c

b6c5399

Fixed typo: struct aws_allocator in cipher.c and cipher.h

512f8a3

Merge branch 'master' of github.com:awslabs/aws-encryption-sdk-c into…

a9394aa

… raw_rsa_kr

bdonlan approved these changes Sep 20, 2018

View reviewed changes

Merge branch 'master' into raw_rsa_kr

fa977fc

david-koenig reviewed Sep 20, 2018

View reviewed changes

david-koenig approved these changes Sep 20, 2018

View reviewed changes

david-koenig mentioned this pull request Sep 20, 2018

Reduce framework duplication in AES/RSA keyring tests #99

Closed

bdonlan merged commit e3b4542 into aws:master Sep 20, 2018

ttjsu-aws deleted the raw_rsa_kr branch December 14, 2018 21:20

Raw rsa keyring decrypt #81

Raw rsa keyring decrypt #81

Conversation

ttjsu-aws commented Sep 6, 2018 • edited Loading

tautschnig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-koenig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttjsu-aws Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttjsu-aws Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-koenig Sep 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-koenig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttjsu-aws commented Sep 6, 2018 •

edited

Loading

ttjsu-aws Sep 12, 2018 •

edited

Loading

ttjsu-aws Sep 12, 2018 •

edited

Loading

david-koenig Sep 17, 2018 •

edited

Loading