bzip2recover: Use snprintf to build the output file nameHEADmaster

The per-block output file name is built in outFileName[BZ_MAX_FILENAME]
with sprintf(split, "rec%5d", wrBlock+1).  split is an interior pointer
into outFileName, past any leading directory component of the input file
name, so the bytes written here come right after attacker-influenced
path data.

The write is currently safe only by way of two non-local invariants: the
input file name is rejected up front once strlen(argv[1]) reaches
BZ_MAX_FILENAME-20, and the number of blocks is limited by
BZ_MAX_HANDLED_BLOCKS so the formatted value stays at five digits.  The
comment on BZ_MAX_HANDLED_BLOCKS explicitly invites raising it, which
would silently eat into that 20 byte margin.

Bound the write with snprintf() using the space left after the directory
prefix (BZ_MAX_FILENAME - ofs) so it can no longer overrun outFileName
regardless of those invariants.  This also removes the deprecated
sprintf(), which recent compilers flag under -Wdeprecated-declarations.

For valid input the result is unchanged: the "rec%5d" prefix always fits,
so snprintf() writes exactly the same bytes.

https://sourceware.org/bugzilla/show_bug.cgi?id=29280

1 files changed, 1 insertions, 1 deletions

diff --git a/bzip2recover.c b/bzip2recover.c
index 8c79a90..e6c0a99 100644
--- a/bzip2recover.c
+++ b/bzip2recover.c
@@ -490,7 +490,7 @@ Int32 main ( Int32 argc, Char** argv )
          }
          /* Now split points to the start of the basename. */
          ofs  = split - outFileName;
-         sprintf (split, "rec%5d", wrBlock+1);
+         snprintf (split, BZ_MAX_FILENAME - ofs, "rec%5d", wrBlock+1);
          for (p = split; *p != 0; p++) if (*p == ' ') *p = '0';
          strcat (outFileName, inFileName + ofs);