cours/content/secu_logicielle/td7-analyse_statique_dynamique/index.md

---
title: "Sécurité logicielle : TD7 analyse statique / dynamique"
date: 2023-03-10
tags: ["Valgrind", "gcc"]
author:
 - Yorick Barbanneau
 - Gwendal Aupee
categories: ["Sécurité logicielle", "TD"]
---

## Partie 1

### getpw

Le message d'avertissement de `gcc` est:

```
 `getpw' function is dangerous and should not be used.
```

D'après le manuel de `getpw`, il n'y a pas de paramètre de taille, cette
fonction  est donc sensible aux attaques par dépassement de tampon (*buffer
overflow*).

Nous avons diminué la taille de `buf` à 12 et observons une erreur de
segmentation lors de la fin du programme. Avec analyse de la pile par `pframe`
nous pouvons observer que le retour de `getpw()` écrase des élément au dessus de
lui-même sur l'adresse de retour.

Il pourrait être possible d'intégrer un *shellcode* dans le champ *gecos* de
notre utilisateur afin d'exploiter ce **dépassement de tampon**.

## mktemp

Cette fonction ne se charge pas de créer le fichier, elle défini juste un nom.
Il est alors possible pour un attaquant de créer le fichier avant celui du
programme en ratissant le plus large possible. Nous avions vu ce cas de figure 
lors du premier TD sur les défis *Nebula*.

## Partie 2

### Question 1

Effectivement `cppcheck` retourne une erreur dans le code source entrainant une
fuite de mémoire :

```
cppcheck test-leak.c
Checking test-leak.c ...
test-leak.c:9:2: error: Memory leak: d [memleak]
 return c;
 ^
```

Dasn le code source, les espaces mémoire doivent être désalloués :

 * dans la fonction `f()` pour `d`
 * dans la fonction `g()` pour `c` (référencé sous `e` dans  cette dernière)

En terme de sécurité, une fuite mémoire permet à une attaque par **déni de
service**.

### Question 2

Ni `cppcheck` ni la version `asan` de l'exécutable ne remontent d'erreur. Seul
`valgrind` nous averti d'un espace mémoire accessible à la terminaison du
programme.

Valgrind nous donne des information sur la fuite de mémoire:

```
valgrind --leak-check=full --show-leak-kinds=all ./test-reachable-leak
==14383== Memcheck, a memory error detector
==14383== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==14383== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info
==14383== Command: ./test-reachable-leak
==14383== 
==14383== 
==14383== HEAP SUMMARY:
==14383==     in use at exit: 10 bytes in 1 blocks
==14383==   total heap usage: 1 allocs, 0 frees, 10 bytes allocated
==14383== 
==14383== 10 bytes in 1 blocks are still reachable in loss record 1 of 1
==14383==    at 0x48407B4: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==14383==    by 0x109146: main (test-reachable-leak.c:6)
==14383== 
==14383== LEAK SUMMARY:
==14383==    definitely lost: 0 bytes in 0 blocks
==14383==    indirectly lost: 0 bytes in 0 blocks
==14383==      possibly lost: 0 bytes in 0 blocks
==14383==    still reachable: 10 bytes in 1 blocks
==14383==         suppressed: 0 bytes in 0 blocks
==14383== 
==14383== For lists of detected and suppressed errors, rerun with: -s
==14383== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
```

En observant le code de notre programme nous pouvons voir que `*c` est un
**pointeur global** (dans le sens variable globale). Dans le `main()`, il prend
l'adresse d'une zone de mémoire créée avec `malloc`. **Cette zone mémoire n'est
pas libérée**, ainsi elle reste accessible à la fin du programme.

## Partie 3

Effectivement, `cppcheck` nous averti à propos d'une zone mémoire créée mais non
initialisées. La version `asan` ne fait aucun rapport mais *Valgrind* est lui un 
peu plus locace :

```
[...]
Conditional jump or move depends on uninitialised value(s)
   at 0x10917A: f (test-uninitialized.c:5)
   by 0x1091D0: main (test-uninitialized.c:13)
[...]
Use --track-origins=yes to see where uninitialised values come from
[...]
```

Lors du check de `test-uninitialized-printf.c` *Valgrind* nous averti sur
l'usage de valeur non-initialisée dont dépendent des *jump* ou *move*
conditionnels. Cette erreur provient de la variable `c` créée par un `malloc`
mais non initialisée.

Avec l'option `--track-origins=yes` suggérée par sa première exécution,
*Valgrind* nous informe de l'origine de l'allocation:

```
[...]
==14921== Conditional jump or move depends on uninitialised value(s)
==14921==    at 0x48BBEC7: __vfprintf_internal (vfprintf-process-arg.c:58)
==14921==    by 0x48B14FA: printf (printf.c:33)
==14921==    by 0x10917F: f (test-uninitialized-printf.c:5)
==14921==    by 0x1091A9: main (test-uninitialized-printf.c:10)
==14921==  Uninitialised value was created by a heap allocation
==14921==    at 0x48407B4: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==14921==    by 0x109194: main (test-uninitialized-printf.c:9)
[...]
```

Valgrind n'averti de ce problème que lors de la première utilisation de la
variable, ici dans `printf()`. C'est un comportement normal : c'est lors de
**l'accès en lecture de cette varible** que Valgrind peut déterminer cet accès
se fait alors que la variable n'est pas initialisée.

## Partie 4

Lors du lancement du programme `test-undefined`, nous pouvons constater que la
valeur afficher de notre variable `i` n'est pas celle attendue:

```bash
./test-undefined
-2147483648
```

Comme nous l'avons vu en cours, nous sommes en présence d'un cas non définis par
la norme C.

*Valgrind* lui de fait part d'aucun problème, il ne se soucis pas du type de la
variable `i` mais de l'espace qu'elle occupe en mémoire. Et il  **n'a pas
changé**.

``` shell
valgrind ./test-undefined
==3381== Memcheck, a memory error detector
==3381== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==3381== Using Valgrind-3.19.0 and LibVEX; rerun with -h for copyright info
==3381== Command: ./test-undefined
==3381== 
-2147483648
==3381== 
==3381== HEAP SUMMARY:
==3381==     in use at exit: 0 bytes in 0 blocks
==3381==   total heap usage: 1 allocs, 1 frees, 1,024 bytes allocated
==3381== 
==3381== All heap blocks were freed -- no leaks are possible
==3381== 
==3381== For lists of detected and suppressed errors, rerun with: -s
==3381== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
```

La version *usan* indique effectivement un overflow de notre variable `i` :

```
./test-undefined.usan 
test-undefined.c:7:3: runtime error: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int'
-2147483648
```

L'option de compilation `-fsanitize=undefined` utilisée pour cette version de
l'exécutable ajoute des éléments pour vérifier les **comportements indéfinis**.

## Partie 5

### Question 1

`cppcheck` nous indique bien un *overrun* : 

```bash
cppcheck test-overrun.c     
Checking test-overrun.c ...
Array 'c[10]' accessed at index 10, which is out of bonds.[arrayIndexOutOfBounds]
 c[10] = 1;
  ^
```

Le fichier en `.asan` nous indique un : *heap-buffer-overflow* avec plus de
détails (registres pc bp et sp, adresses, etc) :

```
/test-overrun.asan 
=================================================================
==6036==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200000001a 
  at pc 0x55f0c2aa51cb bp 0x7ffc2aa141d0 sp 0x7ffc2aa141c8
WRITE of size 1 at 0x60200000001a thread T0
[...]
Shadow bytes around the buggy address:
  0x0c047fff7fb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c047fff7fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c047fff7fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c047fff7fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c047fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c047fff8000: fa fa 00[02]fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8010: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff8020: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
[...]
```

L'élément du tableau `c[10]` est incorrect car `malloc(10)` correspond à un 
tableau de 10 éléments avec des indices de  **0 à 9** donc `c[10]` est 
*out-of-bond*.

LA version `usan` ne reporte aucune erreur, tout comme *Valgrind*.

### Question 2

Pour cette version, *Valgrind* ne reporte aucune erreur. Il ne se rend pas
compte que l'accès en dehors de la variable dans le segment `data`. La version
`usan` de notre exécutable elle affiche des erreurs:

```
./test-overrun-data.usan 
test-overrun-data.c:7:3: runtime error: index 10 out of bounds for type 'char [10]'
test-overrun-data.c:7:8: runtime error: store to address 0x55e89a7ca102 
  with insufficient space for an object of type 'char'
0x55e89a7ca102: note: pointer points here
 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00
              ^ 
test-overrun-data.c:8:3: runtime error: index -1 out of bounds for type 'char [10]'
```

`cppcheck` indique lui aussi deux erreurs pour `c[10]` et `c[-1]` :

```
cppcheck test-overrun-data.c 
Checking test-overrun-data.c ...
test-overrun-data.c:7:3: error: Array 'c[10]' accessed at index 10, which is out of bounds. [arrayIndexOutOfBounds]
 c[10] = 1;
  ^
test-overrun-data.c:8:3: error: Array 'c[10]' accessed at index -1, which is out of bounds. [negativeIndex]
 c[-1] = 1;
  ^
```

La version `asan` ne reporte non plus aucune erreur avec la version de *GCC*
installée au *CREMI*. Cependant sur nos machines -- avec *Debian Unstable* comme
système -- la version `asan` reporte une erreur.

Dans la version installée au CREMI, l'*address sanitizer* inclus dans *GCC*,
l'option `-fno-common` n'est pas activée par défaut. Sans cette option il n'est
pas possible pour lui de détecter le problème : il ne sais définir les limites
des données globales non initialisés (stockées dans le segment *BSS*)

le module *Memcheck* de Valgrind ne détecte pas les erreurs de type 
*out-of-bound* sur la pile, c'est une limitation documentée par l'équipe de
dévellopement :

> Unfortunately, Memcheck doesn't do bounds checking on global or stack arrays. 
> We'd like to, but it's just not possible to do in a reasonable way that fits 
> with how Memcheck works. Sorry.

[source](https://valgrind.org/docs/manual/faq.html#faq.overruns)

### Question 3

Dans la version `asan`, l'option de compilation `-fsanitize=address` active par
défaut l'option `asan-stack` chargée de vérifier les erreurs de type 
`out-of-bounds` et `use-after-free`.

Lorsque nous exécutons `test-overrun-stack` avec *GDB*, après l'instruction
`c[-1] = 1` nous obtenons la pile suivante:

```
[...]
0x7fffffffe430            0x00007fffffffe520
0x7fffffffe428    ret@    0x00007ffff7dfb18a
0x7fffffffe420      bp sp 0x0000000000000001 <- overflow de c[10]
0x7fffffffe418            0x00007ffff7ffdad0
0x7fffffffe410            0x0000010000000000 <- overlow de c[-1]
[...]
```

Avec l'aide le la commande `print`, nous pouvons alors vous les adresses de `c`

```
(gdb) p &c[0]
$4 = 0x7fffffffe416 ""
(gdb) p &c[9]
$5 = 0x7fffffffe41f ""
```

Ici `c[10]` passe dans le segment de pile au dessus, déclenchant l'erreur dans
notre exécutable `asan`.

## Partie 8

### Question 1

Le bug provient d'un problème de priorité : l'égalité est prioritaire sur tout
le reste dans la condition:

```c
// extract bit 0 to check for parity of the number of arguments
if (argc & 1 == 0) {
    // ...
```

Elle sera toujours fausse. La correction est simple:

```c
// extract bit 0 to check for parity of the number of arguments
if ((argc & 1) == 0) {
    // ...
```

### Question 2

C'est à la ligne 718 qu'il faut chercher l'erreur:

```c
 while ((ret = xfs_bulkstat(fsfd,
                                &lastino, GRABSZ, &buf[0], &buflenout) == 0)) {
```

La parenthèse fermante définissant la priorité de l'affectation est mal placée,
la comparaison se transforme donc en affectation. Utiliser cette notation permet
d'affecter `ret` pendant la condition de la boucle, mais en contrepartie
**augmente le risque d'erreur**.

### Question 3

L'instruction `assert` est mal utilisée, le simple signe égal indique une
affectation et non une comparaison 

```cpp
assert(n = int(*(p+1)), "just checkin'..."); 
```

La vérification sera toujours vrai, quoi qu'il arrive. Aucune erreur sera
remontée et le programme continuera son exécution alors **qu'il ne devrait
pas**.

`assert()` n'est utilisé que pour du *debug* dans le cas où la macro `NDEBUG`
n'est pas définie au moment de la compilation. La portée de cette erreur était
donc limitée.

### Question 4

L'outil d'analyse statique a détecté l'erreur car la méthode `screen->empty()`
retourne un booléen, or ici aucune valeur de retour est utilisée.

### Question 5

```c
for (i = 0; g_objArray[i] && i < MAX_WCOMP; i++) {
    MultiMoveRelXY(g_objArray[i], x - center, deltay);
```

L'erreur d'accès est causée par `g_objArray[i]`, en effet le programme teste la
validité de la case mémoire avant la validité de `i`. Le programme accède à la
case mémoire avant de vérifier les limites.

Il est simple de fixer le problème: inverser les conditions comme ci-dessous:

```c
for (i = 0; i < MAX_WCOMP && g_objArray[i]; i++) {
    MultiMoveRelXY(g_objArray[i], x - center, deltay);
```

## Partie 9

### Question 1

La portion de code est appelée *dead code* car quel que soit les conditions,
cette portion ne sera jamais atteinte. Ici, `decode` est définit à `false` et
jamais modifiée (l. 160), ainsi de la condition l. 174 sera toujours fausse et
donc les instructions dans la `if` jamais atteintes.


### Question 2

La valeur de `skip` est modifiée à la ligne 751, puis est suivie d'une
instruction `continue` qui relance un tour de boucle. La valeur de `skip` est
alors réinitialisée à *1* au début de cette boucle. La modification effectuée
alors est **totalement inutile**.

### Question 3

Le bloc d'instruction du `case` ligne 1240 n'est terminé ni par un `break` --
permettant de sortir du `switch` -- ni d'un `return`. Ainsi l'exécution du
programme continuer dans le `case` suivant (ou le `default`). Des portion de
code peuvent alors être exécutées dans des cas non prévus.

### Question 4

Ici le test de la boucle `uint x >= 0` est toujours vrai, on entre alors dans une
boucle infinie.

### Question 5

## Partie 10

### Question 1

### Question 2

`pos` pointe vers une variable locale a locale fonction `Texture::render()`.
Lors de la fin de cette dernière, alors `pos` pointera vers une zone mémoire qui
n'est plus disponible.

### Question 3

Dans la fonction `setParent( ExecutionNode parent)`, si `parent` est *NULL*
alors une erreur de type *Null pointer dereferences* se produit lors de
l'instruction `getContext().setParent(parent.getContext());`. Le développeur
teste cependant la validité du pointeur juste avant.

### Question 4

L'analyseur a détermine que `pos` ne pouvait pas être égal à 0 pour deux
raisons:

 * la signature de la fonction `strchr` renvoie un pointeur, `p` contient donc
   une adresse
 * dans le cas ou aucun caractère correspondant n'est trouvé, le programme ne
   rentre pas dans la boucle c'est la condition 
   `((pos = strchr(lastpos, '&')) != NULL)` qui le défini.

Si `pos` est défini, alors sont adresse est incrémentée de 1 (l. 1716), `pos`
n'est donc jamais égal à 0, cependant il est possible que la case mémoire
**pointée par `p` le soit**, indiquant une fin de chaine de caractères.

### Question 5

Là encore on est face à un problème entre le type pointeur (qui contient une
adresse, sont un `int`) et la zone mémoire vers la quelle il pointe.

L'instruction suivante alloue une zone mémoire égale à un `int`:

```c
Device_Entry *entry = calloc(1, sizeof(entry));
```

Alors que l'instruction suivante alloue nue zone mémoire de la taille de
`Device_Entry`:

```c
Device_Entry *entry = calloc(1, sizeof(*entry));
```

### Question 6

Lors de la première exécution de la boucle ` for (cur_module = wmodules;
cur_module; wmodules = next_module)`, le programme libère la zone mémoire
utilisée par la variable `cur_module` (l. 60). Cependant cette variable est
utilisée ensuite dans le test de la boucle `for`. Tous les modules ne seront
donc pas libérés (s'il y en a plus d'un).

```c
for (cur_module = wmodules; cur_module; wmodules = next_module) {
    next_module = cur_module->next;
    cur_module->context->destroy(cur_module->data);
    free(cur_module);
}
```

Dans la correction proposée, `cur_module` est aussi utilisée dans la définition
de la boucle `for`, ainsi là aussi notre boucle ne fera pas plus d'un tour.

## Partie 11

### Question 1

La dernière fonction appelée par ne donne pas lieu à un test comme les
précédente, ainsi si tous les autres test réussissent mais pas ce dernier, alors
le résultat sera vrai.

Le développeur aura du garder le fonctionnement des élément précédent et ainsi
écrire:

```c
if (!checkPriv(fromDBbackend, QLatin1String("CheckPriv_Cleanup")) {
    result = false;
}
```

### Question 2

`TAINTED_STRING` signifie qu'un ou plusieurs éléments externes influnce(nt) le
comportement de notre programme alors que ces éléments ne sont pas examinés.

Ici le code permet de lancer des commandes SQL contenues dans le fichier pointe
par `optFile` sans aucun contrôle. Un attaquant pourrait alors réaliser une
attaque par **injection SQL**.