#5243 Fix security problem in lilypond-invoke-editor

[Knut Petersen]

David Kastrup - 22 hours ago

More conservative parsing of textedit URIs

Also contains commits:

Let get-editor use shell-quote-argument

Addresses security concerns.

(editor scm): Add shell-quote-argument function

This is mostly stolen from Emacs.

I have no idea how to properly test this or whether it runs at all.

http://codereview.appspot.com/336450043

Initial issue for this Tracker (replace by the info above):
Fix security problem in lilypond-invoke-editor

If lilypond-invoke-editor was installed as a general
uri-helper it was easy to abuse it to execute arbitrary
code on an attacked system for non-textedit URIs.
This part of the problem was discovered and reported
to our bug-lilypond mailing list by Gabriel Corona.

But also pure textedit URIs were vulnerable, an
example is the URI

textedit:///:&xterm -e find ~/&:x:

that executes "find ~/" in a xterm.

With this patch lilypond-invoke-editor only
handles textedit URIs, and it does no longer
use the systems command processor but
guiles system* procedure for those URIs.

Also the script will abort if the line, char and
column fields of a textedit URI contain anything
but digits.

We could have fixed URI passing to the browser,
but it is not our job to provide a general URI helper.
Other software (e.g. xdg-open and friends) should
be used for that.

The security problem fixed now was introduced
into lilypond in the year 2005.

Signed-off-by: Knut Petersen knut_petersen@t-online.de

http://codereview.appspot.com/336240043

[Anonymous]

Passes make, make check and a full make doc.

[Knut Petersen]

Also textedit links are vulnerable, attempt to fix this

http://codereview.appspot.com/336240043

[Knut Petersen]

Fixing a stupid mistake

http://codereview.appspot.com/336240043

[Knut Petersen]

Fix that stupid error in line 1, whitespace cleanup

http://codereview.appspot.com/336240043

[Anonymous]

This now fails make - did you rebase?

[Knut Petersen]

You probably tested patch set #3 that contained a mistake. Set #4 builds fine here.

[David Kastrup]

Ok, I am attaching a prospective diff here. Why not make it a full review? I am not sure that this is the way to go ahead. This is as to yet untested: more likely than not it does not compile/run/work. It's more of a discussion basis than anything else.

The advantage of my approach is that it's API compatible and merely relies on an additional shell-quote-argument. The disadvantage is that this shell-quote-argument, basically rewritten from the equivalent Emacs code, is an unholy mess. It should really be "somebody else's problem". Using system* would make it such.

However, Knut's use of system* changes the (scm editor) APIs or rather ignores them and leaves them insecure, and there are uses in lily.scm as well.

So maybe we should change this to a system*-serving API? One way to do that would be to split into strings before replacing %(...)s style arguments, but that would mean that the command line stored in environment variables needs to get split with some method making use of the operating system conventions, and whether we mimic OS conventions when splitting or when quoting shell arguments does not really change the difficulty of the problem.

And if we find a problem in Guile 1.8.8's implementation of system*, we have a problem indeed.

Probably the safest method to make lilypond-invoke-editor's quoting "somebody else's problem" would be to rewrite in Python: this is the only Guile script I think, and Python has a maintained shell quote function in the pipes module.

But that would not help with lily.scm's use of (scm editor). I'm not even sure whether this use is dead code anyway. But if it isn't, biting the bullet and providing shell-quote-argument might actually be the best bet after all.

[Knut Petersen]

We also have to inspect every other use of scm_system, e.g. backend_library.scm .If someone offers to run lilypond on a server, a similar attack might be (probably is) possible. I think there are characters allowed in filenames that have special meaning to a number of shells. Even if suspcious filenames are filtered: bookOutputSuffix might help.

No, I would not rewrite the script in python.

Your "unholy mess" might be a good idea, but I don't have access to a windows system or a mac. It really might be a good idea if we need it in other parts of lilypond.

Usage of scm_system_star has one big advantage: It is simple and it is available on all supported systems. If there really would be a problem with scm_system_star we simply would fix libguile/simpos.c. The procedure is nothing but a short and simple interface to the well tested functions of the standard c library.

[Knut Petersen]

We also have to inspect every other use of scm_system, e.g. backend_library.scm .If someone offers to run lilypond on a server, a similar attack might be (probably is) possible.

Converting to pdf looks pretty save to me.

We do filter characters in \bookOutputSuffix, but something like

\version "2.21.0"
\book {
    #(set! (paper-variable #f 'output-filename) 
    "/home/knut/\"&xterm -e find ~&" ) {c}
}

is allowed. But converting to pdf does use ly:system, and that in the end translates to a call of glibs g_spawn_sync that uses execve. So we really produce the file /home/knut/"&xterm -e find ~&.pdf here and do not execute anything but ghostscript.

[David Kastrup]

"Knut Petersen" knupero@users.sf.net writes:

We also have to inspect every other use of scm_system,
e.g. backend_library.scm .If someone offers to run lilypond on a
server, a similar attack might be (probably is) possible.

Converting to pdf looks pretty save to me.

We do filter characters in \bookOutputSuffix, but something like

~~~
\version "2.21.0"
\book {
#(set! (paper-variable #f 'output-filename)
"/home/knut/\"&xterm -e find ~&" ) {c}
}
~~~

is allowed. But converting to pdf does use ly:system, and that in the
end translates to a call of glibs g_spawn_sync that uses execve. So we
really produce the file /home/knut/"&xterm -e find ~&.pdf here and do
not execute anything but ghostscript.

Assuming that exec works fine enough on Windows for this (I think
Windows has to direct everything through a single-string command line).
But if it doesn't, we can call it "somebody else's problem" I presume.
Likely that of our cross compilation C standard library.

--
David Kastrup

[Knut Petersen]

Yes, someone should verify that on windows/mac systems.

It's a bit OT here, but thinking about general security in lilypond I ask myself how many people would try to compile a lilypond source file that contains something like #(ly:system-with-shell "some_unexpected_command").

There is a safe mode, and we support a chroot jail, but does John Doe know and care about that?

xxxTeX has the -[no]-shell-escape options ...

[David Kastrup]

More conservative parsing of textedit URIs

Also contains commits:

Let get-editor use shell-quote-argument

Addresses security concerns.

(editor scm): Add shell-quote-argument function

This is mostly stolen from Emacs.

I have no idea how to properly test this or whether it runs at all.

http://codereview.appspot.com/336450043

[Anonymous]

Passes make, make check and a full make doc.

Just to be clear, I am testing David's patch.

[Anonymous]

Patch on countdown for Jan 19th

[Anonymous]

Patch counted down - please push

[Anonymous]

Patch counted down - please push

[Anonymous]

Patch counted down - please push