Opened 2 years ago

Closed 15 months ago

#17779 closed defect (implemented)

Memory leak in routerkeys.c

Reported by: cypherpunks Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7.2-alpha
Severity: Normal Keywords: easy, TorCoreTeam201608
Cc: Actual Points: 0
Parent ID: #15055 Points: .5
Reviewer: Sponsor:

Description

The rsa_ed_crosscert variable is assigned a cross-certification object in the load_ed_keys function but is never freed. Secondly, the variable is only used by the get_master_rsa_crosscert function which, in turn, is not used by the current code base.

My suggestion is to remove the rsa_ed_crosscert variable and the get_master_rsa_crosscert function to fix the memory leak (unless they have some future use-case).

The variable and function were added in 3bee74c6d115131f4850a07a5c12db21ae6f3193.

Child Tickets

Change History (15)

comment:1 Changed 2 years ago by nickm

Milestone: Tor: 0.2.7.x-final

comment:2 Changed 2 years ago by teor

Keywords: TorCoreTeam201512 easy added

It would be great if we could fix this in December.

But we might want to think about how we do it - it appears that get_master_rsa_crosscert is for generating RSA/ED cross-certificates.

Perhaps we can free the rsa_ed_crosscert variable in one of the functions that cleans up Tor memory?
Do we expect that the rsa_ed_crosscert variable will ever change value? If so, it needs to be freed before it's overwritten, too.

comment:3 in reply to:  2 ; Changed 2 years ago by cypherpunks

Replying to teor:

It would be great if we could fix this in December.

But we might want to think about how we do it - it appears that get_master_rsa_crosscert is for generating RSA/ED cross-certificates.

The function itself doesn't generate anything, it just assigns pointers. Also if you grep for its name you only get its declaration and its definition. Nothing calls it. (Makes me wonder why the compiler isn't complaining about it.)

Perhaps we can free the rsa_ed_crosscert variable in one of the functions that cleans up Tor memory?
Do we expect that the rsa_ed_crosscert variable will ever change value? If so, it needs to be freed before it's overwritten, too.

This was my initial solution until it found out nothing uses the variable.

comment:4 in reply to:  3 ; Changed 2 years ago by teor

Status: newneeds_information

Replying to cypherpunks:

Replying to teor:

It would be great if we could fix this in December.

But we might want to think about how we do it - it appears that get_master_rsa_crosscert is for generating RSA/ED cross-certificates.

The function itself doesn't generate anything, it just assigns pointers. Also if you grep for its name you only get its declaration and its definition. Nothing calls it. (Makes me wonder why the compiler isn't complaining about it.)

Perhaps we can free the rsa_ed_crosscert variable in one of the functions that cleans up Tor memory?
Do we expect that the rsa_ed_crosscert variable will ever change value? If so, it needs to be freed before it's overwritten, too.

This was my initial solution until it found out nothing uses the variable.

I think this is a good solution, but I want someone who wrote / reviewed the ed25519 code to review the suggested solution.

comment:5 in reply to:  4 Changed 2 years ago by cypherpunks

Replying to teor:

Replying to cypherpunks:

Replying to teor:

It would be great if we could fix this in December.

But we might want to think about how we do it - it appears that get_master_rsa_crosscert is for generating RSA/ED cross-certificates.

The function itself doesn't generate anything, it just assigns pointers. Also if you grep for its name you only get its declaration and its definition. Nothing calls it. (Makes me wonder why the compiler isn't complaining about it.)

Perhaps we can free the rsa_ed_crosscert variable in one of the functions that cleans up Tor memory?
Do we expect that the rsa_ed_crosscert variable will ever change value? If so, it needs to be freed before it's overwritten, too.

This was my initial solution until it found out nothing uses the variable.

I think this is a good solution, but I want someone who wrote / reviewed the ed25519 code to review the suggested solution.

That would be Nick according to the commit mentioned in the ticket description.

FWIW iff the rsa_ed_crosscert variable gets removed, it also makes the tor_make_rsa_ed25519_crosscert function obsolete because it is only used for assigning its result to rsa_ed_crosscert.

comment:6 Changed 2 years ago by nickm

Keywords: TorCoreTeam201512 removed

IMO the right fix here is to finish #15055.

comment:7 Changed 20 months ago by andrea

Milestone: Tor: 0.2.7.x-finalTor: 0.2.8.x-final

Backport pass: this is marked maint-0.2.7 but isn't actually fixed; moving to 0.2.8.

comment:8 Changed 19 months ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final

comment:9 Changed 17 months ago by nickm

Parent ID: #15055
Points: .5

comment:10 Changed 16 months ago by nickm

Owner: set to nickm
Status: needs_informationassigned

comment:11 Changed 16 months ago by nickm

Keywords: TorCoreTeam201608 added

comment:12 Changed 15 months ago by Jigsaw52

Status: assignedneeds_review

Hi.

I attempted to fix this. My fix is here: https://github.com/Jigsaw52/tor/tree/fix-17779

My commits removes:

  • variables rsa_ed_crosscert and rsa_ed_crosscert_len from or/routerkeys.c
  • function get_master_rsa_crosscert from or/routerkeys.c
  • function tor_make_rsa_ed25519_crosscert from or/torcert.c

comment:13 Changed 15 months ago by nickm

Status: needs_reviewassigned

NAK on that; my #15055 work will fix this by making those fields actually used.

comment:14 Changed 15 months ago by nickm

Actual Points: 0

Done as part of my WIP 15055 branch.

comment:15 Changed 15 months ago by nickm

Resolution: implemented
Status: assignedclosed

These are implemented in 15055_wip; folding them into #15055 as their parent ticket.

Note: See TracTickets for help on using tickets.