Disable TLS1.3 when openssl bug 7712 is present

See #28616 (moved) for the impact of the bug in tor; see https://github.com/openssl/openssl/issues/7712 for the openssl issue.

changed milestone to %Tor: 0.3.3.x-final

added 033-backport 034-backport 035-backport actualpoints::.3 component::core tor/tor milestone::Tor: 0.3.3.x-final owner::nickm priority::high resolution::implemented severity::normal status::closed type::defect version::tor 0.3.4.9 labels

Trac:
Status: new to accepted
Priority: Medium to High
Owner: N/A to nickm

I made branch ticket28973_033 to test a fix here; it should also merge cleanly into 0.3.4, 0.3.5, and 0.4.0.

I expect that a few warnings will still happen with this branch: it waits for the bug to happen once before disabling TLS 1.3, by which point other TLS 1.3 connections may already be in progress.

I have tested this branch with a good OpenSSL version, but not with openssl 1.1.1a: I hope somebody else can do that.

Only servers will encounter this issue.

There is a github PR at https://github.com/torproject/tor/pull/625 .

Trac:
Keywords: N/A deleted, 033-backport 034-backport 035-backport added
Status: accepted to needs_review

I've tested this with chutney, openssl 1.1.1a, and git master, confirming that with this patch, chutney succeeds with openssl 1.1.1a, but fails without it.

Code seems to be called correctly when build against OpenSSL 1.1.1a.

The patch looks reasonable to me. Only minor nitpick I spot is to maybe use bool as type for openssl_bug_7712_is_present - it is not something I have a strong opinion about though.

Do you think this ticket should become about removing this bugfix at some point in the future? Having the checks for the -2 return value as a special case looks a bit funky if seen out of context even with the reference to the OpenSSL bug as a comment.

Trac:
Status: needs_review to merge_ready

Replying to ahf:

Code seems to be called correctly when build against OpenSSL 1.1.1a.

The patch looks reasonable to me. Only minor nitpick I spot is to maybe use bool as type for openssl_bug_7712_is_present - it is not something I have a strong opinion about though.

I don't think we require stdbool in 0.3.4.

Do you think this ticket should become about removing this bugfix at some point in the future? Having the checks for the -2 return value as a special case looks a bit funky if seen out of context even with the reference to the OpenSSL bug as a comment.

I think we should have a separate ticket for removing this fix once openssl 1.1.1a is long forgotten.

squashed and merged to 0.3.3 and forward; added #28977 (moved) to remove this workaround once OpenSSL 1.1.1a is gone.

Trac:
Resolution: N/A to implemented
Status: merge_ready to closed

Trac:
Actualpoints: N/A to .3

Trac:
Milestone: Tor: 0.4.0.x-final to Tor: 0.3.3.x-final
Parent: #28616 (moved) to N/A

closed

added 2h 24m of time spent

mentioned in issue #28977 (moved)

mentioned in issue #29241 (moved)

moved to tpo/core/tor#28973 (closed)

mentioned in issue tpo/core/tor#28977 (closed)

mentioned in issue tpo/core/tor#29241 (closed)