Opened 5 years ago

Closed 5 years ago

Last modified 2 years ago

#2324 closed defect (fixed)

realloc should check SIZE_T_CEILING too?

Reported by: arma Owned by:
Priority: major Milestone:
Component: Tor Version: Tor: 0.2.1.26
Keywords: Cc:
Actual Points: Parent ID:
Points:

Description

Our recent code security fixes made malloc check

  tor_assert(size < SIZE_T_CEILING);

but we didn't add a similar check to tor_realloc().

Assuming we do add it, doors pointed out another gotcha:

In tor_gzip_uncompress() we

        *out = tor_realloc(*out, out_size);
        stream->next_out = (unsigned char*)(*out + offset);
        if (out_size - offset > UINT_MAX) {
          log_warn(LD_BUG,  "Ran over unsigned int limit of zlib while "
                   "uncompressing.");
          goto err;
        }

And since the largest compressed blob we'll accept is MAX_DIR_DL_SIZE (16MBish), a compress bomb (e.g. a consensus answer) could create a string that's more than SIZE_T_CEILING yet less than UINT_MAX, thus remotely triggering the assert in tor_realloc.

Child Tickets

Change History (12)

comment:1 Changed 5 years ago by nickm

  • Milestone set to Tor: 0.2.1.x-final
  • Priority changed from normal to major

Sounds fine; somebody should write the patches.

comment:2 Changed 5 years ago by nickm

  • Resolution set to fixed
  • Status changed from new to closed

Done. Merged because it is so trivial. "What could possibly go wrong".

comment:3 Changed 5 years ago by nickm

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:4 Changed 5 years ago by nickm

Oops, still need to fix the uncompress-bomb case.

comment:5 follow-up: Changed 5 years ago by nickm

  • Status changed from reopened to needs_review

See bug2324_uncompress in my public repository.

comment:6 in reply to: ↑ 5 Changed 5 years ago by cypherpunks

Replying to nickm:

See bug2324_uncompress in my public repository.

if (out_size > SIZE_T_CEILING || out_size > UINT_MAX)

then it must be

if (out_size >= SIZE_T_CEILING || out_size > UINT_MAX)
#define MAX_UNCOMPRESSION_FACTOR 25

Why 25? Why not 20, 30? How it was calculated?

comment:7 Changed 5 years ago by nickm

then it must be [...]

fixed.

Why 25? Why not 20, 30? How it was calculated?

More or less arbitrarily. We only use zlib on our own file formats, and all our current documents all compress around 2x-3x at best, so I figured that anything that compresses an order of magnitude better than that is either a very badly designed file format, or a zlib bomb.

More generally: There needs to be _some_ means to prevent zlib bombs. The options I can think of are: a hard limit on how large something is allowed to uncompress to (e.g, "The result of an uncompress operation may not be larger than X") or a limit on the ratio of uncompressed to compressed size (e.g., "out_size/in_size must be less than Y").

I prefer the second approach, since it more generically captures what is is bad about zlib bombs: that they provide an attack multiplier where an attacker can waste lots of your effort with relatively little bandwidth.

Given that, picking a value for Y is a trade-off: you want Y to be small to limit the attack multiplier, but you also want Y to be large enough so that no legitimate document --even ones we might invent in the future -- ever compresses by a factor of greater than Y. Within those parameters, there's a reasonably large range of possible values. IMO, anything over 8 is probably safe; IMO anything under 50 is probably sufficient.

comment:8 Changed 5 years ago by Sebastian

Looks good to me generally. Maybe the log_warn about the (received) zlib bomb and overrunning SIZE_T_CEILING should be a protocol warning?

comment:9 Changed 5 years ago by arma

Wordo:

+          log_warn(LD_GENERAL, "Input looks look a possible zlib bomb; "

re sebastian's comment, I'm ok leaving the warns at warns rather than protocol_warn: if this starts happening consistently, we'll want to know about it asap. If it happens rarely, so be it.

Other than that, looks great, merge when you like.

comment:10 Changed 5 years ago by nickm

  • Resolution set to fixed
  • Status changed from needs_review to closed

cleaned up , added a comment, and merged to 0.2.1 and later.

comment:11 Changed 3 years ago by nickm

  • Component changed from Tor Client to Tor

comment:12 Changed 2 years ago by nickm

  • Milestone Tor: 0.2.1.x-final deleted

Milestone Tor: 0.2.1.x-final deleted

Note: See TracTickets for help on using tickets.