Tor should truncate log file if loglevel < notice

A lot of relay operators run tor from git for various reasons. These relay operators don't get the advantage of distribution log rotation, and can unknowingly leave tor running at low log level for long periods while running test branches. In some cases, SafeLogging may also be disabled.

Presumably, since they are running git, they are upgrading often. Based on this assumption, an easy fix should be to just change the default log file open mode from O_APPEND to O_TRUNC if the loglevel is below notice, and/or if SafeLogging is off.

Of course, a better fix is to implement our own log rotation. I don't think the corner case is that important. It is a non-default config that makes it risky** in the first place.

Thanks to Marcia Hofmann @ EFF for pointing this out.

** (The reason it is risky is not because logs are terribly dangerous to anonymity in their current form, but moreso because logs can be such a false path due to the multiplexing of circuits over TLS.)

changed milestone to %Tor: unspecified

added component::core tor/tor easy milestone::Tor: unspecified points::small priority::medium severity::normal status::needs-revision tor-relay type::defect labels

Trac:
Description: A lot of relay operators run tor from git for various reasons. These relay operators don't get the advantage of distribution log rotation, and can unknowingly leave tor running at low log level for long periods while running test branches. In some cases, SafeLogging may also be disabled.

Presumably, since they are running git, they are upgrading often. Based on this assumption, an easy fix should be to just change the default log file open mode from O_APPEND to O_TRUNC if the loglevel is below notice, and/or if SafeLogging is off.

Of course, a better fix is to implement our own log rotation. I don't think the corner case is that important. It is a non-default config that makes it risky** in the first place.

Thanks for Marcia Hofmann @ EFF for pointing this out.

** (The reason it is risky is not because logs are terribly dangerous to anonymity in their current form, but moreso because logs can be such a false path due to the multiplexing of circuits over TLS.)

to

A lot of relay operators run tor from git for various reasons. These relay operators don't get the advantage of distribution log rotation, and can unknowingly leave tor running at low log level for long periods while running test branches. In some cases, SafeLogging may also be disabled.

Presumably, since they are running git, they are upgrading often. Based on this assumption, an easy fix should be to just change the default log file open mode from O_APPEND to O_TRUNC if the loglevel is below notice, and/or if SafeLogging is off.

Of course, a better fix is to implement our own log rotation. I don't think the corner case is that important. It is a non-default config that makes it risky** in the first place.

Thanks to Marcia Hofmann @ EFF for pointing this out.

** (The reason it is risky is not because logs are terribly dangerous to anonymity in their current form, but moreso because logs can be such a false path due to the multiplexing of circuits over TLS.)

Trac:
Summary: Tor should not append to file if loglevel < notice to Tor should truncate log file if loglevel < notice

Eh, please no. I use tor from git all the time, and I set up my own logrotate for things where I need it - it's not that hard. Additionally, overwriting a file that might have valuable info in it (for example, if my Tor stopped because it crashed) would be really annoying imo.

We should figure out what (if anything) to do here on an 0.2.3.x timeline

Trac:
Milestone: N/A to Tor: 0.2.3.x-final

Whenever I restart moria1, I manually delete the two log files that it's been writing. It's just part of that routine by now.

So in fact I would make use of a feature like this if there were one.

I'm torn though, because changing behavior on people probably won't make anybody happy.

Trac:
Milestone: Tor: 0.2.3.x-final to Tor: unspecified

Trac:
Keywords: N/A deleted, tor-relay added

Trac:
Component: Tor Relay to Tor

Tagging as "easy": if we decide this is a good idea, it's not hard to do.

Trac:
Keywords: tor-relay deleted, tor-relay easy added

See also #5583 (moved). In #5583 (moved), we truncate optionally. Here, we truncate by default if the logging is unsafe. We could combine the two ideas.

Trac:
Username: chobe

unfinished_2914_fix.patch

update on my progress

Trac:
Username: chobe

I thought this would be a good first issue for me to try and fix, even if it is never implemented.

The above attached patch (submitted by me) truncates the log file when SafeLogging is disabled. It doesn't truncate the log file if the loglevel is less than notice (yet).

I submitted this patch in order to learn more about tor and how to contribute to it. I looked over arlolra's fix to #5583 (moved) and tried to do a similar solution.

You can test my patch by setting the SafeLogging and Log options in a torrc file and verifying that the correct values of SafeLogging will truncate the log file.

I'd just like to know if I'm on the right track and if there's anything I missed. I believe my fix overlooks command line arguments given to tor and only accounts for options set in the torrc file.

EDIT: just noticed my horrible tabbing in config.c, sorry, I won't make that mistake in future patches.

Trac:
Username: chobe

Trac:
Username: chobe
Status: new to needs_review

Trac:
Milestone: Tor: unspecified to Tor: 0.2.6.x-final

Trac:
Username: chobe

fix_2914_v2.patch

Trac:
Username: chobe

This patch includes the "easy fix" described in the ticket.

I may look into log rotation but I think it will be too much work for me with my current understanding of the project and workflow.

Trac:
Username: chobe

Trac:
Username: chobe

fix_2914_v3.patch

Trac:
Username: chobe

Mike, what does "logs can be such a false path" mean?

I think from a technical perspective, if a relay operator has chosen to log at a more verbose level, we have to assume they're doing it for a reason, and clobbering their log file probably won't help with their reason.

(Also, I think the log rotation thing for packages is a bit of a red herring, since no packages log louder than notice by default anyway.)

From a legal perspective, maybe it would be good in some way to have the default be the more conservative choice? But I would say it is already this way, since the default is only notice, and safelogging 1, etc.

What am I missing?

I think by "false path" I meant that the logs provide just enough detail for someone to look at them and assume the wrong thing. ie: "Aha! at the time in question I see that a TLS connection opened to some other node. It must be the next node I'm looking for!"

The patch has a memory leak; the individual members of full_elts are not freed.

I'm not convinced this is a great idea, though. Maybe having a per-log flag would be a good idea?

Throwing this into 0.2.???.

If the memory leak gets fixed, and this turns into something with a per-log flag to configure it, it can get into 0.2.6.

Trac:
Milestone: Tor: 0.2.6.x-final to Tor: 0.2.???
Status: needs_review to needs_revision

Trac:
Severity: N/A to Normal
Points: N/A to small
Sponsor: N/A to N/A

Milestone renamed

Trac:
Milestone: Tor: 0.2.??? to Tor: 0.3.???

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

Trac:
Keywords: tor-relay easy deleted, easy, tor-relay, tor-03-unspecified-201612 added
Milestone: Tor: 0.3.??? to Tor: unspecified

Remove an old triaging keyword.

Trac:
Keywords: tor-03-unspecified-201612 deleted, N/A added

mentioned in issue #5583 (moved)

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