Opened 4 months ago

Closed 4 months ago

Last modified 6 weeks ago

#28973 closed defect (implemented)

Disable TLS1.3 when openssl bug 7712 is present

Reported by: nickm Owned by: nickm
Priority: High Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version: Tor: 0.3.4.9
Severity: Normal Keywords: 033-backport 034-backport 035-backport
Cc: Actual Points: .3
Parent ID: Points:
Reviewer: Sponsor:

Description

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

Child Tickets

Change History (8)

comment:1 Changed 4 months ago by nickm

Owner: set to nickm
Priority: MediumHigh
Status: newaccepted

comment:2 Changed 4 months ago by nickm

Keywords: 033-backport 034-backport 035-backport added
Status: acceptedneeds_review

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 .

comment:3 Changed 4 months ago by nickm

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.

comment:4 Changed 4 months ago by ahf

Status: needs_reviewmerge_ready

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.

comment:5 in reply to:  4 Changed 4 months ago by nickm

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.

comment:6 Changed 4 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

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

comment:7 Changed 3 months ago by nickm

Actual Points: .3

comment:8 Changed 6 weeks ago by teor

Milestone: Tor: 0.4.0.x-finalTor: 0.3.3.x-final
Parent ID: #28616
Note: See TracTickets for help on using tickets.