Opened 5 weeks ago

Closed 5 weeks ago

Last modified 5 weeks ago

#23532 closed defect (fixed)

NETINFO clock skew detection doesn't work on clients

Reported by: catalyst Owned by: catalyst
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: bootstrap clock-skew
Cc: Actual Points:
Parent ID: #23508 Points:
Reviewer: Sponsor:

Description

The NETINFO clock skew detection code in channel_tls_process_netinfo_cell() doesn't work for clients because of an apparently unnecessary call to router_get_by_id_digest(), which will generally return NULL on clients (which generally download only microdescriptors therefore won't populate the routerlist). This NULL return prevented the clock skew reporting code from running. Thanks to arma for pointing this out on IRC. Fixing this would help bootstrap fail more quickly in some clock skew situations.

Child Tickets

Change History (9)

comment:1 Changed 5 weeks ago by catalyst

Status: assignedneeds_review

comment:2 Changed 5 weeks ago by nickm

Hm. I think the original intention of that check was that we should look at the skew on connections with relays, but not on connections from clients. Would this code make relays look at the time in netinfo from clients?

Maybe instead of router_get_by_id_digest() we should be looking at whether the connection started_here or not?

comment:3 Changed 5 weeks ago by catalyst

I think it might cause relays to log client clock skews where they didn't previously. Do we want to suppress that? It probably wouldn't interfere with relay bootstrapping unless an authority had a wrong time.

comment:4 Changed 5 weeks ago by catalyst

Maybe the correct test is

  (labs(apparent_skew) > NETINFO_NOTICE_SKEW &&
   (chan->conn->handshake_state->started_here ||
    router_get_by_id_digest(chan->conn->identity_digest))

?

comment:5 Changed 5 weeks ago by nickm

Good, but how about connection_or_digest_is_known_relay instead of router_get_by_id_digest?

comment:6 Changed 5 weeks ago by catalyst

Thanks, updated the merge request accordingly, along with some bonus refactoring (which helped make some conditionals more readable).

comment:7 Changed 5 weeks ago by nickm

Resolution: fixed
Status: needs_reviewclosed

lgtm; merging now!

comment:8 Changed 5 weeks ago by catalyst

Parent ID: #23508

comment:9 in reply to:  2 Changed 5 weeks ago by arma

Replying to nickm:

Hm. I think the original intention of that check was that we should look at the skew on connections with relays, but not on connections from clients. Would this code make relays look at the time in netinfo from clients?

Good catch!

I'm glad this ticket got resolved well.

Note: See TracTickets for help on using tickets.