[Configuration] Path settings must be valid paths #4023

johnsaigle · 2018-10-23T20:53:13Z

Brief summary of changes

I was recently able to develop an exploit where I could upload and execute scripts via manipulating config path settings. This PR ensures that these settings actually correspond to existing paths on the system before changing their values in the database.

To Test

Try changing one of the Configuration settings under Paths to an invalid path. You'll get a 400 error.

modules/configuration/ajax/process.php

kongtiaowang · 2018-10-29T18:24:38Z

modules/configuration/ajax/process.php

+                    $err = 'Path ' . htmlspecialchars($value, ENT_QUOTES)
+                        . ' is not valid.';
+                    // Set response code and display error message.
+                    $displayError(400, $err);


this is not a function name? typo?

kongtiaowang · 2018-10-29T18:26:53Z

When I test it, there is no error message, and all the paths can't be changed.

johnsaigle · 2018-10-29T18:42:43Z

@kongtiaowang You were right. I fixed the problem. Please test again if you have time.

stale

ridz1208 · 2018-11-05T16:51:46Z

@paiva can you please test this one ?

johnsaigle · 2018-11-05T17:05:21Z

This might cause issues with the Logs path because I think it's deliberately set up as a partial path (i.e. tools/logs/)

I'm not sure where this is used exactly but I think I'll need to issue another PR to update those usages once this is merged.

modules/configuration/ajax/process.php

johnsaigle · 2018-11-13T19:07:01Z

This is now blocked by #4087 as it uses the new data type in its query.

Addressed by using new data types.

PierreEMorin · 2019-02-18T16:39:13Z

This code does what it is supposed to do.

However, at the moment, default path values such as "/PATH/TO/EXTERNAL/LIBRARY/" or "/PATH/TO/Genomic-Data/" are not allowed anymore so the user is left with either:
1. leave it blank
2. put a valid but inadequate path (i.e. data/%project%/data for genomics)
3. add valid paths to the server that may be never used (i.e. a project not using the genomic module)

Should blank be allowed? or should default values from the ConfigTables schema be allowed?

johnsaigle · 2019-02-18T16:43:11Z

I guess ideally we would have a toggle switch on each of the modules saying whether a project will use them or not...

I think it makes sense to allow blank values in this case. The default params don't really make sense anyway and only some of the paths are configured to have one in the /NOT/REAL/PATH format.

Blank paths will probably result in ConfigurationExceptions at some point in whatever modules use them. I don't know if this is the best solution but I think it's what we have available right now.

modules/configuration/ajax/process.php

driusan · 2019-07-22T15:24:16Z

modules/configuration/ajax/process.php

 foreach ($_POST as $key => $value) {
    if (is_numeric($key)) { //update
        if ($value == "") {
-            $DB->delete('Config', array('ID' => $key));
+            $DB->update('Config', array('Value' => null), array('ID' => $key));


Why is this changed?

The basic idea of my changes is to verify whether the incoming data is a web_path type in the DB and if so make sure that it's a valid path.

When debugging a problem another reviewer had, i realized that the script was actually deleting values when they are blank.

This is a problem because we decided in a meeting at some point that the paths should be left blank when they're not configured (rather than allowing for /INVALID/PLACEHOLDER/PATHS as a value).

I figured it would make sense to keep the IDs in the database and set them to null rather than delete them. It makes more sense to me intuitively to have a setting be blank/uninitialized than for it to vanish.

Do you see this causing problems?

Yes, when new config settings are added/unchanged from the default they do not have any entry in the Config table. The update statement will return 0 rows and report success but silently do nothing. As a result, no one will ever be able to set a config setting for the first time for any config that doesn't have a non-empty default. Not having a value in the table when the setting is blank is correct behaviour.

when new config settings are added/unchanged from the default they do not have any entry in the Config table

Aren't defaults stored in the Config table? ConfigSettings doesn't have any information about the value.

driusan · 2019-07-22T15:25:08Z

modules/configuration/ajax/process.php

@@ -52,20 +63,40 @@
        if ($keySplit[0] == 'add') {
            if ($value !== "") {
                if (countDuplicate($keySplit[1], $value) == '0') {
-                    $DB->insert(
+                    $DB->update(


This will not work when the value isn't in the table

Co-Authored-By: Dave MacFarlane <driusan@gmail.com>

driusan · 2019-08-01T19:05:52Z

@johnsaigle Why did you add "Discussion Required" to this PR?

johnsaigle · 2019-08-05T14:32:12Z

@driusan I replied to your above comment and you didn't follow up.

I think that this PR makes sense the way I've done it. Maybe it makes the most sense for us to discuss it in person so I can walk you through my thinking.

driusan · 2019-08-26T19:52:02Z

This PR does not make sense the way it's done. The config tables are normalized. If there is no value, it should be deleted from the value table, not updated to a blank value. Defaults are only stored in the config table of the default schema if the default is non-empty.

…81023-ValidateConfigPaths

johnsaigle · 2019-09-17T21:37:46Z

@driusan I modified this PR to use insert/delete instead of updates, as requested. Please review when you have time.

PR rewritten to address review concerns. New review required.

driusan

mostly minor things and error handling.

modules/configuration/ajax/process.php

driusan · 2019-09-18T19:44:39Z

modules/configuration/ajax/process.php

+            $DB->delete(
+                'Config',
+                array('ConfigID' => $ConfigSettingsID)
+            );


else throw exception?

modules/configuration/ajax/process.php

Co-Authored-By: Dave MacFarlane <driusan@gmail.com>

…81023-ValidateConfigPaths

johnsaigle · 2019-09-23T17:19:26Z

@driusan I made your requested changes.

request changes have been addressed

johnsaigle · 2019-10-15T15:12:20Z

@driusan Tagging you again because this ideally would go into the next minor release as discussed in the LORIS meeting.

johnsaigle added Category: Feature PR or issue that aims to introduce a new feature Category: Security PR or issue that aims to improve security [branch] minor labels Oct 23, 2018

PapillonMcGill reviewed Oct 29, 2018

View reviewed changes

modules/configuration/ajax/process.php Outdated Show resolved Hide resolved

PapillonMcGill previously approved these changes Oct 29, 2018

View reviewed changes

paiva reviewed Oct 29, 2018

View reviewed changes

modules/configuration/ajax/process.php Outdated Show resolved Hide resolved

kongtiaowang previously requested changes Oct 29, 2018

View reviewed changes

kongtiaowang added the State: Needs work PR awaiting additional work by the author to proceed label Oct 29, 2018

johnsaigle dismissed PapillonMcGill’s stale review via b4791b2 October 29, 2018 18:38

johnsaigle removed the State: Needs work PR awaiting additional work by the author to proceed label Oct 29, 2018

PapillonMcGill previously approved these changes Nov 12, 2018

View reviewed changes

driusan previously requested changes Nov 12, 2018

View reviewed changes

modules/configuration/ajax/process.php Outdated Show resolved Hide resolved

johnsaigle dismissed PapillonMcGill’s stale review via fb57303 November 13, 2018 19:03

johnsaigle mentioned this pull request Nov 13, 2018

[SQL] Add 'path' and 'web_path' data types to ConfigSettings #4087

Merged

johnsaigle added the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Nov 13, 2018

johnsaigle mentioned this pull request Nov 29, 2018

Added the modalities for which to compute SNR to the Config module #4159

Merged

2 tasks

johnsaigle added State: Needs work PR awaiting additional work by the author to proceed and removed State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed State: Needs work PR awaiting additional work by the author to proceed labels Feb 4, 2019

driusan mentioned this pull request Feb 11, 2019

Cleanup duplicated data config paths #4314

Merged

4 tasks

cmadjar assigned PierreEMorin Feb 15, 2019

cmadjar assigned driusan Jul 18, 2019

driusan previously requested changes Jul 22, 2019

View reviewed changes

Update modules/configuration/ajax/process.php

5304ace

Co-Authored-By: Dave MacFarlane <driusan@gmail.com>

cmadjar assigned johnsaigle Jul 25, 2019

johnsaigle added the State: Discussion required PR or issue that requires the resolution of a discussion with the relevant parties to proceed label Jul 29, 2019

johnsaigle mentioned this pull request Aug 12, 2019

[Api] /candidates and sub-endpoints #4244

Merged

24 tasks

ridz1208 removed the [branch] minor label Aug 15, 2019

johnsaigle added 2 commits September 17, 2019 17:35

Rewrite

1324783

Merge remote-tracking branch 'john/181023-ValidateConfigPaths' into 1…

32024e1

…81023-ValidateConfigPaths

johnsaigle removed the State: Discussion required PR or issue that requires the resolution of a discussion with the relevant parties to proceed label Sep 17, 2019

johnsaigle requested a review from driusan September 18, 2019 15:33

driusan previously requested changes Sep 18, 2019

View reviewed changes

johnsaigle and others added 4 commits September 18, 2019 16:15

Update modules/configuration/ajax/process.php

4b048e9

Co-Authored-By: Dave MacFarlane <driusan@gmail.com>

Update modules/configuration/ajax/process.php

80ad5de

Co-Authored-By: Dave MacFarlane <driusan@gmail.com>

Add error handling

c858711

Merge remote-tracking branch 'john/181023-ValidateConfigPaths' into 1…

42f2723

…81023-ValidateConfigPaths

johnsaigle requested a review from driusan September 23, 2019 17:19

driusan merged commit a946d2d into aces:minor Oct 15, 2019

ridz1208 modified the milestones: 21.1.0, 22.0.0 Oct 28, 2019

johnsaigle mentioned this pull request Dec 6, 2019

[data_release/document_repository] Make upload directory configurable #5815

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Configuration] Path settings must be valid paths #4023

[Configuration] Path settings must be valid paths #4023

johnsaigle commented Oct 23, 2018 •

edited

Loading

kongtiaowang Oct 29, 2018 •

edited

Loading

kongtiaowang commented Oct 29, 2018

johnsaigle commented Oct 29, 2018

ridz1208 commented Nov 5, 2018

johnsaigle commented Nov 5, 2018

johnsaigle commented Nov 13, 2018

PierreEMorin commented Feb 18, 2019

johnsaigle commented Feb 18, 2019

driusan Jul 22, 2019

johnsaigle Jul 22, 2019

driusan Jul 22, 2019

johnsaigle Jul 23, 2019

driusan Jul 22, 2019

driusan commented Aug 1, 2019

johnsaigle commented Aug 5, 2019

driusan commented Aug 26, 2019

johnsaigle commented Sep 17, 2019

driusan left a comment

driusan Sep 18, 2019

johnsaigle Sep 24, 2019

johnsaigle commented Sep 23, 2019

johnsaigle commented Oct 15, 2019

[Configuration] Path settings must be valid paths #4023

[Configuration] Path settings must be valid paths #4023

Conversation

johnsaigle commented Oct 23, 2018 • edited Loading

Brief summary of changes

To Test

kongtiaowang Oct 29, 2018 • edited Loading

Choose a reason for hiding this comment

kongtiaowang commented Oct 29, 2018

johnsaigle commented Oct 29, 2018

ridz1208 commented Nov 5, 2018

johnsaigle commented Nov 5, 2018

johnsaigle commented Nov 13, 2018

PierreEMorin commented Feb 18, 2019

johnsaigle commented Feb 18, 2019

driusan Jul 22, 2019

Choose a reason for hiding this comment

johnsaigle Jul 22, 2019

Choose a reason for hiding this comment

driusan Jul 22, 2019

Choose a reason for hiding this comment

johnsaigle Jul 23, 2019

Choose a reason for hiding this comment

driusan Jul 22, 2019

Choose a reason for hiding this comment

driusan commented Aug 1, 2019

johnsaigle commented Aug 5, 2019

driusan commented Aug 26, 2019

johnsaigle commented Sep 17, 2019

driusan left a comment

Choose a reason for hiding this comment

driusan Sep 18, 2019

Choose a reason for hiding this comment

johnsaigle Sep 24, 2019

Choose a reason for hiding this comment

johnsaigle commented Sep 23, 2019

johnsaigle commented Oct 15, 2019

johnsaigle commented Oct 23, 2018 •

edited

Loading

kongtiaowang Oct 29, 2018 •

edited

Loading