Bug 195436 - patch utility, line number overflows checks
Summary: patch utility, line number overflows checks
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Pedro F. Giffuni
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-27 01:05 UTC by David CARLIER
Modified: 2014-12-17 00:59 UTC (History)
2 users (show)

See Also:


Attachments
Proposed patch (6.24 KB, patch)
2014-11-27 01:05 UTC, David CARLIER
no flags Details | Diff
Proposed patch (5.88 KB, patch)
2014-11-27 15:23 UTC, David CARLIER
no flags Details | Diff
Proposed patch (6.38 KB, patch)
2014-11-29 09:11 UTC, David CARLIER
no flags Details | Diff
Patch based on OpenBSD (6.12 KB, patch)
2014-12-07 19:42 UTC, Pedro F. Giffuni
no flags Details | Diff
Patch based on OpenBSD (6.08 KB, patch)
2014-12-07 21:00 UTC, Pedro F. Giffuni
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David CARLIER 2014-11-27 01:05:41 UTC
Created attachment 149926 [details]
Proposed patch

Inspired by OpenBSD's version. it does some overflow checks in case line numbers are beyond of LONG_MAX limits and such.
Comment 1 Pedro F. Giffuni freebsd_committer 2014-11-27 11:22:09 UTC
Hello and thank you;

the patch looks good, however I am traveling at this time and I can't play with it.
Comment 2 David CARLIER 2014-11-27 15:23:19 UTC
Created attachment 149941 [details]
Proposed patch
Comment 3 Pedro F. Giffuni freebsd_committer 2014-11-29 07:54:16 UTC
If the changes come from OpenBSD, it would be good to update the CVS revision information as the tags are used for reference in future updates.
Comment 4 David CARLIER 2014-11-29 09:11:24 UTC
Created attachment 149982 [details]
Proposed patch
Comment 5 Pedro F. Giffuni freebsd_committer 2014-12-05 14:39:27 UTC
Grab this.
Comment 6 Pedro F. Giffuni freebsd_committer 2014-12-07 19:42:03 UTC
Created attachment 150316 [details]
Patch based on OpenBSD

I am inclined to follow the complete OpenBSD change.

This revision precedes your original change:

Introduce strtolinenum to properly check line numbers while parsing:
no signs, no spaces, just digits, 0 <= x <= LONG_MAX
Comment 7 Pedro F. Giffuni freebsd_committer 2014-12-07 21:00:03 UTC
Created attachment 150320 [details]
Patch based on OpenBSD

Update the patch, now that I have merged some previous changes from OpenBSD.
Comment 8 commit-hook freebsd_committer 2014-12-08 15:10:59 UTC
A commit references this bug:

Author: pfg
Date: Mon Dec  8 15:10:49 UTC 2014
New revision: 275612
URL: https://svnweb.freebsd.org/changeset/base/275612

Log:
  patch(1): avoid line number overflows

  Introduce strtolinenum to properly check line numbers while parsing:
  no signs, no spaces, just digits, 0 <= x <= LONG_MAX

  Properly validate line ranges supplied in diff file to prevent overflows.
  Also fixes an out of boundary memory access because the resulting values
  are used as array indices.

  PR:	195436
  Obtained from:	OpenBSD (CVS pch.c rev 1.45, 1,46, common.h rev 1.28)
  MFC after:	1 week

Changes:
  head/usr.bin/patch/common.h
  head/usr.bin/patch/pch.c
Comment 9 Pedro F. Giffuni freebsd_committer 2014-12-08 15:16:00 UTC
Thank you for pointing out the issue.

I went with the OpenBSD patch as it makes it easier to keep in sync (we have been diverging in other ways though).
I didn't update the OpenBSD revision number on the affected files because we are skipping at least one change: not sure if we want to drop the SCCS support just yet (legacy ports exist).
Comment 10 commit-hook freebsd_committer 2014-12-16 21:14:33 UTC
A commit references this bug:

Author: pfg
Date: Tue Dec 16 21:13:56 UTC 2014
New revision: 275841
URL: https://svnweb.freebsd.org/changeset/base/275841

Log:
  MFC	r275553, r275612;

  patch(1): Bring fixes from OpenBSD

  Check fstat return value.  Use off_t for file size and offsets.
  Avoid iterating over end of string.

  Introduce strtolinenum to properly check line numbers while parsing:
  no signs, no spaces, just digits, 0 <= x <= LONG_MAX

  Properly validate line ranges supplied in diff file to prevent overflows.
  Also fixes an out of boundary memory access because the resulting values
  are used as array indices.

  PR:		195436
  Obtained from:	OpenBSD

Changes:
_U  stable/10/
  stable/10/usr.bin/patch/common.h
  stable/10/usr.bin/patch/pch.c
Comment 11 Pedro F. Giffuni freebsd_committer 2014-12-17 00:59:58 UTC
OpenBSD has new changes that will have to be analyzed but this issue can be closed now.