bugGNU roff - Bugs: bug #64229, [troff] want diagnostic for...

 
 

bug #64229: [troff] want diagnostic for diversion length overflow

Submitter:  G. Branden Robinson <gbranden>
Submitted:  Sun 21 May 2023 03:29:57 PM UTC
   
 
Category:  Core Severity:  3 - Normal
Item Group:  Warning/Suspicious behaviour Status:  Fixed
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Closed Planned Release:  1.24.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 15 Aug 2024 05:20:38 PM UTC, comment #8: 


commit 41ce02672a795aa52d408e1e64c0c093934fdf3e
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Wed Aug 14 11:01:54 2024 -0500

    [troff]: Fix Savannah #64229.

    Throw diagnostic in event of a diversion's contents overflowing its
    vertical position.  We can't do anything sensible in this case, so make
    it a fatal error.  (It's also more likely that your host environment
    will exhaust memory available to the `troff` process before encountering
    this problem.)

    * src/roff/troff/div.cpp: New constant symbol `DIVERSION_LENGTH_MAX`
      defines the maximum permissible vertical size of a diversion in basic
      units.  Presently, it is `INT_MAX`.

      (macro_diversion::distance_to_next_trap): When there is no diversion
      trap, or it has already been passed, report the distance to
      `DIVERSION_LENGTH_MAX` (rather than `INT_MAX`) divided by the vertical
      resolution.

      (macro_diversion::output): Check for overflow when incrementing
      vertical position upon writing out a line, and emit fatal error
      diagnostic if it occurs.

    Fixes <https://savannah.gnu.org/bugs/?64229>.  Thanks to Dave Kemper for
    the discussion.


G. Branden Robinson <gbranden>
Group administrator
Wed 14 Aug 2024 03:59:41 PM UTC, comment #7: 

comment #6:

> I instrumented `macro_diversion::output()` in "div.cpp" and learned that the Linux OOM killer slays the DoS attack proposed in comment #0 at only about 4 million lines of output on my machine.
>
> While my box isn't supremely specced, that's still 3 orders or magnitude less than INT_MAX, which is the effective maximum "page" length of a diversion.


I misspoke here.  I was using `nroff`, and all the terminal devices have a vertical resolution greater than 1, so the vertical position at OOM death was 4 million.

This doesn't change the analysis much because the overflow condition (and computed distance to the next trap) is also divided by the vertical resolution.

G. Branden Robinson <gbranden>
Group administrator
Wed 14 Aug 2024 11:06:09 AM UTC, comment #6: 

Dave's instincts in comment #3 were pretty good.

I instrumented `macro_diversion::output()` in "div.cpp" and learned that the Linux OOM killer slays the DoS attack proposed in comment #0 at only about 4 million lines of output on my machine.

While my box isn't supremely specced, that's still 3 orders or magnitude less than INT_MAX, which is the effective maximum "page" length of a diversion.

So I think we can get by with a check for diversion length overflow, and emit a fatal error if that overflow occurs.

Changing Item Group and kicking down to normal severity. 


diff --git a/src/roff/troff/div.cpp b/src/roff/troff/div.cpp
index 772540c09..fc7c16570 100644
--- a/src/roff/troff/div.cpp
+++ b/src/roff/troff/div.cpp
@@ -241,6 +241,8 @@ macro_diversion::~macro_diversion()
   dn_reg_contents = vertical_position.to_units();
 }

+static int DIVERSION_LENGTH_MAX = INT_MAX;
+
 vunits macro_diversion::distance_to_next_trap()
 {
   vunits distance = 0;
@@ -250,7 +252,8 @@ vunits macro_diversion::distance_to_next_trap()
   else
     // Do the (saturating) arithmetic ourselves to avoid an error
     // diagnostic from constructor in number.cpp.
-    distance = units(INT_MAX / vresolution);
+    //distance = units(INT_MAX / vresolution);
+    distance = units(DIVERSION_LENGTH_MAX / vresolution);
   assert(distance >= 0);
   return distance;
 }
@@ -300,6 +303,17 @@ void macro_diversion::output(node *nd, int retain_size,
   if (width > max_width)
     max_width = width;
   vunits x = v.pre + v.pre_extra + v.post + v.post_extra;
+  int new_vpos = 0;
+  int vpos = vertical_position.to_units();
+  int lineht = x.to_units();
+  bool overflow = false;
+  if (ckd_add(&new_vpos, vpos, lineht))
+    overflow = true;
+  else if (new_vpos > DIVERSION_LENGTH_MAX)
+    overflow = true;
+  if (overflow)
+    fatal("diversion overflow (vertical position: %1u,"
+         " next line height: %2u)", vpos, lineht);
   if (vertical_position_traps_flag
       && !diversion_trap.is_null() && diversion_trap_pos > vertical_position
       && diversion_trap_pos <= vertical_position + x) {


While I'm at it I'm parameterizing the maximum diversion length, as seen above, instead of using `INT_MAX` as a magic number.

G. Branden Robinson <gbranden>
Group administrator
Fri 18 Aug 2023 05:27:37 AM UTC, comment #5: 

Learned today that mandoc(1) isn't susceptible to this.

Because it doesn't support diversions at all.

https://lists.gnu.org/archive/html/groff/2023-08/msg00080.html

G. Branden Robinson <gbranden>
Group administrator
Wed 24 May 2023 01:11:19 PM UTC, comment #4: 


comment #3:

> "Infinite input causes infinite execution time and consumes infinite resources" doesn't particularly surprise me.  Do other command-line utilities fare any better?  Things like "cat" and "grep" have the luxury of being able to emit output before all input has been read, but "yes abcdefghijklm | sort" seemed happy to churn the CPU as long as I'd let it.


It's not that this fails that concerns me so much as the failure mode.

In all of these cases the formatter got nuked by the kernel OOM killer.  This means an uncontrolled crash.

I don't know what other troff_s will do (perhaps nothing), but I think GNU _troff should be sensitive to diversion content reaching a vertical page location near INT_MAX in basic units and fatal() out.

G. Branden Robinson <gbranden>
Group administrator
Wed 24 May 2023 01:00:22 AM UTC, comment #3: 

"Infinite input causes infinite execution time and consumes infinite resources" doesn't particularly surprise me.  Do other command-line utilities fare any better?  Things like "cat" and "grep" have the luxury of being able to emit output before all input has been read, but "yes abcdefghijklm | sort" seemed happy to churn the CPU as long as I'd let it.

Dave <barx>
Group Member
Mon 22 May 2023 08:01:43 PM UTC, comment #2: 

I'm impressed with how badly Heirloom handled that.


$ time { printf '.di foo\n.nf\n'; yes abcdefghijklm; } | ./bin/nroff
Killed

real    1519m49.629s
user    1518m54.699s
sys     0m40.179s


G. Branden Robinson <gbranden>
Group administrator
Sun 21 May 2023 10:21:36 PM UTC, comment #1: 

Over 7 hours later, RSS now 8.4 GB and growing, and Heirloom still hasn't been killed.

I guess a slow algorithm is one defense against attack, like rate-limiting login attempts...

G. Branden Robinson <gbranden>
Group administrator
Sun 21 May 2023 03:29:57 PM UTC, original submission:  

Affects groff 1.22.4.  Maybe every groff ever.

Also Heirloom Doctools nroff.[1]

Also DWB 3.3 nroff.

Maybe every *roff ever...


$ { printf '.di foo\n.nf\n'; yes abcdefghijklm; } | nroff
groff: troff: Signal 9


[1] Well, I think so...its resident set size is at 1.7 GB and growing without apparent bound but it is way slower at it than groff (27 seconds) or DWB (1 minute 32 seconds), which got killed by the kernel after a couple of minutes.  Heirloom is still grinding after 20 minutes.  So they may have a whole 'nother problem besides this one.

G. Branden Robinson <gbranden>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by barx (Posted a comment)
  • -email is unavailable- added by gbranden (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-08-15 gbranden StatusIn Progress Fixed
        Open/ClosedOpen Closed
        Planned ReleaseNone 1.24.0
    2024-08-14 gbranden Severity4 - Important 3 - Normal
        Item GroupCrash/Unresponsive Warning/Suspicious behaviour
        StatusNone In Progress
        Assigned toNone gbranden
        Summary[troff] I haz a DoS attack [troff] want diagnostic for diversion length overflow

    Back to the top

    Powered by Savane 3.14-9aa3.
    Corresponding source code