Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#4413 closed defect (fixed)

Non-triggerable integer overflow in crypto_random_hostname()

Reported by: asn Owned by:
Priority: Low Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Keywords: easy tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

char *
crypto_random_hostname(int min_rand_len, int max_rand_len, const char *prefix,
                       const char *suffix)
...
  randlen = min_rand_len + crypto_rand_int(max_rand_len - min_rand_len + 1);
...
  rand_bytes_len = ((randlen*5)+7)/8;
  if (rand_bytes_len % 5)
    rand_bytes_len += 5 - (rand_bytes_len%5);
  rand_bytes = tor_malloc(rand_bytes_len);

If randlen overflows in rand_bytes_len = ((randlen*5)+7)/8; we pass a negative value to tor_malloc().

I don't see this happening any time soon, since all the currently used crypto_random_hostname() arguments are very small, but it might be good to fix it for completeness.

Child Tickets

Attachments (2)

0001-Bug-4413.patch (1.4 KB) - added by palmbardier 8 years ago.
proposed fix for the potential integer overflow
0002-Bug-4413.patch (1.3 KB) - added by palmbardier 8 years ago.
revised patch as per nickm's recommendation

Download all attachments as: .zip

Change History (11)

comment:1 Changed 8 years ago by nickm

Keywords: easy added
Priority: normalminor

I agree that it is worth fixing for cleanness's sake.

In practice, though, it will never actually trigger, since the point of this function is to generate a random hostname component. Hostnames aren't supposed to have any pieces longer than 63 characters. So an acceptable fix would be to do "if (randlen > 63) randlen = 63;", replacing 63 with an appropriate macro.

comment:2 Changed 8 years ago by nickm

Milestone: Tor: unspecified

Marking as "unspecified", but I would take a fix if somebody wrote it.

Changed 8 years ago by palmbardier

Attachment: 0001-Bug-4413.patch added

proposed fix for the potential integer overflow

comment:3 Changed 8 years ago by palmbardier

Status: newneeds_review

comment:4 Changed 8 years ago by nickm

You're relying on crypto_rand_int returning the same thing in the check as it did in the assignment to randlen.

Also, you're making the error behavior be "return (char*)INT_MAX;" I'm not sure that makes a lot of sense: NULL is the usual way to indicate an error on returning a pointer.

And even if this patch worked, it wouldn't solve the actual issue noted above, where the overflow happens in the rand_bytes_len calculation.

What's wrong with the fix I suggested above?

Changed 8 years ago by palmbardier

Attachment: 0002-Bug-4413.patch added

revised patch as per nickm's recommendation

comment:5 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Tweaked some and merged; thanks!

comment:6 Changed 8 years ago by Sebastian

Resolution: fixed
Status: closedreopened

Looks like the tweak broke the build unfortunately for recent versions of openssl. bug4413 in my repo for a fix. Also spotted by xiando on #tor-dev

comment:7 Changed 8 years ago by nickm

Resolution: fixed
Status: reopenedclosed

Oops. Merged your patch. Thanks!

comment:8 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:9 Changed 7 years ago by nickm

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