code injection vulnerability in ungit

[jung-kim]

Let's assume an user enters inputs such as above on an ungit server that is hosted on unix flavored OS. Ungit will run the below command ultimately.

git -c color.ui=false -c core.quotepath=false -c core.pager=cat remote add test6 github.com/codingtwinky/ungit && rm package.json

Although above command is rather harmless but I'm sure you can see that this is a classic code injection vulnerability that can be potentially critical.

Will be adding a method to sanitize user inputs via checking for & char but should I be checking for any other characters?

[kinncj]

Or just simply urlencode the data.

[jung-kim]

What you mean? Convert github.com/codingtwinky/ungit && rm package.json to github.com%2Fcodingtwinky%2Fungit%20%26%26%20rm%20package.json?

url encode will escape more then needed, we need to only escape & character.

[Ajedi32]

I'm not all that familiar with node, but why invoke the shell at all?

E.g. Instead of using bash or sh to execute git -c color.ui=false -c core.quotepath=false -c core.pager=cat remote add test6 github.com/codingtwinky/ungit && rm package.json, why not just execute git with the arguments -c, color.ui=false, -c, core.quotepath=false, -c, core.pager=cat, remote, add, test6, and github.com/codingtwinky/ungit && rm package.json, which is all the shell is really doing (or all it should be doing, at least) anyway?

[jung-kim]

So currently what ungit does is it builds git command string and executes it as a shell command. Now I do not know of any plugins that allows a program to execute git within it, like what you have suggested.

[Ajedi32]

@codingtwinky That's odd. Executing a program directly sounds quite a bit simpler than spawning a shell which then executes the program, so I find it kind of strange that Node would include the later functionality but not the former. I know Ruby has both features built right into its core library, no requires or extra packages required.

What about this? http://nodejs.org/docs/v0.6.6/api/child_processes.html#child_process.spawn

[jung-kim]

Sorry when I said executes it as a shell command, what I mean is that child process is spawned and created command get executed within the spawned process, which is exactly what you have linked.

However, how would child process way would help this case? Because child process executes this command created by ungit, git -c color.ui=false -c core.quotepath=false -c core.pager=cat remote add test6 github.com/codingtwinky/ungit && rm package.json. This command, either executed by child process or new bash, it will do two operations. One is to add git remote and another is to remove package.json.

[Ajedi32]

Why would child process do two operations? Again, I'm not experienced with node, but based on that documentation it seems to me that if I run:

child_process.spawn(
  'git',
  [
    '-c',
    'color.ui=false',
    '-c',
    'core.quotepath=false',
    '-c',
    'core.pager=cat',
    'remote',
    'add',
    'test6',
    'github.com/codingtwinky/ungit && rm package.json'
  ]
)

then that's going to spawn one process (git), with all the given arguments. Not two processes.

&& doesn't mean anything special to the operating system. That's just a bit of syntax which is interpreted by bash or sh to mean "if the previous expression evaluates to 0, also execute the following expression". If you pass an argument containing && to a program that's not a shell, it shouldn't mean anything special in that context.

[jung-kim]

So let me apologize for making the assumption and adding on to further confusion.

Ungit uses's child process's exec method when running git commands. Now, confusion came in when I assumed that spawn uses exec underneath therefor there is no difference between spawn and exec function. After a quick testing it is obvious that I was wrong.

I agree that spawn is better option since there is no additional check and monkeying around for each individual inputs and will be implementing spawn unless I run into further problem.

Thanks!

[Ajedi32]

Ah, I get it; ungit uses exec. That explains it.

Yeah, using spawn would definitely be a good way fix this issue. No need to do any special escaping of arguments, just pass them directly as arguments to git instead of as a string to a shell.

[ghost]

@codingtwinky, I guess this should work for you:

var url = require( "url" );

function checkUrl ( dangerousURL ) {

  var urlObj = url.parse( dangerousURL );
  var safeURL = url.format( urlObj );

  return safeURL;

}

checkUrl( "github.com/codingtwinky/ungit && rm package.json" );
// github.com/codingtwinky/ungit%20&&%20rm%20package.json

Info: http://nodejs.org/api/url.html

[Ajedi32]

@motapc97 That's probably not necessary though if you're using child_process.spawn instead of child_process.exec, as I explained.

Or at least, it's not necessary for preventing injection of extra shell commands. There might be UX reasons for wanting to format the string as a valid URL (though I'm not sure why the user would be pasting invalid urls in the first place).

[ghost]

You're probably right (I never worked with child_process stuff).

But I guess my suggestion could be useful for other reasons, as you said, for example, I think Ungit should check if the input is a valid URL, and if not, warn the user about that.

[jung-kim]

That could work partially, but I would have to agree that spawn will be more permanent way to fix this issue. Despite the fact that move to spawn is causing some major operation within ungit and it's little concerning...