Symphony.

Description

This is not critical, because one has to be logged into Symphony to do some evil, but it still may be dangerous.

I discovered it thanks to different problem, which i described in #608.

Field.upload.php accepts two ways of selecting a file by passing value in POST:

1. File upload (file input) - passed as file info array,
2. File path/name (hidden input) - passed as string.

It's second option that is buggy.

When field value is a string, checkPostFieldData() function only checks if file (WORKSPACE + field value) exists, and does not validate it's real path (does not check if it really is located inside WORKSPACE directory, does not check if it still is a valid file, like if it is in target subdirectory and passes validation rule check). That allows malicious user to enter something like this:

/../manifest/config.php

That will be checked and accepted by checkPostFieldData().

Now, that's not the most dangerous part yet (at least not with new versions of Symphony, where direct access to manifest and other directories is blocked with htaccess). That one is located in processRawFieldData() function.

When data is processed and it is a string, field checks if $entry_id was specified (which would mean that we're editing existing entry). If it was specified, then file info is loaded from database, nothing is changed and everything is OK. If it was not specified, bad things can happen because path is not properly validated (same problem as in checkPostFieldData()) and field only checks if file exists and is readable. In case of our example, config.php does exist and is readable, so it will be accepted.

Finally we get to the really evil part. After form is POSTed to symphony/publish/SECTION_NAME/new, new entry will be created, with whatever file path we passed to it.

What is the next step in case of our example above? You can try "delete this entry" and watch whole site go down.

I'm guessing that there are much more "useful" (for malicious users) tricks that can be done thanks to this bug.

Solution

I've seen other CMS using realpath() to check if path is valid. Maybe it could be used by field.upload.php too? Something like:

if (strpos(realpath(WORKSPACE . $file), realpath(WORKSPACE)) === 0) {
   // File path is OK
}

Or, if reaplath is not an option, because of permission requirements:

The running script must have executable permissions on all directories in the hierarchy, otherwise realpath() will return FALSE.

then maybe use something like this:

// remove all ../ and mupltiple slashes, we don't need to make path real
// we just need it cleaned from dirty tricks
$file = preg_replace(array('%/+%', '%(^|/)../%'), '/', $file);
if (file_exists(WORKSPACE.$file)) {
   // rest of code
}

update: fixed preg_replace solution.

Just to clarify, are you saying that this is only possible via DOM hacking in the backend? Is this also a threat to the frontend?

I agree with the statement that it should actually be checking, but just wanting to get an accurate gauge of the severity of this bug.

I did not check how it works with frontend, but if section event passes data to field.upload.php, then it probably bug "works" there too. If that is true, then anyone who can use section event to upload files, can also use this bug/hole to mess up whole site, which makes this bug a critical security hole.

If it works only on backend, then it's not that scary, but still dangerous.

DOM hacking is not as hard as it was (if it ever was) before. I only had to use "Inspect element" function in Chrome and change few things in HTML (all in browser :).

I think it would be quite an elaborate exercise to repeat this on the Frontend as it would require your Upload field on the frontend function exactly how the Symphony backend works.

Nevertheless, it's better to patch than be sorry, so I've implemented your changes in this commit.

I've tested describing the steps you've outlined and this prevents the issue from occurring (both frontend and backend). Would you like to confirm before I close this bug?

I think it would be quite an elaborate exercise to repeat this on the Frontend as it would require your Upload field on the frontend function exactly how the Symphony backend works.

I do not use frontend section events much, so i may be wrong here, but what if "comments" section has upload field? Simple DOM manipulation would allow them to change file field into text field. If section event does not do any additional checks, then it would allow them to create new entry with file field pointing to config.php.

I've tested describing the steps you've outlined and this prevents the issue from occurring (both frontend and backend). Would you like to confirm before I close this bug?

Thank You. There is one problem still there. I probably should have posted new comment here, instead of updating previous one, but i've fixed pattern check in meantime. The one in commit will not prevent things like: ///../manifest/config.php;

Another important thing is that WORKSPACE should be glued only after cleaning up the path (because workspace could include ../ and still be valid, if developer needed that for some reason).

This should work OK:

$file = WORKSPACE . preg_replace(array('%/+%', '%(^|/)../%'), '/', $data);

First it removes multiple slashes, then it removes /../ and prepends WORKSPACE in the end.

I've pushed an updated commit with your updated regex, thanks :)