diff --git a/includes/uploads.php b/includes/uploads.php index d92e612..11740bd 100644 --- a/includes/uploads.php +++ b/includes/uploads.php @@ -33,18 +33,38 @@ * compromised through an upload form had somebody who thought their case was the * exception. * + * Comprehensiveness is the point. The variants that read like typos -- `pht`, + * `phtm`, `php6`, `shtml` -- are the ones a bypass reaches for precisely because + * a shorter list forgot them; a blocklist that stops one spelling of "run this + * as PHP" and not the next is a blocklist that has not done its job. The check + * matches a single, real extension against this flat set, never a pattern or a + * pipe-joined MIME key, so there is no alternation for a crafted name to hide + * inside. + * * @since 0.1.0 */ const ALLTFO_FORBIDDEN_EXTENSIONS = array( 'php', + 'php1', + 'php2', 'php3', 'php4', 'php5', + 'php6', 'php7', 'php8', 'phps', + 'pht', + 'phtm', 'phtml', 'phar', + 'phpt', + 'hphp', + // Server-side includes: `.shtml` and its kin are executed by mod_include + // wherever SSI is on, which makes them as good as a script for this. + 'shtml', + 'shtm', + 'stm', 'pl', 'py', 'rb', diff --git a/tests/phpunit/tests/uploads.php b/tests/phpunit/tests/uploads.php new file mode 100644 index 0000000..dda317b --- /dev/null +++ b/tests/phpunit/tests/uploads.php @@ -0,0 +1,216 @@ + $name, + 'type' => '', + 'tmp_name' => $tmp, + 'error' => UPLOAD_ERR_OK, + 'size' => strlen( $contents ), + ); + } + + /** + * Every executable extension is refused, whatever the field allows. + * + * The blocklist runs before the per-field allow-list and before the byte + * check, and it is not filterable — so even a field that named `.php` in + * its own accepted types could not store one. Each variant here is a real + * way to spell "run this on the server"; a gap in this list is the whole + * site. + * + * @covers ::alltfo_store_uploaded_file + */ + public function test_executable_extensions_are_always_refused() { + $dangerous = array( + 'shell.php', + 'shell.php5', + 'shell.php6', + 'shell.php7', + 'shell.pht', + 'shell.phtm', + 'shell.phtml', + 'shell.phar', + 'shell.phps', + 'shell.shtml', + 'shell.htaccess', + 'shell.svg', + 'shell.exe', + 'shell.jsp', + 'shell.js', + ); + + // A field that names the dangerous extension itself, to prove the + // blocklist overrides the allow-list rather than deferring to it. + $field = array( + 'id' => 'f1', + 'type' => 'file', + 'label' => 'Attachment', + 'filetypes' => array( 'php', 'pht', 'phtml', 'svg', 'exe', 'jsp', 'js', 'phar', 'shtml' ), + ); + + foreach ( $dangerous as $name ) { + $result = alltfo_store_uploaded_file( + $this->fake_upload( $name, "" ), + $field, + 0 + ); + + $this->assertWPError( $result, "{$name} must be refused." ); + $this->assertSame( 'alltfo_file_type', $result->get_error_code(), "{$name} is refused as a bad type." ); + } + } + + /** + * A double extension does not smuggle a script past the gate. + * + * `evil.php.jpg` has a final extension of `jpg`; the byte check then finds + * the contents are not an image and refuses it, so the name trick buys + * nothing. + * + * @covers ::alltfo_store_uploaded_file + */ + public function test_double_extension_is_caught_by_the_byte_check() { + $field = array( + 'id' => 'f1', + 'type' => 'file', + 'label' => 'Attachment', + 'filetypes' => array( 'jpg', 'jpeg', 'png' ), + ); + + $result = alltfo_store_uploaded_file( + $this->fake_upload( 'evil.php.jpg', "" ), + $field, + 0 + ); + + $this->assertWPError( $result ); + $this->assertSame( 'alltfo_file_type', $result->get_error_code() ); + } + + /** + * What a file may be is the stored form's call, not the request's. + * + * This is the Forminator lesson in one assertion: the accepted types come + * from the field the form owner saved. A submission that ships its own + * `filetypes` alongside the file changes nothing — the handler never reads + * upload configuration from the request, only the bytes. + * + * @covers ::alltfo_handle_uploads + */ + public function test_accepted_types_come_from_the_schema_not_the_request() { + $schema = alltfo_normalize_schema( + array( + 'fields' => array( + array( + 'id' => 'cv', + 'type' => 'file', + 'label' => 'Your CV', + 'filetypes' => array( 'pdf' ), + ), + ), + ) + ); + + // The attacker sends a PHP file for the field *and* a forged config + // claiming php is allowed. Only the schema is consulted. + $files = array( + 'alltfo_file_cv' => $this->fake_upload( 'shell.php', "" ), + ); + + $_POST['cv'] = array( 'filetypes' => array( 'php' ) ); + $_POST['filetypes'] = array( 'php' ); + + $result = alltfo_handle_uploads( $schema, $files, 0 ); + + unset( $_POST['cv'], $_POST['filetypes'] ); + + $this->assertArrayHasKey( 'cv', $result['errors'], 'The forged config did not widen what the field accepts.' ); + $this->assertSame( array(), $result['values'], 'Nothing was stored.' ); + } + + /** + * A file that matches the stored allow-list and its own bytes is stored. + * + * The gate refuses the dangerous and accepts the ordinary; a gate that only + * ever says no is not being tested, it is broken. + * + * @covers ::alltfo_store_uploaded_file + */ + public function test_a_legitimate_file_is_accepted() { + $field = array( + 'id' => 'f1', + 'type' => 'file', + 'label' => 'Attachment', + 'filetypes' => array( 'txt' ), + ); + + $result = alltfo_store_uploaded_file( + $this->fake_upload( 'notes.txt', 'Just some plain text.' ), + $field, + 0 + ); + + $this->assertIsInt( $result ); + $this->assertGreaterThan( 0, $result ); + $this->assertSame( 'private', get_post_status( $result ), 'Form uploads are private, never public in the media library.' ); + + wp_delete_attachment( $result, true ); + } +}