Heavy scanner refactoring

[daveschaefer]

While investigating #34 a long time back I found a number of upgrades and fixes we should make to the scanner code. Creating a ticket to track this. I have a local branch with a lot of code that needs to be cleaned up and properly tested.

Fixes include:

• Extract hex strings to constants to document and explain what they mean (easier to understand and maintain)
• Extract list of advertised ciphers to a more usable form that is maintainable, understandable, and documentable
• Fix SNI scans to use localtime rather than hard-coded timestamp
• Fix SNI scans to use random data on each handshake rather than hardcoded value
• Add more, newer, cipher suites so servers don't reject connections with 'Code 40 Handshake Failure' when they find no acceptable ciphers
• Remove compression support to avoid CRIME exploits
• Fix bug: send TLS 'close_notify' record before closing socket (required by RFC 5246)
• Fix bug: only close scanning socket once
• Move fingerprint calculation to function, so other fingerprint types can easily be calculated on the server's certificate

[kuba]

Is it really necessary to implement our own SSL/TLS client code? I don't think the performance gain is big enough... And you will probably agree with me, that the current code base is totally unmaintainable, prone to bugs and errors, and, as far as I can see from the bug description, non-compliant with the standard. Is there any other reason we shouldn't stop using it?

I'd strongly suggest using pyOpenSSL or Python's ssl module, though the latter supports SNI only since 2.7.9, which is a bit problematic. Also, c.f. https://github.com/moxie0/Convergence/blob/master/server/convergence/verifier/NetworkPerspectiveVerifier.py.

That being said, I've already coded down initial implementation (might need some cleaning before release, but stay tuned) and appropriate benchmarks could be performed. Btw, has anyone already done similar comparative testing?

[danwent]

Back a long time ago, we did measurements that the hacked SSL handshake significantly reduced both CPU + Network BW usage by the scanner in comparison to the original model, which shelled out to the openssl binary. In particular, it helped avoid some crypto computation that openssl was doing as part of later parts of the SSL handshake. I don't have any of the data around, so it may make sense to re-run the experiments.

Dan

On Fri, Nov 14, 2014 at 10:41 AM, Jakub Warmuz notifications@github.com wrote:

Is it really necessary to implement our own SSL/TLS client code? I don't think the performance gain is big enough... And you will probably agree with me, that the current code base is totally unmaintainable, prone to bugs and errors, and, as far as I can see from the bug description, non-compliant with the standard. Is there any other reason we shouldn't stop using it?

I'd strongly suggest using pyOpenSSL https://github.com/pyca/pyopenssl or Python's ssl module https://docs.python.org/2/library/ssl.html, though the latter supports SNI only since 2.7.9, which is a bit problematic. Also, c.f. https://github.com/moxie0/Convergence/blob/master/server/convergence/verifier/NetworkPerspectiveVerifier.py .

That being said, I've already coded down initial implementation (might need some cleaning before release, but stay tuned) and appropriate benchmarks could be performed. Btw, has anyone already done similar comparative testing?

— Reply to this email directly or view it on GitHub https://github.com/danwent/Perspectives-Server/issues/45#issuecomment-63109611 .

Dan Wendlandt
650-906-2650

[daveschaefer]

@danwent Hey Dan, do you have any of the test harnesses or setup you used to run those performance comparison tests? Or did you do all of the comparison by hand? How did you do the profiling?

[daveschaefer]

@kuba I'm certainly open to moving to some other method of grabbing certificates if it's better, more maintainable, etc. The main problem I run into right now is that the scanner portion is not very testable - it is difficult to make sure the scanner can properly retrieve certificates from many types of servers (e.g. ones that only accept certain SSL/TLS protocols). I agree that if we weren't rolling our own TCP stack and trying to keep it compliant with specs, etc. that would probably mean a lot fewer bugs.

I'd be very interested to see what you mean/have by "coding your own implementation". The patch I have been working on for this ticket was mainly intended to make the code maintainable again by removing magic numbers and hex string constants, and organize things into classes. I could post the pull request if you are interested in seeing it as part of the discussion. It still needs work before I would feel comfortable merging it though.

Ideally, if we could build/add a way to make the scanner component more testable that would add value regardless of which solution we end up with. i.e. some external unit tests to verify functionality, support of protocols, etc. It could also help us to compare different solutions because we'd be able to make sure they were all correct and make it faster to run performance tests.

[daveschaefer]

An interesting post here about not actually using a time value for the 'time' field in ClientHello, but rather more random data.

http://security.stackexchange.com/questions/85082/do-chrome-and-firefox-send-random-values-rather-than-the-actual-timestamp-in-cli