Run score binaries in a firejail
Closed, ResolvedPublic

Description

MediaWiki-extensions-Score is another extension that shells out to binaries that probably all should be run in firejails

		"ScoreLilyPond": "/usr/bin/lilypond",
		"ScoreAbc2Ly": "/usr/bin/abc2ly",
		"ScoreTimidity": "/usr/bin/timidity",

Forked from T171372

For lilypond, there is a --jail option for running under chroot, but I don't know if that's really feasible

Reedy created this task.Aug 5 2017, 1:40 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 5 2017, 1:40 AM
Reedy updated the task description. (Show Details)Aug 5 2017, 1:54 AM
Reedy updated the task description. (Show Details)
Ebe123 added a comment.Aug 6 2017, 1:51 AM

The only reference I've found to chroot was from Extension:Pipes, but you're right in that it doesn't seem quite feasible here.

Ebe123 moved this task from Backlog to Patch pending review on the Security board.

Check T171372: Find alternative to safe mode in Lilypond for the patches. Removing it as subtask as one is not blocked by the other; both independent solutions.

Reedy added a comment.Aug 6 2017, 2:10 AM

I think you should have left the tasks dependancies as they were on this one... As this is specifically about getting it for WMF deployment. So the chroot task makes sense as a subtask (as changes to the extension were needed. Though probably want to rename it anyway)... The other 3 tasks don't really make so much sense being parents of this one as it is hidden as a security task

Reedy closed this task as Resolved.Aug 28 2017, 9:07 PM
Reedy claimed this task.
Reedy changed the visibility from "Custom Policy" to "Public (No Login Required)".Aug 28 2017, 9:07 PM
Reedy added a comment.Aug 28 2017, 9:17 PM

And tested with score code from https://en.wikipedia.org/wiki/Symphony_No._9_(Beethoven) on a user page, including making modifications and seeing images were still generated

<score>
\new Score {
  \new Staff {
    \relative c {
      \time 4/4
      \key d \major
      \clef bass

      fis2\p( g4 a) | a4( g fis e) | d2( e4 fis) | fis4.( e8) e2 |
      fis2( g4 a) | a4( g fis e) | d2( e4 fis) | e4.( d8) d2 |
    }
  }
}
</score>

Change 370306 merged by jenkins-bot:
[mediawiki/extensions/Score@master] Add option to use -dsafe argument in Lilypond command

https://gerrit.wikimedia.org/r/370306

Reedy reopened this task as Open.Aug 28 2017, 9:31 PM

Seems ABC might be broken (per @Ebe123 on IRC)

Processing `.../file.ly'
Parsing...
.../file.ly:1:17: error: syntax error, unexpected '/', expecting '='
Reading profile 
                /etc/firejail/mediawiki-converters.profile
.../file.ly:2:1: error: undefined character or shorthand: 

]0;firejail /usr/bin/abc2ly -s .../file.abc /usr/bin/abc2ly from LilyPond 2.18.2
.../file.ly:2:1: error: not a symbol

]0;firejail /usr/bin/abc2ly -s .../file.abc /usr/bin/abc2ly from LilyPond 2.18.2
.../file.ly:2:2: error: syntax error, unexpected EVENT_IDENTIFIER, expecting '='

 ]0;firejail /usr/bin/abc2ly -s .../file.abc /usr/bin/abc2ly from LilyPond 2.18.2
.../file.ly:2:4: error: undefined character or shorthand: ;
]0
   ;firejail /usr/bin/abc2ly -s .../file.abc /usr/bin/abc2ly from LilyPond 2.18.2
.../file.ly:2:85: error: undefined character or shorthand: 
]0;firejail /usr/bin/abc2ly -s .../file.abc 
                                                                                    /usr/bin/abc2ly from LilyPond 2.18.2
.../file.ly:2:85: error: not a symbol
]0;firejail /usr/bin/abc2ly -s .../file.abc 
                                                                                    /usr/bin/abc2ly from LilyPond 2.18.2
.../file.ly:2:86: error: syntax error, unexpected '/', expecting '='
]0;firejail /usr/bin/abc2ly -s .../file.abc 
                                                                                     /usr/bin/abc2ly from LilyPond 2.18.2
.../file.ly:2:116: error: syntax error, unexpected REAL, expecting '='
]0;firejail /usr/bin/abc2ly -s .../file.abc /usr/bin/abc2ly from LilyPond 
                                                                                                                   2.18.2
.../file.ly:3:9: error: undefined character or shorthand: `
Parsing 
        `.../file.abc'...
.../file.ly:3:9: error: not a symbol
Parsing 
        `.../file.abc'...
.../file.ly:3:61: error: syntax error, unexpected '\'', expecting '='
Parsing `.../file.abc
                                                            '...
.../file.ly:4:28: error: syntax error, unexpected ':', expecting '='
Line ... lilypond output to
                           : `file.ly'...Traceback (most recent call last):
.../file.ly:4:30: error: undefined character or shorthand: `
Line ... lilypond output to: 
                             `file.ly'...Traceback (most recent call last):
.../file.ly:4:38: error: syntax error, unexpected '\'', expecting '='
Line ... lilypond output to: `file.ly
                                     '...Traceback (most recent call last):
.../file.ly:4:74: error: syntax error, unexpected EVENT_IDENTIFIER, expecting '='
Line ... lilypond output to: `file.ly'...Traceback (most recent call last
                                                                         ):
.../file.ly:5:24: error: not a symbol
  File "/usr/bin/abc2ly
                       ", line 1457, in <module>
.../file.ly:6:17: error: syntax error, unexpected EVENT_IDENTIFIER
    outf = open 
                (global_options.output, 'w')
.../file.ly:6:39: error: syntax error, unexpected ',', expecting '='
    outf = open (global_options.output
                                      , 'w')
.../file.ly:7:48: error: syntax error, unexpected '\'', expecting '='
IOError: [Errno 13] Permission denied: 'file.ly
                                               '
.../file.ly:10:24: error: syntax error, unexpected ',', expecting '='
Parent is shutting down
                       , bye...
.../file.ly:1: warning: no \version statement found, please add

\version "2.18.2"

for future compatibility
fatal error: failed files: ".../file.ly"

exited with status: 1

Change 374426 had a related patch set uploaded (by Reedy; owner: Reedy):
[operations/mediawiki-config@master] Disable firejail profile for wgScoreAbc2Ly

https://gerrit.wikimedia.org/r/374426

Change 374426 merged by jenkins-bot:
[operations/mediawiki-config@master] Disable firejail profile for wgScoreAbc2Ly

https://gerrit.wikimedia.org/r/374426

Mentioned in SAL (#wikimedia-operations) [2017-08-28T21:37:57Z] <reedy@tin> Synchronized wmf-config/CommonSettings.php: Disable wgScoreAbc2Ly firejail T172582 (duration: 00m 44s)

Seems to work outside MW though...

reedy@tin:/srv/mediawiki-staging$ cat /tmp/file.abc
X:1
T:The Legacy Jid
M:6/8
L:1/8
R:jig
K:G
GFG BAB | gfg gab | GFG BAB | d2A AFD |
GFG BAB | gfg gab | age edB |1 dBA AFD :|2 dBA ABd |:
efe edB | dBA ABd | efe edB | gdB ABd |
efe edB | d2d def | gfe edB |1 dBA ABd :|2 dBA AFD |]
reedy@tin:/srv/mediawiki-staging$ /usr/local/bin/mediawiki-firejail-abc2ly -s --output=/tmp/out /tmp/file.abc
Reading profile /etc/firejail/mediawiki-converters.profile
/usr/bin/abc2ly from LilyPond 2.18.2
Parsing `/tmp/file.abc'...
Line ... lilypond output to: `file.ly'...
Parent pid 41502, child pid 41504

Parent is shutting down, bye...
reedy@tin:/srv/mediawiki-staging$
		throw new MWException(
			var_export( $cmd, true ) . "\n" .
			var_export( $code, true ) . "\n" .
			var_export( file_get_contents( $destFile ), true ) . "\n" .
			var_export( $output, true )
		);

Gives

[WaSgTQpEE4AAADDCKgEAAAAI] /w/index.php?title=Score_abc&action=submit MWException from line 862 of /srv/mediawiki/php-master/extensions/Score/Score.body.php: '\'/usr/local/bin/mediawiki-firejail-abc2ly\' -s \'--output=/tmp/MWLP.f52419feef552c0bdec7300016a50b43/file.ly\' \'/tmp/MWLP.f52419feef552c0bdec7300016a50b43/file.abc\' 2>&1'
'
X:1
T:The Legacy Jud
M:6/8
L:1/8
R:jig
K:G
GFG BAB | gfd gab | GFG BAB | d2A AFD |
GFG BAB | gfg gab | age edB |1 dBA AFD :|2 dBA ABd |:
efe edB | dBA ABd | efe edB | gdB ABd |
efe edB | d2d def | gfe edB |1 dBA ABd :|2 dBA AFD |]
'
false
NULL

Why is $destFile null?

firejail --help | grep output
    --output=logfile - stdout logging and log rotation. Copy stdout and stderr

I guess that'll explain it. The firejail log is going to the output file, not abc2ly's output...?

Change 374439 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/Score@master] Swap --output for -o in $wgScoreAbc2Ly shell command

https://gerrit.wikimedia.org/r/374439

Change 374440 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/Score@wmf/1.30.0-wmf.15] Swap --output for -o in $wgScoreAbc2Ly shell command

https://gerrit.wikimedia.org/r/374440

Change 374439 merged by jenkins-bot:
[mediawiki/extensions/Score@master] Swap --output for -o in $wgScoreAbc2Ly shell command

https://gerrit.wikimedia.org/r/374439

Change 374440 merged by jenkins-bot:
[mediawiki/extensions/Score@wmf/1.30.0-wmf.15] Swap --output for -o in $wgScoreAbc2Ly shell command

https://gerrit.wikimedia.org/r/374440

Reedy closed this task as Resolved.Aug 28 2017, 11:38 PM

(Y)

Reedy added a parent task: Restricted Task.Aug 28 2017, 11:39 PM