Bug 204230 - [patch] bsdiff(1) - check file size against SIZE_MAX
Summary: [patch] bsdiff(1) - check file size against SIZE_MAX
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: misc (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Andrey A. Chernov
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2015-11-02 21:52 UTC by tobias
Modified: 2016-09-15 18:49 UTC (History)
2 users (show)

See Also:
ache: mfc-stable10+


Attachments
proposed fix (701 bytes, patch)
2015-11-02 21:52 UTC, tobias
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description tobias 2015-11-02 21:52:26 UTC
Created attachment 162714 [details]
proposed fix

When bsdiff opens binary files, it does not properly check if the file size (off_t 64 bit) is larger than theoretical maximum memory size (size_t 64 or just 32 bit).

Files that large won't work with the current code, because they clearly run out of memory. But due to +1 calculation, the integer value can overflow and binary data is written into unallocated memory (0 bytes), crashing the application.

The fix is simple: If file is too large, handle it as an I/O error.
Comment 1 Andrey A. Chernov freebsd_committer 2015-11-03 04:53:27 UTC
It should be (SIZE_MAX - 1) not ~0
Comment 2 Andrey A. Chernov freebsd_committer 2015-11-03 04:56:08 UTC
(In reply to Andrey A. Chernov from comment #1)
I.e (oldsize>(SIZE_MAX - 1))
Comment 3 Andrey A. Chernov freebsd_committer 2015-11-03 06:52:52 UTC
Comment on attachment 162714 [details]
proposed fix

This fix is not pretty. 
1) It does not set errno (EFBIG)
2) Next overflow is few lines below:
if(((I=malloc((oldsize+1)*sizeof(off_t)))==NULL) ||
    ((V=malloc((oldsize+1)*sizeof(off_t)))==NULL)) err(1,NULL);
Comment 4 commit-hook freebsd_committer 2015-11-03 09:50:31 UTC
A commit references this bug:

Author: ache
Date: Tue Nov  3 09:50:11 UTC 2015
New revision: 290329
URL: https://svnweb.freebsd.org/changeset/base/290329

Log:
  Use meaningful errno for ssize_t overflow in read().
  Catch size_t overflow in malloc().

  PR:     204230
  MFC after:      1 week

Changes:
  head/usr.bin/bsdiff/bsdiff/bsdiff.c
Comment 5 commit-hook freebsd_committer 2015-11-03 17:28:09 UTC
A commit references this bug:

Author: ache
Date: Tue Nov  3 17:27:25 UTC 2015
New revision: 290336
URL: https://svnweb.freebsd.org/changeset/base/290336

Log:
  Check for (old|new)size + 1 overflows off_t.

  PR:     204230
  MFC after:      1 week

Changes:
  head/usr.bin/bsdiff/bsdiff/bsdiff.c
Comment 6 tobias 2015-11-03 21:38:00 UTC
Thanks for your review and adjustments to fit FreeBSD's infrastructure, and spotting the multiplication overflow!

Why do you have added these latest checks though? Isn't the check for SSIZE_MAX enough, because SSIZE_MAX is smaller than SIZE_T_MAX? And even if OFF_MAX is smaller than SSIZE_MAX, there would be no integer overflow.

At least that's how I see it, would be nice to hear your reasoning.
Comment 7 Andrey A. Chernov freebsd_committer 2015-11-04 08:50:08 UTC
(In reply to tobias from comment #6)
I write this check keeping portability in mind, it means any POSIX compliant system, not FreeBSD specifically.
POSIX, introducing off_t and ssize_t, does not guarantee any relations between (s)size_t and off_t. In theoretical worst case (s)size_t can be even bigger, in practical case they all (including ssize_t and size_t) can be equally big. No standard guarantees that ssize_t should be smaller than size_t too. But don't worry, modern C compilers auto-optimize all constant comparison conditions to single smallest one.
Comment 8 Andrey A. Chernov freebsd_committer 2015-11-04 09:04:20 UTC
BTW, bsdiff code can greatly reduce memory (malloc) requirements on some architectures by using ssize_t offsets internally instead of off_t offsets, but I am too lazy to rewrite it in such large scale.
Comment 9 commit-hook freebsd_committer 2015-11-08 14:23:39 UTC
A commit references this bug:

Author: ache
Date: Sun Nov  8 14:22:57 UTC 2015
New revision: 290546
URL: https://svnweb.freebsd.org/changeset/base/290546

Log:
  MFC: r290329,r290336
  PR: 204230

  r290329:

  Use meaningful errno for ssize_t overflow in read().
  Catch size_t overflow in malloc().

  r290336:

  Check for (old|new)size + 1 overflows off_t.

Changes:
_U  stable/10/
  stable/10/usr.bin/bsdiff/bsdiff/bsdiff.c