Page MenuHomeFreeBSD
Differential D7619

bspatch: add sanity checks on sizes
ClosedPublic
Actions

Authored by emaste on Aug 23 2016, 11:26 PM.
Tags
None
Referenced Files
F152617390: D7619.id.diff
Thu, Apr 16, 1:06 AM
F152524874: D7619.id.diff
Wed, Apr 15, 12:02 PM
Unknown Object (File)
Tue, Apr 14, 10:27 PM
Unknown Object (File)
Mon, Apr 13, 11:43 PM
Unknown Object (File)
Sun, Apr 12, 9:06 AM
Unknown Object (File)
Sun, Apr 12, 8:43 AM
Unknown Object (File)
Thu, Apr 9, 12:45 PM
Unknown Object (File)
Wed, Apr 8, 1:25 AM
View All Files
Subscribers
cem
cperciva

Details

Reviewers
kib
cem
allanjude
delphij
glebius
cperciva
Commits
rS305486: bspatch: add sanity checks on sizes to avoid integer overflow
Summary

Note that on i386 this introduces a new 2GB limit, but this is not a concern in practice. Also avoid allocating an extra byte in the old and new file content buffers.

Based on the "NON-CRYPTANALYTIC ATTACKS AGAINST FREEBSD UPDATE COMPONENTS" gist.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

emaste updated this revision to Diff 19599.Aug 23 2016, 11:26 PM
emaste retitled this revision from to bspatch: add sanity checks on sizes.
emaste updated this object.
emaste edited the test plan for this revision. (Show Details)
emaste added reviewers: allanjude, delphij, glebius.
emaste added a subscriber: cperciva.
delphij added a reviewer: cperciva.Aug 23 2016, 11:29 PM
emaste added inline comments.Aug 23 2016, 11:30 PM
usr.bin/bsdiff/bspatch/bspatch.c
76–77 ↗(On Diff #19599)

On i386 ssize_t is 4 bytes, so would truncate from offtin / lseek.

123 ↗(On Diff #19599)

SSIZE_MAX introduces a 2G limit, but we already had an implicit 2G limit for proper operation based on the previous newsize type.

155 ↗(On Diff #19599)

Same 2G argument as above.

emaste updated this revision to Diff 19679.Aug 25 2016, 3:53 PM

rebase after rS304807

allanjude accepted this revision.Aug 25 2016, 4:57 PM
allanjude edited edge metadata.
This revision is now accepted and ready to land.Aug 25 2016, 4:57 PM
cem accepted this revision.Sep 6 2016, 5:05 AM
cem added a reviewer: cem.
cem added a subscriber: cem.

Changes seem fine to me. This code is really ugly, though.

usr.bin/bsdiff/bspatch/bspatch.c
175 ↗(On Diff #19679)

The gist checks for newsize <= 0. Is zero a valid newsize?

emaste added inline comments.Sep 6 2016, 2:36 PM
usr.bin/bsdiff/bspatch/bspatch.c
175 ↗(On Diff #19679)

Yes, a zero byte output file is legitimate.

kib accepted this revision.Sep 6 2016, 2:39 PM
kib added a reviewer: kib.
Closed by commit rS305486: bspatch: add sanity checks on sizes to avoid integer overflow (authored by emaste). · Explain WhySep 6 2016, 7:01 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
head/
usr.bin/
bsdiff/
bspatch/
17 lines

Diff 20101

View Options

head/usr.bin/bsdiff/bspatch/bspatch.c

Loading...