Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#20553 closed defect (fixed)

Memory leak in crypto_write_public_key_to_string() with OpenSSL master

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

Description

The expensive-hardening version of the tests had a memory leak when we were encoding public keys. The fix is easy -- I don't know why we programmed it the way we did in the first place.

Found on https://jenkins.torproject.org/job/tor-ci-linux-master-expensive-hardening/ARCHITECTURE=amd64,SUITE=sid/425/consoleFull -- so is that openssl 1.1 too?

Child Tickets

Change History (5)

comment:1 Changed 3 years ago by nickm

To reproduce, build with --enable-expensive-hardening and an appropriate version of OpenSSL. Then run ./src/test/test crypto/pk . You'll see:

=================================================================
==29032==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x7f2f849e1e60 in malloc (/lib64/libasan.so.3+0xc6e60)
    #1 0x7f2f83c197ed in CRYPTO_zalloc (/home/nickm/opt/openssl//lib/libcrypto.so.1.1+0x15f7ed)

Indirect leak of 32 byte(s) in 1 object(s) allocated from:
    #0 0x7f2f849e1e60 in malloc (/lib64/libasan.so.3+0xc6e60)
    #1 0x7f2f83c197ed in CRYPTO_zalloc (/home/nickm/opt/openssl//lib/libcrypto.so.1.1+0x15f7ed)

SUMMARY: AddressSanitizer: 48 byte(s) leaked in 2 allocation(s).
OK

Looking at the OpenSSL source in bss_mem.c, this appears to have been introduced in their 9fe9d0461ea4bcc, which is in 1.1.

I'd call this an openssl bug, except our code here is just plain bizarre.

comment:2 Changed 3 years ago by nickm

Milestone: Tor: 0.3.0.x-finalTor: 0.2.9.x-final
Status: newneeds_review

Fix in branch bug20553_028. I'm merging this to master, and putting for consideration for backport.

comment:3 Changed 3 years ago by dgoulet

Status: needs_reviewmerge_ready

lgtm; Seems worth it for backport as memleaks can be a source of DoS, that falls well under our policy of backporting security bugs for stable 028.

comment:4 Changed 3 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

okay. Merged to 0.2.8 and forward

comment:5 Changed 3 years ago by nickm

Milestone: Tor: 0.2.9.x-finalTor: 0.2.8.x-final
Note: See TracTickets for help on using tickets.