bugGNU patch - Bugs: bug #53566, Ed support allows arbitrary...

 
 

bug #53566: Ed support allows arbitrary command execution in patch files

Submitted by:  None
Submitted on:  Thu 05 Apr 2018 05:25:12 PM UTC  
 
Category:  None Severity:  3 - Normal
Item Group:  None Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Tue 17 Apr 2018 03:00:30 PM UTC, comment #9:

Hehe ;-) I knew it used ed, but not that it went through patch.

Jean Delvare <khali>
Tue 17 Apr 2018 02:39:53 PM UTC, comment #8:

Jean,

you may be surprised to learn that 'quilt annotate' currently uses ed-style patches -- but could be changed to use ed directly, of course.

I've learned that even small changes to the behavior of patch easily lead to unforeseen problems, so I'm surely not going to remove automatic ed-style patch detection. Configuring out ed-style patches altogether would be less of a problem.

Andreas Gruenbacher <agruen>
Project Administrator
Tue 17 Apr 2018 01:58:17 PM UTC, comment #7:

Hanno, I can't see how people compiling patch on their own or not should matter. As a distribution package maintainer, if there was a configure option to disable ed support completely, I would gladly use it, because I very much doubt our users need that feature, and I'd rather limit the attack surface. Each distribution can make a different choice depending on their user base. People use distributions, instead of building everything themselves, exactly so that we make decisions for them and they don't have to bother.

So I am very favorable to adding such a configure option. Actually I was going to suggest it too.

Ed-style format not being auto-detected was also something I wanted to suggest. Andreas, because something used to work before isn't not a good enough reason to never change it. Being secure is more important than being compatible in this case, in my opinion. Ed-style patches are very specific, so specific that I still have to see any in the wild. I would expect people who use that (if they exist) to know what they are doing and be fine with adjusting their calls for the benefit of the majority.

If you really don't want to disable auto-detection of ed-style patches by default then this could also be a configure option, that is, it would be a tristate instead of a boolean: ed-style patches supported and auto-detected, supported but not auto-detected, or not supported at all.

Jean Delvare <khali>
Mon 09 Apr 2018 08:15:55 AM UTC, comment #6:

Ed restricted mode is a GNU extension and not supported by other implementations of ed, so GNU patch would end up being subtly broken on systems which don't have GNU ed. (I'd rather not add a build-time dependency or a runtime fallback code, either.)

On the patch you refer to: first, ed-style patches have always been auto-detected and changing that behavior can potentially break old programs or scripts, so it's not going to happen. Second, the patch adds another special meaning to the -f flag which would just make the flag even more weird; I'd rather like to avoid. Third, adding warnings like "this could allow arbitrary command execution" to messages seems rather stupid: either we're aware of or at least suspicious about such vulnerabilities and then we should act, or we don't and then we shouldn't warn about problems we don't think exist.

Andreas Gruenbacher <agruen>
Project Administrator
Mon 09 Apr 2018 07:28:56 AM UTC, comment #5:

I'm not keen on having a configure option here, as realistically very few people will compile patch on their own.

I think there are different questions to answer here:

  • Should ed use to restricted mode? I'd say yes, unless this breaks something. I don't think so, and thus this should be done (unconditionally, not as a configure or otherwise option).
  • Should ed patches be autodetected by default? I think no, because they're hardly used in practice. Even with the fix in place I'd recommend applying the first patch that was attached here.
Hanno Boeck <hanno>
Mon 09 Apr 2018 07:04:58 AM UTC, comment #4:

For such cases, how about instead adding a configure option to disable ed-style patch support? This would provide an even stronger guarantee that nothing can go wrong, and I don't think people so highly focused on security would mind restricting patch in that way.

Andreas Gruenbacher <agruen>
Project Administrator
Mon 09 Apr 2018 03:19:45 AM UTC, comment #3:

The patch in [123eaff] looks solid. If I may summarise, the change it makes is to put the ed script in a temporary file and invoke `ed tmpname.ed`, so that scripts with syntax errors will cause ed to abort rather than heroically trying to soldier on, which is what led to it running unvalidated commands previously.

do_ed_script() / ed_get_command_letter() are quite strict in the ed commands they will allow. With this patch in place I can't see any opportunity to subvert their validation.

That said, some distributors may want to add a further layer of defence (against unforseen injection opportunities, or future changes in ed/patch command parsing) by having patch invoke "ed -r". Could this be supported with a configure option?

Anonymous
Fri 06 Apr 2018 10:26:37 AM UTC, comment #2:

Fix pushed to the git repository.

Andreas Gruenbacher <agruen>
Project Administrator
Fri 06 Apr 2018 01:21:35 AM UTC, comment #1:

This has been assigned CVE-2018-1000156.

Anonymous
Thu 05 Apr 2018 05:25:12 PM UTC, original submission:

See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=894667#19 for an example of using `!' in a patch to execute a shell command.

At the very least, Ed should be executed with `-r' (restricted mode) which `disables editing of files out of the current directory and execution of shell commands' (from the manual).

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #43817:  ed-fix.patch added by agruen (4KiB - text/x-patch - Here is first cut of an alternative fix that avoids removing ed-style diff support)
file #43816:  0001-Refuse-to-apply-ed-scripts-by-default.patch added by None (6KiB - text/x-patch - Patch to warn the user that ed scripts are potentially dangerous)
file #43815:  poc.patch added by hanno (111B - text/x-patch - proof of concept (file "b" needs to exist))

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by khali (Posted a comment)
  • -email is unavailable- added by agruen (Updated the item)
  • -email is unavailable- added by hanno (Updated the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-04-06 agruen StatusNone => Fixed
        Open/ClosedOpen => Closed
    2018-04-05 agruen Attached File- => Added ed-fix.patch, #43817
    2018-04-05 None Attached File- => Added 0001-Refuse-to-apply-ed-scripts-by-default.patch, #43816
    2018-04-05 hanno Attached File- => Added poc.patch, #43815

    Back to the top


    Powered by Savane 3.3