Tor sends clock_skew status event warn too liberally

changed milestone to %Tor: unspecified

added component::core tor/tor milestone::Tor: unspecified owner::AltF4 priority::low resolution::fixed status::closed type::defect version::0.2.1.19 labels

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.

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.

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

Trac:
Username: stars

Trac:
Milestone: N/A to Tor: unspecified
Keywords: N/A deleted, N/A added
Description: 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]

to

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]
Parent: N/A to N/A

Made a fix at git@github.com: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.

Trac:
Username: AltF4
Pointsdone: N/A to N/A
Points: N/A to N/A
Actualpointsdone: N/A to N/A
Status: new to needs_review
Actualpoints: N/A to N/A

I just opened #2528 (closed) 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.

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

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

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.)

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 (moved)

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"?

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

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.

Trac:
Username: AltF4
Status: needs_review to assigned
Owner: N/A to AltF4

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

Trac:
Username: AltF4
Status: assigned to needs_review

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.

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.

Trac:
Username: AltF4

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

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

Trac:
Resolution: None to fixed
Status: needs_review to closed

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

Trac:
Status: closed to reopened
Resolution: fixed to N/A

Tor sends clock_skew status event warn too liberally

Child items 0

Activity