droe / sslsplit

Transparent SSL/TLS interception
https://www.roe.ch/SSLsplit
BSD 2-Clause "Simplified" License
1.75k stars 328 forks source link

Deliberate access after free tests and LibreSSL 2.8.2 #241

Closed sonertari closed 5 years ago

sonertari commented 5 years ago

Shall we disable these tests or keep them to detect if/when a future LibreSSL version supports them? (These tests fail with the latest LibreSSL 2.8.2 on OpenBSD 6.4 too.)

Before:

97%: Checks: 140, Failures: 3, Errors: 0
cert.t.c:66:F:cert_refcount_inc:cert_refcount_inc_01:0: refcount mismatch
cachefkcrt.t.c:118:F:cache_fkcrt:cache_fkcrt_04:0: refcount != 0
cachetgcrt.t.c:115:F:cache_tgcrt:cache_tgcrt_04:0: refcount != 0

After:

100%: Checks: 140, Failures: 0, Errors: 0

Also, the second commit fixes a warning on OpenBSD 6.4. Shall we fix it now or later?

sonertari commented 5 years ago

Hmm, I have just realized that if the system time is in the past and we get the following build warning:

gmake[1]: warning:  Clock skew detected.  Your build may be incomplete.

the tests give the following failures (note that I always clean before build, so the build must be complete):

98%: Checks: 140, Failures: 2, Errors: 0
cachefkcrt.t.c:61:F:cache_fkcrt:cache_fkcrt_01:0: cache did not return a certificate
cachefkcrt.t.c:105:F:cache_fkcrt:cache_fkcrt_04:0: refcount != 3

After fixing the system time, the tests are fine again, as before, without any failures. It seems like the test above is clock dependent.

Now I wonder if some of the test failures I had reported on #203 were related with this clock skew. Is there anything in the SSL libraries that may cause such failures if the system time is wrong (I haven't tried OpenSSL with clock skew yet)? Is it about the cert cache?

droe commented 5 years ago

The forged cert cache does a ssl_x509_is_valid() (which uses X509_cmp_current_time()) on retrieval which compares the retrieved cert against current time to determine if the cached cert can be reused or not. If your clock is not monotonous, this may fail.

sonertari commented 5 years ago

I have turned off ntpd, so the clock must be monotonous, but the same result. This happens if the time is a few days in the past, not in the future, so the ntpd adjustments would make the clock monotonously increasing anyway. Nobody should be running sslsplit with a system time so far in the past, but I'm still looking.

droe commented 5 years ago

The test uses extra/pki/rsa.crt, so if the date on that is not fresh this can fail too. This is not removed by a make clean on top level.

sonertari commented 5 years ago

You are spot on Daniel, thanks. Removing just rsa.crt forces make to recreate it in sync with the system time, and the failures disappear.

However, running make clean under extra/pki:

$ make -f BSDmakefile clean
rm -rf rsa.* dsa.* ec.* dh*.param targets *.srl session.pem server.*

and then running the tests gives:

94%: Checks: 140, Failures: 8, Errors: 0
cachedsess.t.c:89:F:cache_dsess:cache_dsess_01:0: creating session failed
cachedsess.t.c:106:F:cache_dsess:cache_dsess_02:0: creating session failed
cachedsess.t.c:120:F:cache_dsess:cache_dsess_03:0: creating session failed
cachedsess.t.c:137:F:cache_dsess:cache_dsess_04:0: creating session failed
cachessess.t.c:84:F:cache_ssess:cache_ssess_01:0: creating session failed
cachessess.t.c:104:F:cache_ssess:cache_ssess_02:0: creating session failed
cachessess.t.c:121:F:cache_ssess:cache_ssess_03:0: creating session failed
cachessess.t.c:141:F:cache_ssess:cache_ssess_04:0: creating session failed

Is make cleaning too much under extra/pki? Syncing with the repo fixes these failures, but I didn't try to find which file it is (session.pem?).

Btw, just FYI, my test system is running on a VM, and its clock is almost always in the past, because I suspend and resume it based on what I am working on at the moment.

droe commented 5 years ago

Those two test sets use extra/pki/session-libressl-2.5.0.pem or extra/pki/session.pem depending on OpenSSL/LibreSSL version. Those two files are tricky to generate reliably and portably, which is why I added them to the repo. If make in extra/pki fails to regenerate session.pem on your box, you probably want to get it back by using git checkout.

droe commented 5 years ago

I'll look into tweaking the extra/pki make file slightly tonight.

sonertari commented 5 years ago

I will fix my system time or cert files before running the tests, which is not something I do often anyway, no worries. I was just worried if there is an issue with the cert cache or the tests, that's all.

droe commented 5 years ago

Also see 209ef11bb0e63681e54a9d74ea635bc6048450a3, session.pem will now be built when running make test while it is missing.

sonertari commented 5 years ago

Thanks Daniel, your last commit recreates session.pem if missing, as expected. But I still think that in general make shouldn't remove any repo files, hence https://github.com/droe/sslsplit/pull/241/commits/a3ce5d5835bfa50e3d3920a07c61ef50c5beeb2f.

droe commented 5 years ago

Agreed

droe commented 5 years ago

And to be specific, yes, please, this can go into develop before the release.

sonertari commented 5 years ago

I have cherry-picked the relevant changes directly to develop. So I am deleting deliberate-access-after-free branch and canceling this pull request. Thanks for your feedback Daniel.

sonertari commented 5 years ago

Ah, btw, I did a few simple tests on OpenBSD 6.4 with LibreSSL 2.8.2. SSLsplit seems to run fine with an https proxy spec, logs fine to connect and masterkey files, logs fine to text and pcap content files, and reopens fine those log files. So, since I haven't seen any issues with such basic tests, I am giving it a pass on OpenBSD 6.4.