#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:

Description

Hello,

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:


int
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 18 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 16 months ago by isabela

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

comment:3 Changed 15 months ago by dgoulet

Parent ID: #19531

comment:4 Changed 12 months ago by teor

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

Milestone renamed

comment:5 Changed 11 months ago by hji

Status: newneeds_review

comment:6 Changed 11 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 11 months ago by nickm

tested and merged. lgtm too.

comment:8 Changed 11 months ago by nickm

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