Opened 4 years ago

Closed 4 years ago

#17739 closed enhancement (fixed)

Refactor clock skew warning code to avoid duplication

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: easy refactor TorCoreTeam201512
Cc: Actual Points:
Parent ID: #9675 Points:
Reviewer: Sponsor:

Description

The following functions contain very similar clock skew code:

  • connection_dir_client_reached_eof
  • channel_tls_process_netinfo_cell
  • or_state_load

We should unify this code to reduce redundancy and increase consistency.

Child Tickets

Attachments (1)

0001-Refactor-clock-skew-warning-code-to-avoid-duplicatio.patch (8.1 KB) - added by arlolra 4 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 4 years ago by arma

I'm a fan!

Also this will make it smoother for us to trigger the "take further actions if it seems your clock might be skewed" ideas, consistently, once we know what we want to do.

comment:2 Changed 4 years ago by teor

Parent ID: #9675

This would really help with #9675

comment:3 Changed 4 years ago by arlolra

Status: newneeds_review

Something like that?

comment:4 in reply to:  3 Changed 4 years ago by dgoulet

Replying to arlolra:

Something like that?

I like it!

Would be really nice if you could add comment on top of this new function explaining the parameters like why do we change severity if it's trusted or not?. Also, maybe you should const a bit of params to make it explicit that we shouldn't change any of them (conn, received and source).

comment:5 Changed 4 years ago by teor

Status: needs_reviewneeds_revision

+1 for dgoulet's comments: let's describe the parameters in a comment (other functions do this), and make the pointers const.

Refactor or_state_load

or_state_load also issues a clock skew warning when the state file contains a last written date in the future.

Can you update the patch so it calls clock_skew_warning from or_state_load?

Note that servers could be wrong or lying about the time

In #17716, nickm wanted the clock skew message to mention that remote servers could be sending the wrong time. I'll remove it from that branch, and we can fix it here.

Please add:
"or the remote server is sending us the wrong time"
when there is a remote server. (There won't be a remote server if we're reading the time from the state file.)

comment:6 Changed 4 years ago by teor

Keywords: TorCoreTeam201512 added
Milestone: Tor: 0.2.9.x-finalTor: 0.2.8.x-final
Version: Tor: 0.2.7Tor: unspecified

comment:7 Changed 4 years ago by arlolra

Status: needs_revisionneeds_review

let's describe the parameters in a comment (other functions do this), and make the pointers const.

Done

Can you update the patch so it calls clock_skew_warning from or_state_load?

The only shared part is the string, "Tor requires an accurate clock to work: please check your time, timezone, and date settings." If you want, we can put somewhere and interpolate it in both places. Otherwise, how would you suggest I update the patch?

Note that servers could be wrong or lying about the time

Done

comment:8 in reply to:  7 Changed 4 years ago by teor

Status: needs_reviewneeds_revision

Replying to arlolra:

Can you update the patch so it calls clock_skew_warning from or_state_load?

The only shared part is the string, "Tor requires an accurate clock to work: please check your time, timezone, and date settings." If you want, we can put somewhere and interpolate it in both places. Otherwise, how would you suggest I update the patch?

However we detect clock skew, we want to take the same actions. At the moment, that's:

  • warn the user
  • send the controller a clock skew event

In or_state_load can you do something like:

clock_skew_warning(NULL, apparent_skew, 1, LD_GENERAL, "local state file", state_file_path);

This would make the logic in clock_skew_warning slightly more complex - please check that conn is not NULL before printing conn->address and conn->port.

comment:9 Changed 4 years ago by arlolra

Status: needs_revisionneeds_review

However we detect clock skew, we want to take the same actions

Done

comment:10 Changed 4 years ago by teor

Status: needs_reviewneeds_revision

We're almost there!

Looks great, except for this one bit:

  if (conn) 
    tor_asprintf(&source, "%s:%s:%d", source, conn->address, conn->port);

We can't guarantee that source will be large enough to hold the combined strings of source, conn->address, and conn->port. (And reading from a string to print into itself is risky, even if it works. It's much nicer to take read-only strings.)

Please use a (stack allocated) buffer that's large enough to hold a reasonable-length source / address / port string:

  • all the existing sources are under 20 characters long,
  • a max-length IPv6 address, and max-length port are MAX_ADDRESS_LENGTH long.

If you use tor_snprintf, it will make sure you don't go over the length of the buffer.

comment:11 Changed 4 years ago by arlolra

We can't guarantee that source will be large enough to hold the combined strings of source, conn->address, and conn->port.

https://gitweb.torproject.org/tor.git/tree/src/common/compat.c#n501

And reading from a string to print into itself is risky, even if it works. It's much nicer to take read-only strings.

https://gitweb.torproject.org/tor.git/tree/src/common/compat.c#n539

comment:12 in reply to:  11 Changed 4 years ago by teor

Replying to arlolra:

We can't guarantee that source will be large enough to hold the combined strings of source, conn->address, and conn->port.

https://gitweb.torproject.org/tor.git/tree/src/common/compat.c#n501

Apologies. I didn't understand tor_asprintf().

Please don't re-use source as a mutable variable. It works, but it changes the function signature, and confuses people. Please declare another char * to hold the result of tor_asprintf, and make source const.

If conn is NULL, you can use tor_strdup() on source and assign the newly allocated string to the same char *. This will make sure that you have a string allocated via malloc whether conn is NULL or not.

Please also call tor_free() on the newly allocated string from tor_asprintf() at the end of the function.

comment:13 Changed 4 years ago by arlolra

Please also call tor_free()

Yikes! Good point.

comment:14 Changed 4 years ago by arlolra

Status: needs_revisionneeds_review

Please declare another char * to hold the result of tor_asprintf, and make source const.

Done

comment:15 Changed 4 years ago by teor

Thanks! Looks great.
Let's get this merged, it will make arma happy.

comment:16 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Yes, it looks good to me. Merged it!

Note: See TracTickets for help on using tickets.