Opened 9 months ago

Closed 9 months ago

Last modified 9 months ago

#27165 closed defect (fixed)

CID 1438153 unlikely overflow in predicted_ports_prediction_time_remaining()

Reported by: catalyst Owned by: rl1987
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: coverity
Cc: Actual Points:
Parent ID: Points:
Reviewer: mikeperry Sponsor:

Description

There is an attempted overflow check in predicted_ports_prediction_time_remaining() that doesn't adequately protect against overflow. Coverity found this as CID 1438153. It seems unlikely to happen in practice.

Maybe making all the types in question into time_t would help.

Child Tickets

Change History (8)

comment:1 Changed 9 months ago by rl1987

Status: newneeds_review

Made a quick fix to change all values we use in above function to time_t:

https://github.com/torproject/tor/pull/277

However further code tends that expects int from predicted_ports_prediction_time_remaining() got some casts now. I have some reservations if this is net positive for code base health.

comment:2 Changed 9 months ago by teor

The time_t changes are a good idea.

But I suggest you do the cast back to int once, in predicted_ports_prediction_time_remaining().

And the new overflow check:

 if (BUG((prediction_timeout - idle_delta) > TIME_MAX)) {

Is wrong, because (time_t)x > TIME_MAX is always false.

Instead, use the correct test:

  /* We check that idle_delta is less than or equal to prediction_timeout, so any overflow is a bug */
  if (BUG((prediction_timeout - idle_delta) > INT_MAX)) {
     return INT_MAX;
  }

But this check may also invoke undefined behaviour, if prediction_timeout is TIME_T_MAX and idle_delta is negative. idle_delta can be negative if now is negative (or now - last_prediction_add_time overflows, because now is TIME_T_MAX and last_prediction_add_time is negative).

But if we try to algebraically rearrange it so it doesn't invoke undefined behaviour, we end up causing undefined behaviour when TIME_T and int are the same size:

  /* We check that idle_delta is less than or equal to prediction_timeout, so any overflow is a bug */
  if (BUG(prediction_timeout > (INT_MAX + idle_delta))) {
     return INT_MAX;
  }

So maybe we want this arrangement:

  /* We check that idle_delta is less than or equal to prediction_timeout, so any overflow is a bug */
  if (BUG(-idle_delta > (INT_MAX - prediction_timeout))) {
     return INT_MAX;
  }

But that's not undefined-behaviour free, either.

Maybe we should think carefully about what we're trying to do here?

Edit: remove fragment

Last edited 9 months ago by teor (previous) (diff)

comment:3 Changed 9 months ago by rl1987

Owner: set to rl1987
Status: needs_reviewaccepted

Will provide a better patch soonish.

comment:4 Changed 9 months ago by rl1987

Status: acceptedneeds_review

Another take at fixing this: https://github.com/torproject/tor/pull/280

comment:5 Changed 9 months ago by dgoulet

Reviewer: mikeperry

comment:6 Changed 9 months ago by mikeperry

Status: needs_reviewmerge_ready

I like this helper function approach. It is easy to see that things are behaving sanely.

Thanks!

comment:7 Changed 9 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

ok; merged!

comment:8 Changed 9 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.5.x-final
Note: See TracTickets for help on using tickets.