[3.1] - burpsuite - take a stab at csrf fix in imagelib/search.php

[Atticus29]

Description

This PR attempts to address a Cross-site request forgery highlighted by burpsuite in /imagelib/search.php. I have to confess that I was quite unfamiliar with this vulnerability and its solution, but I've given it a shot. I suppose we'll know one way or the other whether it was successful after a fresh burpsuite run?

To offer more context, burpsuite recommended the following resolution, which I've tried to implement:

The most effective way to protect against CSRF vulnerabilities is to include within relevant requests an additional token that is not transmitted in a cookie: for example, a parameter in a hidden form field. This additional token should contain sufficient entropy, and be generated using a cryptographic random number generator, such that it is not feasible for an attacker to determine or predict the value of any token that was issued to another user. The token should be associated with the user's session, and the application should validate that the correct token is received before performing any action resulting from the request.

Specifically, this PR:

• Establishes default values for the values extracted from the $_REQUEST object in imagelib/search.php, so that there are no errors even if the csrf equality criteria are not met.
• Extracts the "real" values (if they exist) from the $_REQUEST object in imagelib/search.php ONLY if the csrf token submitted with the form matches the one that currently exists for the user's session.
• Similarly, passes the $_REQUEST object to the setCollectionVariables method ONLY if the csrf token submitted with the form matches the one that currently exists for the user's session.
• Defaults to a random $localCsrfToken value in the hidden form field if the 'csrf' token is missing from the session (only to prevent it from being an easily guessable '').
• Sets a new csrf token to the user's session in the header if it's missing, so that even users using the image search without logging in can have a csfr token.
• Removes the token when a user logs out.
• Removes the csrf token from the user's session when they're using openId, even if authentication is not successful per the existing pattern in classes/OpenIdProfileManager.php.

If this PR successfully resolves the vulnerability, other pages with the same vulnerability could be more easily resolved (by passing a csfr token in a hidden input field for the vulnerable pages/forms/$_REQUEST objects and then ensuring it matches the token in the user session before extracting values from the $_REQUEST object) with the already-established infrastructure herein.

Pull Request Checklist:

Pre-Approval

• There is a description section in the pull request that details what the proposed changes do. It can be very brief if need be, but it ought to exist.
• Hotfixes should be branched off of the master branch and squash and merged back into the master branch.
  • N/A
• Features and backlog bugs should be merged into the Development branch, NOT master
• All new text is preferably internationalized (i.e., no end-user-visible text is hard-coded on the PHP pages), and the spreadsheet tracking internationalizations has been updated either with a new row or with checkmarks to existing rows.
  • N/A
• There are no linter errors
• New features have responsive design (i.e., look aesthetically pleasing both full screen and with small or mobile screens)
  • N/A
• Symbiota coding standards have been followed
• If any files have been reformatted (e.g., by an autoformatter), the reformat is its own, separate commit in the PR
  • N/A
• Comment which GitHub issue(s), if any does this PR address
  • https://github.com/orgs/BioKIC/projects/6/views/10?pane=issue&itemId=25403328
• If this PR makes any changes that would require additional configuration of any Symbiota portals outside of the files tracked in this repository, make sure that those changes are detailed in this document.

Post-Approval

• It is the code author's responsibility to merge their own pull request after it has been approved
• If this PR represents a merge into the Development branch, remember to use the squash & merge option
• If this PR represents a merge from the Development branch into the master branch, remember to use the merge option
• If this PR represents a hotfix into the master branch, a subsequent PR from master into Development should be made merge option (i.e., no squash).
• If the dev team has agreed that this PR represents the last PR going into the Development branch before a tagged release (i.e., before an imminent merge into the master branch), make sure to notify the team and lock the Development branch to prevent accidental merges while QA takes place. Follow the release protocol here.
• Don't forget to delete your feature branch upon merge. Ignore this step as required.

Thanks for contributing and keeping it clean!

[Atticus29]

Just got out of the shower, where I had the thought that... it's also entirely possible that the real problem is just that <form name="imagesearchform" id="imagesearchform" action="search.php?<?=$imgLibManager->getQueryTermStr()?>" method="post" just shouldn't be a POST method? Is there a good reason why this uses the POST method, @egbot? As a publicly exposed endpoint that doesn't seem to insert into to the DB, maybe it should be GET?

[egbot]

Setting the csrf token within header_template.php file will be an issue because it will cause the forms to fail for all previously installed portals, until their header.php file is updated. I suggest setting this token within the symbbase.php file, which would be appropriate we end up integrating into form elements throughout the code. Since the getUuidV4 is a static function, you don't need to create a class instance. Instead, you only need to add the following two lines.

include_once($SERVER_ROOT.'/classes/UuidFactory.php');
if(!isset($_SESSION['csrf'])) $_SESSION['csrf'] = UuidFactory::getUuidV4();

[egbot]

Another issue is that this is going to break pagination, unless you add the token to the pagination links. Furthermore, this would also interfere within link copy and link sharing features that legitimately replicates a form action. There are legitimate situations where we want folks to be able to do the following:

https://lichenportal.org/portal/imagelib/search.php?taxa=acarospora&submitaction=search

However, we do want to protect folks from spoofing an action that would change data within the database, right? Thus, instead of restricting input variables being set, maybe we should just limit write actions? In this case, restrict the batchAssignImageTag function from being triggered if the tokens fail to match. This might not totally satisfy the default BurpSuite scans, but it would add the appropriate security without limiting functionality.

[MuchQuak]

Another issue is that this is going to break pagination, unless you add the token to the pagination links. Furthermore, this would also interfere within link copy and link sharing features that legitimately replicates a form action. There are legitimate situations where we want folks to be able to do the following:

https://lichenportal.org/portal/imagelib/search.php?taxa=acarospora&submitaction=search

However, we do want to protect folks from spoofing an action that would change data within the database, right? Thus, instead of restricting input variables being set, maybe we should just limit write actions? In this case, restrict the batchAssignImageTag function from being triggered if the tokens fail to match. This might not totally satisfy the default BurpSuite scans, but it would add the appropriate security without limiting functionality.

I think this is a sensible. Some of these are a pain because the real exploit is the social engineering side of things which can never really be contained.

[Atticus29]

Thanks for all of the feedback. I'll try to implement these recommended changes on Monday. There's at least one thing I have a question about, but it'll be easier to ask about on video.

[egbot]

Thanks for all of the feedback. I'll try to implement these recommended changes on Monday. There's at least one thing I have a question about, but it'll be easier to ask about on video.

Might be worth chatting about this as a group within our next dev meeting. Thanks for taking the initiative to address this issue.

[Atticus29]

UuidFactory::

Fixed in 96c91fe.

[Atticus29]

Another issue is that this is going to break pagination, unless you add the token to the pagination links. Furthermore, this would also interfere within link copy and link sharing features that legitimately replicates a form action. There are legitimate situations where we want folks to be able to do the following:

https://lichenportal.org/portal/imagelib/search.php?taxa=acarospora&submitaction=search

However, we do want to protect folks from spoofing an action that would change data within the database, right? Thus, instead of restricting input variables being set, maybe we should just limit write actions? In this case, restrict the batchAssignImageTag function from being triggered if the tokens fail to match. This might not totally satisfy the default BurpSuite scans, but it would add the appropriate security without limiting functionality.

Fixed across a number of commits in this PR. The csrf should only be checked for the batchAssignImageTag action now.

[egbot]

Looks good, except for the following error.

You just need to remove "$uf->" from symbbase.php line 59 and it will work perfectly!