I'm CCing the groff list because the message to which I'm replying was erroneously sent to the `bug-groff` list, where it is off-charter.
I would have rejected the message. "About bug-groff GNU roff is a typesetting document formatter. This list is a reflector for ticket traffic originating in groff's Savannah project. Please direct discussion to the groff list." https://lists.gnu.org/mailman/listinfo/bug-groff I further suspect that the entity to which I'm replying is an LLM. Apart from the format of the report(s), the characterization of the problems and their solutions betrays little evidence of the judgment of a seasoned software engineer. Nevertheless I'm briefing these issues for the benefit of my fellow groff developers, and to inform our posture regarding "security issues". This is a long message, so here is the "tl;dr". A. I am annoyed by this sort of bug submission, not because it reports issues (only some of which were valid), but because it is larded with unnecessary analysis, "minimal reproducers" that were anything but minimal, and delusional recommendations for fixes. B. I think addressing the valid reported issues has made groff better. C. Future reports of abnormal exits due to assertion failures or null pointer dereferences are better reduced to a minimal reproducer _that has actually been tested for minimality_, and should omit attempts at analysis or rectification with a patch, unless the submitter is a human being who communicates understanding of the code base. "Understanding" means grasping not merely the details of program logic, but the purpose of the function calls and data structures manipulated, the syntax and semantics of the language or input processed by the program, tool, or script at issue, and the purpose of the feature implemented in the affected logic. As is the case with human engineers, LLMs should not pretend to expertise that they do not possess. At 2026-10-16T21:36:33+0800, DDT Cr wrote: > Hello, > > I found 5 defect(s) in groff. Each has a minimal PoC, The use of the word "minimal" here is either deceptive or a bad joke. Cursory inspection of the reproducers reveals a consistent pattern. Some kind of model *roff input, often (always?) drawn from groff's own source tree, gets fuzzed by replacing bytes, apparently at random, with control characters. The term "minimal" here appears to refer to the _fuzzer_'s procedure; that is, a model input was perturbed until it caused groff to exit abnormally. But this is not a useful application of the term "minimal" for bug reporting, as the fuzzer _is not the system under test_. This mistaken application of the term "minimal" suggests to me that these bug reports were prepared by an...entity...lacking a grasp of basic principles of software testing and engineering. And I do mean engineering in general. When the discipline of mechanical engineering was born in the crucible of unexplained, catastrophic, and deadly steam boiler explosions in the 19th century, the practice of literature in the field quickly converged on describing the nature of defects in the _boilers_. The degree of complexity of the test apparatus or procedure was not directly relevant. If you can reliably get a boiler cast from cold-rolled iron milled at Maidenhead in Berkshire to blow up after cycling it from 20 to 120 degrees Celsius five times, the boiler manufacturers--and the human beings working or otherwise situated nearby them--do not care if your test apparatus involved a hand-stoked wood fire or a Rube Goldberg machine costing a hundred thousand pounds. What a lovely profession we have: human engineers won't read anything, and LLMs read everything and understand nothing. > a reproduction command ...also usually not as "minimal" as it could be... > and a proposed patch. ...usually one that is not correctly scoped, or, as in the case where the "patch" was indiscriminate deletion of assertions, flatly incorrect. > 1. NULL pointer dereference in the file-iterator path (strdup result > used before the NULL check) > CWE-476 — Low (DoS) | CVSS:3.1/AV:L/AC:L/PR:N/UI:R/S:U/C:N/I:N/A:H (5.5 > Medium) > troff builds a file_iterator whose name can be NULL; strdup(fn) is used > before the NULL check on the following line, so a crafted document > dereferences address 0. This is a valid report. I have filed it as Savannah #68783. The reporter's "minimal PoC" is 2.9 KiB. Mine is 8 bytes. The proposed patch is incorrectly scoped, mucking around in the constructor of a `file_iterator` object, when the correct place to handle inappropriately empty *roff request arguments is in the request _handlers_. While GNU troff uses a hand-written recursive descent parser and commingles lexical and semantic analysis ("parsing")[1], it does attempt to handle errors evident in lexical analysis at that time, rather than deferring their resolution to semantic parsing, as the proposed patch does. The patch did prompt me, however, to add error handling to the `file_iterator` constructor. diff --git a/src/roff/troff/input.cpp b/src/roff/troff/input.cpp index af673591e..0ed74edc9 100644 --- a/src/roff/troff/input.cpp +++ b/src/roff/troff/input.cpp @@ -469,6 +469,10 @@ file_iterator::file_iterator(FILE *f, const char *fn, bool popened) : fp(f), lineno(1), was_popened(popened), seen_newline(false), seen_escape(false) { + assert(f != 0 /* nullptr */); + assert(fn != 0 /* nullptr */); + if ((0 /* nullptr */ == f) || (0 /* nullptr */ == fn)) // NDEBUG + return; filename = strdup(const_cast<char *>(fn)); if ((font::use_charnames_in_special) && (fn != 0 /* nullptr */)) { if (!the_output) To save Ingo the trouble, I note that he is scandalized at the notion that anyone would ever compile C (or C++, I suppose) with `NDEBUG` defined. But doing so is supported by the standard C library and we have no written policy that we refuse to support such a build configuration, nor any (known) logic bombs that go off if one tries. My approach has been to support `NDEBUG` on a best-effort basis. I happened across this issue while, over time, greatly increasing the number of `assert()`ion sites in the groff code base. Indeed, some of the issues reported here arose only as of groff 1.24.0, consequent of my festooning of the code base with assert()ions. > 2. Assertion failure (abort) in lookup_request on a missing identifier > CWE-617 — Low (DoS) | CVSS:3.1/AV:L/AC:L/PR:N/UI:R/S:U/C:N/I:N/A:H (5.5 > Medium) > A crafted man page drives request lookup into a state where the > identifier is null, hitting an assertion instead of a diagnostic. This is not valid report against groff Git. It is a duplicate of Savannah #68558, which I found and fixed back in July. commit ba660bda6d4f19a7c1432f8a510ff4f0664328d7 Author: G. Branden Robinson <[email protected]> Date: Sun Jul 26 00:11:54 2026 -0500 [troff]: Fix Savannah #68558. * src/roff/troff/input.cpp (do_request): If given a first argument that is an invalid request or macro name, we cannot sensibly proceed: unwind the "compatibility stack" manipulation we just did and return early, maintaining the integrity of aforesaid stack. Fixes <https://savannah.gnu.org/bugs/?68558>. Problem appears to date back at least to groff 1.22.3 (2014), and possibly much earlier. ChangeLog | 14 ++++++++++++++ src/roff/troff/input.cpp | 9 ++++++++- 2 files changed, 22 insertions(+), 1 deletion(-) As the ticket history notes, this problem likely originates in groff 1.20.0 (January 2009). The reporter's "minimal PoC" is 5.4 KiB. Mine is 6 bytes. Again, the proposed patch is incorrectly scoped; instead of correctly unwinding the "compatibility mode stack", affecting the language accepted by the parser, it mucks around in `lookup_request()`, leaving the state of compatibility stack invalid. The patch was proposed without evident comprehension of how GNU troff's parser works. There's no shame in that ignorance; few people have any command of it at all. But I lament the use of LLMs in aiding brogrammers (and managers, executives, and investors) to pretend to greater competence and knowledge than they actually possess. I suppose that feature alone suffices to explain the "AI bubble". That said, while the proposed patch wipes out an assert()ion that I think should remain, it does suggest that a fallback operation in the `NDEBUG` case is warranted, and that would be to have `lookup_request()` return a null pointer, so I've added that. It might be worthwhile at this point to remember what assert(3) is good for: it is to inform the programmer when their assumptions about the internal interfaces of a program are invalid. Characterizing assertion failures as CVE-worthy security holes is, in my opinion, a corrosive practice. What happens when we don't check the conditions that assertions? Theory and practice agree: the program barrels forward in an invalid state, producing unreliable results or, worse, violates the confidentiality or integrity of host system data. That, combined with the project's nature as a suite of command-line applications and utilities (contrast: network services) is why the groff project generally does not regard assertion failures as security problems. The "availability" leg of the "CIA triad" is not implicated. If the program exits from a SIGABRT signal due to invalid input (which is almost always the case in groff), no "availability" is lost and a remedy is immediately at hand--run the program again with _valid_ input. Moreover, let's keep in mind that the following is _perfectly valid_ *roff input. .ab If the problem you have discovered is operationally indistinguishable from interpretation of an "abort" request, then I submit that you have not discovered a DoS attack. I further note for the reader's edification that I _do_ typically classify such anomalies as problems with "Important" severity in the Savannah ticket tracker, on par with regression of working features from previous releases. What they are not, are "Security" issues. > 3. Global buffer overflow (write) from a capacity mismatch in describe_node > CWE-787 — Medium (out-of-bounds write) | > CVSS:3.1/AV:L/AC:L/PR:N/UI:R/S:U/C:H/I:H/A:H (7.8 High) > A 55-byte static buffer is passed to describe_node() with the caller > buffer size (62), so memset() and snprintf() write past its end. This is not valid report against groff Git. It is a duplicate of Savannah #68552, which Bruno Haible found and I fixed back in July. commit 0ebb2ba4af49199a99e391ff1f359219ede058d6 Author: G. Branden Robinson <[email protected]> Date: Fri Jul 24 16:21:00 2026 -0500 [troff]: Fix Savannah #68552. * src/roff/troff/input.cpp (token::description): Simplify population of buffer describing a node token and avoid potentially overwriting a heap-allocated buffer if that node's description is lengthy (as "non-interpreted character nodes", input in *roff as `\a` and `\t`, are). Discard stale code that uselessly performed a dead store of a dummy node description; `describe_node()` zeroes out its destination buffer so that became pointless a while back. Then, simply measure the buffer as populated by `describe_node()` with strlen(3) and append to it carefully with strncat(3), respecting `bufsz`. Fixes <https://savannah.gnu.org/bugs/?68552>. Thanks to Bruno Haible for the investigation and advice. I believe I introduced this problem during the groff 1.24.0 development cycle in the course of my "node tree dumping death march", but have not tracked it down to a specific commit. See generally <https://lists.gnu.org/archive/html/groff-commit/2025-03/index.html>. As comment #7 to that ticket notes... I later tracked down the troublesome commit, but made a typo so I revised the ChangeLog entry yet again. Here's how it reads now. 2026-07-24 G. Branden Robinson <[email protected]> [troff]: Fix Savannah #68552. * src/roff/troff/input.cpp (token::description): Simplify population of buffer describing a node token and avoid potentially overwriting a heap-allocated buffer if that node's description is lengthy (as "non-interpreted character nodes", input in *roff as `\a` and `\t`, are). Discard stale code that performed a dead store of a dummy node description; `describe_node()` zeroes out its destination buffer, so that became pointless a while back. Then, simply measure the buffer as populated by `describe_node()` with strlen(3) and append to it carefully with strncat(3), respecting `bufsz`. Fixes <https://savannah.gnu.org/bugs/?68552>. Thanks to Bruno Haible for the investigation and advice. Problem apparently introduced by me in commit dd7bf97938, 23 November, but in the instance for which we have a reproducer, this heap overwrite was not reached because it was masked by a prior null pointer dereference (in `lookup_request()`, which was set up to fail by `do_request()`; see Savannah #68558). > 4. Assertion failure (abort) while parsing line-drawing escapes > CWE-617 — Low (DoS) | CVSS:3.1/AV:L/AC:L/PR:N/UI:R/S:U/C:N/I:N/A:H (5.5 > Medium) > read_line_rule_expression() assumes the token produced by a line-drawing > escape carries charinfo; a crafted sequence violates it and aborts. This is a valid report. I have filed it as Savannah #68788. The reporter's "minimal PoC" is 201 bytes. Mine is 7 bytes. The proposed patch is, again, incorrectly scoped, though this time it is close--in the correct function and near the correct location. Its solution is to promote an assertion to an error, which is incorrect. The heretofore unenforced precondition for drawing rules with glyphs--which is what the `\l` and `\L` escape sequences do--is that the glyph one draws with is a *roff character first. Things like tabs and leaders are not "characters". Thus, rather than the reporter's... # CXX-GROFF-04 - suggested fix # note: Diagnostic instead of an assertion. --- a/src/roff/troff/input.cpp +++ b/src/roff/troff/input.cpp @@ -6128,8 +6128,8 @@ && input_stack::get_level() == start_level)) { *cip = tok.get_charinfo(true /* required */); if (0 /* nullptr */ == *cip) - assert(0 == "attempted to use token without charinfo in" - " line-drawing escape sequence"); + error("attempted to use token without charinfo in" + " line-drawing escape sequence"); tok.next(); } if (!(start_token == tok ...we should instead have this. diff --git a/src/roff/troff/input.cpp b/src/roff/troff/input.cpp index 6e56f360e..5b1f5fcb0 100644 --- a/src/roff/troff/input.cpp +++ b/src/roff/troff/input.cpp @@ -6298,6 +6298,11 @@ static bool read_line_rule_expression(units *n, unsigned char si, tok.next(); if (!(start_token == tok && input_stack::get_level() == start_level)) { + if (!tok.is_any_character()) { + error("cannot use %1 to draw a line of glyphs", + tok.description()); + return false; + } *cip = tok.get_charinfo(true /* is_mandatory */); if (0 /* nullptr */ == *cip) assert(0 == "attempted to use token without charinfo in" We still want that assertion. If this escape sequence handler somehow gets hold of a character without `charinfo`, that's a violation of an invariant, and demands programmer attention. We need to explore how we got the formatter into that state, not blame the user's input. > 5. Missing-identifier paths use assertions instead of error handling (four > sites, one root cause) > CWE-617 — Low (DoS) | CVSS:3.1/AV:L/AC:L/PR:N/UI:R/S:U/C:N/I:N/A:H (5.5 > Medium) > Four code paths assume an identifier is non-null and assert otherwise; > crafted input reaches all four. These are valid reports, but the recommended patches, consisting exclusively of assert(3)ion removals, with no code to replace them, reveals lack of understanding of why the developer put the assert(3) calls there in the first place. As that developer, I can attest that these assertions served their purpose: to reveal where presumed invariants of function operation were being violated.[2] Again the proofs-of-concept failed to live up to the report's proffered descriptions as "minimal". -rw-r--r-- 1 branden branden 196 Oct 6 18:35 poc_alias_macro.roff -rw-r--r-- 1 branden branden 7 Oct 18 11:32 poc_alias_macro_gbr.roff -rw-r--r-- 1 branden branden 196 Oct 6 18:35 poc_chop_macro.roff -rw-r--r-- 1 branden branden 8 Oct 22 10:54 poc_chop_macro_gbr.roff -rw-r--r-- 1 branden branden 157 Oct 6 18:35 poc_describe_node.roff -rw-r--r-- 1 branden branden 13 Oct 23 15:40 poc_describe_node_gbr.roff -rw-r--r-- 1 branden branden 167 Oct 6 18:35 poc_environment_copy.roff -rw-r--r-- 1 branden branden 7 Oct 18 11:31 poc_environment_copy_gbr.roff (Again, I would point out how much smaller my reproducers are than the "minimal" ones offered by the reporter.) These issues have little to do with each other apart from the presence of assert(3) calls. I've filed and resolved several Savannah tickets to address these issues. I also independently inspected several other similar issues that I suspected might be present due to my familiarity with the code base; I dare say that this is another example of how "expertise" pays off. Whatever generated these reports, be it a human or an LLM, appears suspiciously myopic for an entity that presumed to submit patches "fixing" the problems. Further, this fifth issue appears to be a miscellaneous collection of unrelated issues, such even the promise of organization that this highly structured report made ends up broken. #68837 [PATCH] [troff] assertion failure when `fzoom` request given invalid font identifier since commit 3dc11a61aa, 2026-04-18 #68836 [PATCH] [troff] assertion failure when `ftr` request given invalid first identifier #68832 [PATCH] [troff] assertion failure when escape squence with deprecated delimiter follows unterminated delimited esc seq #68819 [PATCH] [troff] assertion failure when `chop` request given invalid identifier #68817 [PATCH] [troff] assertion failure when `als` request given invalid first identifier #68815 [PATCH] [troff] ASAN reports heap-use-after-free in `pi` request handler #68797 [PATCH] [troff] assertion failure when `rn` request given invalid first identifier #68796 [PATCH] [troff] assertion failure when `defcolor` request given invalid color identifier #68795 [troff] `pcolor` request handler infloops when given invalid identifier since 1.24.0 #68794 [PATCH] [troff] assertion failure when `evc` request given invalid identifier > PoC files, harness sources and patches are in the attached archive, > one directory per item. > > On the CVE side: no CNA covers these projects upstream, so the ID > would come from MITRE (CNA of last resort). MITRE's preferred route is > for the upstream project to submit the request; if you would rather do > that I can hand you the completed request fields ready to paste. > Otherwise I will file with MITRE myself once we agree the defects are > real. No CVE ID has been assigned yet. Feel free to engage with MITRE however you like. I am disinclined to contribute to the further gamification of CVE allocation. https://lwn.net/Articles/953738/ To my fellow groff developers, I advise a policy of non-cooperation with MITRE regarding CVE characterization. Witness Curl developer Daniel Stenberg's experience. https://daniel.haxx.se/blog/2026/06/24/a-cve-dispute/ groff has a bug tracker. People should use it. https://savannah.gnu.org/bugs/?group=groff For a better example of how to engage with the groff project to report suspected security issues, one could do a lot worse than follow the example of Pavol Sloboda, who in August filed 9 issues as "private" tickets (presumably as a courtesy to let us decide whether we wanted to "embargo" any issues). Six of those issues proved in our (mostly my) assessment to not be security-relevant; the other 3 were, and they were what motivated the groff 1.24.2 release. https://lists.gnu.org/archive/html/info-gnu/2026-09/msg00009.html More importantly, Pavol's reports were vastly better examples of engineer-to-engineer communication of a defect. Here's an exhibit. https://savannah.gnu.org/bugs/?68682 Now that I've pointed these out, I wonder if you'll be scooping them up as additional CVE entries. One can never have enough Pokémon, no? > Regards, > hjh_nju At present, our "ANNOUNCE" file credits names such as the following. Alex Colomar Alexis Hildebrandt Avi Halachmi Benno Schulenberg Bjarni Ingi Gislason ... Russ Allbery Santiago Vila Stefan Möding Stephen Gildea Zack Weinberg I'm happy to credit this reporter, but I must take the opportunity to solicit a more plausible sounding pseudonym, like "Jigar Kumar" or "Roman Seleznev". I don't know what sorts of names "DDT Cr" and "hjh_nju" are, and so I take this opportunity to pronounce a policy of crediting only biological humans as contributors to groff. Regards, Branden [1] as does C, with its infamous "lexer hack", driven by K&T's refusal to _syntactically_ distinguish type identifiers from object identifiers https://en.wikipedia.org/wiki/Lexer_hack [2] These assertions were substantially harder to trip before I made a change in the 1.24.0 development cycle to "ban" from identifiers the handful of C0 control characters that weren't already prohibited (actually, already in use to encode GNU troff input token types).[3] We might thus characterize this fuzzer-driven exposure of the assert(3) code paths as "John Gardner's revenge". ;-) https://lists.gnu.org/archive/html/groff/2026-03/msg00001.html [3] commit b6a737385406f9fd3df4ece0a4814b9fd1a500d9 Author: G. Branden Robinson <[email protected]> Date: Thu Dec 25 23:04:21 2025 -0600 [troff]: Fix Savannah #67734. * src/roff/troff/input.cpp: Add new Boolean-valued parameter to `read_input_until_terminator()`, `want_identifier`, defaulting to false, so that we can distinguish callers that want a GNU troff identifier from those gathering some other kind of input. This is so that can we can reject (all) C0 control and Latin-1 Supplement characters in identifiers. (C1 controls are already rejected on input.) The prohibition against C0 controls is to make the language less tolerant of unreadable input, and the latter is to enable us to pivot to reading UTF-8-encoded input in a future release. (read_input_until_terminator): Update declaration to add new parameter with default value. Update definition to reject, with error diagnostic, character codes less than 32 and greater than 159. Add assertion that the putative identifier character is not a space (character code 32); these have never been valid in *roff identifiers. This function's callers must ensure that the terminator precedes any space in the input. Fixes <https://savannah.gnu.org/bugs/?67734>. NEWS: Report change.
signature.asc
Description: PGP signature
