Make caller enforce invariants when setting AES encryption key #702

ecstatic-morse · 2018-10-11T22:10:18Z

Resolves #113.

GFp_AES_set_encrypt_key can fail if and only if the following invariants are violated.

All pointer arguments must be non-null.
The key size must be either 128 or 256 bits (192 bit keys aren't supported).

The first invariant should not be checked at runtime (except via assertion) as it is the responsibility of the caller.

The second could be expressed statically by accepting an enum with the valid key lengths, but enums don't provide any type safety in C. Instead, we document the acceptable key lengths and make them the responsibility of the caller.

As a result of these changes, GFp_AES_set_encrypt_key (as well as GFp_aes_c_set_encrypt_key) no longer needs to return an error code. The assembly functions still return an error code and do input validation.

briansmith · 2018-10-11T22:16:27Z

crypto/fipsmodule/cipher/e_aes.c

-  // anyway.
-  GFp_AES_set_encrypt_key(key, (unsigned)key_len * 8, &ks);
+  unsigned bits = key_len * 8;
+  if (bits != 128 && bits != 256) {


Instead, let's compare key_len != 16 && key_len != 32 so we don't have to worry about any overflow issues with the multiplication.

crypto/fipsmodule/aes/aes.c

briansmith · 2018-10-11T22:30:05Z

crypto/fipsmodule/aes/aes.c

      }
      rk += 4;
    }
  }
+
  rk[4] = from_be_u32_ptr(key + 16);
  rk[5] = from_be_u32_ptr(key + 20);
  rk[6] = from_be_u32_ptr(key + 24);
  rk[7] = from_be_u32_ptr(key + 28);
  if (bits == 256) {


Remove the if (bits == 256) { here and do this unconditionally.

ecstatic-morse · 2018-10-11T23:55:34Z

If/when this gets merged, the equivalent error handling code should be removed from the assembly implementations.

Also, 192-bit key support was removed from the vectorized AES implementation but not the one using AES-NI.

I can open new issues for these if you like.

briansmith · 2018-10-17T19:12:03Z

crypto/fipsmodule/aes/aes.c

      }
      rk += 4;
    }
  }
+
+  // bits == 256


This should be assert(bits == 256);. That is, we prefer checked (at debug time at least) assertions to unchecked assertions.

briansmith · 2018-10-17T19:14:03Z

src/aead/aes_gcm.rs

@@ -172,8 +174,8 @@ mod tests {
    }

    extern "C" {
-        fn GFp_AES_set_encrypt_key(key: *const u8, bits: usize,


We should have done this change as a separate commit. But, OK.

briansmith · 2018-10-17T19:22:24Z

LGTM! Thanks!

Please squash these commits together, change the comment to an assert(...). and add the "I agree..." message to the commit message.

briansmith · 2018-10-22T03:44:18Z

@ecstatic-morse Are you still interested in this? It would be great if you could make the small changes I requested in the PR soon. I want to refactor the C code (or, if you want, you can do so, in another PR!) to remove the duplication between the 128-bit case and the 256-bit case, and to replace the weird loop constructions with straightforward for loops. I'd prefer to merge this PR before making those changes so that I don't create a merge conflict for you that you'd have to resolve manually.

ecstatic-morse · 2018-10-22T22:32:22Z

Sorry, life got in the way last week. I made the changes and squashed.

My plan was to start working on #660 and #705 tonight. Refactoring AES round key generation would be a nice warm up.

On a more meta note, I'm trying to get accustomed to the programming style here before I tackle more involved issues. If you have any suggestions for next steps, I'd love to hear them.

ecstatic-morse · 2018-10-23T00:55:14Z

I refactored the C AES key expansion routines. I'll submit a PR when this gets merged.

briansmith · 2018-10-23T05:50:39Z

crypto/fipsmodule/cipher/e_aes.c

 #if defined(HWAES)
  if (hwaes_capable()) {
-    return GFp_aes_hw_set_encrypt_key(user_key, bits, key);
+    GFp_aes_hw_set_encrypt_key(user_key, bits, key);


Please prefix this with (void) to make it clear we're ignoring the return value intentionally.

briansmith · 2018-10-23T05:50:50Z

crypto/fipsmodule/cipher/e_aes.c

@@ -149,14 +153,15 @@ int GFp_AES_set_encrypt_key(const uint8_t *user_key, unsigned bits,
 #error "BSAES and VPAES are enabled at the same time, unexpectedly."
 #endif
  if (vpaes_capable()) {
-    return GFp_vpaes_set_encrypt_key(user_key, bits, key);
+    GFp_vpaes_set_encrypt_key(user_key, bits, key);


Ditto: prefix with (void).

briansmith · 2018-10-23T05:52:14Z

Thanks for the quick turnaround @ecstatic-morse. I was just about to merge this but I noticed a couple of things: (1) We are missing a couple places where we should be using (void), and (2) You didn't add "I agree to license..." to the end of your commit message. Please update the commit with those changes and I'll merge ASAP.

briansmith

See the previous comment for the changes requested.

`GFp_AES_set_encrypt_key` can fail if and only if the following invariants are violated. * All pointer arguments must be non-null. * The key size must be either 128 or 256 bits (192 bit keys aren't supported). The first invariant should not be checked at runtime (except via assertion) as it is the responsibility of the caller. The second could be expressed statically by accepting an enum with the valid key lengths, but enums don't provide any type safety in C. Instead, we document the acceptable key lengths and make them the responsibility of the caller. As a result of these changes, `GFp_AES_set_encrypt_key` (as well as `GFp_aes_c_set_encrypt_key`) no longer needs to return an error code. The assembly functions still return an error code and do input validation. This commit also corrects the signature of `GFp_AES_set_encrypt_key`. I agree to license my contributions to each file under the terms given at the top of each file I changed.

ecstatic-morse · 2018-10-24T00:55:12Z

Whoops, sorry about that. I added the disclaimer to the commit message and made the requested changes.

Is the use (void) to explicitly ignore return values documented anywhere? I didn't see it in STYLE.md.

briansmith · 2018-10-24T05:09:26Z

Thanks!

No, I guess it isn't mentioned in the style guide. One reason is that it is unusual for us to ignore return values in the first place. We're only doing so here temporarily until the types of those functions change to be void.

FWIW, I'd normally write (void)x instead of (void) x but NBD.

briansmith reviewed Oct 11, 2018

View reviewed changes

crypto/fipsmodule/aes/aes.c Show resolved Hide resolved

briansmith reviewed Oct 11, 2018

View reviewed changes

briansmith reviewed Oct 17, 2018

View reviewed changes

briansmith mentioned this pull request Oct 18, 2018

AES test (and AES-GCM) fails on ARMv7 (RPi3) #628

Closed

ecstatic-morse force-pushed the return-convention branch 2 times, most recently from 2ec35ef to 69a6be1 Compare October 22, 2018 22:22

briansmith approved these changes Oct 23, 2018

View reviewed changes

briansmith reviewed Oct 23, 2018

View reviewed changes

briansmith requested changes Oct 23, 2018

View reviewed changes

ecstatic-morse force-pushed the return-convention branch from 69a6be1 to 38d54b9 Compare October 24, 2018 00:51

briansmith approved these changes Oct 24, 2018

View reviewed changes

briansmith merged commit 20cfa87 into briansmith:master Oct 24, 2018

ecstatic-morse mentioned this pull request Oct 24, 2018

Refactor C code for AES key expansion. #708

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make caller enforce invariants when setting AES encryption key #702

Make caller enforce invariants when setting AES encryption key #702

ecstatic-morse commented Oct 11, 2018

briansmith Oct 11, 2018

briansmith Oct 11, 2018

ecstatic-morse commented Oct 11, 2018 •

edited

Loading

briansmith Oct 17, 2018 •

edited

Loading

briansmith Oct 17, 2018

briansmith commented Oct 17, 2018

briansmith commented Oct 22, 2018

ecstatic-morse commented Oct 22, 2018

ecstatic-morse commented Oct 23, 2018 •

edited

Loading

briansmith Oct 23, 2018

briansmith Oct 23, 2018

briansmith commented Oct 23, 2018

briansmith left a comment

ecstatic-morse commented Oct 24, 2018

briansmith commented Oct 24, 2018

Make caller enforce invariants when setting AES encryption key #702

Make caller enforce invariants when setting AES encryption key #702

Conversation

ecstatic-morse commented Oct 11, 2018

briansmith Oct 11, 2018

Choose a reason for hiding this comment

briansmith Oct 11, 2018

Choose a reason for hiding this comment

ecstatic-morse commented Oct 11, 2018 • edited Loading

briansmith Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

briansmith Oct 17, 2018

Choose a reason for hiding this comment

briansmith commented Oct 17, 2018

briansmith commented Oct 22, 2018

ecstatic-morse commented Oct 22, 2018

ecstatic-morse commented Oct 23, 2018 • edited Loading

briansmith Oct 23, 2018

Choose a reason for hiding this comment

briansmith Oct 23, 2018

Choose a reason for hiding this comment

briansmith commented Oct 23, 2018

briansmith left a comment

Choose a reason for hiding this comment

ecstatic-morse commented Oct 24, 2018

briansmith commented Oct 24, 2018

ecstatic-morse commented Oct 11, 2018 •

edited

Loading

briansmith Oct 17, 2018 •

edited

Loading

ecstatic-morse commented Oct 23, 2018 •

edited

Loading