bugGNU roff - Bugs: bug #61043, potential integer overflow...

 
 

bug #61043: potential integer overflow vulnerability in src/preproc/grn/hdb.cpp

Submitter:  eqkws
Submitted:  Sun 15 Aug 2021 06:03:08 AM UTC
   
 
Category:  Preprocessor grn Severity:  4 - Important
Item Group:  Incorrect behaviour Status:  Fixed
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Closed Planned Release:  1.23.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Mon 16 Aug 2021 10:03:47 PM UTC, comment #4: 

Setting "privacy" to "public".

I don't think this issue is a sensitive one.  I perceive no confidentiality or integrity violations here.  The issue is one of availability--there are innumerable ways in unprivileged code on POSIX systems to busy-wait, and crude tools like resource limits to manage that problem.

So, in my opinion, this is an ordinary bug.

Also, since I already pushed commits with detailed information, the horse is out of the principled barn.

G. Branden Robinson <gbranden>
Group administrator
Mon 16 Aug 2021 01:19:05 PM UTC, comment #3: 

I've addressed an avenue for a denial-of-service (unbounded use of CPU time)...


commit 50216a235cd8214b9565ef0c0049fc4ed549650e
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Aug 16 03:31:08 2021 +1000

    [grn]: Fix infinite loop on bad input.

    * src/preproc/grn/hdb.cpp (DBRead): Check return value of `sscanf()` and
      call `fatal()` if no conversions succeeded.  The blithe discard of a
      useful return value is bad enough, but this one took place inside a
      do-while such that it could loop forever trying fruitlessly to parse
      two doubles out of strings that didn't contain them (the loop never
      checked the EOF status of the file stream from which it was reading,
      and relied on `fgets()` to keep advancing the stream pointer).
      Discovered while root-causing Savannah #61043.


...added checks of the return value of `malloc()`...


commit 0d56933b896e7bc2c7193daf7b740fe06e9196f6
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Aug 16 10:19:39 2021 +1000

    [grn]: Add and use `malloc()` wrapper.

    * /src/preproc/grn/main.cpp (grnmalloc): New function takes argument of
      `size_t` type and constant string argument to describe what is being
      allocated.  Return non-null pointer from `malloc()`, otherwise call
      `fatal()`, describing what was being allocated and the problem
      reported by the system.

    * src/preproc/grn/hdb.cpp (DBCreateElt):
    * src/preproc/grn/hpoint.cpp (PTMakePoint):
    * /src/preproc/grn/main.cpp (main, interpret): Migrate `malloc()`
      callers to `grnmalloc()`.


...and addressed the reported potential integer overflow.


commit eb4f0675e322d50c69d36625f296f8260fafeb46
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Aug 16 11:09:16 2021 +1000

    [grn]: Perform more input validation.

    * src/preproc/grn/hdb.cpp: Perform more input validation.  Improve
      diagnostics by taking advantage of libgroff infrastructure and
      tracking the line number of the input file.  Add global `lineno`.

      (DBRead): Increment `lineno` after reading newlines from input.  Call
      `error_with_file_and_line()` instead of `error()`.  If input reports a
      negative length for the text (string) to follow in the file, exit with
      a fatal diagnostic.  Check for EOF while reading text string.

      (DBGetType): Convert `fprintf()` call for warning diagnostic to
      `warning_with_file_and_line()`.

      (DBRead, DBGetType): Add contextual information to diagnostic
      messages.

    Fixes <https://savannah.gnu.org/bugs/?61043>.   Thanks to Savannah user
    eqkws for the report.


G. Branden Robinson <gbranden>
Group administrator
Mon 16 Aug 2021 12:11:09 AM UTC, comment #2: 

I've prepared a patch that mitigates the impact of the underlying bug.  Here it is.

I have not pushed this yet; I am making some other changes to address the sloppy error handing of this program.


diff --git a/ChangeLog b/ChangeLog
index 6b5c245b..7bdee9a0 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2021-08-16  G. Branden Robinson <g.branden.robinson@gmail.com>
+
+       * src/preproc/grn/hdb.cpp (DBRead): Check return value of
+       `sscanf()` and call `fatal()` if no conversions succeeded.  The
+       blithe discard of a useful return value is bad enough, but this
+       one took place inside a do-while such that it could loop
+       forever trying fruitlessly to parse two doubles out of strings
+       that didn't contain them (the loop never checked the EOF status
+       of the file stream from which it was reading, and relied on
+       `fgets()` to keep advancing the stream pointer).  Discovered
+       while root-causing Savannah #61043.
+
 2021-08-15  G. Branden Robinson <g.branden.robinson@gmail.com>

        Resolve compiler warnings relating to format string security and
diff --git a/src/preproc/grn/hdb.cpp b/src/preproc/grn/hdb.cpp
index c61e099b..0310d7ac 100644
--- a/src/preproc/grn/hdb.cpp
+++ b/src/preproc/grn/hdb.cpp
@@ -148,7 +148,11 @@ DBRead(register FILE *file)
          if (string[0] == '*') {       /* SUN gremlin file */
            lastpoint = TRUE;
          } else {
-           (void) sscanf(string, "%lf%lf", &x, &y);
+           if (!sscanf(string, "%lf%lf", &x, &y)) {
+             error("expected coordinate pair, got '%1';"
+                   " giving up on this picture", string);
+             return(elist);
+           }
            if ((x == -1.00 && y == -1.00) && (!SUNFILE))
              lastpoint = TRUE;
            else {


Now, instead of looping forever, I get the following output.


$ ./build/test-groff -g -me -z gremlin.me
grn: error: expected coordinate pair, got 'CENTCENT'; giving up on this picture


G. Branden Robinson <gbranden>
Group administrator
Sun 15 Aug 2021 05:04:23 PM UTC, comment #1: 


original submission:

> Hi, I found some integer overflow bug in the source code of groff.
>
> In src/preproc/grn/hdb.cpp:189,
>
> 189 (void) fscanf(file, "%d", &len);    /* text length */
> 190 (void) getc(file);                  /* eat blank */
> 191 txt = (char *) malloc((unsigned) len + 1);
>
> The program reads the value of len from an input file and calls malloc with len + 1.
>
> If a maliciously crafted input that sets len to -1 is given, it will cause an integer overflow, and allocation with 0 leads to buggy behavior like denial of service.


Thank you for the report.

Using the grn from groff 1.22.4, if I contrive incorrect input, I get an error diagnostic.


$ groff -ww -g -me gremlin.me >| gremlin.ps
grn: fatal error: unknown element type


Here are my input files.


$ cat gremlin.me
.GS
roman 1
italics 2
bold 3
special 4
narrow 1
medium 3
thick 5
width 5.5
l mg
file gremlin.g
.GE
$ cat gremlin.g
sungremlinfile
1 0.0 0.0
CENTCENT
5.0 5.0
*
1 1
-1 Savannah
-1


The second to last line, "-1 Savannah", is the culprit.  It should read "8 Savannah".  (There are many other valid possibilities.)

However, if I change "Savannah" to another ASCII string, I can in fact provoke an infinite loop.

I'm looking into this.

G. Branden Robinson <gbranden>
Group administrator
Sun 15 Aug 2021 06:03:08 AM UTC, original submission:  

Hi, I found some integer overflow bug in the source code of groff.

In src/preproc/grn/hdb.cpp:189,

189 (void) fscanf(file, "%d", &len);    /* text length */
190 (void) getc(file);                  /* eat blank */
191 txt = (char *) malloc((unsigned) len + 1);

The program reads the value of len from an input file and calls malloc with len + 1.

If a maliciously crafted input that sets len to -1 is given, it will cause an integer overflow, and allocation with 0 leads to buggy behavior like denial of service.

Thank you.

eqkws

 

(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 gbranden (Posted a comment)
  • -email is unavailable- added by eqkws (Submitted the item)
  • -email is unavailable- added by eqkws
  •  

    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 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-08-16 gbranden PrivacyPrivate Public
    2021-08-16 gbranden StatusIn Progress Fixed
        Open/ClosedOpen Closed
        Planned ReleaseNone 1.23.0
    2021-08-15 gbranden CategoryNone Preprocessor grn
        Severity3 - Normal 4 - Important
        Item GroupNone Incorrect behaviour
        StatusNone In Progress
        Assigned toNone gbranden
    2021-08-15 eqkws Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code