From be43330208342c62b26105076e9d6765ff820a41 Mon Sep 17 00:00:00 2001 From: James Allen Date: Mon, 9 May 2016 11:37:35 +0100 Subject: [PATCH] Allow convert command to be prefixed by security commands --- .../filestore/app/coffee/FileConverter.coffee | 16 ++++++++++------ services/filestore/app/coffee/SafeExec.coffee | 2 +- .../filestore/config/settings.defaults.coffee | 4 ++++ .../unit/coffee/FileConverterTests.coffee | 19 +++++++++++++++---- .../test/unit/coffee/SafeExec.coffee | 8 ++++---- 5 files changed, 34 insertions(+), 15 deletions(-) diff --git a/services/filestore/app/coffee/FileConverter.coffee b/services/filestore/app/coffee/FileConverter.coffee index aaf56388dd..429fa7fccb 100644 --- a/services/filestore/app/coffee/FileConverter.coffee +++ b/services/filestore/app/coffee/FileConverter.coffee @@ -3,6 +3,7 @@ metrics = require("metrics-sharelatex") logger = require("logger-sharelatex") safe_exec = require("./SafeExec") approvedFormats = ["png"] +Settings = require "settings-sharelatex" fourtySeconds = 40 * 1000 @@ -22,8 +23,9 @@ module.exports = err = new Error("invalid format requested") return callback err width = "600x" - args = "nice convert -define pdf:fit-page=#{width} -flatten -density 300 #{sourcePath} #{destPath}" - safe_exec args, childProcessOpts, (err, stdout, stderr)-> + command = ["convert", "-define", "pdf:fit-page=#{width}", "-flatten", "-density", "300", sourcePath, destPath] + command = Settings.commands.convertCommandPrefix.concat(command) + safe_exec command, childProcessOpts, (err, stdout, stderr)-> timer.done() if err? logger.err err:err, stderr:stderr, sourcePath:sourcePath, requestedFormat:requestedFormat, destPath:destPath, "something went wrong converting file" @@ -36,8 +38,9 @@ module.exports = destPath = "#{sourcePath}.png" sourcePath = "#{sourcePath}[0]" width = "260x" - args = "nice convert -flatten -background white -density 300 -define pdf:fit-page=#{width} #{sourcePath} -resize #{width} #{destPath}" - safe_exec args, childProcessOpts, (err, stdout, stderr)-> + command = ["convert", "-flatten", "-background", "white", "-density", "300", "-define", "pdf:fit-page=#{width}", sourcePath, "-resize", width, destPath] + command = Settings.commands.convertCommandPrefix.concat(command) + safe_exec command, childProcessOpts, (err, stdout, stderr)-> if err? logger.err err:err, stderr:stderr, sourcePath:sourcePath, "something went wrong converting file to thumbnail" else @@ -49,8 +52,9 @@ module.exports = destPath = "#{sourcePath}.png" sourcePath = "#{sourcePath}[0]" width = "548x" - args = "nice convert -flatten -background white -density 300 -define pdf:fit-page=#{width} #{sourcePath} -resize #{width} #{destPath}" - safe_exec args, childProcessOpts, (err, stdout, stderr)-> + command = ["convert", "-flatten", "-background", "white", "-density", "300", "-define", "pdf:fit-page=#{width}", sourcePath, "-resize", width, destPath] + command = Settings.commands.convertCommandPrefix.concat(command) + safe_exec command, childProcessOpts, (err, stdout, stderr)-> if err? logger.err err:err, stderr:stderr, sourcePath:sourcePath, destPath:destPath, "something went wrong converting file to preview" else diff --git a/services/filestore/app/coffee/SafeExec.coffee b/services/filestore/app/coffee/SafeExec.coffee index 217aab4748..aa8121a360 100644 --- a/services/filestore/app/coffee/SafeExec.coffee +++ b/services/filestore/app/coffee/SafeExec.coffee @@ -10,7 +10,7 @@ child_process = require('child_process') module.exports = (command, options, callback = (err, stdout, stderr) ->) -> # options are {timeout: number-of-milliseconds, killSignal: signal-name} - [cmd, args...] = command.split(' ') + [cmd, args...] = command child = child_process.spawn cmd, args, {detached:true} stdout = "" diff --git a/services/filestore/config/settings.defaults.coffee b/services/filestore/config/settings.defaults.coffee index f84928598e..bb4a885478 100644 --- a/services/filestore/config/settings.defaults.coffee +++ b/services/filestore/config/settings.defaults.coffee @@ -30,6 +30,10 @@ module.exports = path: uploadFolder: Path.resolve(__dirname + "/../uploads") + + commands: + # Any commands to wrap the convert utility in, for example ["nice"], or ["firejail", "--profile=/etc/firejail/convert.profile"] + convertCommandPrefix: [] # Filestore health check # ---------------------- diff --git a/services/filestore/test/unit/coffee/FileConverterTests.coffee b/services/filestore/test/unit/coffee/FileConverterTests.coffee index f8a8add22f..bdb908be98 100644 --- a/services/filestore/test/unit/coffee/FileConverterTests.coffee +++ b/services/filestore/test/unit/coffee/FileConverterTests.coffee @@ -16,6 +16,9 @@ describe "FileConverter", -> "logger-sharelatex": log:-> err:-> + "settings-sharelatex": @Settings = + commands: + convertCommandPrefix: [] @sourcePath = "/this/path/here.eps" @format = "png" @@ -27,8 +30,8 @@ describe "FileConverter", -> @safe_exec.callsArgWith(2) @converter.convert @sourcePath, @format, (err)=> args = @safe_exec.args[0][0] - args.indexOf(@sourcePath).should.not.equal -1 - args.indexOf(@format).should.not.equal -1 + args.indexOf("#{@sourcePath}[0]").should.not.equal -1 + args.indexOf("#{@sourcePath}.#{@format}").should.not.equal -1 done() it "should return the dest path", (done)-> @@ -48,13 +51,21 @@ describe "FileConverter", -> @converter.convert @sourcePath, "ahhhhh", (err)=> expect(err).to.exist done() + + it "should prefix the command with Settings.commands.convertCommandPrefix", (done) -> + @safe_exec.callsArgWith(2) + @Settings.commands.convertCommandPrefix = ["nice"] + @converter.convert @sourcePath, @format, (err)=> + command = @safe_exec.args[0][0] + command[0].should.equal "nice" + done() describe "thumbnail", -> it "should call converter resize with args", (done)-> @safe_exec.callsArgWith(2) @converter.thumbnail @sourcePath, (err)=> args = @safe_exec.args[0][0] - args.indexOf(@sourcePath).should.not.equal -1 + args.indexOf("#{@sourcePath}[0]").should.not.equal -1 done() describe "preview", -> @@ -62,5 +73,5 @@ describe "FileConverter", -> @safe_exec.callsArgWith(2) @converter.preview @sourcePath, (err)=> args = @safe_exec.args[0][0] - args.indexOf(@sourcePath).should.not.equal -1 + args.indexOf("#{@sourcePath}[0]").should.not.equal -1 done() diff --git a/services/filestore/test/unit/coffee/SafeExec.coffee b/services/filestore/test/unit/coffee/SafeExec.coffee index b63851aa57..10d920df11 100644 --- a/services/filestore/test/unit/coffee/SafeExec.coffee +++ b/services/filestore/test/unit/coffee/SafeExec.coffee @@ -19,24 +19,24 @@ describe "SafeExec", -> describe "safe_exec", -> it "should execute a valid command", (done) -> - @safe_exec "/bin/echo hello", @options, (err, stdout, stderr) => + @safe_exec ["/bin/echo", "hello"], @options, (err, stdout, stderr) => stdout.should.equal "hello\n" should.not.exist(err) done() it "should execute a command with non-zero exit status", (done) -> - @safe_exec "/usr/bin/env false", @options, (err, stdout, stderr) => + @safe_exec ["/usr/bin/env", "false"], @options, (err, stdout, stderr) => stdout.should.equal "" stderr.should.equal "" err.message.should.equal "exit status 1" done() it "should handle an invalid command", (done) -> - @safe_exec "/bin/foobar", @options, (err, stdout, stderr) => + @safe_exec ["/bin/foobar"], @options, (err, stdout, stderr) => err.code.should.equal "ENOENT" done() it "should handle a command that runs too long", (done) -> - @safe_exec "/bin/sleep 10", {timeout: 500, killSignal: "SIGTERM"}, (err, stdout, stderr) => + @safe_exec ["/bin/sleep", "10"], {timeout: 500, killSignal: "SIGTERM"}, (err, stdout, stderr) => err.should.equal "SIGTERM" done()