Opened 7 years ago

Closed 7 years ago

Last modified 4 years ago

#2324 closed defect (fixed)

realloc should check SIZE_T_CEILING too?

Reported by: arma Owned by:
Priority: High Milestone:
Component: Core Tor/Tor Version: Tor: 0.2.1.26
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

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 7 years ago by nickm

Milestone: Tor: 0.2.1.x-final
Priority: normalmajor

Sounds fine; somebody should write the patches.

comment:2 Changed 7 years ago by nickm

Resolution: fixed
Status: newclosed

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

comment:3 Changed 7 years ago by nickm

Resolution: fixed
Status: closedreopened

comment:4 Changed 7 years ago by nickm

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

comment:5 Changed 7 years ago by nickm

Status: reopenedneeds_review

See bug2324_uncompress in my public repository.

comment:6 in reply to:  5 Changed 7 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 7 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 7 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 7 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 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

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

comment:11 Changed 5 years ago by nickm

Component: Tor ClientTor

comment:12 Changed 4 years ago by nickm

Milestone: Tor: 0.2.1.x-final

Milestone Tor: 0.2.1.x-final deleted

Note: See TracTickets for help on using tickets.