4 lezioni di codice da una falla di sicurezza

Settembre complicato, per Magento

Una critica falla di sicurezza di Adobe Commerce / Magento Open Source, la CVE-2026-75650 (bollettino APSB26-146), ha turbato il rientro dalle ferie di diversi merchants. L'agenzia statunitense CISA è intervenuta d'autorità imponendo alle ammnistrazioni federali appena 3 giorni di tempo per installare la patch; patch che a sua volta era arrivata a soli 3 giorni dalla scoperta del problema. Il tutto testimonia di quanto la situazione sia delicata, visto che il bug è stato scoperto quando erano stati già registrati i primi attacchi in grado di sfruttarla.

Quanto all'installazione della patch, rimandiamo alla classica procedura di Adobe. Qui vorremmo invece analizzarne il codice per capire cosa è successo e magari imparare qualcosa di nuovo.

Prima lezione: verificare il codice non significa eseguirlo!

"Qualsiasi cosa dirà potrà essere usata contro di Lei", cita la retorica giudiziaria. Dalla patch che stiamo esaminando, vediamo che è cosi anche per il codice; parafrasando potremmo dire "qualsiasi codice eseguirai potrà essere usato contro di te". Sembra scontato, ma guardate questo snippet in vendor/magento/module-backend/Model/Widget/Grid/Row/UrlGeneratorFactory.php:

<?php
$rowUrlGenerator = $this->_objectManager->create($generatorClassName, $arguments);
if (false === $rowUrlGenerator instanceof GeneratorInterface) {
    throw new \InvalidArgumentException('Passed wrong parameters');
}
return $rowUrlGenerator;

Questo è parte della falla di sicurezza rilevata. Quando entra in funzione instanceof, l'oggetto è già stato istanziato dalla riga precedente tramite ObjectManager. instanceof si limita a verificare che l'oggetto appartenga alla classe attesa, ma a quel punto il costruttore è già stato eseguito. Va da se quanto diventa pericoloso questo scenario quando il nome della classe e i parametri con cui costruirla sono controllabili dall'esterno: hai voglia a verificare la classe di appartenenza, se intanto gli abbiamo dato modo di.. aprir bocca, con __construct, prima ancora di poter effettuare il controllo.
La soluzione adottata dagli sviluppatori è questa:

<?php
if (!is_a($generatorClassName, GeneratorInterface::class, true)) {
    throw new \InvalidArgumentException('Passed wrong parameters');
}
return $this->_objectManager->create($generatorClassName, $arguments);

Il terzo parametro di is_a impostato a true indica che il primo parametro viene trattato come il nome di una classe, e non come un oggetto da analizzare. In questo modo il controllo viene effettuato prima che l'oggetto venga creato e il problema viene troncato a monte, visto che.. l'indagato non ha diritto di parola.

Esiste però un altro caso più sottile, nella classe che crea i blocchi html (vendor/magento/framework/View/Element/BlockFactory.php). Qui è stato necessario precedere "is_a" con una riga di decodifica della classe:

<?php
$resolvedType = $this->objectManagerConfig->getInstanceType(
    $this->objectManagerConfig->getPreference($blockName)
);
if (!is_a($resolvedType, BlockInterface::class, true)) {
    throw new \LogicException($blockName . ' does not implement BlockInterface');
}
$block = $this->objectManager->create($blockName, $arguments);

getPreference e getInstanceType servono per risalire alla classe reale che verrà istanziata. Questo perchè in Magento una classe può arrivare anche da una preference, ovvero un alias configurato in un file xml. Per cui è necessario capire bene l'identità dell'imputato :)
Adesso che BlockFactory lancia una LogicException, questa va aggiunta al codice chiamante. In Layout/Generator/Block.php:

<?php
} catch (\ReflectionException | \LogicException $e) {
    $this->logger->critical($e->getMessage());
}

nulla di nuovo, ma ci rinfresca la memoria: da php 7.1, l'operatore "|" consente di catturare più tipi di eccezione in un unico catch.

Seconda lezione: validare gli input dei metodi magici

Questa istruzione

<?php
$template->setTemplateText('Ciao {{var nome}}');

in Magento diventa, grazie alla classe DataObject, quello che effettivamente serve:

<?php
$template->setData('template_text', 'Ciao {{var nome}}');

senza bisogno di creare metodi "setNome()". Questo avviene grazie al metodo magico di php __call(). Comodissimo, ma i metodi magici possono ricevere valori di qualunque tipo; se lavoriamo JSON di Api Rest o GraphQL possiamo aspettarci cose ben peggiori di una stringa di testo.
Ecco allora la soluzione degli sviluppatori Adobe, in vendor/magento/module-email/Model/AbstractTemplate.php:

<?php
public function setTemplateText($value)
{
    return $this->setData('template_text', is_string($value) ? $value : '');
}

public function setTemplateStyles($value)
{
    return $this->setData('template_styles', is_string($value) ? $value : '');
}

un metodo reale ha sempre la precedenza sui 'magici' per cui adesso il codice passa per forza di cose da qui.

Terza lezione: marca a uomo, se vuoi proteggere davvero

Questa correzione è semplice (vendor/magento/module-email/Block/Adminhtml/Template/Preview.php):

<?php
protected function _toHtml()
{
    if (!$this->_authorization->isAllowed(self::ADMIN_RESOURCE)) {
        return '';
    }

in pratica viene aggiunta la verifica dei permessi su alcuni blocchi. Il problema finora non si era posto perché il controllo dei permessi c'era già, ma solo sul controller chiamante. L'aggiunta di un ulteriore check del blocco ci fa capire che è bene marcare a vista la possibile falla d'attacco, proprio nei punti in cui verrà effettivamente eseguita e non solo filtrando a monte la chiamata.
La stessa protezione viene aggiunta anche ai preview dei template e delle code del modulo Newsletter, con i relativi controlli ACL.

Quarta lezione: attento ai log innocui

Questa è semplicissima (vendor/magento/framework/Webapi/ErrorProcessor.php):

<?php
private const REPORT_EXECUTION_GUARD = '<?php exit; ?>';

$reportData = $this->sanitizeReportData($reportData);
@file_put_contents(
    $this->_reportFile,
    self::REPORT_EXECUTION_GUARD . PHP_EOL . $this->serializer->serialize($reportData) . PHP_EOL
);

Si tratta di files di report e si adottano misure di sicurezza per evitare che dentro possano finirci dei tag o peggio ancora istuzioni php. Sono cose che già sappiamo ma che, come vedete, è facile dimenticare o lasciare 'appese'. Sono infatti file che possono contenere dati provenienti dalla richiesta dell'utente e vengono scritti in una posizione prevedibile. Se una falla del genere è sfuggita ad una struttura come Adobe, non pensiamo che i nostri software saranno esenti da distrazioni.

La patch interviene inoltre in pub/errors/processor.php, dove i report vengono scritti in var/report/: anche qui viene aggiunto un prefisso e i dati vengono ripuliti in modo ricorsivo, sostituendo le occorrenze di <? con una forma non interpretabile come apertura di codice PHP.

L'obiettivo non è impedire che Magento possa leggere normalmente il report: quando il report viene successivamente visualizzato, il prefisso di protezione viene rimosso. L'obiettivo è fare in modo che, anche se quel file dovesse essere successivamente interpretato come PHP, l'eventuale contenuto malevolo non possa essere eseguito.