New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DSA signatures in SKE #425
DSA signatures in SKE #425
Conversation
This pull request introduces 1 alert when merging 551139b into 9951ec1 - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Hedgehog5040)
tlslite/keyexchange.py, line 195 at r1 (raw file):
hashBytes = serverKeyExchange.hash(clientRandom, serverRandom)
don't we have to verify that the signature matches the hashBytes
?
tlslite/messages.py, line 1571 at r1 (raw file):
if self.cipherSuite in CipherSuite.certAllSuites or \ self.cipherSuite in CipherSuite.ecdheEcdsaSuites or \ self.cipherSuite in CipherSuite.dheDsaSuites:
technically this is part of the "making DSA signatures in SKE" :) but i'ts fine if it will make writing test cases easier
unit_tests/test_tlslite_keyexchange.py, line 364 at r1 (raw file):
None) class TestServerKeyExchangeDSA(unittest.TestCase):
- Negative test case missing, we should have one with malformed signature
- since in TLS 1.1 and earlier DSA SKEs are special, we should have tests with them too
- we should also have a test to verify that a SKE message with correct signature will be rejected when we have the hash it used disabled
unit_tests/test_tlslite_keyexchange.py, line 390 at r1 (raw file):
def test_verify_dsa_signature_in_TLS1_2_SHA384(self): skemsg = a2b_hex("0001b90080b10b8f96a080e01dde92de5eae5d54ec52c99fbcfb06a3c69a6a9dca52d23b616073e28675a23d189838ef1e2ee652c013ecb4aea906112324975c3cd49b83bfaccbdd7d90c4bd7098488e9c219a73724effd6fae5644738faa31a4ff55bccc0a151af5f0dc8b4bd45bf37df365c1a65e68cfda76d4da708df1fb2bc2e4a43710080a4d1cbd5c3fd34126765a442efb99905f8104dd258ac507fd6406cff14266d31266fea1e5c41564b777e690f5504f213160217b4b01b886a5e91547f9e2749f4d7fbd7d3b9a92ee1909d0d2263f80a76a6a24c087a091f531dbf0a0169b6a28ad662a4d18e73afa32d779d5918d08bc8858f4dcef97c2a24855e6eeb22b3b2e500807c4707b1ebadeafb719be08d627b90e2cd07ccf91aacd62b8803399d425766e2d87a82824edf2a507feb56ed417cf452600749c70d4cf7f5cc22a074d63ea6e39871a8613e36a717d5d1672407ae6761d1527821ee6ab4f00c92ec42ec0da07bcaa33b287225202d5bfbafc377c41a42e4cacf80c978390ecd98c3316021c79d0402002f302d0215008ed71b80e574d68ae9685ba36b191671dd7d9f9402147cc8b92935fc9a98332676830abeabe2a3b25f90")
please split up this line; while we have monitors that can fit more than 80 columns, when resolving merge conflicts it's necessary to look at 240 columns of text
unit_tests/test_tlslite_messages.py, line 2394 at r1 (raw file):
b'\xbaQ\xedo\xa13Z\xa5}')) def test_hash_with_dsa_in_tls1_1(self):
we should also have unit tests for serialising and de-serialising DHE DSS SKEs, similar to test_parser_with_ECDH
and test_createECDH
551139b
to
410ae90
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:( sorry, I don't see anything obviously wrong, I'll need to run this code tomorrow and check what's what
Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Hedgehog5040)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Hedgehog5040)
unit_tests/test_tlslite_keyexchange.py, line 401 at r2 (raw file):
"ad6b382013b6bc6ed3044fbbab0b2f9d26bcce3bae0402002f302d021500b5" "dd272c8ca096c5d5ea999ea0d369aecd492441021440bd8a5f340991b41c17" "f9d0724f3ca8ded4f5d7")
it's weird, when I extract the params and signature, save them to file and try to verify with openssl, the verification is successful
but python_dsakey can't verify it, at the same time, I can make and verify signatures with python_dsakey and they do verify with openssl...
At this point I'd suggest adding more test vectors to the DSA implementation
unit_tests/test_tlslite_messages.py, line 2397 at r2 (raw file):
ske = ServerKeyExchange( CipherSuite.TLS_DHE_DSS_WITH_AES_128_CBC_SHA256, (3, 1))
(3, 1) == TLS 1.0; but the ciphersuite is TLS 1.2 only and the hashAlg is set below
unit_tests/test_tlslite_messages.py, line 2405 at r2 (raw file):
self.assertEqual(hash1, bytearray(b'\x4a\x95\x6f\x85\x4e\xed\x6c\x5f\x84\x17\x92\xd6\xa8\x8d\x8c\xd7\xb2\xd0\xf7\xfb'))
this is a hash of this SKE with SHA1, not SHA256
I remember that there were some interoperability issues with TLS 1.2... maybe try two things: capture SKE in TLS 1.1 or TLS 1.0, and one with forced SHA-1 signatures in TLSv1.2 |
410ae90
to
bcf080b
Compare
This pull request introduces 1 alert when merging bcf080b into 08467ca - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the failures on python2.6 need fixes, looks like some call is missing compatHMAC()
Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Hedgehog5040)
tlslite/utils/python_dsakey.py, line 16 at r3 (raw file):
from .dsakey import DSAKey import math
unused import
tlslite/utils/python_dsakey.py, line 84 at r3 (raw file):
N = int(numBits(self.q) / 8) hashData = hashData[:N] digest = bytesToNumber(hashData)
what if bit length of q isn't a multiple of 8?
getRandomPrime
won't generate ones like this, but other implementations can
unit_tests/test_tlslite_keyexchange.py, line 386 at r3 (raw file):
cls.x509 = x509 def test_verify_dsa_signature_in_TLS1_1_SHA1(self):
technically, this is TLS 1.0 ((3, 0)
==SSL3, (3, 1)
== TLS1.0, (3, 2)
==TLS1.1)
unit_tests/test_tlslite_keyexchange.py, line 419 at r3 (raw file):
server_random, [(HashAlgorithm.sha256, SignatureAlgorithm.dsa)])
for TLS 1.0 we should be able to skip the allowed algorithms here... or do I remember the verifyServerKeyExchange code wrong?
tlslite/utils/python_dsakey.py, line 16 at r3 (raw file): Previously, tomato42 (Hubert Kario) wrote…
Done. |
74a553c
to
2929b0b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 5 of 7 files reviewed, 7 unresolved discussions (waiting on @Hedgehog5040 and @tomato42)
tlslite/utils/python_dsakey.py, line 84 at r3 (raw file):
Previously, tomato42 (Hubert Kario) wrote…
what if bit length of q isn't a multiple of 8?
getRandomPrime
won't generate ones like this, but other implementations can
Done.
unit_tests/test_tlslite_keyexchange.py, line 401 at r2 (raw file):
Previously, tomato42 (Hubert Kario) wrote…
it's weird, when I extract the params and signature, save them to file and try to verify with openssl, the verification is successful
but python_dsakey can't verify it, at the same time, I can make and verify signatures with python_dsakey and they do verify with openssl...At this point I'd suggest adding more test vectors to the DSA implementation
Done.
unit_tests/test_tlslite_keyexchange.py, line 386 at r3 (raw file):
Previously, tomato42 (Hubert Kario) wrote…
technically, this is TLS 1.0 (
(3, 0)
==SSL3,(3, 1)
== TLS1.0,(3, 2)
==TLS1.1)
Done.
unit_tests/test_tlslite_keyexchange.py, line 419 at r3 (raw file):
Previously, tomato42 (Hubert Kario) wrote…
for TLS 1.0 we should be able to skip the allowed algorithms here... or do I remember the verifyServerKeyExchange code wrong?
Done.
This pull request introduces 1 alert when merging 2929b0b into 9f2a19e - view on LGTM.com new alerts:
|
unit_tests/test_tlslite_keyexchange.py, line 364 at r1 (raw file): Previously, tomato42 (Hubert Kario) wrote…
What do you mean by hash disabled? |
Not passed in as one of the valid ones I've noticed that we'll also need changes to HandshakeSettings, to have something like |
2929b0b
to
a81a94b
Compare
@Hedgehog5040 the CI failed... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Hedgehog5040 and @tomato42)
tlslite/utils/python_dsakey.py, line 85 at r4 (raw file):
digest = bytesToNumber(hashData) if N < digest_len: digest = (digest & (~0 << (digest_len - N))) >> (digest_len - N)
wouldn't digest >>= digest_len - N
be simpler? why AND it against ~0
?
tlslite/utils/python_dsakey.py, line 99 at r4 (raw file):
if N < digest_len: digest = (digest & (~0 << (digest_len - N))) >> (digest_len - N)
same here, digest >>= digest_len -N
would be simpler, wouldn't it?
5dc813f
to
866e344
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 5 of 10 files reviewed, 7 unresolved discussions (waiting on @Hedgehog5040 and @tomato42)
tlslite/utils/python_dsakey.py, line 85 at r4 (raw file):
Previously, tomato42 (Hubert Kario) wrote…
wouldn't
digest >>= digest_len - N
be simpler? why AND it against~0
?
Done.
tlslite/utils/python_dsakey.py, line 99 at r4 (raw file):
Previously, tomato42 (Hubert Kario) wrote…
same here,
digest >>= digest_len -N
would be simpler, wouldn't it?
Done.
unit_tests/test_tlslite_keyexchange.py, line 364 at r1 (raw file):
Previously, tomato42 (Hubert Kario) wrote…
Not passed in as one of the valid ones
I've noticed that we'll also need changes to HandshakeSettings, to have something like
rsaSigHashes
andecdsaSigHashes
for DSA
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 5 files at r5.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Hedgehog5040)
a discussion (no related file):
do we have a test case to verify we can actually negotiate DSA? I don't see one in tests/tlstest.py
... or the implementation is still limited to just verifying signatures, not making them?
tlslite/tlsconnection.py, line 4429 at r5 (raw file):
is "dsa"
is
should not be used with literals, use ==
tlslite/tlsconnection.py, line 4432 at r5 (raw file):
for schemeName in settings.dsaSigHashes: if version > (3, 3): continue;
nit: leftover ;
tlslite/tlsconnection.py, line 4434 at r5 (raw file):
continue; if version < (3, 3) and hashName != "sha1":
do we reach this code in TLS 1.1? when?
tlslite/tlsconnection.py, line 4435 at r5 (raw file):
if version < (3, 3) and hashName != "sha1": continue;
nit: leftover;
tlslite/utils/python_dsakey.py, line 85 at r4 (raw file):
Previously, Hedgehog5040 (Frantisek Krenzelok) wrote…
Done.
I don't see this updated code...
tlslite/utils/python_dsakey.py, line 99 at r4 (raw file):
Previously, Hedgehog5040 (Frantisek Krenzelok) wrote…
Done.
same here
tlslite/utils/python_dsakey.py, line 101 at r5 (raw file):
digest = (digest & (~0 << (digest_len - N))) >> (digest_len - N) signature = bytes(signature) #fix for python 2.6
I think we want utils.compat.compatHMAC()
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Hedgehog5040)
a discussion (no related file):
remember to rebase on top of master
866e344
to
420a65f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 7 of 10 files reviewed, 7 unresolved discussions (waiting on @Hedgehog5040 and @tomato42)
a discussion (no related file):
Previously, tomato42 (Hubert Kario) wrote…
remember to rebase on top of master
Done.
tlslite/tlsconnection.py, line 4429 at r5 (raw file):
Previously, tomato42 (Hubert Kario) wrote…
is "dsa"
is
should not be used with literals, use==
Done.
tlslite/tlsconnection.py, line 4434 at r5 (raw file):
Previously, tomato42 (Hubert Kario) wrote…
do we reach this code in TLS 1.1? when?
Done.
tlslite/utils/python_dsakey.py, line 101 at r5 (raw file):
Previously, tomato42 (Hubert Kario) wrote…
I think we want
utils.compat.compatHMAC()
here
Done.
420a65f
to
fb7c97f
Compare
fb7c97f
to
9c1cb12
Compare
digest_size = numBits(digest) | ||
|
||
# extract min(|hAlg|, N) left bits of digest | ||
def verify(self, signature, hashData): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function verify
has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.
tlslite/handshakesettings.py
Outdated
if not other.rsaSigHashes and not other.ecdsaSigHashes and \ | ||
other.maxVersion >= (3, 3): | ||
not other.dsaSigHashes and other.maxVersion >= (3, 3): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple spaces after keyword
tlslite/keyexchange.py
Outdated
@@ -183,6 +183,19 @@ def _tls12_verify_ecdsa_SKE(serverKeyExchange, publicKey, clientRandom, | |||
raise TLSDecryptionFailed("Server Key Exchange signature " | |||
"invalid") | |||
|
|||
@staticmethod | |||
def _tls12_verify_dsa_SKE(serverKeyExchange, publicKey, clientRandom, | |||
serverRandom, validSigAlgs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Continuation line over-indented for visual indent
tlslite/keyexchange.py
Outdated
|
||
elif serverKeyExchange.signAlg == SignatureAlgorithm.dsa: | ||
return KeyExchange._tls12_verify_dsa_SKE(serverKeyExchange, | ||
publicKey, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Continuation line over-indented for visual indent
tlslite/keyexchange.py
Outdated
elif serverKeyExchange.signAlg == SignatureAlgorithm.dsa: | ||
return KeyExchange._tls12_verify_dsa_SKE(serverKeyExchange, | ||
publicKey, | ||
clientRandom, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Continuation line over-indented for visual indent
tlslite/keyexchange.py
Outdated
return KeyExchange._tls12_verify_dsa_SKE(serverKeyExchange, | ||
publicKey, | ||
clientRandom, | ||
serverRandom, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Continuation line over-indented for visual indent
tlslite/keyexchange.py
Outdated
publicKey, | ||
clientRandom, | ||
serverRandom, | ||
validSigAlgs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Continuation line over-indented for visual indent
@@ -4425,6 +4425,15 @@ def _sigHashesToList(settings, privateKey=None, certList=None, | |||
sigAlgs.append((getattr(HashAlgorithm, hashName), | |||
SignatureAlgorithm.ecdsa)) | |||
|
|||
|
|||
if not certType or certType == "dsa": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too many blank lines (2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 3 files at r7, 6 of 6 files at r8.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @Hedgehog5040)
tlslite/constants.py, line 1335 at r8 (raw file):
keyExchangeSuites += CipherSuite.srpCertSuites if "srp_sha_dsa" in keyExchangeNames: keyExchangeSuites += CipherSuite.srpDsaSuites
if they're unsupported, they shouldn't be added here
tlslite/keyexchange.py, line 113 at r8 (raw file):
serverKeyExchange.signAlg = SignatureAlgorithm.dsa serverKeyExchange.hashAlg = getattr(HashAlgorithm, sigHash) keyType = 'rsa'
this is a dsa method...
unit_tests/test_tlslite_keyexchange.py, line 278 at r8 (raw file):
def test_signServerKeyExchange_with_sha1_dsa_in_TLS1_2:
empty test case
tlslite/handshakesettings.py, line 532 at r6 (raw file): Previously, codeclimate[bot] wrote…
Done. |
d57e316
to
7eda6d7
Compare
This pull request introduces 2 alerts when merging 7eda6d7 into e86a285 - view on LGTM.com new alerts:
|
please remember to rebase when submitting a new version, as we switched CI runners |
7eda6d7
to
e6a2447
Compare
This pull request introduces 2 alerts when merging e6a2447 into ba2b058 - view on LGTM.com new alerts:
|
e6a2447
to
f5c2b5b
Compare
This pull request introduces 2 alerts when merging f5c2b5b into ba2b058 - view on LGTM.com new alerts:
|
Look like the py3.8 run fails because of codeclimate/test-reporter#453, could you create a new PR that runs the codeclimate submission based on the environment var being set or not? |
ok, should be fixed now, please try rebasing |
f5c2b5b
to
cf018cb
Compare
This pull request introduces 2 alerts when merging cf018cb into a6cde37 - view on LGTM.com new alerts:
|
cf018cb
to
f79b5d4
Compare
you need to add those certificates in the same commit as you add the tests |
27b9afc
to
fb5935b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 5 of 18 files reviewed, 15 unresolved discussions (waiting on @codeclimate[bot] and @tomato42)
tlslite/handshakesettings.py, line 514 at r8 (raw file):
not other.dsaSigHashes and o
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 3 files at r9, 6 of 9 files at r10, 6 of 6 files at r11.
Dismissed @codeclimate[bot] from 3 discussions.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @codeclimate[bot] and @Hedgehog5040)
tests/tlstest.py, line 852 at r11 (raw file):
test_no += 1 print("Test {0} - good X.509 DSA, SSLv3".format(test_no))
could you add TLS 1.2? since the hash algorithm is negotiated there, the handshake is different...
tlslite/handshakesettings.py, line 526 at r10 (raw file):
unknownSigHash = not_matching(other.dsaSigHashes, ALL_RSA_SIGNATURE_HASHES)
shouldn't this be DSA_SIGNATURE_HASHES
? we don't want to support MD5 signatures for DSA
tlslite/tlsconnection.py, line 1868 at r11 (raw file):
else: # for RSA and DSA keys if len(publicKey) < settings.minKeySize:
this is correct, but we should update the minKeySize and maxKeySize description to include reference to DSA
unit_tests/test_tlslite_keyexchange.py, line 361 at r10 (raw file):
return_value=bytearray(b'wrong')) keyExchange.signServerKeyExchange(server_key_exchange) """
that's a commented out test case
fb5935b
to
5391770
Compare
5391770
to
88d9c81
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 15 of 18 files reviewed, 11 unresolved discussions (waiting on @codeclimate[bot] and @tomato42)
tests/tlstest.py, line 852 at r11 (raw file):
Previously, tomato42 (Hubert Kario) wrote…
could you add TLS 1.2? since the hash algorithm is negotiated there, the handshake is different...
Done.
tlslite/handshakesettings.py, line 526 at r10 (raw file):
Previously, tomato42 (Hubert Kario) wrote…
shouldn't this be
DSA_SIGNATURE_HASHES
? we don't want to support MD5 signatures for DSA
Done.
tlslite/tlsconnection.py, line 1868 at r11 (raw file):
Previously, tomato42 (Hubert Kario) wrote…
this is correct, but we should update the minKeySize and maxKeySize description to include reference to DSA
Done.
unit_tests/test_tlslite_keyexchange.py, line 361 at r10 (raw file):
Previously, tomato42 (Hubert Kario) wrote…
that's a commented out test case
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r12.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @codeclimate[bot] and @Hedgehog5040)
a discussion (no related file):
so it looks good, I can use openssl client and get a connection with DSA cert on tlslite-ng side, but it doesn't work with TLS 1.0 or with TLS 1.2 and SHA-1 signatures
i.e.
openssl s_client -connect localhost:4433 -tls1_2 -cipher aDSS -sigalgs SHA256+DSA
works
but
openssl s_client -connect localhost:4433 -tls1_2 -cipher aDSS -sigalgs SHA1+DSA
and
openssl s_client -connect localhost:4433 -tls1 -cipher aDSS
don't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @codeclimate[bot])
a discussion (no related file):
Previously, tomato42 (Hubert Kario) wrote…
so it looks good, I can use openssl client and get a connection with DSA cert on tlslite-ng side, but it doesn't work with TLS 1.0 or with TLS 1.2 and SHA-1 signatures
i.e.
openssl s_client -connect localhost:4433 -tls1_2 -cipher aDSS -sigalgs SHA256+DSA
works
butopenssl s_client -connect localhost:4433 -tls1_2 -cipher aDSS -sigalgs SHA1+DSA
and
openssl s_client -connect localhost:4433 -tls1 -cipher aDSS
don't
scratch that, with master
I need to specify :@SECLEVEL=0
for SHA-1 to work
Looks good, thank you! |
Work towards #99
This change is