search

flowjs / flow-php-server

flow.js php server library, validates uploaded chunks and safely merges all chunks to a single file
http://flowjs.github.io/ng-flow/
MIT License
245 stars 42 forks source link

Input sanitization ? #36

Closed IP-Developer closed 7 years ago

IP-Developer commented 7 years ago

How are the file names / flowIdentifier sanitized before being used ? Maybe I'm wrong, but looking at the code, I can't see any checks being made that would prevent a directory traversal attack for example.

AidasK commented 7 years ago

Good catch. Feel free to overwrite any file in any project.

Fix can be placed on this line, we need to sanitize identifier and chunk index. https://github.com/flowjs/flow-php-server/blob/master/src/Flow/File.php#L61

Can you contribute on this?

IP-Developer commented 7 years ago

I think it is a really bad idea to use any user input when processing chunks. The first step is to validate everything, presence of flow* variables in $_REQUEST, and the values accordingly (flowChunkNumber, flowTotalChunks, flowChunkSize, flowTotalSize being integers). Chunks should have some slugified/sanitized names (you can use SplFileInfo to sanitize, more like using getBaseName(), to prevent directory traversal), and the package should prompt for the final file name and path.

AidasK commented 7 years ago

I think it would be enough to change one line in this lib to make it safe. https://github.com/flowjs/flow-php-server/blob/master/src/Flow/File.php#L61

Should be changed to: return $this->config->getTempDir().DIRECTORYSEPARATOR.basename($this->identifier).''. (int) $index;

This method is reused in every case.

Final file name is not a concern of this library, it is up to the developer to make it safe.