From 5777a9348a331e32e880cedebe70524d2cb28fc5 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 25 Sep 2026 12:31:36 -0400 Subject: [PATCH] fix: strip a systemd journal entry's own block padding, not its data A journal entry whose last field lacks the format's mandatory trailing newline let the PCAP-NG block's own 32-bit alignment padding (0-3 NUL octets) be read as part of that field's value, silently -- no exception, no warning. Measured: b'MESSAGE=hello' + 3 padding NULs returned MESSAGE == 'hello\x00\x00\x00'. - The existing padding-only-line guard (#699) only catches padding that lands on a line of its own, which requires the preceding field to have ended with a real newline. Without one, readline() runs straight through the value and into the padding behind it. - Reachable only on text fields: readline() returns a line without its own newline only at end of stream, so a binary field's own length- prefixed value read is never in play, and a binary field *name* landing on such a line fails its own length-prefix read regardless. - Chose the tolerant fix (strip, not refuse): block padding is always 0-3 octets, so at most the trailing 3 octets of a terminator-less line are stripped as padding, with a SchemaWarning; anything past that cannot be padding and is left as data. Gated on the *raw*, unstripped line ending in NUL -- not the whitespace-trimmed one -- since a real trailing newline, or any other raw last octet including ordinary ASCII whitespace, proves the true padding is zero. - Cross-review caught two defects in the first cut. First, gating on raw_line but stripping NULs off line (raw_line.strip()), so a whitespace-tailed, zero-padding entry lost real trailing NULs -- wrong on 3/12 simulated cases, fixed to 0/12. Second, once that gate requires raw_line to end in NUL, bytes.strip() cannot remove it, so the stripping loop always runs and a since-dead `if pad_octets:` guard around the warn+assign was never false; dropped. Closes #794 Test: tests/protocols/misc/test_pcapng_unit.py and tests/integration/test_pcapng_end_to_end.py both green (93/1762 and 6/1/25), cross-checked under python -m unittest; coverage on the changed module 100% (564 stmts, 84 branches, 0 missed); no new mypy or pylint findings against main. --- pcapkit/protocols/schema/misc/pcapng.py | 80 ++++++++++++++++ tests/protocols/misc/test_pcapng_unit.py | 114 +++++++++++++++++++++++ 2 files changed, 194 insertions(+) diff --git a/pcapkit/protocols/schema/misc/pcapng.py b/pcapkit/protocols/schema/misc/pcapng.py index ccceb1384..63bbe1edc 100644 --- a/pcapkit/protocols/schema/misc/pcapng.py +++ b/pcapkit/protocols/schema/misc/pcapng.py @@ -1833,6 +1833,45 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Self': own NUL padding or plain end of data, still starts the next entry -- even an empty one -- matching what splitting on it always did. + The guard above ends the entry on a line of *nothing but* NUL + padding, but only catches it when the real content ahead of the + padding ended with its own newline -- which put the padding on a + line by itself for :meth:`~io.BytesIO.readline` to return alone. + An entry whose last field is missing that trailing newline, as + the format requires, has no such separation: :meth:`readline` + runs straight through the field's own bytes and into the + padding behind them, returning both as one line, and + :meth:`bytes.strip` still will not take the NUL octets off since + they are not ASCII whitespace. The padding then went out as part + of the field's value with nothing to flag it, silently, however + small -- see `#794 + `__. + :meth:`~io.BytesIO.readline` returns a line without its own + trailing newline only at end of stream, so a terminator-less + line is necessarily the buffer's last one; a binary field's + *name* landing on such a line fails its own 8-octet + length-prefix read for the same reason, nothing being left + behind it to hold one, so the guard below firing on that shape + of line too is harmless. A binary field's *value*, once its + name and length prefix are known, is read by that length prefix + directly, never by scanning for a line ending, so real block + padding immediately behind one cannot land inside it this way: + the one-octet terminator check already in place is always + reached in that case, and either reads the real separator + newline and passes silently, or reads padding's first octet + instead, fails, and reports it. Block padding is always 0-3 + octets, whatever the last field turns out to be, so at most the + trailing three octets of a terminator-less line are stripped as + padding and warned about, and only when the line's own raw, + unstripped tail is itself NUL -- anything past three such + octets, or a line whose actual last octet is not NUL at all, + cannot be padding and is left as data. This cannot tell a + padding octet from a text value that itself legitimately ends + in one -- NUL is valid UTF-8 -- but that entry is already + malformed for lacking the newline the format mandates, and + returning the block's own padding as field data is the one + outcome that must not survive it. + """ self = cast('Self', super().post_process(packet)) @@ -1857,6 +1896,47 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Self': if not line.strip(b'\x00'): break + if not raw_line.endswith(b'\n') and raw_line.endswith(b'\x00'): + # the format requires this line's own trailing newline, + # and it is missing -- so nothing separates its data from + # the block's 32-bit alignment padding that immediately + # follows it, unlike the padding-only line above, which + # only exists as its own line *because* a real newline + # put it there. Padding is at most three octets (0-3, to + # round the entry up to the next multiple of four), so at + # most the last three octets of `line` can be it; strip + # only that many. Anything past that cannot be padding + # and is left as the data the sender actually sent. + # + # gated on the *raw*, unstripped line ending in NUL: pad + # octets are the only NULs to strip, and it is `raw_line`, + # not `line`, that names the entry's actual last octet. + # `line` is `raw_line.strip()`, which has already dropped + # ASCII whitespace from both ends -- so if the entry's + # real last octet were itself whitespace, `line`'s tail + # would no longer match `raw_line`'s, and NUL octets + # further in would be mistaken for the tail. Padding is + # always NUL, never whitespace, so a `raw_line` ending in + # anything else -- ordinary trailing whitespace included + # -- proves the true padding is zero, and nothing here + # may be stripped. See #794. + # the outer gate above already requires `raw_line` -- + # and therefore `line`, since `bytes.strip()` cannot + # remove a trailing NUL -- to end in one, so this loop + # always runs at least once and `pad_octets` is never 0 + # here; there is no zero-padding case left to guard + # against once the gate has passed. + stripped = line + pad_octets = 0 + while pad_octets < 3 and stripped.endswith(b'\x00'): + stripped = stripped[:-1] + pad_octets += 1 + warn(f'PCAP-NG: [systemd Journal Export] entry field {line!r} ' + f'has no terminating newline; treating its last ' + f'{pad_octets} NUL octet(s) as the block\'s own alignment ' + 'padding, not data', SchemaWarning, stacklevel=stacklevel()) + line = stripped + line_split = line.split(b'=', maxsplit=1) if len(line_split) == 2: key, value = line_split diff --git a/tests/protocols/misc/test_pcapng_unit.py b/tests/protocols/misc/test_pcapng_unit.py index d827edd5f..158fc0555 100644 --- a/tests/protocols/misc/test_pcapng_unit.py +++ b/tests/protocols/misc/test_pcapng_unit.py @@ -4509,6 +4509,120 @@ def test_a_journal_field_that_is_not_utf8_is_replaced_and_reported(self) -> None self.assertIn('is not UTF-8', str(schema_warnings[0].message)) self.assertEqual(entries[0][key], value) + def test_a_journal_entry_missing_its_final_newline_does_not_return_padding(self) -> None: + """#794: no trailing newline lets the block's own padding read as data. + + The block pads its content to a 32-bit boundary with NULs, and the + existing padding-only-line guard only fires when the real content + ahead of it ended with its own newline, putting the padding on a + line of its own. Without that newline, ``readline()`` runs straight + through the field's value and into the padding behind it, returning + both together -- and ``bytes.strip()`` still will not touch the NUL + octets, since they are not ASCII whitespace. Swept over every + padding count the alignment rule can produce (0-3 octets), keyed by + how many octets ``MESSAGE=...`` needs trimmed from ``entry`` to land + on each remainder mod four. + + """ + from pcapkit.utilities.warnings import SchemaWarning + + cases = { + 0: b'MESSAGE=abcd', # 12 octets, already a multiple of four + 1: b'MESSAGE=abc', # 11 octets, one NUL pads it to 12 + 2: b'MESSAGE=hi', # 10 octets, two NULs pad it to 12 + 3: b'MESSAGE=hello', # 13 octets, three NULs pad it to 16 + } + + for pad_octets, entry in cases.items(): + with self.subTest(pad_octets=pad_octets): + entries, caught = self._extract_journal(entry) + + self.assertEqual(len(entries), 1) + self.assertEqual(entries[0]['MESSAGE'], entry.split(b'=', 1)[1].decode()) + + schema_warnings = [item for item in caught + if item.category is SchemaWarning] + if pad_octets == 0: + # nothing was stripped, so there is nothing to warn about + self.assertEqual(schema_warnings, []) + else: + self.assertEqual(len(schema_warnings), 1) + self.assertIn('has no terminating newline', str(schema_warnings[0].message)) + self.assertIn(f'last {pad_octets} NUL octet(s)', str(schema_warnings[0].message)) + + def test_a_terminated_journal_value_ending_in_a_nul_octet_is_kept_whole(self) -> None: + """A legitimate value ending in NUL is not the same shape as padding. + + NUL is valid UTF-8, so a text field's value may end in one. Given its + own trailing newline, this line is one ``readline()`` call away from + the block's own padding -- exactly as it always was -- so #794's fix, + which only triggers when that newline is missing, must leave it + untouched. + + """ + entries, caught = self._extract_journal(b'MESSAGE=hi\x00\n') + + self.assertEqual(len(entries), 1) + self.assertEqual(entries[0]['MESSAGE'], 'hi\x00') + self.assertEqual([item for item in caught + if item.category.__name__ == 'SchemaWarning'], []) + + def test_a_terminator_less_line_whose_raw_tail_is_whitespace_has_zero_padding(self) -> None: + """Cross-review on #795: gating on ``raw_line``, not ``line``, matters. + + ``line`` is ``raw_line.strip()`` -- ASCII whitespace already gone from + both ends. The first cut of #794's fix checked ``raw_line`` for the + missing newline but then counted padding off ``line``, so whenever + the entry's real last octet was itself ASCII whitespace (not the + newline the format wants, but whitespace all the same), ``strip()`` + had already eaten it, exposing whatever NUL octets came *before* it + as if they were now the tail -- and those get read as block padding + that never existed. Padding is NUL, never whitespace, and + ``bytes.strip()`` does not touch NUL, so a ``raw_line`` ending in + whitespace is proof on its own that the true padding is zero: the + block's own last octet, unpadded, IS that whitespace. Swept over + every octet :meth:`bytes.strip` treats as ASCII whitespace -- space, + CR, tab, vtab and formfeed -- against a value that ends in NUL for a + genuine reason of its own -- a NUL is valid UTF-8 -- rather than by + accident. + + """ + from pcapkit.utilities.warnings import SchemaWarning + + cases = { + 'space': (b'MESSAGE=hi\x00 ', 'hi\x00'), + 'CR': (b'MESSAGE=hi\x00\r', 'hi\x00'), + 'tab': (b'MESSAGE=\x00\x00\x00\t', '\x00\x00\x00'), + 'vtab': (b'MESSAGE=hi\x00\x0b', 'hi\x00'), + 'formfeed': (b'MESSAGE=\x00\x00\x00\x0c', '\x00\x00\x00'), + } + + for tail, (entry, expected) in cases.items(): + with self.subTest(tail=tail): + entries, caught = self._extract_journal(entry) + + self.assertEqual(len(entries), 1) + self.assertEqual(entries[0]['MESSAGE'], expected) + self.assertEqual([item for item in caught + if item.category is SchemaWarning], []) + + def test_a_journal_binary_value_ending_in_nul_octets_is_unreachable_by_794(self) -> None: + """A binary field's value is read by its own length, never by line. + + Nothing here needs a trailing newline to bound it, so there is no + terminator-less line for #794's fix to act on -- confirmed by giving + the value itself the shape of alignment padding, then padding the + block on top of that, and getting both back distinctly. + + """ + entries, caught = self._extract_journal( + b'BIN\n' + struct.pack(' None: """#594's amplification band, on the ``captured_len`` vector.