Opened 4 weeks ago

Closed 7 days ago

#26882 closed defect (fixed)

IP address is not scrubbed in info logs, channel_tls_process_netinfo_cell() AND manpage for SafeLogging overgeneralizes

Reported by: dmr Owned by: rl1987
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-log, tor-client, tor-doc
Cc: asn, arma Actual Points:
Parent ID: Points:
Reviewer: mikeperry Sponsor:

Description

Here's a log snippet from an info log I was manually reviewing to scrub before sharing.

[info] channel_tls_process_netinfo_cell(): Got good NETINFO cell from [scrubbed]:443; OR connection is now open, using protocol version 5. Its ID digest is <redacted>. Our address is apparently <redacted>.

In the above, <redacted> is my notation; [scrubbed] is from SafeLogging.
(I'm not sure I had to redact the digest, but was just being conservative.)

SafeLogging 1 was set (default).
Tor 0.3.3.7

asn mentioned on #tor-dev that he thinks this is a bug.
Some brief notes from asn:

"<none>" : fmt_and_decorate_addr(&my_apparent_addr));
hm yeah that's I think a bug
it should be safe_str_client()
so weird that no one has mentioned htis before

It's worth noting the manpage for SafeLogging says:

...
If this option is set to 0, Tor will not perform any scrubbing, if it is set to 1, all potentially sensitive strings are replaced.
...

arma advocated for a different resolution:

if i were filing this ticket i would file a "scale back safelogging claims in the man page" ticket :)

(My preference is to scrub the IP address, but I also acknowledge the rabbit hole of trying to scrub anything "sensitive", especially in info/debug logs)

Child Tickets

Change History (11)

comment:1 Changed 4 weeks ago by dmr

Keywords: tor-log added

Adding keyword

comment:2 Changed 4 weeks ago by nickm

Generally we scrub well at notice or higher, and we should say so in the manpage.

That said, it is fine to improve scrubbing at info and debug-- but we should not claim that our scrubbing is always honored there.

comment:3 Changed 4 weeks ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:4 Changed 4 weeks ago by rl1987

https://github.com/torproject/tor/pull/244

Also opened #26892 for a related issue.

comment:5 Changed 4 weeks ago by rl1987

Status: acceptedneeds_review

comment:6 Changed 4 weeks ago by catalyst

Milestone: Tor: unspecified

comment:7 Changed 4 weeks ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.5.x-final

Moving to 0.3.5. (Anything with a small volunteer-submitted patch goes into the 0.3.5 milestone by default.)

comment:8 in reply to:  description Changed 4 weeks ago by dmr

Summary: IP address is not scrubbed in info logs, channel_tls_process_netinfo_cell()IP address is not scrubbed in info logs, channel_tls_process_netinfo_cell() AND manpage for SafeLogging overgeneralizes

Why not both?

Replying to dmr:

arma advocated for a different resolution:

if i were filing this ticket i would file a "scale back safelogging claims in the man page" ticket :)

Replying to rl1987:

https://github.com/torproject/tor/pull/244

Based on rl1987's changes, which address both things... editing the title for clearer ticket scope at a glance. :)

comment:9 Changed 4 weeks ago by asn

Reviewer: mikeperry

comment:10 Changed 8 days ago by mikeperry

Status: needs_reviewmerge_ready

I am a fan of doing both things. This is simple and looks good. +1 for the manpage update.

comment:11 Changed 7 days ago by nickm

Resolution: fixed
Status: merge_readyclosed

lgtm too; merged!

Note: See TracTickets for help on using tickets.