#19222 closed defect (fixed)

base64_decode() unreachable heap corruption on 32-bit systems

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: tor-bug-bounty, 028-backport, isaremoved
Cc: Actual Points:
Parent ID: #19531 Points: 1
Reviewer: Sponsor:



this is a bug by Guido Vranken from our bug bounty program. After analysis, we found that there are no codepaths that allow the attacker to specify such a big input size to base64_decode() hence this bug should not be exploitable. More checking should be done, and there might be more instances of this rounding pattern around our codebase.

Here follows the bug report as received:

base64_decode(char *dest, size_t destlen, const char *src, size_t srclen)
  if (destlen < (srclen*3)/4)
    return -1;
  if (destlen > SIZE_T_CEILING)
    return -1;

The problem here is that the multiplication (by 3) occurs before the division (by 4).

For source strings larger than 0xFFFFFFFF / 3 == 0x55555555, an overflow will occur within this calculation. If the result of the overflow-affected calculation is smaller than what destlen is, then
this check will be passed and memory will be corrupted.

Child Tickets

Change History (8)

comment:1 Changed 21 months ago by nickm

Keywords: 028-backport added; 029-proposed removed
Milestone: Tor: 0.2.???Tor: 0.2.9.x-final

Adding this (and #19223) to 0.2.9 on the theory that we have very seldom regretted hardening a function against future mistakes

comment:2 Changed 19 months ago by isabela

Keywords: isaremoved added
Milestone: Tor: 0.2.9.x-finalTor: 0.2.???

comment:3 Changed 18 months ago by dgoulet

Parent ID: #19531

comment:4 Changed 15 months ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:5 Changed 15 months ago by hji

Status: newneeds_review

comment:6 Changed 15 months ago by teor

Milestone: Tor: 0.3.???Tor: 0.3.0.x-final
Status: needs_reviewmerge_ready

Looks good to me, but I haven't run it.
It should be tested on at least i386 and x86_64 before we merge.

(Or we could just merge, and let the buildbots do that.)

comment:7 Changed 14 months ago by nickm

tested and merged. lgtm too.

comment:8 Changed 14 months ago by nickm

Resolution: fixed
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.