Opened 10 months ago

Closed 6 months ago

#23271 closed defect (fixed)

control_auth_cookie isn't deleted when tor stops

Reported by: yurivict271 Owned by:
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version: Tor: 0.3.0.10
Severity: Normal Keywords: easy tor-control
Cc: mcs Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

When tor is stopped gracefully ('service stop tor' on FreeBSD), it leaves the file /var/db/tor/control_auth_cookie.

Due to the sensitive nature of this file, it should be only present when tor runs and deleted once it stops gracefully.

Child Tickets

Attachments (2)

Change History (24)

comment:1 Changed 10 months ago by yurivict271

Additionally, please consider deleting this file when it is present and tor is starting with CookieAuthentication 0.

comment:2 Changed 10 months ago by catalyst

Component: - Select a componentCore Tor/Tor
Milestone: Tor: unspecified
Status: newneeds_information

I thought a new random cookie got generated every time tor starts but I'll have to check.

comment:3 Changed 10 months ago by yurivict271

A new cookie is generated, my question is why is this file left over?

comment:4 in reply to:  3 Changed 10 months ago by catalyst

Replying to yurivict271:

A new cookie is generated, my question is why is this file left over?

I don't know right now. I'd like to understand: once the process that used it terminates, what risk does that file pose if its contents get disclosed?

comment:5 Changed 10 months ago by yurivict271

what risk does that file pose if its contents get disclosed?

IMO, no risk. However, people might assume that control port is available since the cookie is still present. IMO, this is a minor defect that the file is left over when it isn't needed. This can create some confusion when it isn't needed to be that way.

comment:6 Changed 10 months ago by mcs

Cc: mcs added

comment:7 Changed 10 months ago by catalyst

Keywords: easy tor-control added

It looks like tor_cleanup() in main.c unlinks some ephemeral files (like the pid file) but not others. I'm not sure I can easily enumerate which additional ones need cleaning up.

Fixing just this one instance might be easy but probably the right thing to do when working on the bigger issue is to refactor the cleanup code somewhat.

comment:8 in reply to:  7 Changed 10 months ago by cypherpunks

Replying to catalyst:

It looks like tor_cleanup() in main.c unlinks some ephemeral files (like the pid file) but not others. I'm not sure I can easily enumerate which additional ones need cleaning up.

FWIW the tor man page contains a list of files that are in the data directory so no need to go through the code and find which files are generated.

comment:9 Changed 10 months ago by catalyst

Status: needs_informationnew

comment:10 Changed 7 months ago by ffmancera

Status: newneeds_review

Done, I hope everything is fine!

Note: Consider the second patch.

comment:11 in reply to:  10 Changed 7 months ago by asn

Milestone: Tor: unspecifiedTor: 0.3.3.x-final
Status: needs_reviewneeds_revision

Replying to ffmancera:

Done, I hope everything is fine!

Note: Consider the second patch.

Thanks for the patch! You are very close!

A few things:

  • Instead of providing a patch file, it's better to publish your work on a git branch somewhere (e.g. on github) and link us to your repository. It's easier for us this way.
  • The changes file could be improved. It's user-facing so words like unlink and tor_cleanup() are not so helpful. Instead saying Tor now deletes the CookieAuthFile when it stops might be better. Also we say Closes ticket 23271 instead of Closes 23271. You can run make check-changes to get warnings for these sort of stuff.

comment:12 Changed 6 months ago by ffmancera

Status: needs_revisionneeds_review

All done! Check my github branch bug23271. I hope everything is fine!

comment:13 Changed 6 months ago by ffmancera

Status: needs_reviewnew

As discussed in #tor-dev with asn, it could be great to add all the ephimeral files to the tor_cleanup() function.

I can implement it but first I need a reliable list of files to remove when tor stops. I think that maybe teor could help here, so add him to Cc (I have been unable) please.

Current list:

  • PidFile
  • ControlPortWriteToFile
  • CookieAuthFile

I will mark the ticket as need information.

Last edited 6 months ago by ffmancera (previous) (diff)

comment:14 Changed 6 months ago by ffmancera

Status: newneeds_information

comment:15 Changed 6 months ago by ffmancera

Here is the update:

After searching in the docs and combine chutney with strace, I came up with this new list of ephimeral files:

  • PidFile
  • ControlPortWriteToFile
  • CookieAuthFile
  • ExtORPortCookieAuthFile

Also, I have splitted tor_cleanup() function into a new function that removes a specified file. I think that is nice to reuse code.

I hope everything is fine, check my github branch bug23271.

comment:16 Changed 6 months ago by ffmancera

Status: needs_informationneeds_review

comment:17 Changed 6 months ago by asn

Here is a review:

1) In tor_remove_file(char *file) you can constify file as follows: tor_remove_file(const char *file). Also perhaps worth renaming to filename.

2) This will slightly complicate the patch, but you probably need to use get_ext_or_auth_cookie_file_name() and get_controller_cookie_file_name() to get the right filename, since those files will be created even if the CookieAuthFile torrc option is not set but the control port is enabled.

I suggest the following way to do this: Use file_status() to check if the filename that those functions returned exists, and unlink it if so. It's fine to do this extra overhead since this will happen only on shutdown. Example usage:

  if (file_status(fname) == FN_FILE) {
    if (tor_unlink(fname) != 0) {

3) Consider using tor_unlink() instead of unlink(), so that it's more easily testable in the future if need be.

comment:18 Changed 6 months ago by asn

Status: needs_reviewneeds_revision

comment:19 Changed 6 months ago by ffmancera

Status: needs_revisionneeds_review

All done! I didn't know about get_ext_or_auth_cookie_file_name() and get_controller_cookie_file_name(), thank you! :-)

Check my github branch bug23271.

Last edited 6 months ago by ffmancera (previous) (diff)

comment:20 Changed 6 months ago by asn

Status: needs_reviewmerge_ready

LGTM!

comment:21 Changed 6 months ago by nickm

Do we have to check whether the file exists before removing it? I'd suggest instead:

void
tor_remove_file(const char *filename)
{
  if (tor_unlink(filename) != 0 && errno != ENOENT) {
    log_warn(LD_FS, "Couldn't unlink %s: %s",
             filename, strerror(errno));
  }
}

If that's okay with you, I'll merge it.

comment:22 Changed 6 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

merged per discussion on IRC!

Note: See TracTickets for help on using tickets.