AES fixes #386
AES fixes #386
Conversation
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 2 of 2 files at r2, 1 of 1 files at r3.
Reviewable status:complete! all files reviewed, all discussions resolved
|
will need to be rebased on top of #387 |
t184256
force-pushed
the
aes-fixes
branch
4 times, most recently
from
4601cba
to
44e7ed1
Compare
|
Several pylint directive applications later, Travis is finally green now. |
There was a problem hiding this comment.
Reviewed 3 of 3 files at r4, 2 of 2 files at r5.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @t184256)
tlslite/utils/openssl_aes.py, line 15 at r5 (raw file):
class OpenSSL_AES(AES): # pylint disable=no-member
same here: why it's used, and it should be re-enabled
tlslite/utils/openssl_aes.py, line 17 at r5 (raw file):
# pylint disable=no-member def __init__(self, key, mode, IV): # pylint: enable=invalid-name
why it's a line comment, not on a line of its own?
tlslite/utils/python_aes.py, line 19 at r4 (raw file):
class Python_AES(AES): def __init__(self, key, mode, IV): # pylint: disable=invalid-name
- comment explaining why ignoring it is fine
- after the offending variable was dealt with, it should be re-enabled
tomato42
added
the
maintenance
Python_AES makes a copy of plaintext and mutates it with item assignment. Copying is currently done by slicing, and this assumes a datatype that allows item assignment. This commit replaces copying-by-slicing with copying by bytearray(...). It also converts key and IV to bytearrays, so that passing an str works on Python 2.
M2Crypto defaults to padding the ciphertext, and the previous implementation danced around that awkwardly by padding and unpadding ciphertext on decryption and updating the IV manually. `m2.cipher_set_padding(context, 0)` allows to shoulder the IV handling back to where it belongs and to get rid of unnecessary context reinitializations. (Investigative work courtesy of @tomato42: tlsfuzzer#377 (review)). This commit mirrors changes from tlsfuzzer#377, but for AES and adds the same unit test as well.
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @tomato42)
tlslite/utils/openssl_aes.py, line 15 at r5 (raw file):
Previously, tomato42 (Hubert Kario) wrote…
same here: why it's used, and it should be re-enabled
added a comment, it has block scoping
tlslite/utils/openssl_aes.py, line 17 at r5 (raw file):
Previously, tomato42 (Hubert Kario) wrote…
why it's a line comment, not on a line of its own?
it was line-scoped, moved to block scoping to cover both argname and field. I think it's fine-grained enough with the block scope.
tlslite/utils/python_aes.py, line 19 at r4 (raw file):
Previously, tomato42 (Hubert Kario) wrote…
- comment explaining why ignoring it is fine
- after the offending variable was dealt with, it should be re-enabled
added a comment, it's block-scoped (and I don't think narrowing it further is worth the resulting pragma noise)
There was a problem hiding this comment.
Reviewed 3 of 3 files at r6, 2 of 2 files at r7.
Reviewable status:
|
looks good, thanks! |
Basically most of the #377, but for AES.
This change is