fix(connlib): don't attempt to encrypt too large packets (#7263)

When encrypting packets, we need to reserve a buffer within which
boringtun will encrypt the IP packet. Unfortunately, `boringtun` panics
if that buffer is not big enough which essentially brings all of
`connlib` down.

Really, we should never see a packet that is too large and ideally, we
enforce this at compile-time by creating different variants of
`IpPacket` that are sized accordingly. That is a large refactoring so
until then, we simply discard them instead of panicking.

---------

Signed-off-by: Thomas Eizinger <thomas@eizinger.io>
Co-authored-by: Jamil <jamilbk@users.noreply.github.com>
This commit is contained in:
Thomas Eizinger
2024-11-05 15:17:21 +11:00
committed by GitHub
parent 269060669a
commit 271c480357
7 changed files with 37 additions and 5 deletions

View File

@@ -439,6 +439,19 @@ where
.get_established_mut(&connection)
.ok_or(Error::NotConnected)?;
let packet_len = packet.packet().len();
let max_len = if self.mode.is_client() {
ip_packet::PACKET_SIZE
} else {
ip_packet::PACKET_SIZE + ip_packet::NAT46_OVERHEAD
};
// TODO: This is a it of a hack, we should compile-time enforce this.
if packet_len > max_len {
tracing::warn!("Packet is too large; max={max_len}, actual={packet_len}");
return Ok(None);
}
// Encode the packet with an offset of 4 bytes, in case we need to wrap it in a channel-data message.
let Some(packet_len) = conn
.encapsulate(packet.packet(), &mut buffer.inner[4..], now)?

View File

@@ -37,7 +37,7 @@ pub const MAX_DATAGRAM_PAYLOAD: usize =
/// Wireguard has a 32-byte overhead (4b message type + 4b receiver idx + 8b packet counter + 16b AEAD tag)
const WG_OVERHEAD: usize = 32;
/// In order to do NAT46 without copying, we need 20 extra byte in the buffer (IPv6 packets are 20 byte bigger than IPv4).
const NAT46_OVERHEAD: usize = 20;
pub const NAT46_OVERHEAD: usize = 20;
/// TURN's data channels have a 4 byte overhead.
const DATA_CHANNEL_OVERHEAD: usize = 4;

View File

@@ -11,7 +11,11 @@ export default function Android() {
title="Android"
>
{/* When you cut a release, remove any solved issues from the "known issues" lists over in `client-apps`. This must not be done when the issue's PR merges. */}
<Unreleased></Unreleased>
<Unreleased>
<ChangeItem pull="7263">
Mitigates a crash in case the maximum packet size is not respected.
</ChangeItem>
</Unreleased>
<Entry version="1.3.6" date={new Date("2024-10-31")}>
<ChangeItem>Handles DNS queries over TCP correctly.</ChangeItem>
<ChangeItem pull="7151">

View File

@@ -11,7 +11,11 @@ export default function Apple() {
title="macOS / iOS"
>
{/* When you cut a release, remove any solved issues from the "known issues" lists over in `client-apps`. This must not be done when the issue's PR merges. */}
<Unreleased></Unreleased>
<Unreleased>
<ChangeItem pull="7263">
Mitigates a crash in case the maximum packet size is not respected.
</ChangeItem>
</Unreleased>
<Entry version="1.3.7" date={new Date("2024-10-31")}>
<ChangeItem>Handles DNS queries over TCP correctly.</ChangeItem>
<ChangeItem pull="7152">

View File

@@ -14,7 +14,9 @@ export default function GUI({ title }: { title: string }) {
return (
<Entries href={href} arches={arches} title={title}>
{/* When you cut a release, remove any solved issues from the "known issues" lists over in `client-apps`. This must not be done when the issue's PR merges. */}
<Unreleased></Unreleased>
<Unreleased>
<ChangeItem pull="7263">Mitigates a crash in case the maximum packet size is not respected.</ChangeItem>
</Unreleased>
<Entry version="1.3.10" date={new Date("2024-10-31")}>
<ChangeItem>Handles DNS queries over TCP correctly.</ChangeItem>
<ChangeItem enable={title === "Windows"} pull="7009">

View File

@@ -10,6 +10,11 @@ export default function Gateway() {
return (
<Entries href={href} arches={arches} title="Gateway">
<Unreleased>
<ChangeItem pull="7263">
Mitigates a crash in case the maximum packet size is not respected.
</ChangeItem>
</Unreleased>
<Entry version="1.4.0" date={new Date("2024-11-04")}>
<ChangeItem pull="6960">
Separates traffic restrictions between DNS Resources CIDR Resources,

View File

@@ -11,7 +11,11 @@ export default function Headless() {
return (
<Entries href={href} arches={arches} title="Linux headless">
{/* When you cut a release, remove any solved issues from the "known issues" lists over in `client-apps`. This must not be done when the issue's PR merges. */}
<Unreleased></Unreleased>
<Unreleased>
<ChangeItem pull="7263">
Mitigates a crash in case the maximum packet size is not respected.
</ChangeItem>
</Unreleased>
<Entry version="1.3.5" date={new Date("2024-10-31")}>
<ChangeItem>Handles DNS queries over TCP correctly.</ChangeItem>
<ChangeItem pull="7164">