Codeigniter: Sendmail and Blind OS Injection

[larcho]

We recently made a security/penetration test of a codeigniter site/application we made. It came to our attention that the tester was able do blind os injection, executing arbitrary commands from the server through the ci mail library.

He was able to do this because, first, user data was not sanitized and second, CI E-mail library uses 'popen' to send mails through 'sendmail'.

The first problem is something a developer should always do, sanitize user data before sending it to the server. But I can see that this might be a very common security breach in many ci applications using sendmail. I'm wondering if there isn't a factored in solution for arbitrary commands sent when using the sendmail option. Or maybe at least there should be a bigger warning when a developer uses sendmail for sending mail because of the nature in which it is set up in CI (through fopen).

[narfbg]

Sorry, but you're not really giving us anything here ...

[kakysha]

Maybe he talking about https://github.com/bcit-ci/CodeIgniter/blob/develop/system/libraries/Email.php#L1872?

[larcho]

Maybe he talking about https://github.com/bcit-ci/CodeIgniter/blob/develop/system/libraries/Email.php#L1872?

Yes, that's what I'm talking about. Sorry for not being specific enough.

[narfbg]

I knew that was probably the line you're talking about, but that alone says nothing ...

You've only said that some _unknown_ kind of usage, assuming _unvalidated user input_ (and user input to the email library is rare in the first place) enables command injection.

No details of which piece in there needs validation or sanitization.
No explanation of your use case (as I said, passing user input to the email library is not common).
No concrete suggestions.

It's not even clear if this is a feature request, a bug report or you're just asking for help here (in which case you're in the wrong place; we have a forum board for people seeking assistance with something).

Also, no version numbers (@kakysha's link points to the _develop_ branch, which surely isn't what you're using).

I mean, assume _you_ are the maintainer here and try to decide what to do with this issue ... I'm sure you'd see how the information at hand is far from enough.

[sui77]

don't you think "popen + unsanitized input" sounds serious enough to spend 5 minutes for investigation?

$this->load->library('email');
$config['protocol'] = 'sendmail';
$this->email->initialize($config);

$this->email->from('[email protected]; rm -rf /;', 'Your Name');
// ...
$this->email->send();

[sui77]

The other popen (haven't searched for systen/exec/etc yet) in Upload.php also looks fishy...

        $cmd = function_exists('escapeshellarg')
            ? 'file --brief --mime '.escapeshellarg($file['tmp_name']).' 2>&1'
            : 'file --brief --mime '.$file['tmp_name'].' 2>&1';

I don't think not escaping at all should be an option if there is no escaping available (which on the other hand seems to be very unlikely).

[narfbg]

@sui77 Don't you think you're too quick to point fingers?

Yes, popen() + unsanitized (and FFS, it should be validated, not sanitized) user input is serious enough to spend 5 minutes on it.
I did spend time on it, and found nothing. And I like to think that I do have great attention to detail, but I too can miss something - hence why I left the issue open.

Then comes the question of unvalidated, unsanizitzed user input to a library that is not made for that. Seriously, who sends emails with a user-supplied From header? Especially without a single check on the input? Should I also take the blame for pointing that out instead of spending 5 hours on it?

And finally (and I noticed this now), apparently somebody has thought even of that, long before I've joined the project - the library has a validate option, which turns on email address validation and would prevent your example from working.

---

The thing in CI_Upload was changed because of #1494, and that probably was a bad decision indeed. But I can guarantee it is safe with or without escapeshellarg().

[narfbg]

@sui77 Either way, thanks for shedding some light on this. I was already getting somewhat nervous with the lack of feedback here.