Opened 2 months ago

Last modified 2 weeks ago

#31466 merge_ready defect

Consider demoting ".exit is disabled" log message to info

Reported by: asn Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: bug-bounty, hackerone, tor-security?, 042-should 029-backport? 035-backport 040-backport 041-backport BugSmashFund dgoulet-merge
Cc: Actual Points: .1
Parent ID: Points:
Reviewer: asn Sponsor:

Description

  /* Check for whether this is a .exit address.  By default, those are
   * disallowed when they're coming straight from the client, but you're
   * allowed to have them in MapAddress commands and so forth. */
  if (!strcmpend(socks->address, ".exit")) {
    log_warn(LD_APP, "The  \".exit\" notation is disabled in Tor due to "
             "security risks.");
    control_event_client_status(LOG_WARN, "SOCKS_BAD_HOSTNAME HOSTNAME=%s",
                                escaped(socks->address));
    out->end_reason = END_STREAM_REASON_TORPROTOCOL;
    out->should_close = 1;
    return;
  }

The above log message can be remotely triggered by websites and some people consider this a security issue: https://github.com/brave/brave-browser/issues/4629

This has also been reported to our h1 bug bounty program.

We should consider fixing this one particular instance, but I bet there is more of this lying around.

Child Tickets

Change History (8)

comment:1 Changed 5 weeks ago by nickm

Keywords: 042-should added

comment:2 Changed 4 weeks ago by nickm

Owner: set to nickm
Status: newaccepted

I think it's more appropriate to use a rate-limiter.

comment:3 Changed 4 weeks ago by nickm

Actual Points: .1
Keywords: 029-backport? 035-backport 040-backport 041-backport BugSmashFund added

See branch bug31466_029 with PR at https://github.com/torproject/tor/pull/1342 .

There's a merge conflict with 0.3.5, because of the removal of AllowDotExit. See bug31466_035 for a resolution to that conflict, with a PR at https://github.com/torproject/tor/pull/1343 .

This latter branch merges cleanly to master. To see CI running on master, the branch id is bug31466_042 with a PR at https://github.com/torproject/tor/pull/1344 .

I decided to go for a rate-limit here instead of demoting the log message's severity. We do want people to see this message if they are going to attempt to use the obsolete .exit notation... we just don't want to spam their logs with it.

I'll put this in needs_review once CI is happy.

comment:4 Changed 4 weeks ago by nickm

Status: acceptedneeds_review

comment:5 Changed 3 weeks ago by asn

Reviewer: asn

comment:6 Changed 3 weeks ago by asn

Status: needs_reviewmerge_ready

Looks good to me!

comment:7 Changed 3 weeks ago by nickm

Keywords: dgoulet-merge added

comment:8 Changed 2 weeks ago by dgoulet

Milestone: Tor: 0.4.2.x-finalTor: 0.4.1.x-final

PR 1344 merged. Moving to 041 milestone for backport.

Note: See TracTickets for help on using tickets.