Avoid buffer underwrite in icmp_error

[emaste]

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

[emaste]

Alternate approach from jtl@

[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.

[bz]

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

[emaste]

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

[cem]

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

[emaste]

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.