#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]

author  David Kastrup <dak@gnu.org> 
Tue, 28 Nov 2017 11:18:07 +0000 (12:18 +0100)
committer   David Kastrup <dak@gnu.org> 
Thu, 25 Jan 2018 11:25:41 +0000 (12:25 +0100)
commit  807f5eb8cd631133da3be6897e3e8fa7202e089d

author  David Kastrup <dak@gnu.org> 
Tue, 28 Nov 2017 11:19:02 +0000 (12:19 +0100)
committer   David Kastrup <dak@gnu.org> 
Thu, 25 Jan 2018 11:25:48 +0000 (12:25 +0100)
commit  39f800a7e5acb7cc5da6424c99fd2690e389495a

author  David Kastrup <dak@gnu.org> 
Tue, 28 Nov 2017 11:19:30 +0000 (12:19 +0100)
committer   David Kastrup <dak@gnu.org> 
Thu, 25 Jan 2018 11:25:53 +0000 (12:25 +0100)
commit  aee02594be68a968bb843f87d3264777099e46b4

[Gabriel Corona]

AFAIU, the fix of lilypond-invoke-editor is not merged. I still have this:

(define (run-browser uri)
  (system
   (if (getenv "BROWSER")
       (format #f "~a ~a" (getenv "BROWSER") uri)
       (format #f "firefox -remote 'OpenURL(~a,new-tab)'" uri))))

[David Kastrup]

The fix is merged. It just does not involve run-browser. This would likely warrant additional shell-quoting here or possibly just removing the run-browser functionality if we see no clean way to make this work.

[Gabriel Corona]

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.

AFAIU, this is not completely true. It does handle other URIs. If there's no intent to fix the command injection vulnerability in lilypond-invoke-editor, run-browser and the (run-browser ...) branch in main should be removed altogether.

Another solution would be to (shell-quote-argument uri) in run-browser (though I'd be more confident with using system on non-Windows).

[Don Armstrong]

I have just uploaded a fix to Debian which switches to using system* instead of system:
https://salsa.debian.org/debian/lilypond/commit/788b56e4b7f62637481af65b4b2929649c30fe78

Not sure if this is cross-platform enough, but it solves the issue for systems with a working system* call.

[Anonymous]

Don, as this issue was already closed, I have thake your patch, rebased it to current master and attached it to a new issue - https://sourceforge.net/p/testlilyissues/issues/5334/

The developer who had committed the patch for this tracker had asked why the fix (on this issue) was not enough. Would you mind commenting in that new tracker listed?

Thank you for your time.

James

[Don Armstrong]

On June 2, 2018 11:06:09 AM PDT, pkx166h lilypond-pkx@users.sourceforge.net wrote:

Don, as this issue was already closed, I have thake your patch, rebased
it to current master and attached it to a new issue -
https://sourceforge.net/p/testlilyissues/issues/5334/

The developer who had committed the patch for this tracker had asked
why the fix (on this issue) was not enough. Would you mind commenting
in that new tracker listed?

The fix in the tracker looks good to me too. So long as system* is being used, everything should be ok.

I wasn't aware of that patch, and I didn't see it fixed in git, so I went ahead and fixed it with the least invasive method.

This is not a signature.

[Anonymous]

Hey Don,

The original fix for this issue was in three commits

author David Kastrup dak@gnu.org
Tue, 28 Nov 2017 11:18:07 +0000 (12:18 +0100)
committer David Kastrup dak@gnu.org
Thu, 25 Jan 2018 11:25:41 +0000 (12:25 +0100)
commit 807f5eb8cd631133da3be6897e3e8fa7202e089d

http://git.savannah.gnu.org/gitweb/?p=lilypond.git;a=commit;h=807f5eb8cd631133da3be6897e3e8fa7202e089d

author David Kastrup dak@gnu.org
Tue, 28 Nov 2017 11:19:02 +0000 (12:19 +0100)
committer David Kastrup dak@gnu.org
Thu, 25 Jan 2018 11:25:48 +0000 (12:25 +0100)
commit 39f800a7e5acb7cc5da6424c99fd2690e389495a

http://git.savannah.gnu.org/gitweb/?p=lilypond.git;a=commit;h=39f800a7e5acb7cc5da6424c99fd2690e389495a

author David Kastrup dak@gnu.org
Tue, 28 Nov 2017 11:19:30 +0000 (12:19 +0100)
committer David Kastrup dak@gnu.org
Thu, 25 Jan 2018 11:25:53 +0000 (12:25 +0100)
commit aee02594be68a968bb843f87d3264777099e46b4

http://git.savannah.gnu.org/gitweb/?p=lilypond.git;a=commit;h=aee02594be68a968bb843f87d3264777099e46b4

Would you mind taking a look and seeing if this is good enough or if not how your patch would improve what was already done.

Thanks again