Updater doesn't check for large negative offsets when applying patch

UNCONFIRMED
Unassigned

Status

()

Toolkit
Application Update
P4
minor
UNCONFIRMED
11 months ago
11 months ago

People

(Reporter: dov.murik, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

11 months ago
From: toolkit/mozapps/update/updater/bspatch.cpp@c2526f6786f0 around line 172:

      /* "seek" forwards in oldfile by z bytes */

      if (fbuffer + ctrlsrc->z > fbufend) {
        rv = UNEXPECTED_BSPATCH_ERROR;
        goto end;
      }
      fbuffer += ctrlsrc->z;

      /* and on to the next control block */


The `if` check on line 172 doesn't take into account z values which are negative. Supposedly bsdiff can create patches which have negative offsets for the z field.

If an attacker can control the patch file (which is very serious even without this bug), they can set the z value to be a large negative value. The `if (fbuffer + ctrlsrc->z > fbufend)` check will pass, and then we'll advance fbuffer into addresses possibly outside the memory actually held by fbuffer (lower addresses than the original fbuffer), possibly exposing sensitive memory areas.


I didn't produce such an illegal patch file.

Suggestions:

We should properly cover valid areas for moving the fbuffer pointer so it remains within the area allocated for it. Maybe save fbufstart (like we have fbufend) and make sure we don't move outside [fbufstart, fbufend). 

Or, if we know that z should never be negative, then abort on negative values for z (or define it as uint32_t, like x and y).
An attacker would have to break mar signing to take control of the patch file.
Severity: normal → minor
Priority: -- → P4
You need to log in before you can comment on or make changes to this bug.