#26892 closed defect (implemented)

log_addr_has_changed() does not heed SafeLogging

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

Description

No IP address scrubbing is performed at any loglevel:

2694   if (!tor_addr_is_null(prev))
2695     log_fn(severity, LD_GENERAL,
2696            "Our IP Address has changed from %s to %s; "
2697            "rebuilding descriptor (source: %s).",
2698            addrbuf_prev, addrbuf_cur, source);
2699   else
2700     log_notice(LD_GENERAL,
2701              "Guessed our IP address as %s (source: %s).",
2702              addrbuf_cur, source);

Child Tickets

Change History (10)

comment:1 Changed 15 months ago by nickm

I'd be glad to take a patch for this.

comment:2 Changed 15 months ago by rl1987

Status: newneeds_review

comment:3 Changed 15 months ago by dmr

Cc: dmr added
Keywords: tor-log added

comment:4 Changed 15 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.5.x-final

comment:5 Changed 15 months ago by asn

Reviewer: mikeperry

comment:6 Changed 15 months ago by asn

Reviewer: mikeperryteor

comment:7 Changed 15 months ago by teor

Status: needs_reviewneeds_revision

log_addr_has_changed() is only called in server_mode():

  • router_new_address_suggestion() returns early if !server_mode()
  • router_new_address_suggestion() returns early if !router_get_my_routerinfo()

Relay addresses are public, so there isn't any point scrubbing them from logs.

Bridge addresses are not public, but the diagnostic value of the logs probably outweighs the occasional risk that operators copy unscrubbed addresses into tickets.

I would take a patch to log_addr_has_changed() that exits early on clients as a precaution:

  if (!server_mode(options)) {
    return;
  }

comment:8 Changed 15 months ago by rl1987

Status: needs_revisionneeds_review

comment:9 Changed 15 months ago by teor

Status: needs_reviewmerge_ready

Looks fine to me.

comment:10 Changed 15 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

LGTM too; merged!

Note: See TracTickets for help on using tickets.