Menu
▾
▴
1 Attachments
#5334 Use system* instead of system when invoking browser
Don Armstrong - 2018-05-11
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.
From Don:
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.
I sent Don link to the three commits for comment.
This actually concerns a different problem than the one fixed in the referenced patch: when an uri is given to
lilypond-invoke-editor.scmthat isn't actually a textedit URI, the program tries calling a web browser instead. Both manners of trying to do that (depending on whether BROWSER is defined or not) look seriously broken. It may be saner to just output an error message and exit with a non-zero exit code.Is there any expectation of
lilypond-invoke-editor.scmever getting called with anything except a textedit URI?I think that lilypond-invoke-editor only should only handle textedit URIs. It might be a good idea to have a 2nd look at the patch I suggested in 2017.
https://codereview.appspot.com/336240043
https://sourceforge.net/p/testlilyissues/issues/5243/
On top of current master
git revert aee02594be68a968bb843f87d3264777099e46b4
git revert 39f800a7e5acb7cc5da6424c99fd2690e389495a
git revert 807f5eb8cd631133da3be6897e3e8fa7202e089d
wget https://codereview.appspot.com/download/issue336240043_60001.diff
would be needed to for a test build.
In 2017 one objection was that my patch does not change the code in lily.scm ... do you we really need to change that code? I don't see a problem as the code is executed by lilypond, we give the arguments. But maybe I don't have the imagination to see a security hole ...
Having taken a look at the Usage Guide, it would appear that at last in connection with xpdf use, the recommended use would pass all URIs through lilypond-invoke-editor. I have problems finding a reasonable spec for the BROWSER environment variable and the mozilla fallback looks like it would require a nesting of shell and JavaScript(?) quoting, a definite "ugh".
See The Secure BROWSER Specification for some analysis on how the BROWSER variable could/should work.
The BROWSER variable is not really specified and at least 3 different behaviors exist:
%s-expansion.Some programs some don't expand the program in several argument, some do expand the program in different arguments based on spaces, some pass the result to system (alowing shell commands in the BROWSER variable).
In contract, the .desktop spec clearly defines how the string should be split in different arguments.
The Firefox
-remote OpenURL(...)in many different programs is a remain from a long past. I doesn't work on recent versions of Firefox (and I think it has not been working for quite a few years).If you checkout on aee02594be68a968bb843f87d3264777099e46b4 you still have this vulnerable code:
(define (run-browser uri) (system (if (getenv "BROWSER") (format #f "~a ~a" (getenv "BROWSER") uri) (format #f "firefox -remote 'OpenURL(~a,new-tab)'" uri))))I am not sure what needs to be tested now. Do I go with Don's patch or not?
From Gabriel's comment above it seems that this patch(es) need more work.
The patch is ok. It eliminates an obvious and easily exploitable security problem.
Knut, as I was getting confused and that Don's patch - which this tracker was for - was 'a different issue' (I inferred from David K) I have created https://sourceforge.net/p/testlilyissues/issues/5342/ for your re-instatement, so to speak, of your suggestion back in 2017 and have moved all the relevant information there.
Don's patch (attached at the start of this issue) passes make, make check and a full make doc.
It's easy to get confused in this matter.
On 11/15 2017 Gabriel reported the BROWSER bug, see http://lists.gnu.org/archive/html/bug-lilypond/2017-11/msg00024.html.
Eight days later I opend issue 5243 and proposed a patch to fix the BROWSER bug and a 2nd security problem related to TEXTEDIT links. My proposed solution was to fix the TEXTEDIT code and to completely kill the vulnerable BROWSER code.
Later David proposed an alternative patch in the same issue 5243, that patch was choosen to be integrated into lilypond master. Maybe that patch was the better solution for the TEXTEDIT problem, but David's patch did nothing to fix the BROWSER bug.
Now Don Armstrong reminds us with his patch that the BROWSER bug is still present and proposes a valid solution of that security problem.
Does 'firefox --remote URL' still work? I don't know, I don't care. I'd remove the code, but I probably will not complain if it survives another decade. Maybe someone will propose a patch to adapt the BROWSER related code to our modern software environments.
David's TEXTEDIT code is already in master, apply Don's patch and both security holes are closed in that branch.
Probably the TEXTEDT and BROWSER patches should also be part of a security-fix-release 2.18.3.
Knut, thanks. Do we still need https://sourceforge.net/p/testlilyissues/issues/5342/ ?
(Don's) Patch on countdown for June 13.
No
Patch counted down - I'll push.
author Don Armstrong <don@donarmstrong.com> Sat, 2 Jun 2018 18:56:52 +0100 (18:56 +0100) committer James Lowe <pkx166h@runbox.com> Sat, 16 Jun 2018 11:05:02 +0100 (11:05 +0100) commit d493d5f4bc8c87afdf44f42e5cc5f421f8d0545dThank you Don.