bugmake - Bugs: bug #59881, Segmentation Fault through...

 
 

bug #59881: Segmentation Fault through manipulated Makefile

Submitter:  None
Submitted:  Fri 15 Jan 2021 01:18:53 PM UTC
   
 
Severity:  3 - Normal Item Group:  Bug
Status:  Fixed Privacy:  Public
Assigned to:  psmith Open/Closed:  Closed
Component Version:  4.4 Operating System:  Any
Fixed Release:  4.4 Triage Status:  Medium Effort
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 15 Mar 2021 05:19:39 AM UTC, comment #8: 

I pushed a fix.  Thanks all!

Paul D. Smith <psmith>
Group administrator
Sun 14 Mar 2021 11:19:42 PM UTC, comment #7: 

I found the real bug and have a fix.  I will also provide changes to fix the variable_buffer issue Dmitry noticed; although I'm not convinced it is actually possible to expand the buffer in situations where we don't expect it, it's certainly better to not risk it.

Paul D. Smith <psmith>
Group administrator
Sun 14 Mar 2021 10:31:14 PM UTC, comment #6: 

I think it's probably better to use variable_buffer here, for safety.  But, there's something much deeper and more mysterious happening.  It shouldn't really be possible to hit this problem.  The reason that the buffer is resized is that the stem of the pattern contains spaces; it's "000000000 0000000000 0000000".  Because of this, the expansion of the pattern on the RHS happens multiple times for this single file instead of just one time.

It's related to the vertical TAB characters in this file; if I replace them with spaces I don't see this behavior.  The problem is that in some places in the parser we're treating VT as whitespace (word separators) and in other places we are treating it as non-whitespace.

I'm investigating this.

Paul D. Smith <psmith>
Group administrator
Mon 18 Jan 2021 08:00:33 PM UTC, comment #5: 

I believe, this is useful. You found a genuine bug. Do you mind sharing you fuzzing tecqnique?

Dmitry Goncharov <dgoncharov>
Mon 18 Jan 2021 01:32:14 PM UTC, comment #4: 

You're welcome Dmitry. I found this makefile by fuzzing the make parser. If you want I could maybe spent another few rounds on fuzzing this project. I am just not sure if it is worth the effort on both sides (for me to fuzz the project, and for you to fix obscure makefile inputs).

I am personally and professionally more interested in finding security related bugs ;-)

Fabian Helfert <fabse333>
Mon 18 Jan 2021 02:38:22 AM UTC, comment #3: 

Thank you for your report.
How did you manage to obtain this makefile?

Dmitry Goncharov <dgoncharov>
Mon 18 Jan 2021 02:37:30 AM UTC, comment #2: 

Here is a test.

diff --git a/tests/scripts/functions/error b/tests/scripts/functions/error
index 998afe4..cb8fcc4 100644
--- a/tests/scripts/functions/error
+++ b/tests/scripts/functions/error
@@ -63,6 +63,28 @@ $answer = "Some stuff\n$makefile:17: * error is definitely.  Stop.\n";
 $answer = "$makefile:22: * Error found!.  Stop.\n";
 &compare_output($answer,&get_logfile(1));
 
+
+# Test #6
+# A makefile which causes variable_buffer_output to realloc and renders buffer
+# invalid and used to crash make.
+# sv 59881.
+my $makestring = ':
+
+0:
+00000000
+000
    000000000000
                000000000:
+0%0:00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000%0000000000000
+0';
+
+open(MAKEFILE, "> $makefile") or die "Failed to open $makefile: $!\n";
+print MAKEFILE $makestring;
+close(MAKEFILE) or die "Failed to write $makefile: $!\n";
+
+&run_make_with_options($makefile, '', &get_logfile, 512);
+my $answer = "$makefile:4: * missing separator.  Stop.\n";
+&compare_output($answer, &get_logfile(1));
+
+
 # This tells the test driver that the perl test script executed properly.
 1;

Dmitry Goncharov <dgoncharov>
Mon 18 Jan 2021 02:36:04 AM UTC, comment #1: 

This makefile causes variable_buffer_output to realloc. This renders buffer in enter_prereqs invalid.

Here is a patch.

diff --git a/src/file.c b/src/file.c
index a979ca5..61f0a56 100644
--- a/src/file.c
+++ b/src/file.c
@@ -524,8 +524,12 @@ enter_prereqs (struct dep *deps, const char *stem)
                   continue;
                 }

-              /* Save the name.  */
-              dp->name = strcache_add_len (buffer, o - buffer);
+              /* Save the name.
+               * VARIABLE_BUFFER_OUTPUT could realloc, which'd render BUFFER
+               * invalid.
+               sv 59881.  /
+              dp->name = strcache_add_len (variable_buffer,
+                                           o - variable_buffer);
             }
           dp->stem = stem;
           dp->staticpattern = 1;

Dmitry Goncharov <dgoncharov>
Fri 15 Jan 2021 01:18:53 PM UTC, original submission:  

I found another bug which causes a segfault when reading with "make -f min".

The issue is this line:
https://github.com/mirror/make/blob/master/src/file.c#L528

There the value of "o" is smaller than "buffer" which causes an integer underflow and a very large "len" value. When now calling "strcache_add_len" the memcpy has such a large length that it actually tries to write to a value below the stack:
https://github.com/mirror/make/blob/master/src/strcache.c#L248

I hadn't the time to check if there is a previous root cause which could be detected sooner, but one fix (at least for that example) would be to just check if "o" is smaller than "buffer".

On this example I am not sure if it is possible to exploit. This could be potentially upgraded to an arbitrary write primitive to execute code. I know there are easier ways to execute code (Since it is a makefile), but it could be a way to hide malware within the Makefile of open source projects.

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #50723:  min added by None (206B - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by psmith (Posted a comment)
  • -email is unavailable- added by fabse333 (Posted a comment)
  • -email is unavailable- added by dgoncharov (Posted a comment)
  •  

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

    Date Changed by Updated Field Previous Value => Replaced by
    2021-03-15 psmith StatusNone Fixed
        Assigned toNone psmith
        Open/ClosedOpen Closed
        Operating SystemNone Any
        Fixed ReleaseNone 4.4
        Triage StatusNone Medium Effort
    2021-01-15 None Attached File- Added min, #50723

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code