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

Status: UNCONFIRMED

(Toolkit :: Application Update, defect, P3)

Description

[dov.murik]

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).

Comment 1

[Robert Strong (they/them - no direct email)]

An attacker would have to break mar signing to take control of the patch file.