CTR mode tries to be both a stream and block cipher and fails at both

Review Request #2458 — Created Nov. 19, 2019 and submitted


CTR mode doesn't work with segment sizes other than AES_BLOCK_LEN. As it is a stream cipher, it should work with any sized input, and as a stream cipher shouldn't accumulate any input data between calls -- it should always be able to act upon an arbitrary size input and immediately produce output.

Updated crypto tests to test different segment sizes, ran updated crypto tests (with and without this change) to verify it works.

  • 1
  • 0
  • 10
  • 0
  • 11
Description From Last Updated
Previously we cared about pData being NULL, but now we don't care about it at all and are instead only ... rm rm
  2. usr/src/common/crypto/modes/ctr.c (Diff revision 1)
    In should probably be const as we're never going to modify it, right?
  3. usr/src/common/crypto/modes/ctr.c (Diff revision 1)
    It seems like this can afford to be const as we should never be modifying the key from this function and this is just used for doing xor computations.
  4. usr/src/common/crypto/modes/ctr.c (Diff revision 1)
    Why don't we just design this such that we get us to 4-byte alignment, then do 4-byte work, and then when it's not finish it up, rather than doing the architecture specific stuff. This way, even the other archs will get the benefit and x86 will perform better as well. Just because you can do unaligned access on x86 doesn't mean that it's performant to do so.
    I realize that we won't always be able to do this kind of stuff. But still, seems like it's kind of useful to try and phrase this towards getting us to 4-byte aligned stuff if we can.
    1. The other modes aren't as fancy -- they either do 32-bit if aligned, or per-byte if not, though no reason we can't be better here (and the code is different enough being a stream mode vs block mode hat I think we can justify being different here.

  5. usr/src/common/crypto/modes/ctr.c (Diff revision 1)

    in32 should probably be const as we're never going to modify it, right? Same with key32 in this context.

  6. usr/src/common/crypto/modes/ctr.c (Diff revision 1)
    I think it would be clearer if you indicated that data and length were related to the input as part of their naming.
  7. usr/src/common/crypto/modes/ctr.c (Diff revision 1)
    Can we use more descriptive names for these? It's weird, for example, that out_data_2 is unused. At least, naively.
    1. I'll try to think of something -- though the usage matches the use by all the other modes and by the crypto_{get,init}_ptrs functions as well (though I agree the naming for all of them leaves something to be desired).

  8. usr/src/common/crypto/modes/ctr.c (Diff revision 1)
    Do we need overflowing checking for this?
    1. It probably wouldn't hurt, though all of the modes have a variation of this (and probably should also have the relevant checks). It might be better to have a separate ticket and address all of them instead of piecemeal.

    2. It's hard to tell from all the refactoring, but it seems like this is new code. I don't believe we should punt on correctness of new code even if there are other existing pieces that have similar issues.

  9. Previously we cared about pData being NULL, but now we don't care about it at all and are instead only looking at pEncryptedData for this. Does that matter?
    1. No. The new code has the same net effect. The callers into this ensure if pData == NULL, that ulDataLen == 0, which means 0 bytes will be processed and *pulEncryptedDataLen is set to 0. I'll add something like IMPLY(pData == NULL, ulDataLen == 0) (this is true for all callers) to hopefully make it clearer.

    2. I don't believe they're the same. You've changed the behavior that occurs when pData is NULL and uDataLen is non-zero. Previously this would hit the case that you've changed and return zero. However, in the current code if you pass pData as NULL, but ulDataLen is not NULL, if DEBUG is set, we'll abort (which doesn't seem good given the old behavior) or we'll segfault in ctr_xor.

    3. C_Encrypt() guarantees (pData == NULL && ulDataLen == 0) || (pData != NULL) and C_EncryptUpdate() guarantees pdata != NULL. The other internal calls always call with a non-NULL value of pData. I could rewrite the IMPLY into VERIFY((pData != NULL) || (pData == NULL && ulDataLen == 0) if that would be better. I think some sort of assertion (VERIFY/IMPLY/etc) should be ok here -- this is an internal function, so the external interfaces should be doing the checking the arguments and returning errors.

    4. Fair enough. If you're confident that this should always be handled by callers and we shouldn't gracefully handle it and instead abort, I'll defer to you. I'll admit it still feels a bit weird, given that we can properly error out if it happens.
  10. usr/src/uts/common/crypto/io/aes.c (Diff revision 1)
    Shouldn't this have the same assert as the default case? I presume we really should move it to be treated like the default case.
    1. This is part of the 'tries to be a block cipher' part of the bug. The original code was buffering input data into AES_BLOCK_LEN sized chunks (but messing up the ctr_mode_final call), and so was using ac_remainder_len. The new CTR code does not buffer or segment data, so it never uses ac_remainder_len. It won't hurt to add assert (I'll go ahead and do that), but add a note that unlike the other cases (where the assert is used to ensure no unprocessed data remains), in this instance it is ensuring that no data was ever buffered between calls to aes_{en,de}crypt_contiguous_blocks

  11. usr/src/uts/common/crypto/io/aes.c (Diff revision 1)
    I'd strongly recommend making this an ASSERT3U since the other value comes from the stack.
  1. Thank you for the additional comments.

  2. usr/src/common/crypto/modes/ctr.c (Diff revisions 2 - 4)
    larger than an individual (s/a/an)
  1. Ship It!
  1. I also diffed this patch against -gate vs. what is in -joyent currently, and see one of the main changes is allowing XOR-stream bits to get synched from unaligned to aligned. I saw some ASSERT->ASSERT3U, disambiguating TRY32 (it does read better the new way) as part of the synching, and some other commenting.

Review request changed

Status: Closed (submitted)