Sécuriser le cache meta #5066

Open
opened 9 months ago by cerdic · 11 comments
cerdic commented 9 months ago
Owner

Suite à #5064

@g0uz proposait

Une autre proposition pour protéger globalement tmp/meta_cache.php (ce fichier contient beaucoup d'information qui mériterait d'être protégé) d'un attaquant qui peut lire des fichiers serait de le stocker avec un nom non prédictible (et qui n'implique pas une requête en DB pour le reconstituer).

Est ce qu'on peut trouver une valeure sécrète et constante (qui change rarement) uniquement récupérable via PHP, très rapidement, qu'on pourrait utiliser pour stocker tmp/meta_cache_[md5(mavaleure)].php ?

gouz@T530 /home/gouz $ time php -r 'for ($i=0; $i< 1000;$i++) { 
$r=md5(serialize(ini_get_all())); }'

real    0m0,119s

C'est peut être une très mauvaise idée, je ne sais pas bien :)

Si bonne idée, l'implémentation peut être assez rapide si on conserve tmp/meta_cache.php qui contiendrait uniquement le code suivant :

<?php

$token=md5(serialize(ini_get_all()));
include("meta_cache_".$token.".php");
Suite à https://git.spip.net/spip/spip/pulls/5064#issuecomment-33588 @g0uz proposait > Une autre proposition pour protéger globalement tmp/meta_cache.php (ce fichier contient beaucoup d'information qui mériterait d'être protégé) d'un attaquant qui peut lire des fichiers serait de le stocker avec un nom non prédictible (et qui n'implique pas une requête en DB pour le reconstituer). > > Est ce qu'on peut trouver une valeure sécrète et constante (qui change rarement) uniquement récupérable via PHP, très rapidement, qu'on pourrait utiliser pour stocker tmp/meta_cache_[md5(mavaleure)].php ? > > ``` > gouz@T530 /home/gouz $ time php -r 'for ($i=0; $i< 1000;$i++) { > $r=md5(serialize(ini_get_all())); }' > > real 0m0,119s > ``` > > C'est peut être une très mauvaise idée, je ne sais pas bien :) > > > Si bonne idée, l'implémentation peut être assez rapide si on conserve tmp/meta_cache.php qui contiendrait uniquement le code suivant : > > > ``` > <?php > > $token=md5(serialize(ini_get_all())); > include("meta_cache_".$token.".php"); > ```
Poster
Owner

Je vois plusieurs points d'attention à creuser/valider sur cette piste:

  • par construction, le token va être potentiellement le même pour tous les sites d'un serveur, ça limite l'intérêt. Y inclure le $_SERVER['document_root'] peut-être ?
  • je vois un risque de bug si un plugin/squelette joue avec ini_set() sur un environnement où c'est possible : le token va possiblement changer.

Sur ce dernier point on peut limiter le risque en le mettant en static pour s'assurer qu'il ne change pas en cours de hit, mais on a quand même un risque en particulier de non invalidation du cache.
Actuellement quand on fait un ecrire_meta() pour changer la valeur d'une meta, SPIP fait un touch sur le fichier cache pour l'antidater, ce qui le rechargera au prochain hit. Cela évite de ré-écrire le fichier N fois si on fait N ecrire_meta() dans un hit. Mais le risque est que le hit ait un token différent des autres et n'invalide pas le bon cache.

Il faudrait donc faire un glob() pour invalider tous les autres caches possiblement présents dans ce cas ? C'est peut-être la bonne solution et pas si redhibitoire mais ça demande un peut de test

Je vois plusieurs points d'attention à creuser/valider sur cette piste: * par construction, le token va être potentiellement le même pour tous les sites d'un serveur, ça limite l'intérêt. Y inclure le `$_SERVER['document_root']` peut-être ? * je vois un risque de bug si un plugin/squelette joue avec `ini_set()` sur un environnement où c'est possible : le token va possiblement changer. Sur ce dernier point on peut limiter le risque en le mettant en static pour s'assurer qu'il ne change pas en cours de hit, mais on a quand même un risque en particulier de non invalidation du cache. Actuellement quand on fait un `ecrire_meta()` pour changer la valeur d'une meta, SPIP fait un touch sur le fichier cache pour l'antidater, ce qui le rechargera au prochain hit. Cela évite de ré-écrire le fichier N fois si on fait N `ecrire_meta()` dans un hit. Mais le risque est que le hit ait un token différent des autres et n'invalide pas le bon cache. Il faudrait donc faire un `glob()` pour invalider tous les autres caches possiblement présents dans ce cas ? C'est peut-être la bonne solution et pas si redhibitoire mais ça demande un peut de test

Est ce qu'agir uniquement sur la fonction cache_meta (pour essayer de ne garder qu'un seul fichier) serait suffisant ?

72a024a68f/ecrire/inc/meta.php (L252)

En adoptant la logique suivante :

function cache_meta($table = 'meta') {
	static $files = [];
	
	if ( isset($files[$table]) ){
		return $files[$table];
	}
	
	if ( $table == 'meta' ) {
		
		$caches_existants = glob(_DIR_CACHE . $table . '_*.php');
		if ( isset($caches_existants[0]) ){
			$cachefile = $caches_existants[0];
		} else {
			$token = md5(serialize(ini_get_all()).$_SERVER['document_root']);
			$cachefile = _DIR_CACHE . $table . '_' . $token . '.php';
		}
		
	} else {
		$cachefile = _DIR_CACHE . $table . '.php';
	}
	
	$files[$table] = $cachefile;
	return $cachefile;
}

J'avoue que j'ai du mal a comprendre comment un init_set() perso pourrait poser problème si on garde le nom du fichier dans une variable static et si ce code est bien executé avant le init_set() perso.

Niveau perf c'est pas terrible de faire un glob() a chaque hit no ?

Est ce qu'agir uniquement sur la fonction cache_meta (pour essayer de ne garder qu'un seul fichier) serait suffisant ? https://github.com/spip/SPIP/blob/72a024a68fd763ebb70926809a12d24efca5baa0/ecrire/inc/meta.php#L252 En adoptant la logique suivante : ``` function cache_meta($table = 'meta') { static $files = []; if ( isset($files[$table]) ){ return $files[$table]; } if ( $table == 'meta' ) { $caches_existants = glob(_DIR_CACHE . $table . '_*.php'); if ( isset($caches_existants[0]) ){ $cachefile = $caches_existants[0]; } else { $token = md5(serialize(ini_get_all()).$_SERVER['document_root']); $cachefile = _DIR_CACHE . $table . '_' . $token . '.php'; } } else { $cachefile = _DIR_CACHE . $table . '.php'; } $files[$table] = $cachefile; return $cachefile; } ``` J'avoue que j'ai du mal a comprendre comment un init_set() perso pourrait poser problème si on garde le nom du fichier dans une variable static et si ce code est bien executé avant le init_set() perso. Niveau perf c'est pas terrible de faire un glob() a chaque hit no ?
Poster
Owner

oui je pense pas qu'on veut fairer un glob à chaque hit en effet. Eventuellement lorsqu'on veut invalider le cache parce qu'on ecrit une meta.

Mais peut-être c'est pas nécessaire en effet, il faut vérifier et tester si la fonction est appelée suffisament tôt dans le hit pour exclure tout risque de dédoublement des caches

oui je pense pas qu'on veut fairer un glob à chaque hit en effet. Eventuellement lorsqu'on veut invalider le cache parce qu'on ecrit une meta. Mais peut-être c'est pas nécessaire en effet, il faut vérifier et tester si la fonction est appelée suffisament tôt dans le hit pour exclure tout risque de dédoublement des caches
Owner

$_SERVER['document_root'] n’est pas définit en CLI.
A priori un simple __DIR__ ou __FILE__ ou _ROOT_RACINE conviendrait tout autant.

Je propose aussi de déplacer dans tmp/cache/meta/{$table}_{$token}.php qui semble plus approprié (tmp/cache) pour ce(s) fichier(s).

`$_SERVER['document_root']` n’est pas définit en CLI. A priori un simple `__DIR__` ou `__FILE__` ou `_ROOT_RACINE` conviendrait tout autant. Je propose aussi de déplacer dans `tmp/cache/meta/{$table}_{$token}.php` qui semble plus approprié (tmp/cache) pour ce(s) fichier(s).
Owner

Je ne comprends pas pourquoi le test ne serait fait que sur 'meta' par ailleurs. Ni l’intérêt du glob()

// TODO: deprecated 4.1 _FILE_META
function cache_meta($table = 'meta') {
	static $files = [];
    static $token = null;
	
	if (isset($files[$table])) {
		return $files[$table];
	}
    if ($token === null) {
        $token = md5(serialize(ini_get_all()) . __FILE__);
    }

    sous_repertoire(_DIR_CACHE, 'meta');
    $files[$table] = _DIR_CACHE . 'meta/' . $table . '_' . $token . '.php';

	return $files[$table];
}

Cette version suppose que le token est identique pour toutes les tables de meta (bon comme on en utilise pas d’autres que spip_meta de toutes façons, ça change pas grand chose).

mais sinon il faut ajouter $table dans le md5 avec static $tokens = []

Par contre base_delete_all_dist appelle _FILE_META, mais un supprimer_repertoire('_DIR_CACHE . 'meta') devrait pouvoir le faire à la place.

Éventuellement avec un _DIR_CACHE_META définit en plus (il y a déjà _DIR_CACHE_XML après tout), mais bon… c’est certainement pas très utile.

Je ne comprends pas pourquoi le test ne serait fait que sur 'meta' par ailleurs. Ni l’intérêt du glob() ```php // TODO: deprecated 4.1 _FILE_META function cache_meta($table = 'meta') { static $files = []; static $token = null; if (isset($files[$table])) { return $files[$table]; } if ($token === null) { $token = md5(serialize(ini_get_all()) . __FILE__); } sous_repertoire(_DIR_CACHE, 'meta'); $files[$table] = _DIR_CACHE . 'meta/' . $table . '_' . $token . '.php'; return $files[$table]; } ``` Cette version suppose que le token est identique pour toutes les tables de meta (bon comme on en utilise pas d’autres que spip_meta de toutes façons, ça change pas grand chose). mais sinon il faut ajouter `$table` dans le md5 avec `static $tokens = []` Par contre `base_delete_all_dist` appelle `_FILE_META`, mais un `supprimer_repertoire('_DIR_CACHE . 'meta')` devrait pouvoir le faire à la place. Éventuellement avec un `_DIR_CACHE_META` définit en plus (il y a déjà `_DIR_CACHE_XML` après tout), mais bon… c’est certainement pas très utile.
Owner

Même

// TODO: deprecated 4.1 _FILE_META
function cache_meta($table = 'meta') {
	static $token = null;
	
	if ($token === null) {
		$token = md5(serialize(ini_get_all()) . __FILE__);
	}

	sous_repertoire(_DIR_CACHE, 'meta');
	return _DIR_CACHE . 'meta/' . $table . '_' . $token . '.php';
}

semble largement suffisant. Pas besoin de statifier une simple concaténation de chaines il me semble.

Même ```php // TODO: deprecated 4.1 _FILE_META function cache_meta($table = 'meta') { static $token = null; if ($token === null) { $token = md5(serialize(ini_get_all()) . __FILE__); } sous_repertoire(_DIR_CACHE, 'meta'); return _DIR_CACHE . 'meta/' . $table . '_' . $token . '.php'; } ``` semble largement suffisant. Pas besoin de statifier une simple concaténation de chaines il me semble.
Owner

Notons que ini_get_all() ne retourne pas le même contenu en CLI qu’en web (même avec le même php / php.ini semble t’il d’ailleurs.

Je sais pas si ça aurait une importance en utilisant spip-cli pour réaliser certains traitements ?

Notons que ini_get_all() ne retourne pas le même contenu en CLI qu’en web (même avec le même php / php.ini semble t’il d’ailleurs. Je sais pas si ça aurait une importance en utilisant spip-cli pour réaliser certains traitements ?
Poster
Owner

oui et non : à la lecture on va de toute façon charger depuis la base si on a pas le cache (d'ailleurs on pourrait forcer le cli à toujours lire depuis la base ?)

Mais je pense que de toute façon il faut prévoir pour l'invalidation de faire le touch sur tous les fichiers présents sur le disque, donc avec un glob (qui normalement n'est pas trop lourd à ce moment là)...

oui et non : à la lecture on va de toute façon charger depuis la base si on a pas le cache (d'ailleurs on pourrait forcer le cli à toujours lire depuis la base ?) Mais je pense que de toute façon il faut prévoir pour l'invalidation de faire le touch sur tous les fichiers présents sur le disque, donc avec un glob (qui normalement n'est pas trop lourd à ce moment là)...

Dans la function touch_meta comme ci dessous ca conviendrait ?

function touch_meta($antidate = false, $table = 'meta') {
	$file = cache_meta($table);
	if (!$antidate or !@touch($file, $antidate)) {
		$r = $GLOBALS[$table] ?? [];
		if ($table == 'meta') {
			unset($r['alea_ephemere']);
			unset($r['alea_ephemere_ancien']);
			// le secret du site est utilise pour encoder les contextes ajax que l'on considere fiables
			// mais le sortir deu cache meta implique une requete sql des qu'on a un form dynamique
			// meme si son squelette est en cache
			//unset($r['secret_du_site']);
			if ($antidate) {
				$r['touch'] = $antidate;
			}
	
			foreach ($file as glob(_DIR_CACHE . $table . '/*.php')){
			    ecrire_fichier_securise($file, serialize($r));
			}            
		} else {
			ecrire_fichier_securise($file, serialize($r));
		}   
	}
}
Dans la function touch_meta comme ci dessous ca conviendrait ? ``` function touch_meta($antidate = false, $table = 'meta') { $file = cache_meta($table); if (!$antidate or !@touch($file, $antidate)) { $r = $GLOBALS[$table] ?? []; if ($table == 'meta') { unset($r['alea_ephemere']); unset($r['alea_ephemere_ancien']); // le secret du site est utilise pour encoder les contextes ajax que l'on considere fiables // mais le sortir deu cache meta implique une requete sql des qu'on a un form dynamique // meme si son squelette est en cache //unset($r['secret_du_site']); if ($antidate) { $r['touch'] = $antidate; } foreach ($file as glob(_DIR_CACHE . $table . '/*.php')){ ecrire_fichier_securise($file, serialize($r)); } } else { ecrire_fichier_securise($file, serialize($r)); } } } ```
b_b added the
sécurité
label 9 months ago
b_b added this to the 4.1 milestone 9 months ago
b_b added the
amélioration
label 9 months ago

Même

// TODO: deprecated 4.1 _FILE_META
function cache_meta($table = 'meta') {
	static $token = null;
	
	if ($token === null) {
		$token = md5(serialize(ini_get_all()) . __FILE__);
	}

	sous_repertoire(_DIR_CACHE, 'meta');
	return _DIR_CACHE . 'meta/' . $table . '_' . $token . '.php';
}

semble largement suffisant. Pas besoin de statifier une simple concaténation de chaines il me semble.

C'est vrai que sous_repertoire() fait déjà ce travail.

Je peux proposer une PR avec ces fonctions cache_meta()/touch_meta() modifiées ?

> Même > ```php > // TODO: deprecated 4.1 _FILE_META > function cache_meta($table = 'meta') { > static $token = null; > > if ($token === null) { > $token = md5(serialize(ini_get_all()) . __FILE__); > } > > sous_repertoire(_DIR_CACHE, 'meta'); > return _DIR_CACHE . 'meta/' . $table . '_' . $token . '.php'; > } > ``` > > semble largement suffisant. Pas besoin de statifier une simple concaténation de chaines il me semble. C'est vrai que sous_repertoire() fait déjà ce travail. Je peux proposer une PR avec ces fonctions cache_meta()/touch_meta() modifiées ?
Owner

Des nouvelles de cette amélioration ?

Des nouvelles de cette amélioration ?
Sign in to join this conversation.
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.