Opened 9 years ago

Closed 8 years ago

Last modified 4 months ago

#1074 closed defect (fixed)

Tor sends clock_skew status event warn too liberally

Reported by: arma Owned by: AltF4
Priority: Low Milestone: Tor: unspecified
Component: Core Tor/Tor Version: 0.2.1.19
Severity: Keywords:
Cc: arma, Sebastian, nickm, edmanm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by nickm)

In command.c, if netinfo tells us our clock is skewed, we log based on:

if (router_digest_is_trusted_dir(conn->identity_digest))

severity = LOG_WARN;

else

severity = LOG_INFO;

But then we complain to the controller regardless of whether it's an authority:

control_event_general_status(LOG_WARN,

"CLOCK_SKEW SKEW=%ld SOURCE=OR:%s:%d",
apparent_skew, conn->_base.address, conn->_base.port);

So the simple fix is for 0.2.1.20 to only send the status event if
severity == LOG_WARN.

Then Vidalia should workaround it by ignoring SOURCE=OR: clock skew events
from Tor <= 0.2.1.19 and 0.2.2.1-alpha.

The broader challenge is that tor clients avoid going to the authorities, so
if their clock is wrong, they'll never do more than log at log-level info, and
all this status event work is for naught.

We should fix this in 0.2.2.x, either with a more aggressive proposal, or at least
with a bit we remember that makes us connect to an authority (and thus get a
trusted opinion on our time) if we keep getting skew problems from other relays.

[Automatically added by flyspray2trac: Operating System: All]

Child Tickets

TicketStatusOwnerSummaryComponent
#2528closedchiiphDoes Vidalia display the CLOCK_SKEW event to the user even on severity notice?Archived/Vidalia

Change History (29)

comment:1 Changed 9 years ago by arma

Another option is to send the clock skew status event at severity "notice"
if we're not concerned about it, and "warn" if we are, and leave it to
the controller to decide whether it should care. This would give Vidalia
more flexibility to use algorithms like "if we get complaints from n different
places, then tell the user".

This approach seems like a poor move though, for the same reason punting
security decisions to users is a poor option. Tor should figure out what
means there's a problem, and tell the controllers when there's a problem.

comment:2 Changed 9 years ago by arma

Ok, 64f393d56f8ff58 does a start at solving the problem. Now vidalia should
do the workaround I describe above.

Future Tors should try to figure out a more useful way to get the warning to
the user if the clock is actually wrong -- step one is finding a reliable way
to learn if the clock is actually wrong.

comment:3 Changed 9 years ago by stars

yes, it don't warn anymore for the clock yet. Thanks very much .

comment:4 Changed 8 years ago by nickm

Description: modified (diff)
Milestone: Tor: unspecified

comment:5 Changed 8 years ago by AltF4

Status: newneeds_review

Made a fix at git@…:altf4/tor.git 08b147f65a2213178924. (That exact version has some small formatting issues. A corrected version is forthcoming)

The basic idea in the change is that when we get a large skew from an untrusted source, we contact a trusted dir. Then we set a flag we set any time skew data is received from a trusted dir so that we only bother the dir once.

Per Sebastian: This does not cover the case of bridges with large time skews, as they do not use NETINFO cells.

comment:6 Changed 8 years ago by arma

I just opened #2528 as a child for this bug, to investigate if Vidalia is telling the user something that Tor didn't think was worth telling the user but for some reason Tor thought was still worth telling Vidalia.

comment:7 Changed 8 years ago by Sebastian

Review comments:

+      const tor_addr_t trusted_dir_addr;
+      tor_addr_from_ipv4n(&trusted_dir_addr, any_trusted_dir->addr);

You have to lose the const here (try compiling your code with --enable-gcc-warnings argument for configure)

Generally the patch had me fooled a bit. A slightly changed version is in branch bug1074 in my repository. See https://gitweb.torproject.org/sebastian/tor.git/shortlog/refs/heads/bug1074 and https://gitweb.torproject.org/sebastian/tor.git/commitdiff/34008d860e3c385128148bdb619fa4fd1322369e

Also more nitpicking: please wrap your commit messages too, that makes them easier to read with the normal git tools

comment:8 Changed 8 years ago by Sebastian

erm, that last link to the diff was supposed to be https://gitweb.torproject.org/sebastian/tor.git/commitdiff/bug1074

comment:9 Changed 8 years ago by nickm

Looks okay to me now. I'm thinking of applying it to master, not maint-0.2.2, on the grounds that it's a new feature. Thoughts?

Altf4, I'll hold off merging for since Sebastian wants to see if you're okay with his changes.

Also, the flag should at least turn into a tristate of "Haven't queried an authority; have launched a query, but not gotten an answer; have gotten an answer." Or it could turn into a rate-limiter. (I want to avoid the case where if you open 3 connections, and all 3 say you're skewed, you immediately launch 3 connections to trusted dir servers.)

comment:10 Changed 8 years ago by arma

See bug1074-part2 in my arma for the fix to the original 1074 bug. (I named it part2 so it won't conflict with the feature that altf4 is adding under the same name.)

My branch applies to maint-0.2.1, since a) it's so simple, what could possibly go wrong, and b) actual users continue to experience this bug en masse, e.g. #2523

comment:11 in reply to:  10 Changed 8 years ago by arma

Replying to arma:

See bug1074-part2 in my arma for the fix to the original 1074 bug. (I named it part2 so it won't conflict with the feature that altf4 is adding under the same name.)

Ok, I merged it.

I believe bug 1074 is now fixed. Should we open a new trac entry for the thing that altf4 is working on, which is better described as "A Tor client with a skewed clock will sometimes never warn about the clock skew"?

comment:12 Changed 8 years ago by nickm

let's leave this open for now and in needs_review. AltF4's code is almost done.

comment:13 in reply to:  9 Changed 8 years ago by AltF4

Owner: set to AltF4
Status: needs_reviewassigned

I went and made a new revision in my github: git://github.com/altf4/tor.git rev 8035f422180435259296.

Nickm said on irc that he has a better way of doing tristates, so I'll look into that. But functionally, I think this is correct.

Oh, and I referred to this bug as "1074 part 3" for reference.

comment:14 Changed 8 years ago by AltF4

Status: assignedneeds_review

(didn't mean to change ticket status. Don't know how that happened. Changing back.)

comment:15 Changed 8 years ago by nickm

The "tor_addr_from_ipv4n" part looks wrong: the n at the end means that the input IPv4 address is in network order, but I am 99% sure that the one in routersatus_t.addr is in host order (so tor_addr_from_ipv4h would be appropriate). Also, if you're going to do a connection_or_connect(), the third parameter needs to be identity digest (the hash of the public key of the host you're connecting to), rather than the descriptor digest (the hash of the descriptor you have for that host).

It looks like we only do "received_netinfo_from_trusted_dir = 2" if we get a netinfo from an authority *and* it's not very skewed. Is that right?

This is a decent first step: I wonder if we can merge this with the skew detection from authenticated Http to directory servers, and I also think we should switch the logic to use a timeout rather than a simple "have we ever checked" flag. But this is the perfect being the enemy of the good: once the above stuff is fixed, the patch is probably mergeable.

comment:16 Changed 8 years ago by AltF4

Thank you for noticing those first two mistakes. I went and made an update to my git repo with those changes. Rev 2ccf2dca329d466e84b6. Let me actually test this code out before it gets merged in. I'll try to do that in the next few days.

And to your question, no. There are two "received_netinfo_from_trusted_dir = 2;" statements. One when a trusted dir returns a large skew. Then one when a trusted dir returns a small skew. IE: Anytime we get an answer from a trusted dir server.

comment:17 Changed 8 years ago by nickm

Resolution: Nonefixed
Status: needs_reviewclosed

Looks good. Rebasing, squashing a little, fixing whitespace again, and merging into master.

I've added a ticket #2628 for some remaining issues.

comment:18 Changed 8 years ago by cypherpunks

Resolution: fixed
Status: closedreopened

It is wrong (harm) idea. It is broken (harmfull) code.

comment:19 Changed 8 years ago by nickm

Cypherpunks, it is no good just to say "This is bad!" if you do not say what is bad about it.

comment:20 Changed 8 years ago by cypherpunks

I don't want my client connect to auth just because some broken relay. Error of code doesn't matter if idea is wrong.

Last edited 4 months ago by cypherpunks (previous) (diff)

comment:21 Changed 8 years ago by cypherpunks

Resolution: fixed
Status: reopenedclosed

comment:22 Changed 8 years ago by AltF4

Cypherpunks. what about the connection to the trusted server is of concern? From a privacy standpoint? Or a bandwidth for the server issue?

Would a voting system be better? Like, only connect to a trusted authority if a majority of at least some X untrusted servers tell us we're skewed?

comment:23 Changed 8 years ago by rransom

Resolution: fixed
Status: closedreopened

See #2639. mobmix has complained about commit b8bef61a:

2011-02-27 07:18 <mobmix> router_pick_trusteddirserver(NO_AUTHORITY, 0) can't return anything but NULL.

comment:24 Changed 8 years ago by Sebastian

Indeed, this is exactly what happens (which leads to a segfault if either we have the wrong time or someone lies to us about the time).

      const routerstatus_t *any_trusted_dir =
        router_pick_trusteddirserver(NO_AUTHORITY, 0);
      tor_addr_t trusted_dir_addr;
      tor_addr_from_ipv4h(&trusted_dir_addr, any_trusted_dir->addr);

is where the segfault happens.

comment:25 Changed 8 years ago by nickm

Sounds like we should:

  • fix the arguments to router_pick_trusteddirserver, and
  • make the "launch a connection" stuff happen for relays only.

Who will write the patch?

comment:26 Changed 8 years ago by Sebastian

Why make it happen for relays only?

comment:27 Changed 8 years ago by nickm

On reflection, having more connections from clients to authorities makes me unhappy too.

comment:28 Changed 8 years ago by nickm

Resolution: fixed
Status: reopenedclosed

Okay, 42c1a471 reverts the branch. This needs more design and less crashing before it can go in 0.2.3.x. Closing this bug report; #2628 is a fine place for figuring out the right behavior.

comment:29 Changed 6 years ago by nickm

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