Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#4367 closed defect (fixed)

command_process_auth_challenge_cell() mishandles lack of expected auth type

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version: Tor: 0.2.3.7-alpha
Severity: Keywords: tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In command_process_auth_challenge_cell() we start with

use_type = -1;

Then we do

  for (i=0; i < n_types; ++i, cp += 2) {
    uint16_t authtype = ntohs(get_uint16(cp));
    if (authtype == AUTHTYPE_RSA_SHA256_TLSSECRET)
      use_type = authtype;
  }

Then we check

  if (use_type && public_server_mode(get_options())) {

So if we didn't find any authtypes of 1 offered, use_type will remain -1, which is true, so we'll call connection_or_send_authenticate_cell(), which will fail, and we'll mark the conn.

It's not so bad since connection_or_send_authenticate_cell() just does a log_warn at LD_BUG and returns.

But it's not entirely harmless, since in the far future if we stop listing authtype 1, these tors will close the conn when they ought to (at least based on what the code is trying to do; it's up for grabs if this is actually the right behavior) be just not authenticating.

I think we can fix just by initting use_type to 0.

Reported and diagnosed by frosty_un.

Child Tickets

Change History (7)

comment:1 in reply to:  description Changed 8 years ago by arma

Replying to arma:

I think we can fix just by initting use_type to 0.

Unless we want to support 0 as a valid auth type in the future, in which case we should leave use_type initialized to -1 and change the check to "if use_type >= 0 &&".

comment:2 in reply to:  description Changed 8 years ago by arma

Version: Tor: 0.2.3.7-alpha

Replying to arma:

...these tors will close the conn when they ought to (at least based on what the code is trying to do; it's up for grabs if this is actually the right behavior) be just not authenticating.

If the response is "just drop it", then we end up never sending the netinfo cell. That's a separate bug: I've opened it as #4368.

comment:3 Changed 8 years ago by nickm

Status: newneeds_review

This one is trivial; see branch bug4367 in my public repo. Ignoring the #4368 issues here; talking about them on #4368.

comment:4 Changed 8 years ago by Sebastian

Looks good to me.

comment:5 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Applying and closing. Thanks for the review!

comment:6 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:7 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.