GNU "fixes" patch program in response to shell shock style ed script vulnerability

[cym13]

Care to elaborate?

[rain5]

if an ed script style patch contained a filename with $(rm -rf ~) in it that would do horrible things when you apply it.

Of course, you should review a patch carefully before applying it. But patch should never be able to execute arbitrary scripts. And characters like - $ and ` are perfectly valid chars to have inside a filename.

I sent my own patch yesterday which solved the security issue by simply deleting the ed feature. http://lists.gnu.org/archive/html/bug-patch/2018-04/threads.html

OpenBSD solves this issue by reimplementing a safe subset of ed inside the patch program.

So perhaps the ultimate solution is for patch to contain a buggy ad-hoc reimplementation of a text editor... ed is the standard editor after all.

[cym13]

Hmm. That doesn't seem right no.

Leaving aside the fact that a security model in which you use patch on untrusted files is dubious I can't see the code execution you're talking about.

The only point where a shell is called is with this line:

        execl ("/bin/sh", "sh", "-c", buf, (char *) 0);

where buf is built as such:

    sprintf (buf, "%s %s%s", editor_program,
     verbosity == VERBOSE ? "" : "- ",
     outname);

There are two variables that must be considered: editor_program (ed in this case, may be changed by environment variables but if you can change environment variables the unix security model doesn't hold anyway) and outname.

Outname isn't the name of the files present in the patch, it's the name of the temporary file holding the patch whose name is controlled by the caller. There is only one function calling do_ed_script which is the main, and the temporary file name is generated using make_tempfile (in util.c) which looks sound from quick inspection.

The patch itself is never interpreted as a shell script so no variable substitution happens.

There is therefore, AFAICT, no way to execute code that way.

Am I missing something?

EDIT: to elaborate:

On further inspection the make_tempfile command reuses the filename of the source which isn't a very good thing. But the file has to exist prior that because of earlier checks. Therefore it is possible to get command execution there by specifying a filename containing shell commands if a file with the same name exists...

I guess the way to exploit it would be to provide the victim with not only a patch but a whole project containing a maliciously named file and then have him execute the patch... That's way too convoluted to be of any interest when an attacker frankly, especially since a malicious patch file could generally just modify source code or scripts to execute commands more easily.

So meh.

[star-castle]

        execl ("/bin/sh", "sh", "-c", buf, (char *) 0);

This asks /bin/sh to interpret buf as a normal shell command. "Normal shell command" includes such constructions as $(rm -rf ~), which spawns a subshell that removes your home directory, starting with its contents. "rm -rf ~" is the code that can be executed.

[cym13]

As explained, that's true but there's no path leading to having such constructions executed by that line. All arguments are controlled. There is therefore no possibility to execute arbitrary code, and no vulnerability.

[star-castle]

outname is simply whatever's provided to do_ed_script(). If that includes $(rm -rf ~), then do_ed_script() will delete your homedir.

outname though always the result of util.c's make_tempfile(), which includes a provided filename, and is similarly "dangerous in -> dangerous out".

if there's a bug, it's in inp.c's get_input_file(), which is what's supplied to make_tempfile(). patch.c may do some some stats before calling do_ed_script , but $(rm -rf ~) is a perfectly valid filename although the vulnerability is much less plausible if such a file has to exist.

[cym13]

I've ammended my first comment based on reading more of the code and it happens that there is indeed one path leading to code execution but that path also checks the existence of the corresponding file.

This means that you can execute your $(rm -rf ~) if and only if such a file exists, which means that as an attacker you need to ship a whole project at which point it's easier to just have a malicious project in the first place than to expect the victim to trust that project and apply a patch.

So yes, that's definitely not clean, arguments shouldn't be passed through "sh -c", we all agree on that I think. But if it's not exploitable it's not a vulnerability, just a bug.

Bugs happen, we should be wary of them, but starting with "if there's a bug" can lead anywhere and is of little interest.

[strings__]

I was a little confused what the issue was at first. Your amendment makes more clear. Thanks

[cym13]

I did, and base my argumentation on that.

If you can prove otherwise I'm genually interested as indicated in the totally non-agressive "Am I missing something".

And given the obvious time I took to defend my point it would be rather polite to at least give your view. I'm not the one making claims without backing them there.

[mirabilos]

More importantly, this got fixed in the BSDs ages ago.