Avoid buffer underwrite in icmp_error
ClosedPublic
Actions

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

Details

Reviewers

bz
jtl
rrs

Commits

rS340260: Avoid buffer underwrite in icmp_error

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 - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

emaste created this revision.Nov 8 2018, 4:00 PM

Alternate approach from jtl@

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.Nov 8 2018, 4:47 PM

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

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.Nov 8 2018, 5:19 PM

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

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.

Closed by commit rS340260: Avoid buffer underwrite in icmp_error (authored by emaste). · Explain WhyNov 8 2018, 8:17 PM

This revision was automatically updated to reflect the committed changes.

Herald added a subscriber: imp. · View Herald TranscriptNov 8 2018, 8:17 PM