Page MenuHomeFreeBSD

Avoid buffer underwrite in icmp_error
ClosedPublic

Authored by emaste on Thu, Nov 8, 4:00 PM.

Details

Summary

icmp_error allocates either an mbuf w/ pkthdr or a cluster depending on the size of data to be quoted, but the calculation failed to account for additional padding that may be added by m_align.

Include the ip header in the size passed to m_align. On 64-bit archs this will have the net effect of moving everything 4 bytes later in the mbuf or cluster and will result in slightly pessimal alignment for the icmp data copy.

Also add an assertion that we will not move m_data before the beginning of the mbuf or cluster.

Report in https://www.reddit.com/r/BSD/comments/9v6xwg/remotely_triggerable_icmp_buffer_underwrite_in/

Reported by: a reddit user

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

emaste created this revision.Thu, Nov 8, 4:00 PM
emaste updated this revision to Diff 50173.Thu, Nov 8, 4:17 PM
emaste edited the summary of this revision. (Show Details)

Alternate approach from jtl@

jtl accepted this revision.Thu, Nov 8, 4:47 PM

If alignment is important, we can (theoretically) do something more intelligent to maintain it. However, I think this is both safe and good enough for an emergency fix.

This revision is now accepted and ready to land.Thu, Nov 8, 4:47 PM
bz added a comment.Thu, Nov 8, 4:52 PM

Can you please site the Apple CVE and possibly the original writeup instead of a (random) reddit thing?

emaste added a comment.Thu, Nov 8, 4:58 PM

Well, note that this issue is not the one in the Apple CVE although it was almost certainly found as a result of u/TheGrandSchlonging looking into our ip_icmp.c because of that bug. The original reporter is clearly happy just to see it fixed (per their write-up) and does not want to be mentioned in the commit message so I'm not entirely sure what to write.

Reported by: a reddit user who does not want to appear in the commit message

bz accepted this revision.Thu, Nov 8, 5:19 PM
cem added a subscriber: cem.Thu, Nov 8, 6:50 PM

I'm happy with this as the fix that goes out today, but doesn't this pattern seem way too fragile in general?

emaste added a comment.Thu, Nov 8, 7:10 PM
In D17909#382538, @cem wrote:

I'm happy with this as the fix that goes out today, but doesn't this pattern seem way too fragile in general?

Yes, it still feels fragile to me - in part why I added the KASSERT. We should revisit this logic specifically, and review/audit the rest of this file.

This revision was automatically updated to reflect the committed changes.