control_auth_cookie isn't deleted when tor stops

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.

changed milestone to %Tor: 0.3.3.x-final

added component::core tor/tor easy milestone::Tor: 0.3.3.x-final priority::medium resolution::fixed severity::normal status::closed tor-control type::defect version::tor 0.3.0.10 labels

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

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

Trac:
Component: - Select a component to Core Tor/Tor
Status: new to needs_information
Milestone: N/A to Tor: unspecified

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

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?

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.

Trac:
Cc: N/A to mcs

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.

Trac:
Keywords: N/A deleted, easy tor-control added

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.

Trac:
Status: needs_information to new

Trac:
0001-Remove-CookieAuthFile-when-tor_cleanup-is-called.patch

Trac:
0001-Unlink-CookieAuthFile-when-tor_cleanup-is-called.patch

Done, I hope everything is fine!

Note: Consider the second patch.

Trac:
Status: new to needs_review

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.

Trac:
Milestone: Tor: unspecified to Tor: 0.3.3.x-final
Status: needs_review to needs_revision

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

Trac:
Status: needs_revision to needs_review

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.

Trac:
Status: needs_review to new

Trac:
Status: new to needs_information

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.

Trac:
Status: needs_information to needs_review

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.

Trac:
Status: needs_review to needs_revision

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.

Trac:
Status: needs_revision to needs_review

LGTM!

Trac:
Status: needs_review to merge_ready

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.

merged per discussion on IRC!

Trac:
Resolution: N/A to fixed
Status: merge_ready to closed

closed

mentioned in issue #23288 (moved)

mentioned in issue #24693 (moved)

moved to tpo/core/tor#23271 (closed)

mentioned in issue tpo/core/tor#23288 (closed)

mentioned in issue tpo/core/tor#24693 (closed)