Most bugs are a missing safety check. This one is the safety check. ModemManager’s cell broadcast parser has a g_assert_not_reached() in it, the kind of line you add so the program dies loudly if the impossible ever happens. So who gets to decide when the impossible happens? The cell tower does. Send one cell broadcast message carrying a value the mobile standard says is fine, the assert fires, and the daemon goes down. ModemManager runs as root, which makes that safe-looking line a remote kill switch.
This is the story of finding it, the moment I stopped trusting my reading of the code and ran every possible input through the real parser instead, and the ending I did not expect, where the maintainer wrote the fix and then asked me whether it was the right one.
Why I was reading a cell broadcast parser
ModemManager is the thing on Linux that talks to your cellular modem. If your laptop has a WWAN card, or you have ever used a USB LTE stick, or you are staring at some embedded router that has a modem bolted to it, ModemManager is probably the daemon driving it. It runs as root, it lives on the system D-Bus, and a lot of what it does is parse bytes that came off the air.
I like input parsers that run as root and take orders from something the user does not control, and cellular is full of them. The modem hands ModemManager a steady diet of unsolicited messages: signal reports, SMS, and cell broadcast. Cell broadcast is the interesting one. It is the one-to-many channel that carries emergency alerts, the loud “take shelter” messages, and operator area notices. Your phone does not ask for these. The tower pushes them, every device in the cell decodes them, and on Linux the decoding happens inside ModemManager without any request from the host. That interface turns itself on when the modem says it supports cell broadcast, so there is nothing a user has to enable and nothing they can misconfigure to be exposed. That is a good place to look.
So I opened src/mm-cbm-part.c and started reading the function that turns a raw cell broadcast PDU into a message, mm_cbm_part_new_from_binary_pdu().
The line that stopped me
I read the function top to bottom once, the way you do, not really looking for anything, just building a map. It reads a serial number, a channel, a data coding scheme byte, works out the text encoding from that byte, then reads the message. Standard parser shape. Then near the bottom there was this:
switch (cbm_part->encoding) {
case MM_SMS_ENCODING_GSM7:
len = ((pdu_len - offset) * 8) / 7;
break;
case MM_SMS_ENCODING_UCS2:
len = pdu_len - offset;
if (len % 2)
len--;
break;
case MM_SMS_ENCODING_8BIT:
case MM_SMS_ENCODING_UNKNOWN:
default:
g_assert_not_reached ();
}An assert in a parser that eats network data is a smell. g_assert_not_reached() means abort, and this one is guarding a switch on encoding. The question was obvious: can I make encoding be something other than GSM7 or UCS2 by the time we get here? If I can, this aborts. And 8BIT and UNKNOWN are sitting right there in the default case, so the author clearly thought those values were impossible at this point. I wanted to know if they were right.
Where the encoding actually comes from
I scrolled up to how encoding gets set. The first thing that mattered was the allocation:
cbm_part = mm_cbm_part_new (); /* g_slice_new0 under the hood */g_slice_new0 zeroes everything. And in the encoding enum, zero is not nothing:
MM_SMS_ENCODING_UNKNOWN = 0x0,So before the parser touches the coding scheme, encoding is already UNKNOWN. The whole question becomes: does every path set it to something real before the switch? If any path forgets, the field keeps this UNKNOWN and the switch aborts.
Then I found the branch that forgets. The general coding groups and the UDH group share this:
guint16 charset = pdu[offset] & CBS_DATA_CODING_GENERAL_CHARSET_MASK;
/* We don't handle compression or 8 bit data */
if (charset == CBS_DATA_CODING_GENERAL_GSM7)
cbm_part->encoding = MM_SMS_ENCODING_GSM7;
else if (charset == CBS_DATA_CODING_GENERAL_UCS2)
cbm_part->encoding = MM_SMS_ENCODING_UCS2;The charset is two bits, so it has four values: GSM7, 8-bit, UCS2, and reserved. This sets the encoding for two of them and says nothing about the other two. The comment admits it, “We don’t handle compression or 8 bit data,” but not handling it means the if/else if just ends, and encoding is still UNKNOWN. There is no else. Nothing catches the 8-bit or reserved case.
That is the whole bug. A DCS like 0x44 is the general group with 8-bit charset bits. It is a legal value in 3GPP TS 23.038, not a malformed byte. It flows through this branch, sets nothing, and arrives at the switch as UNKNOWN, and the switch aborts. I had read my way to it, and it looked airtight on paper.
But “airtight on paper” is how I have talked myself into bugs that were not there. I did not want to report a crash I had only reasoned about.
I stopped trusting the read
The thing I have learned to do at this point is stop reading and start measuring. Reasoning about C control flow is how you convince yourself of a bug that is not there, or miss the guard three functions away that would have caught it. I did not want a theory. I wanted the real parser to abort in front of me.
So I did the dumbest complete thing I could think of. There are only 256 possible values for the data coding scheme byte. I would send all of them to the actual mm_cbm_part_new_from_binary_pdu() and see which ones crashed. The catch is that a crash is an abort(), and abort() takes the whole test process with it, so I could not just loop. Each value had to run in its own fork(). The parent forks, the child runs the parser on one DCS byte, and the parent reads how the child died: normal exit, clean GError, or a signal.
I built a small harness against the same in-tree helper library the unit tests use, and let it walk 0x00 to 0xFF. The result was not ambiguous:
parsed OK : 73
clean GError : 159
abort (SIGABRT) : 24Twenty-four values took the child down with SIGABRT. Not a theory anymore. And the 24 were exactly the ones the code predicted:
0x44 0x45 0x46 0x47 0x4C 0x4D 0x4E 0x4F
0x54 0x55 0x56 0x57 0x5C 0x5D 0x5E 0x5F
0x94 0x95 0x96 0x97 0x9C 0x9D 0x9E 0x9FThe three coding groups that share that branch, crossed with the two charset values nobody handled. Running one value on its own, DCS 0x44, printed the crash directly:
ERROR:../src/mm-cbm-part.c:286:mm_cbm_part_new_from_binary_pdu: code should not be reachedExit 134. That is 128 plus signal 6.
The “but is this real in production” worry
Every time I find a crash that hinges on an assert, the same doubt shows up: asserts get compiled out of release builds, so maybe this only bites developers. I have talked myself out of real bugs this way, so I chased it down properly this time.
GLib’s g_assert_not_reached() is only removed when G_DISABLE_ASSERT is defined. That is a different switch from NDEBUG, and it is not what meson -Db_ndebug=true sets. I grepped the ModemManager tree and it never defines G_DISABLE_ASSERT, and distributions do not add it either. So the assert is there in the builds people actually run. The crash is real on a normal Fedora or Debian ModemManager, not just on my debug tree.
Who can actually send this
So how bad is it, really? It is a denial of service and nothing more. No shell, no memory disclosure, no read or write primitive. The process aborts. That is the whole effect.
But look at which process and how it is reached. ModemManager runs as root and systemd restarts it when it dies, so a message that crashes it on startup crashes it again on the next start, and you get a loop. And the input arrives over the air. Cell broadcast is sent by the network to every device in an area. The realistic attacker is someone running a rogue base station within radio range, the same IMSI-catcher hardware that already exists, or a compromised carrier. The upstream commit message frames it the same way: a compromised base station or modem firmware could craft or pass through these messages. So the picture is a device that drops its modem management the moment it camps on a hostile cell, for as long as that cell keeps broadcasting the message. For a phone or a router or a piece of field equipment that depends on the modem, that is the modem going away and not coming back.
Reporting it, and the ending I did not plan for
I wrote it up with the proof of concept, the full sweep, both ingestion paths that reach the parser, and a suggested direction for the fix: give the missing charsets a fallback and turn the assert into a normal error return so an odd DCS drops the message instead of the daemon.
Dan Williams picked it up and wrote the fix. It is smaller and cleaner than what I sketched. Instead of adding cases, he made the existing error check unconditional:
} else if (charset == CBS_DATA_CODING_GENERAL_UCS2)
cbm_part->encoding = MM_SMS_ENCODING_UCS2;
- } else {
+ }
+
+ if (cbm_part->encoding == MM_SMS_ENCODING_UNKNOWN) {
g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
"Unhandled cbm message encoding: 0x%02x", pdu[offset]);
return NULL;
}The old else only fired when the coding group itself was unrecognised. Making it a post-check that runs whenever encoding is still UNKNOWN catches both that case and the 8-bit and reserved charsets, because all of them leave the field untouched. He also added two regression tests, one for the 8-bit charset and one for the reserved charset, each asserting that the parser returns an error rather than aborting.
Then he sent the patch back with a question: is this actually the correct fix?
It is a fair thing to ask, and the honest way to answer it was not to read the patch and nod. It was to run the same experiment that proved the bug, against the patched tree this time. If the fix is right, the 24 crashing values should stop crashing, the 73 that parsed fine before should still parse fine, and nothing should abort. So I built his patch and re-ran the full 256-value sweep:
parsed OK : 73 (unchanged)
clean GError : 183 (the old 159, plus the 24 that used to abort)
abort (SIGABRT) : 0Every value that used to kill the process now returns a clean error. Not one legitimate message changed behaviour. And his two new tests, plus the seven that were already there, all pass. That was the answer: yes. The same measurement that found the bug now reads zero.
What I took from it
Two things, both duller than the bug.
The first is about zero-initialised defaults. g_slice_new0 gave encoding the value MM_SMS_ENCODING_UNKNOWN, and UNKNOWN is a real enum value with real meaning, not an obviously-invalid sentinel. So a branch that never touched the field left behind something that looked like a decision. The parser could not tell “I decided the encoding is unknown” apart from “I never set the encoding,” and it did not find out until an assert several lines later forced the question the hard way. A default that is a meaningful value is only safe if every path is ready to see it.
The second is that g_assert_not_reached() on a boundary that parses network input is a crash you wrote for yourself. It reads like caution. On a root daemon eating bytes from the air, it is an abort() with a comment. The fix is not clever, it just turns “this must be impossible” into “if this happens, drop the message,” which is what a parser at that boundary should have done in the first place.
Now go look at a parser you maintain. How many default: cases quietly end in an abort? How many g_assert_not_reached() calls are one forgotten branch away from a value some attacker gets to pick? Those are the lines I grep for first now.
And a small habit that keeps paying off: when a bug hinges on control flow, stop reasoning and run every input through the real thing. All 256 bytes, one fork each, was a twenty-line harness, and it turned a plausible reading into a fact I could hand over and, later, into the check that told the maintainer his patch was right.