Opened 18 months ago

Closed 9 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 18 months ago by nickm

  • Milestone set to Tor: 0.2.7.x-final

comment:2 follow-up: Changed 18 months 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 ; follow-up: Changed 18 months 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 ; follow-up: Changed 18 months ago by teor

  • Status changed from new to needs_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 18 months 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 18 months ago by nickm

  • Keywords TorCoreTeam201512 removed

IMO the right fix here is to finish #15055.

comment:7 Changed 14 months ago by andrea

  • Milestone changed from Tor: 0.2.7.x-final to Tor: 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 13 months ago by nickm

  • Milestone changed from Tor: 0.2.8.x-final to Tor: 0.2.9.x-final

comment:9 Changed 11 months ago by nickm

  • Parent ID set to #15055
  • Points set to .5

comment:10 Changed 10 months ago by nickm

  • Owner set to nickm
  • Status changed from needs_information to assigned

comment:11 Changed 10 months ago by nickm

  • Keywords TorCoreTeam201608 added

comment:12 Changed 9 months ago by Jigsaw52

  • Status changed from assigned to needs_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 9 months ago by nickm

  • Status changed from needs_review to assigned

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

comment:14 Changed 9 months ago by nickm

  • Actual Points set to 0

Done as part of my WIP 15055 branch.

comment:15 Changed 9 months ago by nickm

  • Resolution set to implemented
  • Status changed from assigned to closed

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

Note: See TracTickets for help on using tickets.