Opened 4 months ago

Closed 4 months ago

#26116 closed defect (fixed)

OpenSSL 1.1.1 changed the semantics of the password callback return value

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 029-backport 031-backport 032-backport 033-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: catalyst Sponsor:

Description

In c82c3462267afdbbaa5, openssl changed its interpretation of getting a "0" result from a PEM password callback. Previously, this indicated an error, and the documentation said:

The callback must return the number of characters in the passphrase or 0 if an error occurred.

But now, 0 means an empty passphrase, and -1 means that an error occurred.

To avoid strange bugs and compatibility issues, we should have our pem_no_password_cb function return -1 instead. (-1 was always a supported return value, even if it isn't documented.)

Child Tickets

Change History (7)

comment:1 Changed 4 months ago by nickm

See also https://github.com/openssl/openssl/pull/6271 for a PR to change the openssl documentation.

comment:2 Changed 4 months ago by nickm

Status: assignedneeds_review

See branch bug26116_029 (https://github.com/torproject/tor/pull/111) and branch bug26116_033 (https://github.com/torproject/tor/pull/112).

Also let's see whether the openssl folks think this change was a mistake or not.

comment:3 Changed 4 months ago by asn

Reviewer: catalyst

comment:4 Changed 4 months ago by catalyst

Status: needs_reviewmerge_ready

Looks mostly good. It looks like no tests are currently calling the callback? Is there ever a case where we expect to handle a password-protected key through these functions? If so, maybe we want to add a test.

comment:5 Changed 4 months ago by nickm

See also https://github.com/openssl/openssl/issues/6347 for an OpenSSL bug exposed by this bug. I'm working on tests. :)

comment:6 Changed 4 months ago by nickm

aeb4be1d5a17f8ff836e370f8942c09c66b31e1d has a unit test to check these cases.

We don't expect to get a legit password-protected key, but somebody could maliciously (or accidentally) send one in.

I'm merging this to 0.2.9 and forward

comment:7 Changed 4 months ago by nickm

Resolution: fixed
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.