Suricata

Reported by Communications Security Establishment (CSE):

## Summary

`DecodePPPOESession()` validates that the incoming buffer is at least `PPPOE_SESSION_HEADER_MIN_LEN` (7) bytes before overlaying an 8-byte `PPPOESessionHdr` struct and unconditionally reading the 16-bit `protocol` field at offsets 6–7. When an attacker delivers a PPPoE-Session segment of exactly 7 bytes with a non-zero `pppoe_length`, the `SCNtohs(pppoesh->protocol)` load at line 147 reads one byte past the validated buffer. The over-read is network-reachable through any L2 decoder that dispatches ethertype `0x8864` (Ethernet, SLL/SLL2, VLAN, CHDLC, 802.1ah) and is most easily triggered via Linux-cooked or tunneled captures that are not subject to Ethernet minimum-frame padding. Impact is limited to a single-byte heap over-read into packet-buffer slack — it trips ASAN/fuzzers but does not crash or leak data in stock builds.

## Affected Piece of Code

- **File:** `src/decode-pppoe.c`
- **Function / Location:** `DecodePPPOESession()` ~L129–163 (read at L147: `uint16_t ppp_protocol = SCNtohs(pppoesh->protocol);`)
- **Subsystem:** decode-l2 — L2 decoders and core decode dispatch

```c
src/decode-pppoe.c:
129        if (len < PPPOE_SESSION_HEADER_MIN_LEN) {
130            ENGINE_SET_INVALID_EVENT(p, PPPOE_PKT_TOO_SMALL);
131            return TM_ECODE_FAILED;
132        }
133    
134        PPPOESessionHdr *pppoesh = (PPPOESessionHdr *)pkt;
...
143        if (SCNtohs(pppoesh->pppoe_length) > 0) {
144            /* decode contained PPP packet */
145    
146            uint8_t pppoesh_len;
147            uint16_t ppp_protocol = SCNtohs(pppoesh->protocol);   /* <-- reads pkt[6] AND pkt[7]; pkt[7] OOB when len==7 */
148    
149            /* According to RFC1661-2, if the least significant bit of the most significant octet is
150             * set, we're dealing with a single-octet protocol field */
151            if (ppp_protocol & 0x0100) {
152                /* Single-octet variant */
153                ppp_protocol >>= 8;
154                pppoesh_len = PPPOE_SESSION_HEADER_MIN_LEN;
155            } else {
156                /* Double-octet variant; increase the length of the session header accordingly */
157                pppoesh_len = PPPOE_SESSION_HEADER_MIN_LEN + 1;
158    
159                if (len < pppoesh_len) {
160                    ENGINE_SET_INVALID_EVENT(p, PPPOE_PKT_TOO_SMALL);
161                    return TM_ECODE_FAILED;
162                }
163            }

src/decode-pppoe.h:
28    #define PPPOE_SESSION_HEADER_MIN_LEN     7
35    typedef struct PPPOESessionHdr_ {
36        uint8_t pppoe_version_type;
37        uint8_t pppoe_code;
38        uint16_t session_id;
39        uint16_t pppoe_length;
40        uint16_t protocol;
41    } PPPOESessionHdr;
```

## The Bug

### Root cause

`DecodePPPOESession()` guards only against `len < PPPOE_SESSION_HEADER_MIN_LEN`, where `PPPOE_SESSION_HEADER_MIN_LEN` is defined as **7** in `src/decode-pppoe.h:28`. Immediately afterwards it casts the raw `pkt` pointer to `PPPOESessionHdr *`. That struct, however, is **8** bytes long:

| Offset | Size | Field                |
|-------:|-----:|----------------------|
| 0      | 1    | `pppoe_version_type` |
| 1      | 1    | `pppoe_code`         |
| 2      | 2    | `session_id`         |
| 4      | 2    | `pppoe_length`       |
| 6      | 2    | `protocol`           |

The `protocol` member therefore spans byte indices **6 and 7**. With `len == 7`, only index 6 is inside the validated buffer; index 7 is one byte beyond it.

The constant was deliberately set to 7 to accommodate RFC 1661's single-octet PPP-protocol compression, but the code that distinguishes the single-octet from the double-octet form runs **after** the full 16-bit field has already been loaded. At line 147 the function unconditionally executes `SCNtohs(pppoesh->protocol)`, which compiles to a 2-byte read of `pkt[6..7]`. Only afterwards does it test bit `0x0100` to decide whether the second octet was actually needed, and only on the double-octet branch does it perform the secondary `len < pppoesh_len` check at L159. By that point the out-of-bounds byte has already been read.

The attacker controls every byte needed to steer execution into the vulnerable block: bytes 4–5 (`pppoe_length`) are fully in-bounds, so setting them to any non-zero value satisfies the `SCNtohs(pppoesh->pppoe_length) > 0` test at L143 and reaches L147.

### Call chain / reachability

Entry is the ordinary per-packet decode pipeline; no rules, scripts or non-default configuration are required because PPPoE decoding is dispatched unconditionally from `DecodeNetworkLayer()`.

Concrete chain using the pcap-file run mode with a Linux-cooked capture (DLT 113 / `LINKTYPE_LINUX_SLL`), which carries no 46-byte Ethernet minimum-payload padding and therefore allows a 7-byte PPPoE segment to survive intact:

1. `DecodePcapFile()` — `src/source-pcap-file.c:456-470` — invokes the per-DLT `decoder()` selected by `ValidateLinkType()`; for DLT 113 this is `DecodeSll()`.
2. `DecodeSll()` — `src/decode-sll.c:41-63` — parses the 16-byte SLL header, extracts `sll_protocol`, and calls `DecodeNetworkLayer()` with the residual 7-byte payload.
3. `DecodeNetworkLayer()` — `src/decode.h:1495-1510` — for `proto == ETHERNET_TYPE_PPPOE_SESS (0x8864)` calls `DecodePPPOESession(tv, dtv, p, data, len)`.
4. `DecodePPPOESession()` — `src/decode-pppoe.c:122` — `len == 7` passes the L129 guard; `pppoe_length != 0` enters the inner block; L147 performs the 2-byte read at `pkt+6`, touching `pkt[7]` out-of-bounds.

An equivalent live-traffic chain exists: `DecodePcap()` (`src/source-pcap.c:622`) → `DecodeLinkLayer()` (`src/decode.h:1458`) → `DecodeSll()` → `DecodeNetworkLayer()` → `DecodePPPOESession()`. The same sink is also reachable behind `DecodeVLAN()`, `DecodeCHDLC()`, `DecodeIEEE8021ah()` and `DecodeSll2()`, all of which funnel inner ethertypes through `DecodeNetworkLayer()`. Crafted pcaps, Linux cooked-mode captures, and tunneled inner frames are the realistic delivery vectors because they are not forced up to the 46-byte Ethernet minimum.

### Required attacker-controlled field values

- SLL `sll_protocol` = `0x8864` (PPPoE Session)
- Residual length after the SLL header = **7**
- PPPoE bytes: `ver/type = 0x11`, `code = 0x00`, `session_id` = any, `pppoe_length` ≠ 0 (e.g. `0x0002`), `protocol[0]` = any
- No 8th PPPoE byte present

**Vulnerability class:** network-reachable out-of-bounds read.

## Reproduction Results

The trigger below is **analytically derived** from source review of the call chain and struct layout; it has not been executed against a live ASAN build in this audit environment. All offsets, constants and branch conditions have been verified against the source, and the construction is precise enough to feed directly to `suricata -r` or to the existing `fuzz_decodepcapfile` libFuzzer target.

1. Build Suricata with AddressSanitizer (or use the existing `fuzz_decodepcapfile` libFuzzer target) so that the 1-byte over-read is observable. In a stock build the read lands in packet-buffer slack and is silent.
2. Create a pcap whose global header sets `network = 113` (`LINKTYPE_LINUX_SLL`) and which contains a single record with `incl_len = orig_len = 23`.
3. Record payload (23 bytes, hex):

   **SLL header (16 B):** `00 00  00 01  00 06  00 00 00 00 00 00 00 00  88 64`
   — `sll_pkttype=0`, `ARPHRD=1`, `halen=6`, `addr=zeros`, `sll_protocol=0x8864` (PPPoE-Session)

   **PPPoE Session (7 B):** `11  00  00 01  00 02  00`
   — `version/type=0x11`, `code=0x00`, `session_id=0x0001`, `pppoe_length=0x0002` (non-zero), first protocol octet `=0x00`. No 8th PPPoE byte is present.

4. Full pcap file as hex (little-endian global + record headers, 47 bytes total):

   ```
   d4 c3 b2 a1 02 00 04 00 00 00 00 00 00 00 00 00 ff ff 00 00 71 00 00 00
   00 00 00 00 00 00 00 00 17 00 00 00 17 00 00 00
   00 00 00 01 00 06 00 00 00 00 00 00 00 00 88 64 11 00 00 01 00 02 00
   ```

5. Run `suricata -r trigger.pcap` (or feed the 23-byte record body to `fuzz_decodepcapfile`). Under ASAN the expected report is `heap-buffer-overflow READ of size 2` at `decode-pppoe.c:147` (`SCNtohs(pppoesh->protocol)`), 1 byte past a 7-byte region.

No special `suricata.yaml` options or rules are required; PPPoE decoding is unconditional in `DecodeNetworkLayer()`.

## Severity

**LOW** — This is a 1-byte out-of-bounds heap read of the byte immediately following the packet data. In production deployments the packet buffer is over-allocated (governed by `default_packet_size` or the AF_PACKET ring frame size), so the read lands in initialized slack memory and neither crashes the process nor leaks attacker-observable data: the over-read value only influences which internal decode-event is raised and is never reflected back on the wire or into logs in a way the attacker can retrieve. Practical impact is therefore limited to AddressSanitizer / fuzzer trips and a formal bounds-correctness violation. There is no denial of service, no information leak, and no path to remote code execution.

## Suggested Fix

Determine the single- vs double-octet protocol form by reading only the **first** (in-bounds) protocol octet, and load the second octet only after the length has been re-validated. Patch for `src/decode-pppoe.c`, replacing lines 146–163:

```diff
-        uint8_t pppoesh_len;
-        uint16_t ppp_protocol = SCNtohs(pppoesh->protocol);
-
-        /* According to RFC1661-2, if the least significant bit of the most significant octet is
-         * set, we're dealing with a single-octet protocol field */
-        if (ppp_protocol & 0x0100) {
-            /* Single-octet variant */
-            ppp_protocol >>= 8;
-            pppoesh_len = PPPOE_SESSION_HEADER_MIN_LEN;
-        } else {
-            /* Double-octet variant; increase the length of the session header accordingly */
-            pppoesh_len = PPPOE_SESSION_HEADER_MIN_LEN + 1;
-
-            if (len < pppoesh_len) {
-                ENGINE_SET_INVALID_EVENT(p, PPPOE_PKT_TOO_SMALL);
-                return TM_ECODE_FAILED;
-            }
-        }
+        uint8_t pppoesh_len;
+        uint16_t ppp_protocol;
+        /* RFC1661: if LSB of the first protocol octet is set, it is a single-octet field.
+         * Inspect only pkt[6] (guaranteed in-bounds by the MIN_LEN check above). */
+        const uint8_t proto_hi = pkt[PPPOE_SESSION_HEADER_MIN_LEN - 1];
+        if (proto_hi & 0x01) {
+            /* Single-octet variant */
+            ppp_protocol = proto_hi;
+            pppoesh_len = PPPOE_SESSION_HEADER_MIN_LEN;
+        } else {
+            /* Double-octet variant; need one more byte */
+            pppoesh_len = PPPOE_SESSION_HEADER_MIN_LEN + 1;
+            if (len < pppoesh_len) {
+                ENGINE_SET_INVALID_EVENT(p, PPPOE_PKT_TOO_SMALL);
+                return TM_ECODE_FAILED;
+            }
+            ppp_protocol = (uint16_t)proto_hi << 8 | pkt[PPPOE_SESSION_HEADER_MIN_LEN];
+        }
```

**Alternative (simpler but slightly stricter than current behaviour):** change the initial guard at line 129 to `if (len < sizeof(PPPOESessionHdr))` (i.e. 8). This removes the over-read at the cost of flagging a legitimate 7-byte single-octet-protocol frame with no payload as `PPPOE_PKT_TOO_SMALL`.