Opened 8 years ago

Closed 6 years ago

Last modified 6 years ago

#5584 closed enhancement (implemented)

Raise awareness of safer logging

Reported by: bastik Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Keywords: easy hack awareness of logging tor-relay
Cc: bastik Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Current releases of Tor give out a notice like:
[Notice] Tor can't help you if you use it wrong! Learn how to
be safe at (...)

Whenever logging is enabled Tor could give out a notice like:
[Notice] Please log safely. Don't log unless it's serves an important reason. Please overwrite the log afterwards.

It's for clients and relays.

Child Tickets

Attachments (1)

0001-Fix-5584-raise-awareness-of-safer-logging-warn-about.patch (2.3 KB) - added by marek 6 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 8 years ago by arma

The default is to log at log-level notice, and we aim for all logs at that level to not be harmful to keep.

So we definitely shouldn't do this "whenever logging is enabled". For example, it really frustrates me that the fedora rpm chose to turn off logs entirely -- we output useful things to the log, for example if your time is wrong, and it's worth having them.

That said, we might consider a line like this if you are at info or more verbose, or if you set SafeLogging to 0.

comment:2 in reply to:  1 Changed 8 years ago by nickm

Replying to arma:

The default is to log at log-level notice, and we aim for all logs at that level to not be harmful to keep.

We might also want to audit our notice, warning, and error logs to make sure they are as safe as we think.

comment:3 Changed 8 years ago by nickm

Milestone: Tor: unspecified

comment:4 in reply to:  1 Changed 8 years ago by bastik

Replying to arma:

The default is to log at log-level notice, and we aim for all logs at that level to not be harmful to keep.

I see those in vidalia, those don't get written to disc.

So we definitely shouldn't do this "whenever logging is enabled". For example, it really frustrates me that the fedora rpm chose to turn off logs entirely -- we output useful things to the log, for example if your time is wrong, and it's worth having them.

It wasn't my intention to scare people, so they stop giving out notices, which are indeed useful. I just wanted it be simple. It's like the "Tor can't help you if you use it wrong!" line, which gets logged even when "Bridge" is enabled and "SocksPort" is set to "0". (Which seemed unconditional to me, so I used it for this ticket)

That said, we might consider a line like this if you are at info or more verbose, or if you set SafeLogging to 0.

A conditional logging line sounds wise and was what I intended, but kept it simple. I agree with the conditional logging.

comment:5 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:6 Changed 7 years ago by nickm

Component: Tor RelayTor

comment:7 Changed 6 years ago by marek

I gave this bug a go, and added this lines to main.c:tor_init():

+  if (get_options()->SafeLogging_ != SAFELOG_SCRUB_ALL) {
+    log_warn(LD_GENERAL, "Your log may contain sensitive information - you "
+            "disabled SafeLogging. Please log safely. Don't log unless it "
+            "serves an important reason. Overwrite the log afterwards.");
+  } else {
+    log_info(LD_GENERAL, "Your log may contain sensitive information - you're "
+            "logging above \"notice\". Please log safely. Don't log unless it "
+            "serves an important reason. Overwrite the log afterwards.");
+  }

But there's a problem: this code must be run after options_init_from_torrc, we need get_options()->SafeLogging_ to be initialized. It's okay for the first warning if logging level is <= notice.

The second warning also requires config to be initialized - we want to write this message to an initialized log _file_, not only to the temporary stderr. But, if one actually sets logging level to debug, this message will disappear between many many other debug messages written during options_init_from_torrc().

In other words - we must write this messages after options_init_* as we want to write them to real log files, but if we do it after this call they will be too deep in the log file for anyone read it.

The alternative solution would be to put the latter message in log.c:log_tor_version: https://github.com/torproject/tor/blob/master/src/common/log.c#L215 But that's ugly, and log_tor_version is not always run - for example not in the case of writing to syslog. Ideal solution would be to have a dedicated callback in main.c or config.c that will be run from logging every time a new log is initialized and before the log gets spammed. This call could replace log_tor_version. That looks like a more significant change though.

comment:8 Changed 6 years ago by nickm

yeah, I agree that a new callback would be necessary.

As another issue: logging the second message at "info" is incorrect; we should log it whenever we log anything at info, but the message itself is semantically a warning (since it tells the user about a potentially problematic situation. Also, whether we should complain about SafeLogging is logically independent from whether we should warn about the log level.

Fortunately, there's an easy solution: just rewrite the logic to be:

    if (get_options()->SafeLogging_ != SAFELOG_SCRUB_ALL) {
      log_warn(...);
    }
    if (get_min_log_level() >= LOG_INFO) {
       log_warn(...);

(Though I might have gotten the direction of that >= comparison wrong.)

comment:9 Changed 6 years ago by marek

Status: newneeds_review

Aaargh. I hate this patch. It does work, but certainly doesn't make the code any nicer. The warnings are randomly spilled all over the code! Whatever. It does the work, doesn't make the code much worse than it already is. I couldn't find any better way of doing it without rewriting everything. Maybe the second warning could be moved around options_act, there is no reason why I put it right after Tor2webMode warning.

comment:10 Changed 6 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.2.5.x-final

comment:11 Changed 6 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Agreed that it's not pretty, but it seems okay. Fixing tabs and merging. Thanks!

comment:12 Changed 6 years ago by arma

Nick, in the future if you could clean up the changes files too, that'd be grand. (I just spent a while wondering what clearning is.)

Note: See TracTickets for help on using tickets.