Skip to content
Browse files

Merge pull request #1 from thallgren/bug/master/nodejs-security

Bug/master/nodejs security
  • Loading branch information...
2 parents d9b7bf0 + 56f63aa commit beaafb75d4a01692545def0ff5526bdd807a7370 @daniel-pittman daniel-pittman committed Feb 6, 2013
Showing with 360 additions and 110 deletions.
  1. +120 −82 bin/api.js
  2. +61 −0 bin/common.js
  3. +31 −12 bin/image_svc.js
  4. +46 −16 lib/project_razor/cli.rb
  5. +102 −0 spec/nodejs-api/rz_injection_prevention_spec.rb
View
202 bin/api.js
@@ -1,9 +1,13 @@
// Node.js endpoint for ProjectRazor API
-var razor_bin = __dirname+ "/razor -w"; // Set project_razor.rb path
+var razor_bin = __dirname+ "/razor"; // Set project_razor.rb path
console.log(razor_bin);
-var exec = require("child_process").exec; // create our exec object
+var execFile = require("child_process").execFile; // create our execFile object
var express = require('express'); // include our express libs
+var common = require('./common.js');
+var InvalidURIPathError = common.InvalidURIPathError;
+var urlDecode = common.urlDecode;
+var returnError = common.returnError;
app = express.createServer(); // our express server
app.use(express.bodyParser()); // Enable body parsing for POST
@@ -13,113 +17,147 @@ app.use(express.bodyParser()); // Enable body parsing for POST
// Exception for boot API request
app.get('/razor/api/boot*',
function(req, res) {
- args = req.path.split("/");
- args.splice(0,3);
- var args_string = getArguments(args);
- if (args.length < 2) {
- args_string = args_string + "default "
+ try {
+ args = getRequestArgs(req);
+ if (args.length < 3)
+ args.push('default');
+ args.push(JSON.stringify(req.query));
+ console.log(razor_bin + getArguments(args));
+ execFile(razor_bin, args, function (err, stdout, stderr) {
+ if(err instanceof Error)
+ returnError(res, err);
+ else
+ res.send(stdout, 200, {"Content-Type": "text/plain"});
+ });
+ } catch(e) {
+ returnError(res, e);
}
- query_param = "'" + JSON.stringify(req.query) + "'";
- console.log(razor_bin + args_string + query_param);
- //process.stdout.write('\033[2J\033[0;0H'); - screen clearing trick
- exec(razor_bin + args_string + query_param, function (err, stdout, stderr) {
- res.send(stdout, 200, {"Content-Type": "text/plain"});
- });
- });
+ });
app.get('/razor/api/*',
- // TODO - need to decode vars
-
function(req, res) {
- console.log("GET called");
- args = req.path.split("/");
- args.splice(0,3);
- var args_string = getArguments(args);
- if (args.length < 2) {
- args_string = args_string + "default "
+ console.log("GET " + req.path);
+ try {
+ args = getRequestArgs(req);
+ if (args.length < 3)
+ args.push('default');
+
+ args.push(JSON.stringify(req.query));
+ console.log(razor_bin + getArguments(args));
+ execFile(razor_bin, args, function (err, stdout, stderr) {
+ if(err instanceof Error)
+ returnError(res, err);
+ else
+ returnResult(res, stdout);
+ });
+ } catch(e) {
+ returnError(res, e);
}
- query_param = "'" + JSON.stringify(req.query) + "'";
- console.log(razor_bin + args_string + query_param);
- exec(razor_bin + args_string + query_param, function (err, stdout, stderr) {
- returnResult(res, stdout);
- });
});
-
-
app.post('/razor/api/*',
function(req, res) {
- console.log("POST called");
- args = req.path.split("/");
- args.splice(0,3);
- if (command_included(args, "add") == undefined &&
- command_included(args, "checkin") == undefined &&
- command_included(args, "register") == undefined) {
- args.push("add");
+ console.log("POST " + req.path);
+ try {
+ args = getRequestArgs(req);
+ if (!(command_included(args, "add") || command_included(args, "checkin") || command_included(args, "register"))) {
+ args.push("add");
+ }
+ args.push(req.param('json_hash', null));
+ //process.stdout.write('\033[2J\033[0;0H');
+ console.log(razor_bin + getArguments(args));
+ execFile(razor_bin, args, function (err, stdout, stderr) {
+ if(err instanceof Error)
+ returnError(res, err);
+ else
+ returnResult(res, stdout);
+ });
+ } catch(e) {
+ returnError(res, e);
}
- var json_data = "'" + req.param('json_hash', null) + "'";
- var args_string = getArguments(args);
- //process.stdout.write('\033[2J\033[0;0H');
- console.log(args_string);
- console.log(razor_bin + args_string + json_data);
- exec(razor_bin + args_string + json_data, function (err, stdout, stderr) {
- returnResult(res, stdout);
- });
});
app.put('/razor/api/*',
function(req, res) {
- console.log("PUT called");
- args = req.path.split("/");
- args.splice(0,3);
- if (command_included(args, "update") == undefined) {
- args.splice(-1,0,"update");
+ console.log("PUT " + req.path);
+ try {
+ args = getRequestArgs(req);
+ if (!command_included(args, "update")) {
+ args.splice(-1, 0, "update");
+ }
+ args.push(req.param('json_hash', null));
+ console.log(razor_bin + getArguments(args));
+ execFile(razor_bin, args, function (err, stdout, stderr) {
+ if(err instanceof Error)
+ returnError(res, err);
+ else
+ returnResult(res, stdout);
+ });
+ } catch(msg) {
+ returnError(res, e);
}
- var json_data = "'" + req.param('json_hash', null) + "'";
- var args_string = getArguments(args);
- console.log(args_string);
- console.log(razor_bin + args_string + json_data);
- exec(razor_bin + args_string + json_data, function (err, stdout, stderr) {
- returnResult(res, stdout);
- });
});
app.delete('/razor/api/*',
function(req, res) {
- console.log("DELETE called");
- args = req.path.split("/");
- args.splice(0,3);
- if (command_included(args, "remove") == undefined) {
- args.splice(-1,0,"remove");
+ console.log("DELETE " + req.path);
+ try {
+ args = getRequestArgs(req);
+ if (!command_included(args, "remove")) {
+ args.splice(-1, 0, "remove");
+ }
+ console.log(razor_bin + getArguments(args));
+ execFile(razor_bin, args, function (err, stdout, stderr) {
+ if(err instanceof Error)
+ returnError(res, err);
+ else
+ returnResult(res, stdout);
+ });
+ } catch(msg) {
+ returnError(res, e);
}
- var json_data = '{}';
- var args_string = getArguments(args);
- console.log(args_string);
- console.log(razor_bin + args_string);
- exec(razor_bin + args_string, function (err, stdout, stderr) {
- returnResult(res, stdout);
- });
});
app.get('/*',
function(req, res) {
switch(req.path)
{
- case "/":
- res.send('404 Error: Bad Request', 404);
- break;
case "/razor":
- res.send('404 Error: Bad Request(No module selected)', 404);
+ res.send('Bad Request(No module selected)', 404);
break;
case "/razor/api":
- res.send('404 Error: Bad Request(No slice selected)', 404);
+ res.send('Bad Request(No slice selected)', 404);
break;
default:
- res.send('404 Error: Bad Request', 404);
+ res.send('Bad Request', 404);
}
});
+/**
+ * Assembles an array of argument, starting with the string '-w' and then
+ * followed by URI decoded path elements from the request path. The first
+ * two path elements are skipped.
+ *
+ * @param req The Express Request object
+ * @returns An array of arguments
+ * @throws An 'Illegal path component' if some path element is considered unsafe
+ */
+function getRequestArgs(req) {
+ args = req.path.split("/");
+ args.splice(0,3);
+ if(args.length > 0) {
+ if(args[args.length-1] == '')
+ // Path ended with slash. Just skip this one
+ args.pop();
+
+ for(var i = 0; i < args.length; ++i)
+ args[i] = urlDecode(args[i]);
+ }
+ args.unshift('-w');
+ return args;
+}
+
function returnResult(res, json_string) {
var return_obj;
var http_err_code;
@@ -132,30 +170,31 @@ function returnResult(res, json_string) {
}
catch(err)
{
- // Parsing Error | Razor sent us something wrong - we just assume output
- res.send(json_string, 200, {"Content-Type": "application/json"});
+ // Apparently not JSON and should be sent as plain text.
+ // TODO: This approach is bad. We should know what to do with
+ // the json_string, not guess. What if the response can be parsed but
+ // still isn't JSON?
+ res.send(json_string, 200, {'Content-Type': 'text/plain'});
}
}
function getArguments(args) {
var arg_string = " ";
for (x = 0; x < args.length; x++) {
- arg_string = arg_string + args[x] + " "
+ arg_string = arg_string + "'" + args[x] + "' "
}
return arg_string;
}
function getConfig() {
- exec(razor_bin + " config read", function (err, stdout, stderr) {
+ execFile(razor_bin, ['-j', 'config', 'read'], function (err, stdout, stderr) {
console.log(stdout);
startServer(stdout);
});
}
function command_included(arr, obj) {
- for(var i=0; i<arr.length; i++) {
- if (arr[i] == obj) return true;
- }
+ return arr.indexOf(obj) >= 0;
}
// TODO Add catch for if project_razor.js is already running on port
@@ -172,4 +211,3 @@ function startServer(json_config) {
getConfig();
-
View
61 bin/common.js
@@ -0,0 +1,61 @@
+var util = require('util');
+
+var safeArgPattern = /^[^\x00-\x1f&\|]+$/;
+var safeArgMaxLength = 200;
+
+/**
+ * Abstract error class. Needed to get the constructor stuff right. Can also
+ * be used for generic trap of all sub-classes.
+ */
+var AbstractError = function(msg, constructor) {
+ Error.captureStackTrace(this, constructor);
+ this.message = msg;
+}
+util.inherits(AbstractError, Error);
+
+/**
+ * Error thrown when we receive URI paths that doesn't pass our validation checks.
+ */
+var InvalidURIPathError = function(msg) {
+ InvalidURIPathError.super_.call(this, msg, this.constructor);
+}
+util.inherits(InvalidURIPathError, AbstractError);
+
+/**
+ * Perform URI decoding on the given argument and assert that it
+ * matches the regular expression that defines what we consider safe
+ * @param arg The argument to decode and check
+ * @returns The decoded arg
+ * @throws An 'Illegal path component' if the arg is considered unsafe
+ */
+exports.urlDecode = function(arg) {
+ arg = decodeURIComponent(arg);
+ if(!(arg.length <= safeArgMaxLength && arg.indexOf('..') < 0 && safeArgPattern.test(arg)))
+ throw new InvalidURIPathError("Illegal path component: '" + arg + "'");
+ return arg;
+}
+
+/**
+ * Respond with an error code based on a trapped exception.
+ * @param res The response object
+ * @param e The error object
+ */
+exports.returnError = function(res, e) {
+ if(e === undefined || e == null) {
+ console.error('Error 500: Exception with no furter info');
+ res.send('Internal error', 500);
+ } else if(e instanceof InvalidURIPathError || e instanceof URIError) {
+ console.error('Error 404: ' + e.message);
+ res.send(e.message, 404);
+ } else if(e instanceof Error){
+ console.error('Error 500: ' + e.message);
+ res.send(e.message, 500);
+ } else {
+ // Assume that e can be converted into a string
+ console.error('Error 500: ' + e);
+ res.send('' + e, 500);
+ }
+}
+
+exports.AbstractError = AbstractError;
+exports.InvalidURIPathError = InvalidURIPathError;
View
43 bin/image_svc.js
@@ -1,22 +1,41 @@
// Node.js Endpoint for ProjectRazor Image Service
-var razor_bin = __dirname+ "/razor -w"; // Set project_razor.rb path
-var exec = require("child_process").exec; // create our exec object
+var razor_bin = __dirname + "/razor"; // Set project_razor.rb path
+var execFile = require("child_process").execFile; // create our execFile object
var express = require('express'); // include our express libs
var mime = require('mime');
var fs = require('fs');
+var path = require('path');
var http_range_req = require('./http_range_req.js');
+var common = require('./common.js');
+var InvalidURIPathError = common.InvalidURIPathError;
+var urlDecode = common.urlDecode;
+var returnError = common.returnError;
var image_svc_path;
app = express.createServer(); // our express server
-app.get('/razor/image/*',
- function(req, res) {
- path = decodeURIComponent(req.path.replace(/^\/razor\/image/, image_svc_path));
- console.log(path);
- respondWithFile(path, res, req);
- });
-
+app.get('/razor/image/*', function(req, res) {
+ try {
+ var args = req.path.split('/');
+ args.splice(0, 3);
+ if (args.length > 0) {
+ if (args[args.length - 1] == '')
+ // Path ended with slash. Just skip this one
+ args.pop();
+ for ( var i = 0; i < args.length; ++i)
+ args[i] = urlDecode(args[i]);
+ }
+ var absPath = path.resolve(image_svc_path + args.join('/'));
+ if(absPath.indexOf(image_svc_path) != 0)
+ throw new InvalidURIPathError("Illegal path: '" + path + "'");
+
+ console.log("Image requested: '" + absPath + "'");
+ respondWithFile(absPath, res, req);
+ } catch (e) {
+ returnError(res, e);
+ }
+});
function respondWithFile(path, res, req) {
if (path != null) {
@@ -64,7 +83,7 @@ function respondWithFile(path, res, req) {
}
function getConfig() {
- exec(razor_bin + " config read", function (err, stdout, stderr) {
+ execFile(razor_bin, [ '-j', 'config', 'read' ], function(err, stdout, stderr) {
//console.log(stdout);
startServer(stdout);
});
@@ -81,10 +100,10 @@ function getRange() {
function startServer(json_config) {
var config = JSON.parse(json_config);
if (config['@image_svc_port'] != null) {
- image_svc_path = config['@image_svc_path'];
+ image_svc_path = path.resolve(config['@image_svc_path']) + '/';
app.listen(config['@image_svc_port']);
console.log("");
- console.log('ProjectRazor Image Service Web Server started and listening on:%s', config['@api_port']);
+ console.log('ProjectRazor Image Service Web Server started and listening on:%s', config['@image_svc_port']);
console.log("Image root path: " + image_svc_path);
} else {
console.log("There is a problem with your ProjectRazor configuration. Cannot load config.");
View
62 lib/project_razor/cli.rb
@@ -36,6 +36,24 @@ def run(*argv)
end
@web_command = @options[:webcommand]
+ @cli_private = false
+
+ if @options[:jsoncommand] then
+ if @web_command then
+ # We do not allow -j if it's combined with -w since that would create
+ # security problem.
+ puts JSON.dump({
+ "slice" => "ProjectRazor::Slice",
+ "result" => "BadRequest",
+ "http_err_code" => 400
+ })
+ # We must return true here to avoid a "500 Internal Server Error"
+ return true
+ end
+ @web_command = true
+ @cli_private = true
+ end
+
@debug = @options[:debug]
@verbose = @options[:verbose]
@@ -57,30 +75,37 @@ def colorize(string, options = {})
slice = argv.shift
if call_razor_slice(slice, argv)
return true
- else
- if @web_command
- puts JSON.dump({
- "slice" => "ProjectRazor::Slice",
- "result" => "InvalidSlice",
- "http_err_code" => 404
- })
- else
- puts optparse
- print_available_slices
- if slice
- print "\n [#{slice}] ".red
- print "<-Invalid Slice \n".yellow
- end
- end
- return false
end
+
+ if @web_command then
+ puts JSON.dump({
+ "slice" => "ProjectRazor::Slice",
+ "result" => "InvalidSlice",
+ "http_err_code" => 404
+ })
+ # We must return true here to avoid a "500 Internal Server Error"
+ return true
+ end
+
+ puts optparse
+ print_available_slices
+ if slice
+ print "\n [#{slice}] ".red
+ print "<-Invalid Slice \n".yellow
+ end
+ return false
end
private
def call_razor_slice(raw_name, args)
return nil if raw_name.nil?
+ if raw_name == 'config' and @web_command and !@cli_private then
+ @logger.error "Razor config called as web command"
+ return false # Will yield 404 which is good. This slice doesn't exist in the web UI
+ end
+
name = file2const(raw_name)
razor_module = Object.full_const_get(SLICE_PREFIX + name).new(args)
razor_module.web_command = @web_command
@@ -141,6 +166,11 @@ def get_optparse
@options[:webcommand] = true
end
+ @options[:jsoncommand] = false
+ opts.on( '-j', '--jsoncommand', 'Same as -w but not exposed in web UI.'.yellow ) do
+ @options[:jsoncommand] = true
+ end
+
@options[:nocolor] = false
opts.on( '-n', '--no-color', 'Disables console color. Useful for script wrapping.'.yellow ) do
@options[:nocolor] = true
View
102 spec/nodejs-api/rz_injection_prevention_spec.rb
@@ -0,0 +1,102 @@
+require "spec_helper"
+
+describe "ProjectRazor::NodeJS::API" do
+
+ let(:data) { ProjectRazor::Data.instance }
+ let(:config) {
+ data.check_init
+ data.config
+ }
+
+ describe ".Image Service" do
+
+ it "should return the README file" do
+ uri = URI "http://127.0.0.1:#{config.image_svc_port}/razor/image/README"
+ res = Net::HTTP.get_response(uri)
+ res.code.should == '200'
+ res.body.should =~ /Razor images/
+ end
+
+ it "should bail on 404 when presented with URI encoded directory traversal using ..%2F" do
+ uri = URI "http://127.0.0.1:#{config.image_svc_port}/razor/image/..%2FGemfile"
+ res = Net::HTTP.get_response(uri)
+ res.code.should == '404'
+ end
+
+ it "should bail on 404 when presented with URI encoded directory traversal using %2E%2E%2F" do
+ uri = URI "http://127.0.0.1:#{config.image_svc_port}/razor/image/%2E%2E%2FGemfile"
+ res = Net::HTTP.get_response(uri)
+ res.code.should == '404'
+ end
+
+ it "should bail on 404 when presented with URI encoded directory traversal using %2E%2E%5C" do
+ uri = URI "http://127.0.0.1:#{config.image_svc_port}/razor/image/%2E%2E%5CGemfile"
+ res = Net::HTTP.get_response(uri)
+ res.code.should == '404'
+ end
+
+ it "should bail on 404 when presented with binary zero in path" do
+ uri = URI "http://127.0.0.1:#{config.image_svc_port}/razor/image/%00%2Fetc%2Fpassword"
+ res = Net::HTTP.get_response(uri)
+ res.code.should == '404'
+ end
+
+ it "should bail on 500 when presented with UTF-8 encoded directory traversal %C0%AF" do
+ uri = URI "http://127.0.0.1:#{config.image_svc_port}/razor/image/..%C0%AFGemfile"
+ res = Net::HTTP.get_response(uri)
+ res.code.should == '500'
+ end
+
+ it "should bail on 500 when presented with UTF-8 encoded directory traversal %C1%1C" do
+ uri = URI "http://127.0.0.1:#{config.image_svc_port}/razor/image/..%C1%1CGemfile"
+ res = Net::HTTP.get_response(uri)
+ res.code.should == '500'
+ end
+
+ end
+
+ describe ".RESTful Interface" do
+
+ it "should bail on 404 when presented with URI encoded directory traversal" do
+ uri = URI "http://127.0.0.1:#{config.api_port}/razor/api/..%2FGemfile"
+ res = Net::HTTP.get_response(uri)
+ res.code.should == '404'
+ end
+
+ it "should not do shell expansion and perform cat ~/.ssh/id_rsa" do
+ uri = URI "http://127.0.0.1:#{config.api_port}/razor/api/-V/&&/cd/~/&&/cd/.ssh/&&/cat/id_rsa/;"
+ res = Net::HTTP.get_response(uri)
+ res.code.should == '404'
+ end
+
+ it "should bail on 404 when presented with binary zeroes" do
+ uri = URI "http://127.0.0.1:#{config.api_port}/razor/api/-V/%00cd/~/&&/cd/.ssh/&&/cat/id_rsa/;"
+ res = Net::HTTP.get_response(uri)
+ res.code.should == '404'
+ end
+
+ it "should not do URI encoded shell expansion" do
+ uri = URI "http://127.0.0.1:#{config.api_port}/razor/api/-V/%20%26%26%20cat%20%7E%2F.ssh%2Fid_rsa/;"
+ res = Net::HTTP.get_response(uri)
+ res.code.should == '404'
+ end
+
+ it "should bail on 404 when presented with -x" do
+ uri = URI "http://127.0.0.1:#{config.api_port}/razor/api/-x"
+ res = Net::HTTP.get_response(uri)
+ res.code.should == '404'
+ end
+
+ it "should refuse to deliver the config slice" do
+ uri = URI "http://127.0.0.1:#{config.api_port}/razor/api/config"
+ res = Net::HTTP.get_response(uri)
+ res.code.should == '404'
+ end
+
+ it "should refuse to combine web call with -j option" do
+ uri = URI "http://127.0.0.1:#{config.api_port}/razor/api/-j/config"
+ res = Net::HTTP.get_response(uri)
+ res.code.should == '400'
+ end
+ end
+end

0 comments on commit beaafb7

Please sign in to comment.
Something went wrong with that request. Please try again.