Opened 2 years ago

Closed 2 years ago

#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 2 years 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 2 years ago by isabela

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

comment:3 Changed 2 years ago by dgoulet

Parent ID: #19531

comment:4 Changed 2 years ago by teor

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

Milestone renamed

comment:5 Changed 2 years ago by hji

Status: newneeds_review

comment:6 Changed 2 years 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 2 years ago by nickm

tested and merged. lgtm too.

comment:8 Changed 2 years ago by nickm

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