Add CompatForm based version of ConfigForm

[TheSyscall]

An implementation of an INI based configuration form for CompatForms.

It allows developers to create a configuration form for an INI file and have it automatically populate form elements and store the results back into the specified file or section.

If writing the configuration file fails, an error is displayed alongside the full contents of the file to copy and paste manually by the admin.

This form can optionally be used to delete a section of the configuration file or create a new one.

As suggested here

resolves #5479

[Al2Klimov]

Test

1. Install Fedora 42
2. Apply https://github.com/Al2Klimov/ansible-icinga-fedora using snapshot packages
3. yum remove icingaweb2-module-monitoring
4. Upgrade to https://git.icinga.com/packages/icingaweb/-/jobs/905522
5. Create /usr/share/icingaweb2/modules/test5480
6. icingacli mod en test5480
7. Visit /icingaweb2/test5480
8. Input something
9. Press enter
10. cat /etc/icingaweb2/modules/test5480/config.ini (ok)
11. chmod 0440 /etc/icingaweb2/modules/test5480/config.ini
12. Repeat steps 7-9 (fails, prints helpful message)

So far so good...

/usr/share/icingaweb2/modules/test5480

library/Test5480/Form.php

<?php

namespace Icinga\Module\Test5480;

use Icinga\Web\Form\ConfigForm;

class Form extends ConfigForm
{
    protected function assemble()
    {
        $this->addElement('text', 'foo__bar');
    }
}

application/controllers/IndexController.php

<?php

namespace Icinga\Module\Test5480\Controllers;

use GuzzleHttp\Psr7\ServerRequest;
use Icinga\Module\Test5480\Form;
use ipl\Web\Compat\CompatController;

class IndexController extends CompatController
{
    public function indexAction(): void
    {
        $form = new Form($this->Config());
        $form->handleRequest(ServerRequest::fromGlobals());
        $this->addContent($form);
    }
}

[Al2Klimov]

CSRF – ConfigSectionForm section deletion bypasses all validation

Summary

ConfigSectionForm (branch origin/config-form-5479) is vulnerable to
Cross-Site Request Forgery. An attacker who can lure an authenticated Icinga
Web 2 user to a malicious page can delete any configuration section managed by
that form without knowledge of the victim's CSRF token — because the delete
path in handleRequest() never reaches isValid().

Affected code

File	Method / location	Class
library/Icinga/Web/Form/ConfigSectionForm.php	constructor (ON_SENT registration), onSent(), handleDelete(), shouldDelete()	ConfigSectionForm
library/Icinga/Web/Form/ConfigForm.php	addButtonElements()	ConfigForm

Root-cause analysis

1 — How handleRequest() routes the request

ConfigSectionForm extends ConfigForm → CompatForm → ipl\Html\Form.
CompatForm adds neither FormUid nor CsrfCounterMeasure, so hasBeenSent()
reduces to a plain HTTP-method check:

// ipl\Html\Form::hasBeenSent()
return $this->request->getMethod() === $this->getMethod(); // true for any POST

Inside handleRequest() the branching is:

if ($this->hasBeenSubmitted()) {   // store button pressed?
    if ($this->isValid()) {        // CSRF would be checked here
        $this->emit(ON_SENT, …);
        $this->onSuccess();
    } else {
        $this->onError();
    }
} else {                           // ← taken when delete button is pressed
    $this->validatePartial();
    $this->emit(ON_SENT, …);       // fires WITHOUT isValid()
}

hasBeenSubmitted() calls getSubmitButton()->hasBeenPressed(). The submit
button tracked by ipl-html is the first addElement('submit', …) call,
which is the store button added by ConfigForm::addButtonElements(). The
delete button is registered via registerElement() (not addElement()), so
it is never the tracked submit button.

Pressing only the delete button means store was not pressed →
hasBeenSubmitted() = false → the else branch runs → ON_SENT fires.

2 — Delete handler registered on ON_SENT

// ConfigSectionForm::__construct()
$this->on(static::ON_SENT, $this->onSent(...));

// ConfigSectionForm::onSent()
protected function onSent(): void
{
    if ($this->shouldDelete()) {
        $this->handleDelete();         // deletes the INI section
        $this->emit(static::ON_DELETE, [$this]);
    }
}

shouldDelete() only checks whether the delete button element (delete) has
been pressed:

return $deleteButton->hasBeenPressed(); // getValue() === 'y'

getValue() reads directly from the populated request body. If the POST
body contains delete=y, this returns true — regardless of whether a CSRF
token was presented.

3 — No CSRF anywhere on this form stack

Neither CompatForm nor ConfigForm adds a CSRF counter-measure element.
The ipl\Web\Common\CsrfCounterMeasure trait is absent from the entire
inheritance chain. All state-mutating actions on ConfigSectionForm (create,
save, rename, delete) are therefore CSRF-vulnerable. The delete path is
the most critical because it bypasses even field-level validation.

Exploit

Minimal PoC (attacker-controlled HTML page)

<form id="f" method="POST"
      action="https://icinga.example.com/icingaweb2/<module>/edit?name=<section>">
  <input type="hidden" name="delete" value="y">
</form>
<script>document.getElementById('f').submit();</script>

The victim only needs to have an active Icinga Web 2 session. No CSRF token,
no form UID, no knowledge of any secret is required. The endpoint must use a
ConfigSectionForm subclass that has not called setAllowDeletion(false).

Step-by-step request trace

1. Victim's browser sends POST /<module>/edit?name=<section> with session
   cookie and body delete=y.
2. hasBeenSent() → true (POST).
3. populate(['delete' => 'y']).
4. ensureAssembled() → form built, delete button element added with
   submitValue = 'y'.
5. hasBeenSubmitted() → false (store button absent from body).
6. else branch: validatePartial() (no elements with values to validate
   because no field names match), then emit(ON_SENT).
7. ON_SENT handler (onSent()): shouldDelete() →
   deleteButton->getValue() === 'y' → true.
8. handleDelete():
   • $this->config->removeSection('<section>')
   • $this->config->saveIni()
   • Section permanently deleted from disk.

Impact

• Any authenticated Icinga Web 2 user can be CSRF'd into deleting arbitrary
  named sections from any INI config file managed by a ConfigSectionForm
  subclass that permits deletion (i.e. has not called setAllowDeletion(false)).
• Every module that adopts ConfigSectionForm from this branch inherits the
  vulnerability without any additional mistakes on the module author's part.

Fix recommendation

Register the delete logic on ON_SUBMIT (fires only after isValid()) rather
than ON_SENT. If skipping field validation for invalid-config deletion is
genuinely needed, validate at least the CSRF element explicitly before
calling handleDelete(). Alternatively, add CsrfCounterMeasure to
ConfigForm so all subclasses inherit CSRF protection automatically.

[Al2Klimov]

Nice that I get CSRF protection automatically, but #5480 (comment) seems not to work anymore:

No CSRF counter measure ID set

#0 /usr/share/php/Icinga/Web/Form/ConfigForm.php(149): Icinga\Web\Form\ConfigForm->addCsrfCounterMeasure()
#1 /usr/share/php/Icinga/Web/Form/ConfigForm.php(37): Icinga\Web\Form\ConfigForm->addRequiredElements()
#2 /usr/share/icinga-php/ipl/vendor/ipl/html/src/Form.php(235): Icinga\Web\Form\ConfigForm->ensureAssembled()
#3 /usr/share/icingaweb2/modules/test5480/application/controllers/IndexController.php(14): ipl\Html\Form->handleRequest()
#4 /usr/share/icinga-php/vendor/vendor/icinga/zf1/library/Zend/Controller/Action.php(528): Icinga\Module\Test5480\Controllers\IndexController->indexAction()
#5 /usr/share/php/Icinga/Web/Controller/Dispatcher.php(78): Zend_Controller_Action->dispatch()
#6 /usr/share/icinga-php/vendor/vendor/icinga/zf1/library/Zend/Controller/Front.php(954): Icinga\Web\Controller\Dispatcher->dispatch()
#7 /usr/share/php/Icinga/Application/Web.php(296): Zend_Controller_Front->dispatch()
#8 /usr/share/php/Icinga/Application/webrouter.php(107): Icinga\Application\Web->dispatch()
#9 /usr/share/icingaweb2/public/index.php(4): require_once(String)
#10 {main}

[TheSyscall]

That is correct. All ConfigForms require CsrfCounterMeasure. This is on request of @lippserd

[Al2Klimov]

Yeah, but shouldn't your ensureAssembled() make it working automagically in my mentioned test form?

[TheSyscall]

@lippserd would like to have the controller call it explicitly rather than burry the call site of setCsrfCounterMeasureId inside the form.

[Al2Klimov]

• We must get Add Str::isEmpty() ipl-stdlib#77 done before merging this PR

[Al2Klimov]

Passwords vanish from the config if I edit something, but not the password.

[TheSyscall]

Changed password elements to revert to their old value if submitted empty.

[Al2Klimov]

I can bypass the enforced delete button absence by emptying all fields and saving.

[TheSyscall]

Removed automatic removal of sections for ConfigSectonForm