Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#2317 closed defect (fixed)

Missing sanity checks for cbtnummodes consensus parameter

Reported by: Sebastian Owned by:
Priority: High Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by nickm)

[]<doors> wtf. devs inserted trapdoors!? "tor_malloc_zero(num_modes*sizeof(build_time_t))" how much? anything else?

I think what doors was referring to is that we don't do any sanity checks on the value of the consensus parameter, so we can either request ridiculous amounts of memory or worse request 0 modes. Since doors immediately left irc I had no time to confirm if there was more.

Child Tickets

Change History (31)

comment:1 Changed 9 years ago by Sebastian

We should also check all other consensus parameters to make sure they are accepted in safe and sane limits only.

comment:2 Changed 9 years ago by Sebastian

Here we go with some more love from doors:

<doors> last version was backdoored. If you can't fix it don't use it.

<doors> you don't do any sanity checks on the value of the consensus parameterS.


<doors> "if (x <= a) then check if (x >= a) then CRASH"  is backdoor masked as 3.y.o. kid's mistake.

comment:3 Changed 9 years ago by nickm

Description: modified (diff)
Milestone: Tor: 0.2.1.x-final
Priority: normalmajor

Okay, let me know if the patch isn't going to be timely and I'll work on one.

Re doors: If he tells us about bugs, we can fix them. If he tells us we're three-year-olds, there isn't much we can do about that.

comment:4 Changed 9 years ago by Sebastian

My branch 2317 has the current progress. I'm still working on it and will continue to update it, but if someone else wants to finish that would be ok too. (Just posting it here in case you want it done more quickly)

comment:5 Changed 9 years ago by Sebastian

I should add that the limits I added so far come from mikeperry/cbt-param-limits

comment:6 Changed 9 years ago by Sebastian

Status: newneeds_review

First review-worthy version pushed.

comment:7 Changed 9 years ago by Sebastian

And updated, so that this merges cleanly with the whitespace fixes now in maint22

comment:8 Changed 9 years ago by nickm

Hm. I'm not seeing any commits to sebastian/bug2317 since 30 Dec?

comment:9 Changed 9 years ago by Sebastian

That's ok; I rebased.

comment:10 Changed 9 years ago by nickm

ah, ok. The minimal commit message had me fooled.

comment:11 Changed 9 years ago by Sebastian

Looks like I should fix that. doorss also noticed an issue with the changes file. Will update both. The rest should be ok.

comment:12 Changed 9 years ago by nickm

Please also have a look at parameters as used in maint-0.2.1. I only see one instance of networkstatus_get_param, but it should get audited.

comment:13 Changed 9 years ago by nickm

Reviewing:

  • All of our platforms are two's complement, so INT32_MAX is indeed always the same as (1<<31)-1 .
  • The non-constant minimum/maxiumum valuse in circuit_build_times_close_quantile() and circuit_build_times_initial_timeout() and networkstatus_get_bw_weight() make me a little twitchy. Can those become constants, or get their checks moved out of the networkstatus_get_param() arguments? My reasoning is that anything that makes networkstatus_get_param() have to clip a value is probably something to warn about loudly, but if the range is ever non-constant, we can't really have the warnings mean "something has gone wrong with your consensus".

comment:14 Changed 9 years ago by Sebastian

I think we can still warn, because the values are always dependant on the consensus (and nothing else) as far as I can see. Can you clarify your first comment?

comment:15 Changed 9 years ago by nickm

On my first comment: You have an inline comment in one of the specs asking if INT32_MAX or 2147483647 is best and if they're always equal. They're always equal, and I think "INT32_MAX" is a better way to refer to the value.

On the second: I'm not saying we shouldn't warn; I'm saying that it would be much cleaner to only use networkstatus_get_param() to enforce hard constant limits on values, and do any additional clipping and warning in the wrapper functions.

comment:16 in reply to:  14 Changed 9 years ago by arma

Replying to Sebastian:

I think we can still warn, because the values are always dependant on the consensus (and nothing else) as far as I can see. Can you clarify your first comment?

In general, we should be looking at two different questions here: first, are we getting a value that will cause overflows or otherwise threaten the stability or security of the Tor process? Second, are we getting a value that will cause weird or not-presently-expected behavior.

We should focus on the first question. If we enforce constraints for the second, we should be as liberal as we can -- part of the point of putting the value in the consensus is so we can change the value in the consensus and automatically change the behavior of Tors.

comment:17 in reply to:  12 Changed 9 years ago by arma

Replying to nickm:

Please also have a look at parameters as used in maint-0.2.1. I only see one instance of networkstatus_get_param, but it should get audited.

It looks solid. networkstatus_get_param() returns an int32_t, and checks via tor_parse_long() for a value between INT32_MIN and INT32_MAX. The result is written in circuit_initial_package_window() into an int32_t, which then does its own slightly tighter bounds checking.

circuit_initial_package_window() then returns an int32_t, which is written into an int for the various package_window elements.

So unless there's a platform where int can't fit an int32_t, I think we're in good shape.

There is a way to cause an overflow though, which is to send an exit relay 20 million sendme cells. At that point package_window will go negative, and the exit relay will assert. I'm not particularly worried, but at some point we might consider capping package_window at the value of circuit_initial_package_window().

comment:18 in reply to:  15 Changed 9 years ago by Sebastian

Replying to nickm:

On my first comment: You have an inline comment in one of the specs asking if INT32_MAX or 2147483647 is best and if they're always equal. They're always equal, and I think "INT32_MAX" is a better way to refer to the value.

Oh. My comment was meant to say "do people think these values (-1 to INT32_MAX) are sane". Sorry for the confusion.

comment:19 Changed 9 years ago by Sebastian

I updated commit msg and changes file entry. Haven't yet worked on the review comments from nick, not sure what to do with arma's comments.

comment:20 Changed 9 years ago by nickm

Milestone: Tor: 0.2.1.x-finalTor: 0.2.2.x-final

wrt arma's comments: I think no action is required for 0.2.1 (since his auditing shows it's ok). I think that the possible overflow on receiving 20 million sendmes is a separate bug; best open a ticket for that when we close this one.

comment:21 Changed 9 years ago by arma

         research indicates that a lower value would mean fewer cells in
         transit in the network at any given time. Obeyed by Tor 0.2.1.20
         and later.
+        Min: 1, Max: 100000 XXX are these sane

If an exit relay sets its circ package window higher than 1000, it will cause current Tor clients that use it to close those circuits because it will violate their deliver_window >= 0 checks.

But if we later change Tor clients to handle that better (e.g. to read the circwindow out of the consensus and assume that exit relays are using that value), then we wouldn't want to have put a constraint in.

Also, if the circ package window is 1, current Tor clients will fail to use that circuit well, because they won't send any sendmes back until they've gotten 100 cells. So we could argue for a minimum of 100. But at the same time, I don't want to dictate to future clients what their constraints should be, in case we change the sendme design in the future.

So why not [0, MAX_INT32] here?

comment:22 Changed 9 years ago by arma

         "CircuitPriorityHalflifeMsec" -- the halflife parameter used when
         weighting which circuit will send the next cell. Obeyed by Tor
         0.2.2.10-alpha and later.  (Versions of Tor between 0.2.2.7-alpha
         and 0.2.2.10-alpha recognized a "CircPriorityHalflifeMsec" parameter,
         but mishandled it badly.)
+        Min: -1, Max: 2147483647 (INT32_MAX) XXX are these sane?

Currently we use < 0 to declare that we're not using it, with -1 being the suggested value of < 0. But what if we want to use -2 later to mean something else? The current clients would continue ignoring the feature, but new clients would have new behavior.

I'm not saying I know we'll want to do that, but why constrain ourselves?

comment:23 Changed 9 years ago by arma

While I'm at it, here's a typo fix:

       cbtnummodes
         Default: 3
         Effect: This value governs how many modes to use in the weighted
-        average calculation of Pareto paramter Xm. A value of 3 introduces
+        average calculation of Pareto parameter Xm. A value of 3 introduces

I'd do this myself, but I bet I'll clobber your patch.

comment:24 in reply to:  22 Changed 9 years ago by nickm

Replying to arma:

Currently we use < 0 to declare that we're not using it, with -1 being the suggested value of < 0. But what if we want to use -2 later to mean something else? The current clients would continue ignoring the feature, but new clients would have new behavior.

IMO, if we want to have something we would do with a new value for this parameter, we should do it with a new parameter instead. There's no real reason to have a -2 when a new parameter would be cleaner.

comment:25 in reply to:  21 Changed 9 years ago by Sebastian

If an exit relay sets its circ package window higher than 1000, it will cause current Tor clients that use it to close those circuits because it will violate their deliver_window >= 0 checks.

But if we later change Tor clients to handle that better (e.g. to read the circwindow out of the consensus and assume that exit relays are using that value), then we wouldn't want to have put a constraint in.

Also, if the circ package window is 1, current Tor clients will fail to use that circuit well, because they won't send any sendmes back until they've gotten 100 cells. So we could argue for a minimum of 100. But at the same time, I don't want to dictate to future clients what their constraints should be, in case we change the sendme design in the future.

So why not [0, MAX_INT32] here?

Because we can change the min/max in these newer versions of Tor, and at least the older versions of Tor don't break. If your argument is that people will be confused about seeing warnings, what will they be more confused about - a Tor that still works but with a warning, or a Tor that breaks without a warning?

comment:26 Changed 9 years ago by Sebastian

New version (rebased onto maint-0.2.2, merges cleanly into master) pushed.

comment:27 Changed 9 years ago by Sebastian

ooops, updated with a spec patch

comment:28 Changed 9 years ago by rransom

Looks good to me.

comment:29 Changed 9 years ago by arma

Merged. I fixed a typo in the changelog, and added a bugfix on, too.

comment:30 Changed 9 years ago by arma

Resolution: fixed
Status: needs_reviewclosed

comment:31 Changed 7 years ago by nickm

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