Opened 13 months ago

Closed 13 months ago

Last modified 11 months 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 13 months ago by nickm

Owner: set to nickm
Priority: MediumHigh
Status: newaccepted

comment:2 Changed 13 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 13 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 13 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 13 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 13 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 13 months ago by nickm

Actual Points: .3

comment:8 Changed 11 months 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.