Security concern: uploadMany with Mjoin returning all records on filtered table

maxmedia · June 17

I am successfully using uploadMany using Mjoin and reference files table. In some circumstances I am further filtering results returned to client, for example using GET parameter for record id. If that returns non-zero main records, everything is fine, in returned data you have only files associated with main records. But if filtering results in zero main records, you get all existing files on server.

Example
Using modified code from this https://editor.datatables.net/examples/advanced/upload-many.html example:

Editor::inst( $db, 'users' )
    ->fields(
        Field::inst( 'users.id' ),
        Field::inst( 'users.first_name' ),
        Field::inst( 'users.last_name' ),
    )
    ->join(
        Mjoin::inst( 'files' )
            ->link( 'users.id', 'users_files.user_id' )
            ->link( 'files.id', 'users_files.file_id' )
            ->fields(
                Field::inst( 'id' )
                    ->upload( Upload::inst( $_SERVER['DOCUMENT_ROOT'].'/uploads/__ID__.__EXTN__' )
                        ->db( 'files', 'id', array(
                            'filename'    => Upload::DB_FILE_NAME,
                            'filesize'    => Upload::DB_FILE_SIZE,
                            'web_path'    => Upload::DB_WEB_PATH,
                            'system_path' => Upload::DB_SYSTEM_PATH
                        ) )
                        ->validator( Validate::fileSize( 500000, 'Files must be smaller that 500K' ) )
                        ->validator( Validate::fileExtensions( array( 'png', 'jpg', 'jpeg', 'gif' ), "Please upload an image" ) )
                    )
            )
    )
    ->where( 'id', $_GET['userId'] )
    ->process( $_POST )
    ->json();

If there is no user with id requested in $_GET['userId'], you get zero records in data array, but all existing server files in files array.
I guess this is not a bug, but it's how Mjoin works. Any ideas how to filter correctly?
This might be huge security concern.

allan · June 17

For such a case I would absolutely suggest you check that the GET parameter is submitted before using it - e.g.:

if (! isset($_GET['userId'])) {
  echo json_encode(["data" => []]);
  exit;
}

Depending on the setup, you might want to check if the user has access to the userId value submitted as well. If it is per user filtering that is happening (which it looks like here), then a session variable would normally be preferred over a query parameter.

Allan

maxmedia · June 17

Well, that was very simplified example, just a proof-of-concept. We are filtering on much more complicated criteria, every one coming from authenticated, session-based user variables and permissions. Sometimes that results in zero returned rows.

Imagine simple eshop orders table, each order with multiple associated files. Authenticated user can list his/hers own orders, where user id comes from session. Every request and call to API is sanitized and authenticated. User that don't have any order can see all associated files of all other users!

As a workaround we probably can check and manipulate resulting data before calling json() method, and clean files array if data array is empty. Even when using more uploadMany fields in single editor (as discussed here ), this seems to be a simple safeguard, though not really solution to primary problem.

allan · June 18

Good stuff - I expected it was a simplified case, but just needed to make sure .

User that don't have any order can see all associated files of all other users!

Yeah - that's not good. Is that because whatever session variable you are using is not set, and therefore the filter isn't being applied? Can you show me that code? A check for that value being set would be the way to address this I think.

Allan

maxmedia · June 18

My real code is really complex, filtering records on multiple criteria. Point is that even when all variables are set correctly, there are situations when no record satisfies criteria, resulting in empty data array.
Like my example: eshop user with no orders yet. Or maybe - server side search for non-existent string.
Looking at front-end everything work as expected, datatable showing no records. But if user look into browser dev tools to ajax request, he/she can see all files of other users in files array. These are not used on front-end (table is empty), but user can find them in network communication.

allan · June 19

If it is returning everything, it suggests that a condition value isn't being filled in and therefore the condition isn't being applied.

If we go back to the initial example, you could do something like:

->where( 'id', isset($_GET['userId']) ? $_GET['userId'] : '-1' )

So if it isn't set, a search will be performed for a value that doesn't exist, and thus give zero results.

Without being able to see or debug the code, I'm guessing you have a session variable that isn't set, and thus the effect is the same? You can add ->debug(true) just before ->process(...) which will cause the SQL generated to be returned to the client-side, so you can check which condition isn't being added, and thus look to see what is happening to that value, and put a fallback in for it.

Allan

maxmedia · June 19

Hello Allan,
I am afraid we don't understand each other. The condition is evaluated correctly and returning zero results from main table (in data array) to user is expected behaviour.
What is unexpected is files array full of records not belonging to that user.
Problem is probably in how Mjoin works: you got rows from linked table associated to rows in main table. But when main table is filtered resulting in zero rows (expected result), linked table return ALL rows (unexpected result).

Should have done that earlier, but now I looked into debug array in resulting JSON and I can see what is happening: when selecting files using Mjoin and relation table (as in example) ju do 3 consecutive steps (here is simplified SQL):

SELECT users.id FROM users table, applying any filters specified
SELECT files.id FROM users_files JOIN users JOIN files WHERE users.id IN ( [results from step 1] )
SELECT FROM files WHERE id IN ( [results from step 2] )

The trouble is that when step 1 return zero rows, you skip step 2 entirely then doing step 3 without any condition. Maybe this was intended as optimization, but breaks logic.

I can also see situation where step 1 returns some rows, step 2 returns zero rows (because there are no files) and you skip step 3 (which would return zero rows anyway). This is ok.

allan · June 19

Apologies, yes, I was getting the wrong end of the stick. Many thanks for clarifying the issue for me.

I suspect that it is here where the issue is. Let me dig into that a bit next week and I'll get back to you - I agree, that needs to be changed.

Allan

Security concern: uploadMany with Mjoin returning all records on filtered table

Security concern: uploadMany with Mjoin returning all records on filtered table

Replies