Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix Abitrary File Read and Sql Injection #61

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix Abitrary File Read and Sql Injection #61

wants to merge 2 commits into from

Conversation

trichimtrich
Copy link

@trichimtrich trichimtrich commented Jul 26, 2017

Sql Injection: #47
Abitrary File Read: #48

@trichimtrich trichimtrich changed the title fix Abitrary File Read fix Abitrary File Read and Sql Injection Jul 26, 2017
@matlam
Copy link
Contributor

matlam commented Jul 30, 2017

The fix for the arbitrary file read breaks the help function, because the help files are in subdirectories and url is e.g. circulation/index.md
The solution that I came up with, is to use this function instead of file_exists:

function isValidHelpFile($file) {
    global $sysconf;
    $iterator = new RecursiveIteratorIterator(new RecursiveDirectoryIterator(HELP . '/' . $sysconf['default_lang'] . '/', FilesystemIterator::SKIP_DOTS | FilesystemIterator::CURRENT_AS_PATHNAME));
    foreach ($iterator as $helpfile) {
        if ($helpfile === $file) {
            return true;
        }
    }
    return false;
}

But it seems a bit wasteful

@trichimtrich
Copy link
Author

I think it's still ok, but when we add new help file, we need to update config too.
Another the solution is use realpath($file_path) and check if it starts with HELP directory.
With that we can confirm request file is valid or not, and also can use sub directory.

@heroesoebekti
Copy link
Collaborator

I have a solution, that is by filtering the file extension .md

if(!file_exists($file_path) || !preg_match("/^.*\.(md)$/i", $file_path)) {..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants