Opened 7 years ago

Last modified 15 months ago

#2914 needs_revision defect

Tor should truncate log file if loglevel < notice

Reported by: mikeperry Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-relay, easy
Cc: Actual Points:
Parent ID: Points: small
Reviewer: Sponsor:

Description (last modified by mikeperry)

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.)

Child Tickets

Attachments (3)

unfinished_2914_fix.patch (2.6 KB) - added by chobe 4 years ago.
update on my progress
fix_2914_v2.patch (1.8 KB) - added by chobe 4 years ago.
fix_2914_v3.patch (2.4 KB) - added by chobe 4 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 7 years ago by mikeperry

Description: modified (diff)

comment:2 Changed 7 years ago by mikeperry

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

comment:3 Changed 7 years ago by Sebastian

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.

comment:4 Changed 7 years ago by nickm

Milestone: Tor: 0.2.3.x-final

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

comment:5 Changed 7 years ago by arma

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.

comment:6 Changed 7 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: unspecified

comment:7 Changed 6 years ago by nickm

Keywords: tor-relay added

comment:8 Changed 6 years ago by nickm

Component: Tor RelayTor

comment:9 Changed 4 years ago by nickm

Keywords: easy added

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

comment:10 Changed 4 years ago by nickm

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

Changed 4 years ago by chobe

Attachment: unfinished_2914_fix.patch added

update on my progress

comment:11 Changed 4 years ago by 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 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.

Last edited 4 years ago by chobe (previous) (diff)

comment:12 Changed 4 years ago by chobe

Status: newneeds_review

comment:13 Changed 4 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.2.6.x-final

Changed 4 years ago by chobe

Attachment: fix_2914_v2.patch added

comment:14 Changed 4 years ago by 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.

Changed 4 years ago by chobe

Attachment: fix_2914_v3.patch added

comment:15 Changed 4 years ago by arma

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?

comment:16 Changed 4 years ago by mikeperry

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!"

comment:17 Changed 4 years ago by nickm

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?

comment:18 Changed 4 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.???
Status: needs_reviewneeds_revision

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.

comment:19 Changed 3 years ago by nickm

Points: small
Severity: Normal

comment:20 Changed 21 months ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:21 Changed 20 months ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

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

comment:22 Changed 15 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

Note: See TracTickets for help on using tickets.