Skip to content

N°9654 - Reduce surface attack on config file rights during setup #932

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.

Sign up for GitHub

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

Merged
Lenaick merged 2 commits into from
Jun 9, 2026
Merged

N°9654 - Reduce surface attack on config file rights during setup #932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ec48479
N°9654 - Reduce surface attack on config file rights during setup
Lenaick Jun 9, 2026
ca3eab0
Merge branch 'develop' into feature/9654-reduce-surface-attack-on-con…
Lenaick Jun 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions datamodels/2.x/combodo-data-feature-removal/src/Controller/DataFeatureRemovalController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,8 @@ public function OperationAnalysisResult(): void
{
$aParams = [];

try {
//from setup wizard/mtp
SetupUtils::CheckSetupToken();
SetupUtils::EraseSetupToken();
} catch (SecurityException $e) {
//from setup wizard/mtp
if (!SetupUtils::IsSessionSetupTokenValid()) {
//from same module
$this->ValidateTransactionId();
}
Expand Down Expand Up @@ -184,7 +181,6 @@ public function OperationAnalysisResult(): void
$aParams['aSetupParams'] = [
"_class" => "WizStepLandingBeforeAudit",
"operation" => "next",
"_params[authent]" => SetupUtils::CreateSetupToken(),
];

foreach ($aHiddenInputs as $sInputName => $sInputValue) {
Expand All @@ -200,6 +196,10 @@ public function OperationAnalysisResult(): void
$aParams['bDeletionNeeded'] = ($aParams['iQueryCount'] > 0);
Session::Set('aDeletionExecutionSummary', serialize($this->aDeletionExecutionSummary));

if (!$aParams['bHasDeletionNeeded']) {
SetupUtils::CreateSetupToken();
}
Comment on lines +199 to +201

@greptile-apps greptile-apps Bot Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Use the right flag

This branch checks bHasDeletionNeeded, but the code only sets bDeletionNeeded a few lines above. The missing key is treated as null, so this condition is always true and PHP can emit a warning. When the analysis still has deletion work to do, this still creates a fresh setup token even though the cleanup flow has not completed.

Suggested change
if (!$aParams['bHasDeletionNeeded']) {
SetupUtils::CreateSetupToken();
}
if (!$aParams['bDeletionNeeded']) {
SetupUtils::CreateSetupToken();
}


$this->DisplayPage($aParams, 'AnalysisResult');
}

Expand Down
15 changes: 6 additions & 9 deletions setup/ajax.dataloader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
* 'percent': integer 0..100 the percentage of completion once the file has been loaded
*/

use Combodo\iTop\Application\Helper\Session;
use Combodo\iTop\Application\WebPage\AjaxPage;

$bBypassMaintenance = true; // Reset maintenance mode in case of problem
Expand Down Expand Up @@ -129,7 +130,10 @@ function FatalErrorCatcher($sOutput)
*/
$sOperation = utils::ReadParam('operation', '');
try {
SetupUtils::CheckSetupToken();
Session::Start();
if (!SetupUtils::IsSessionSetupTokenValid()) {
throw new SecurityException("Invalid session token");
}

switch ($sOperation) {
case 'async_action':
Expand All @@ -150,14 +154,7 @@ function FatalErrorCatcher($sOutput)
/** @var WizardStep $oStep */
$oStep = new $sClass($oDummyController, $sState);
$sConfigFile = utils::GetConfigFilePath(ITOP_DEFAULT_ENV);
if (file_exists($sConfigFile) && !is_writable($sConfigFile) && $oStep->RequiresWritableConfig()) {
$sRelativePath = utils::GetConfigFilePathRelative(ITOP_DEFAULT_ENV);
$sErrorMsg = "<b>Error:</b> the configuration file '".$sRelativePath."' already exists and cannot be overwritten.";
$sErrorMsg .= "The wizard cannot modify the configuration file for you. If you want to upgrade ".ITOP_APPLICATION.", make sure that the file '<b>".$sRelativePath."</b>' can be modified by the web server.";
throw new Exception($sErrorMsg);
} else {
$oStep->AsyncAction($oPage, $sActionCode, $aParams);
}
$oStep->AsyncAction($oPage, $sActionCode, $aParams);
}
$oPage->output();
break;
Expand Down
3 changes: 1 addition & 2 deletions setup/setup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@ function WizardAsyncAction(sActionCode, oParams, OnErrorFunction)
{
var sStepClass = $('#_class').val();
var sStepState = $('#_state').val();
var sAuthent = $('#authent_token').val();

var oMap = { operation: 'async_action', step_class: sStepClass, step_state: sStepState, code: sActionCode, authent : sAuthent, params: oParams };
var oMap = { operation: 'async_action', step_class: sStepClass, step_state: sStepState, code: sActionCode, params: oParams };

var ErrorFn = OnErrorFunction;
$(document).ajaxError(function(event, request, settings) {
Expand Down
31 changes: 29 additions & 2 deletions setup/wizard.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,34 @@ function json_decode($json, $assoc = null)
$oWizard->Run();
} else {
SetupUtils::ExitMaintenanceMode(false);
// Force initializing the setup
$oWizard->Start();

$sConfigFile = utils::GetConfigFilePath(ITOP_DEFAULT_ENV);
if (file_exists($sConfigFile)) {
// The configuration file already exists
if (!is_writable($sConfigFile)) {
SetupUtils::ExitReadOnlyMode(false); // Reset readonly mode in case of problem
SetupUtils::EraseSetupToken();
$sRelativePath = utils::GetConfigFilePathRelative(ITOP_DEFAULT_ENV);
$oP = new SetupPage('Installation Cannot Continue');
$oP->add("<h2>Fatal error</h2>\n");
$oP->error("<b>Error:</b> the configuration file '".$sRelativePath."' already exists and cannot be overwritten.");
$oP->p("The wizard cannot modify the configuration file for you. If you want to upgrade ".ITOP_APPLICATION.", make sure that the file '<b>".$sRelativePath."</b>' can be modified by the web server.");

$sButtonsHtml = <<<HTML
<button type="button" class="ibo-button ibo-is-regular ibo-is-primary" onclick="window.location.reload()">Reload</button>
HTML;
$oP->p($sButtonsHtml);

$oP->output();
// Prevent token creation
exit;
} else {
chmod($sConfigFile, 0440);
}
}

SetupUtils::CreateSetupToken();

// Start the setup
$oWizard->Start();
}
24 changes: 0 additions & 24 deletions setup/wizardcontroller.class.inc.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,30 +195,6 @@ protected function DisplayStep(WizardStep $oStep): void
{
SetupLog::Info("=== Setup screen: ".$oStep->GetTitle().' ('.get_class($oStep).')');
$oPage = new SetupPage($oStep->GetTitle());
if ($oStep->RequiresWritableConfig()) {
$sConfigFile = utils::GetConfigFilePath(ITOP_DEFAULT_ENV);
if (file_exists($sConfigFile)) {
// The configuration file already exists
if (!is_writable($sConfigFile)) {
SetupUtils::ExitReadOnlyMode(false); // Reset readonly mode in case of problem
SetupUtils::EraseSetupToken();
$sRelativePath = utils::GetConfigFilePathRelative(ITOP_DEFAULT_ENV);
$oP = new SetupPage('Installation Cannot Continue');
$oP->add("<h2>Fatal error</h2>\n");
$oP->error("<b>Error:</b> the configuration file '".$sRelativePath."' already exists and cannot be overwritten.");
$oP->p("The wizard cannot modify the configuration file for you. If you want to upgrade ".ITOP_APPLICATION.", make sure that the file '<b>".$sRelativePath."</b>' can be modified by the web server.");

$sButtonsHtml = <<<HTML
<button type="button" class="ibo-button ibo-is-regular ibo-is-primary" onclick="window.location.reload()">Reload</button>
HTML;
$oP->p($sButtonsHtml);

$oP->output();
// Prevent token creation
exit;
}
}
}
$oPage->LinkScriptFromAppRoot('setup/setup.js');
$oPage->add('<form id="wiz_form" class="ibo-setup--wizard" method="post">');
$oPage->add('<div class="ibo-setup--wizard--content">');
Expand Down
3 changes: 1 addition & 2 deletions setup/wizardsteps/WizStepDBParams.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
/**
* Database Connection parameters screen
*/

use Combodo\iTop\Application\WebPage\WebPage;

class WizStepDBParams extends WizardStep
Expand Down Expand Up @@ -76,8 +77,6 @@ public function Display(SetupPage $oPage): void
$sTlsCA,
$sNewDBName
);
$sAuthentToken = $this->oWizard->GetParameter('authent', '');
$oPage->add('<input type="hidden" id="authent_token" value="'.$sAuthentToken.'"/>');
$oPage->add('</table>');
$sCreateDB = $this->oWizard->GetParameter('create_db', 'yes');
if ($sCreateDB == 'no') {
Expand Down
7 changes: 0 additions & 7 deletions setup/wizardsteps/WizStepDataAudit.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
* You should have received a copy of the GNU Affero General Public License
*/

use Combodo\iTop\Application\Helper\Session;

require_once(APPROOT.'setup/sequencers/DataAuditSequencer.php');

/**
Expand Down Expand Up @@ -77,9 +75,6 @@ public function Display(SetupPage $oPage): void

$sJSONData = json_encode($aInstallParams);
$oPage->add('<input type="hidden" id="installer_parameters" value="'.utils::EscapeHtml($sJSONData).'"/>');

$sAuthentToken = $this->oWizard->GetParameter('authent', '');
$oPage->add('<input type="hidden" id="authent_token" value="'.$sAuthentToken.'"/>');
if (!$this->CheckDependencies()) {
$oPage->error($this->sDependencyIssue);
$oPage->add_ready_script(<<<JS
Expand Down Expand Up @@ -126,12 +121,10 @@ public function PostFormDisplay(SetupPage $oPage)
<input type="hidden" name="$sParamName" value="$sElements"/>
INPUT;
}
$sUID = Session::Get('setup_token');
$oPage->add(
<<<HTML
<form id="data-feature-removal" class="ibo-setup--wizard ibo-is-hidden" method="post" action="$sApplicationUrl">
<input type="hidden" name="operation" value="AnalysisResult"/>
<input type="hidden" name="authent" value="$sUID"/>
$aHiddenInputs
</form>
HTML
Expand Down
15 changes: 2 additions & 13 deletions setup/wizardsteps/WizStepDone.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,13 @@ public function Display(SetupPage $oPage): void
}

$bHasBackup = false;
if (($this->oWizard->GetParameter('mode', '') == 'upgrade') && $this->oWizard->GetParameter('db_backup', false) && $this->oWizard->GetParameter('authent', false)) {
if (($this->oWizard->GetParameter('mode', '') == 'upgrade') && $this->oWizard->GetParameter('db_backup', false)) {
$sBackupDestination = $this->oWizard->GetParameter('db_backup_path', '');
if (file_exists($sBackupDestination.'.tar.gz')) {
$bHasBackup = true;
// To mitigate security risks: pass only the filename without the extension, the download will add the extension itself
$oPage->p('Your backup is ready');
$oPage->p('<a style="background:transparent;" href="'.utils::GetAbsoluteUrlAppRoot(true).'setup/ajax.dataloader.php?operation=async_action&step_class=WizStepDone&params[backup]='.urlencode($sBackupDestination).'&authent='.$this->oWizard->GetParameter('authent', '').'" target="_blank"><img src="../images/icons/icons8-archive-folder.svg" style="border:0;vertical-align:middle;">&nbsp;Download '.basename($sBackupDestination).'</a>');
$oPage->p('<a style="background:transparent;" href="'.utils::GetAbsoluteUrlAppRoot(true).'setup/ajax.dataloader.php?operation=async_action&step_class=WizStepDone&params[backup]='.urlencode($sBackupDestination).'" target="_blank"><img src="../images/icons/icons8-archive-folder.svg" style="border:0;vertical-align:middle;">&nbsp;Download '.basename($sBackupDestination).'</a>');
} else {
$oPage->p('<img src="../images/error.png"/>&nbsp;Warning: Backup creation failed !');
}
Expand Down Expand Up @@ -121,8 +121,6 @@ public function Display(SetupPage $oPage): void

$sTargetEnv = utils::HtmlEntities($this->oWizard->GetParameter('target_env', ITOP_DEFAULT_ENV));
$sForm = '<div class="ibo-setup--wizard--buttons-container" style="text-align:center"><form method="post" class="ibo-setup--enter-itop" action="'.$this->oWizard->GetParameter('application_url').'pages/UI.php?switch_env='.$sTargetEnv.'">';
$sForm .= '<input type="hidden" name="auth_user" value="'.utils::EscapeHtml($this->oWizard->GetParameter('admin_user')).'">';
$sForm .= '<input type="hidden" name="auth_pwd" value="'.utils::EscapeHtml($this->oWizard->GetParameter('admin_pwd')).'">';
$sForm .= "<button id=\"enter_itop\" class=\"ibo-button ibo-is-regular ibo-is-primary\" type=\"submit\">Enter ".ITOP_APPLICATION."</button></div>";
$sForm .= '</form>';

Expand Down Expand Up @@ -151,15 +149,6 @@ public function JSCanMoveBackward()
return 'return false;';
}

/**
* Tells whether this step of the wizard requires that the configuration file be writable
* @return bool True if the wizard will possibly need to modify the configuration at some point
*/
public function RequiresWritableConfig()
{
return false; //This step executes once the config was written and secured
}

public function AsyncAction(WebPage $oPage, $sCode, $aParameters)
{
SetupUtils::EraseSetupToken();
Expand Down
3 changes: 0 additions & 3 deletions setup/wizardsteps/WizStepInstall.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,6 @@ public function Display(SetupPage $oPage): void

$sJSONData = json_encode($aInstallParams);
$oPage->add('<input type="hidden" id="installer_parameters" value="'.utils::EscapeHtml($sJSONData).'"/>');

$sAuthentToken = $this->oWizard->GetParameter('authent', '');
$oPage->add('<input type="hidden" id="authent_token" value="'.$sAuthentToken.'"/>');
if (!$this->CheckDependencies()) {
$oPage->error($this->sDependencyIssue);
$oPage->add_ready_script(<<<JS
Expand Down
2 changes: 0 additions & 2 deletions setup/wizardsteps/WizStepInstallMiscParams.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,6 @@ public function Display(SetupPage $oPage): void
$sChecked = ($sSampleData == 'no') ? 'checked ' : '';
$oPage->p('<input id="sample_data_no" name="sample_data" type="radio" value="no" '.$sChecked.'><label for="sample_data_no">&nbsp;I am installing a <b>production</b> instance, create an empty database to start from.');
$oPage->add('</fieldset>');
$sAuthentToken = $this->oWizard->GetParameter('authent', '');
$oPage->add('<input type="hidden" id="authent_token" value="'.$sAuthentToken.'"/>');
$oPage->add_ready_script(
<<<EOF
$('#application_url').on('change keyup', function() { WizardUpdateButtons(); } );
Expand Down
3 changes: 1 addition & 2 deletions setup/wizardsteps/WizStepInstallOrUpgrade.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
*
* You should have received a copy of the GNU Affero General Public License
*/

use Combodo\iTop\Application\WebPage\WebPage;

/**
Expand Down Expand Up @@ -124,9 +125,7 @@ public function Display(SetupPage $oPage): void
$sTlsCA
);

$sAuthentToken = $this->oWizard->GetParameter('authent', '');
$oPage->add('</div>');
$oPage->add('<input type="hidden" id="authent_token" value="'.$sAuthentToken.'"/>');
//$oPage->add('</fieldset>');
$oPage->add_ready_script(
<<<JS
Expand Down
4 changes: 0 additions & 4 deletions setup/wizardsteps/WizStepLandingBeforeAudit.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,6 @@ public function Display(SetupPage $oPage): void
*/
public function UpdateWizardStateAndGetNextStep($bMoveForward = true): WizardState
{
// Change the rights to production config file !
$sBuildConfigFile = APPCONF.ITOP_DEFAULT_ENV.'/'.ITOP_CONFIG_FILE;
@chmod($sBuildConfigFile, 0770); // In case it exists: RWX for owner and group, nothing for others

$aWizardSteps = $this->GetWizardSteps();
$this->oWizard->SetWizardSteps($aWizardSteps);
$this->sCurrentState = count($aWizardSteps) - 1;
Expand Down
3 changes: 0 additions & 3 deletions setup/wizardsteps/WizStepSummary.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,9 +238,6 @@ public function Display(SetupPage $oPage): void

}

$sAuthentToken = $this->oWizard->GetParameter('authent', '');
$oPage->add('<input type="hidden" id="authent_token" value="'.$sAuthentToken.'"/>');

$oPage->add_ready_script(
<<<JS
$("#db_backup_path").on('change keyup', function() { WizardAsyncAction('check_backup', { db_backup_path: $('#db_backup_path').val() }); });
Expand Down
2 changes: 0 additions & 2 deletions setup/wizardsteps/WizStepUpgradeMiscParams.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,6 @@ public function Display(SetupPage $oPage): void
$oPage->add('</table>');
$oPage->add('<span id="graphviz_status"></span>');
$oPage->add('</fieldset>');
$sAuthentToken = $this->oWizard->GetParameter('authent', '');
$oPage->add('<input type="hidden" id="authent_token" value="'.$sAuthentToken.'"/>');
$oPage->add_ready_script(
<<<EOF
$('#application_url').on('change keyup', function() { WizardUpdateButtons(); } );
Expand Down
2 changes: 0 additions & 2 deletions setup/wizardsteps/WizStepWelcome.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,6 @@ public function GetPossibleSteps()

public function UpdateWizardStateAndGetNextStep($bMoveForward = true): WizardState
{
$sUID = SetupUtils::CreateSetupToken();
$this->oWizard->SetParameter('authent', $sUID);
return new WizardState(WizStepInstallOrUpgrade::class);
}

Expand Down