bug: Allow sign and verify to use different hashes than sha256 #53
Conversation
This patch is dependent on two other library patches landing. * mpdavis/python-jose#39 * yann2192/pyelliptic#53 Temporarily pointing to personal forks with patches applied. closes #753
This patch is dependent on two other library patches landing. * mpdavis/python-jose#39 * yann2192/pyelliptic#53 Temporarily pointing to personal forks with patches applied. closes #753
This patch is dependent on two other library patches landing. * mpdavis/python-jose#39 * yann2192/pyelliptic#53 Temporarily pointing to personal forks with patches applied. closes #753
There was a problem hiding this comment.
The code change looks OK to me; I've got some nits you should consider.
That said, in a crypto interface library, I'd strongly recommend checking return values out of OpenSSL on the important calls. For that reason, I'm not going to hit 'Approve', but as it's not my project, I'll hold off on specifically requesting changes. :)
pyelliptic/openssl.py
Outdated
| @@ -100,6 +100,10 @@ def __init__(self, library): | |||
| self.BN_bin2bn.argtypes = [ctypes.c_void_p, ctypes.c_int, | |||
| ctypes.c_void_p] | |||
|
|
|||
| self.BN_bn2dec = self._lib.BN_bn2dec | |||
There was a problem hiding this comment.
Nit: I'm not clear on why this function is being exposed in this PR. It seems fine, but maybe add a comment to the commit?
pyelliptic/ecc.py
Outdated
| "sha512": OpenSSL.EVP_sha512(), | ||
| } | ||
| self.hashval = hasher | ||
| self.hasher = _hashers[hasher] |
There was a problem hiding this comment.
Nit: This is going to raise without an explanation if they pick an invalid hash; not sure how developer-friendly you're aiming to be.
pyelliptic/ecc.py
Outdated
| @@ -414,7 +424,7 @@ def sign(self, inputb): | |||
| raise Exception("[OpenSSL] EC_KEY_check_key FAIL ... " + OpenSSL.get_error()) | |||
|
|
|||
| OpenSSL.EVP_MD_CTX_init(md_ctx) | |||
| OpenSSL.EVP_DigestInit_ex(md_ctx, OpenSSL.EVP_sha256(), None) | |||
| OpenSSL.EVP_DigestInit_ex(md_ctx, self.hasher, None) | |||
There was a problem hiding this comment.
EVP_DigestInit_ex can fail (it returns 0)... that could happen if OpenSSL was built with SHA512 explicitly disabled. (That used to happen on ARMv5; probably not a huge concern...)
Still, good security coding practices say you should check the return whereever you can; that'd include the EVP_MD_CTX_init, too.
pyelliptic/ecc.py
Outdated
| if (OpenSSL.EVP_DigestUpdate(md_ctx, binputb, len(inputb))) == 0: | ||
| raise Exception("[OpenSSL] EVP_DigestUpdate FAIL ... " + OpenSSL.get_error()) | ||
|
|
||
| OpenSSL.EVP_DigestFinal_ex(md_ctx, digest, dgst_len) |
There was a problem hiding this comment.
If EVP_DigestFinal_ex returns 0, you shouldn't use the output digest.
pyelliptic/ecc.py
Outdated
| OpenSSL.EVP_MD_CTX_init(md_ctx) | ||
| OpenSSL.EVP_DigestInit_ex(md_ctx, OpenSSL.EVP_sha256(), None) | ||
| OpenSSL.EVP_DigestInit_ex(md_ctx, self.hasher, None) |
There was a problem hiding this comment.
Same as above about checking EVP_DigestInit and EVP_MD_CTX_init
pyelliptic/hash.py
Outdated
| d = OpenSSL.malloc(m, len(m)) | ||
| md = OpenSSL.malloc(0, 64) | ||
| i = OpenSSL.pointer(OpenSSL.c_int(0)) | ||
| OpenSSL.HMAC(OpenSSL.EVP_sha384(), key, len(k), d, len(m), md, i) |
There was a problem hiding this comment.
Nit: The retval should be checked... as should the other calls to HMAC.
|
@jcjones, thanks for the r? I'm always happy to get whatever nits or oversights folks are willing to offer, so thank you for your time. Will address and push a new version shortly. |
yann2192
force-pushed
the
master
branch
2 times, most recently
from
79de7ba to
7157714
Compare
closes #52