Always escape shell arguments before mail() #140
Conversation
|
Pretty much agree with the patch, but I'm cracking my whip about test changes that need to be made. |
|
I'm doing it one-file-at-a-time because I'm using github.com, but sure :) |
|
Some existing tests will also need fixing. I will try prioritizing this today, as it kinda looks bad :| |
|
Yeah :\ |
|
Sorry, didn't get to it due to IRL issues. I'll work on it asap. |
|
So, I picked this up, and an exception is fired very early on (while setting the As discussed with @paragonie-scott, this is not a security issue report, but just hardening the test suite and improving current reliability against the already reported CVE. |
That means ZF2 is safe, given a typical usage. This is just defense in depth. :) |
|
@paragonie-scott the fact that the shell arg escaping is not applied is still bothersome, so the test should indeed verify the exception as it does before paragonie-scott/zend@7551343, but with a different escape sequence (valid domain, valid mail, shell escape). One possible approach I can think of is to mock the |
Upstream fix: zendframework/zend-mail#140 (still not merged)
|
I'm really not sure how to do fancy stuff with object mocking in unit tests. :\ It looks like the underlying issue was reported previously as ZF2016-04, but the fix was incomplete. |
Always escape shell arguments before mail()
This patch refactors the new test for zendframework#140 as follows: - Moves the test `SendmailTest::testSecondCodeInjectionInFromHeader()` to `MessageTest` - Updates that test to expect an `InvalidArgumentException` instead of a `RuntimeException`. - Updates that test to simply instantiate the message, and then call `setFrom()`, as that's all that's necessary to trigger the exception. - Adds two new tests to `SendmailTest`: - `testPrepareParametersEscapesSenderUsingEscapeShellArg` tests that a malformed sender address will be escaped. - `testPrepareParametersEscapesFromAddressUsingEscapeShellArg` tests that a malformed from address will be escaped. These last two conditions should not occur, but could occur if somebody were to provide custom From/Sender implementations that did not contain the checks we currently have that prevent invalid arguments.
This patch refactors the new test for zendframework#140 as follows: - Moves the test `SendmailTest::testSecondCodeInjectionInFromHeader()` to `MessageTest` - Updates that test to expect an `InvalidArgumentException` instead of a `RuntimeException`. - Updates that test to simply instantiate the message, and then call `setFrom()`, as that's all that's necessary to trigger the exception. - Adds two new tests to `SendmailTest`: - `testPrepareParametersEscapesSenderUsingEscapeShellArg` tests that a malformed sender address will be escaped. - `testPrepareParametersEscapesFromAddressUsingEscapeShellArg` tests that a malformed from address will be escaped. These last two conditions should not occur, but could occur if somebody were to provide custom From/Sender implementations that did not contain the checks we currently have that prevent invalid arguments.
|
Hey, @paragonie-scott — I've pushed a commit to your branch that performs the testing that @Ocramius suggested. If travis looks okay (minus expected issues with the zend-servicemanager compat tests), I'll merge this today. Thanks! |
Forward port #140 Conflicts: test/Transport/SendmailTest.php
Validate Sender address Consistent use of empty()
No description provided.