fix aesccm when using m2crypto

[inikolcev]

fixes #406

The first issue was that the CBC IV was not reset properly which is now handled by the setter.
The second issue was with the _8 ciphers where the authentication value was not padded before going through CTR.

---

This change is 

[tomato42]

Reviewed 1 of 2 files at r1.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @inikolcev)

a discussion (no related file):
test coverage for this?

---

---

tlslite/utils/aesccm.py, line 109 at r1 (raw file):

            assert self.tagLength == 8
            self._pad_with_zeroes(mac, 16)
            auth_value = self._ctr.encrypt(mac)[:-8]

why -8 and not 8?

[tomato42]

Reviewed 1 of 2 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @inikolcev)

[inikolcev]

I added tests that open/seal 2 messages one after another for both openssl and python and both 16 and 8 tags. This should help us catch errors like not proprely reseting IV or not properly incrementing the counter.

[inikolcev]

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @inikolcev and @tomato42)

---

tlslite/utils/aesccm.py, line 109 at r1 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

why -8 and not 8?

Changed it to 8

[tomato42]

Reviewed 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @inikolcev)

---

unit_tests/test_tlslite_utils_aesccm.py, line 410 at r3 (raw file):

            self.assertEqual(ciphertext, encData)

        def test_open_multiple_messages(self):

isn't this incorrectly indented?

---

unit_tests/test_tlslite_utils_aesccm.py, line 452 at r3 (raw file):

                                     b'\xda\xaa\xc0\xa9\'\xb3\xd5\x12\xa2\x1fF,'
                                     b'\x8e\x04\xf5{\xf8\xfdN\xfe\xe2\xe9x\xfe1'
                                     b'\x175\xa6\xc4\\Q3\x80\xf4\xcaR\x8c')]

those are the same variables as in test_seal_multiple_messages, wouldn't it be better to move those two tests to a separate class and use class variables to store them, to clearly show that one test is the inverse of the other?

---

unit_tests/test_tlslite_utils_aesccm.py, line 548 at r3 (raw file):

                                 b'\'\xb3\xd5\x12\xa2\x1fF,\x8e\x04'
                                 b'\xf5{\xf8\xfdN\xfe\xe2\xde\xb8\x0e'
                                 b'\xb6\xb6M+\x02')]

same here, redefines variables from test_seal_multiple_messages_small_tag instead of reusing them

---

unit_tests/test_tlslite_utils_aesccm.py, line 856 at r3 (raw file):

                                     b'\'\xb3\xd5\x12\xa2\x1fF,\x8e\x04'
                                     b'\xf5{\xf8\xfdN\xfe\xe2\xde\xb8\x0e'
                                     b'\xb6\xb6M+\x02')]

the openssl tests should definitely use the same variables as the python tests, to show that they create and process ciphertext the same way

[tomato42]

I added tests that open/seal 2 messages one after another for both openssl and python and both 16 and 8 tags. This should help us catch errors like not proprely reseting IV or not properly incrementing the counter.

how those tests do that? Don't we need to encrypt two same plaintexts to show that a new IV is used (as it will result in different ciphertexts)? Don't we need to encrypt two blocks of identical plaintext to show that the counter is incremented (as that will show two different blocks of ciphertext)?

[inikolcev]

I added tests that open/seal 2 messages one after another for both openssl and python and both 16 and 8 tags. This should help us catch errors like not proprely reseting IV or not properly incrementing the counter.

how those tests do that? Don't we need to encrypt two same plaintexts to show that a new IV is used (as it will result in different ciphertexts)? Don't we need to encrypt two blocks of identical plaintext to show that the counter is incremented (as that will show two different blocks of ciphertext)?

Here I'm talking about the CBC-MAC IV which should always be 0. This was why it was not working properly with openssl before, because after the first message it was not being reset to 0 properly, and the current tests didn't catch it because they only open/seal 1 message, and it never came to using the non 0 IV after the first message. Encrypting/decrypting 2 or more messages should ensure us that the IV is properly reset to 0.

[tomato42]

so encrypting the same plaintext with the same key and nonce wouldn't create the same ciphertext? so a test should actually use the same object to encrypt the same plaintext twice and compare outputs of those, to check if they are identical?

what about the counter?

[inikolcev]

Encrypting the same plaintext with the same key and same nonce will create the same ciphertext, why do you think it wouldn't?
Yeah encrypting the same plaintext twice is another way to do it, but it doesn't matter what the second plaintext is as long we encrypt two messages with the same object and get the correct ciphertext for the second message.

I was thinking the same way for the counter, it is reset after each message, if we regress down the line and not reset properly after the first message, this should catch it.
I'll rewrite the tests to reuse the variables later today.

[tomato42]

Encrypting the same plaintext with the same key and same nonce will create the same ciphertext, why do you think it wouldn't?

my point is that the old code wouldn't do that, would it? It would produce two different ciphertexts on encryption, no?

Yeah encrypting the same plaintext twice is another way to do it, but it doesn't matter what the second plaintext is as long we encrypt two messages with the same object and get the correct ciphertext for the second message.

yes, using known good second ciphertext will work, but the test doesn't make it apparent that:

• running the same instance over the same data will produce the same outputs
• that the second output is known-good

or to put it other way: yes, the test does test what it should test but it isn't obvious why it does test what it should

I was thinking the same way for the counter, it is reset after each message, if we regress down the line and not reset properly after the first message, this should catch it.

aah, so it's for handling possible bugs, not present ones, got it

[inikolcev]

Moved the tests to a separate class and made it to use the same plaintext/ciphertext twice.

[tomato42]

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @inikolcev)

---

unit_tests/test_tlslite_utils_aesccm.py, line 547 at r4 (raw file):

            self.assertEqual(self.plaintext, decData)

    if m2cryptoLoaded:

why not @unittest.skipUnless(m2cryptoLoaded)?

[inikolcev]

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @inikolcev)

unit_tests/test_tlslite_utils_aesccm.py, line 547 at r4 (raw file):

            self.assertEqual(self.plaintext, decData)

    if m2cryptoLoaded:

why not @unittest.skipUnless(m2cryptoLoaded)?

No reason really, just didn't think of it. I changed it to use skipUnless.

[tomato42]

Reviewed 1 of 1 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved

[tomato42]

looks good, feel free to merge as soon as Travis passes. Thanks!