Skip to content

portal_osi: read_procargs flattens the argv NUL separators, destroying boundaries the host cannot reconstruct #104

Description

@lacraig2

handle_op_read_procargs rewrites every NUL separator in the argv block to a space before handing it to the host. The host then tries to rebuild the argument boundaries with str.split() and gets it wrong in five distinct ways. The conversion is pure information loss with nothing bought: the true byte count is already on the wire.

The code

src/portal/portal_osi.c:719-731:

/* In Linux, arguments in the memory are already null-terminated.
 * For our use, we need to convert these null terminators to spaces,
 * except for the final one. This matches get_mm_cmdline behavior.
 */
for (i = 0; i < len - 1; i++) {
    if (buf[i] == '\0')
        buf[i] = ' ';
}

/* Ensure the buffer is null-terminated */
buf[len] = '\0';

mem_region->header.size = (len);

header.size is set to len on the very next line, so the host already knows exactly how many bytes it got and does not need an in-band terminator to find the end. The NULs are destroyed for no purpose.

The get_mm_cmdline precedent does not carry over. fs/proc/base.c flattens because /proc/<pid>/cmdline is a byte stream with no out-of-band length — a consumer there genuinely has nothing else to go on. Our transport has a length header.

What it costs

The host consumer is plugins.osi.get_args (penguin pyplugins/apis/osi.py:119-126), which does decoded.split() and filters on str.isprintable(). Given a flattened block it cannot do better, and the failures are not exotic:

real argv what the host returns why
["sh", "-c", "echo hi > /tmp/x"] ["sh", "-c", "echo", "hi", ">", "/tmp/x"] a space inside an argument becomes a boundary
["prog", "", "-x"] ["prog", "-x"] an empty argument leaves adjacent separators, which split() eats
["prog", "a\tb"] ["prog", "a", "b"] bare split() splits on any whitespace
["prog", "--msg=one\ntwo"] ["prog", "--msg=one", "two"] so does a newline
["prog", b"\xa0x"] ["prog", "x"] latin-1 0xA0 decodes to U+00A0, which str.split() treats as whitespace — so a UTF-8 argument is cut at its continuation bytes
["prog", "a\x01b"] ["prog"] .isprintable() drops the whole argument, shifting every later index

The most common user-visible symptom is a wrong process name, since get_proc_name returns get_args(pid)[0]: a binary at /opt/My App/httpd is reported as /opt/My.

Fix

Delete the loop. Leave the argv block exactly as the kernel laid it out and let the host split on b"\0".

Beyond correctness this makes truncation detectable. len = min_t(size_t, arg_end - arg_start, CHUNK_SIZE - 1) can cut mid-argument; today that is indistinguishable from a short final argument, whereas with separators intact a truncated payload is exactly the one whose final element has no terminating NUL.

Transition — no flag day required

The host half is rehosting/penguin#941. Neither repo has to wait for the other, because the two formats are distinguishable with certainty:

  • An old-format payload can never contain an interior NUL — every one was overwritten.
  • A new-format payload contains an interior NUL whenever there is more than one argument.
  • For a single-argument process the two formats are byte-identical, and both decoders return the same answer.

So a host that checks for an interior NUL and picks its decoder accordingly is correct against both drivers, and the version pin can move whenever it likes. That check belongs on the penguin side; nothing is needed here beyond dropping the loop.

Blast radius

Eleven osi.get_args/get_proc_name call sites in penguin. Eight take args[0] as a process name or log the list. Three compare the whole list exactly — pyplugins/interventions/kmods.py:206, pyplugins/testing/ioctl_interaction_test.py:44, pyplugins/testing/portal_test.py:40-47 — and not one expected argument contains whitespace, so all three are unaffected. pyplugins/analysis/processes.py:211 takes argv from the Exec event, not from get_args, so the process tree is not on this path at all.

Both the current lossy behaviour and the exact behaviour a fix produces are already asserted in penguin's tests/unit/test_osi_procinfo_holes.py, so the fix flips a documented assertion rather than discovering a surprise.

handle_op_read_procenv (portal_osi.c:744) already copies the env block verbatim with no conversion, which is the behaviour proposed here.

Depends on #102 (which touches portal_osi.c).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions