Protection contre les buffer overflows

Version imprimable

Je commence en c et j'ai découvert la joie des buffer overflows :roll:

J'ai fait un petit exercice et je me demandais si le code était sécuritaire. Le but de mon petit exercice était justement qu'il soit sécuritaire (ne pas utiliser gets() ou de choses du genre) . . .

Voilà la partie du code en question:
Code:

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 char *input; FILE *filename; input = (char *)malloc(32); fgets(input,32,stdin); filename = fopen("yourname.txt","w"); if(!filename){ puts("FILE ERROR"); return 0; } fprintf(filename,input); fclose(filename); free(input);
P.S. Pour Emmanuel Delahaye (je suis presque certain que vous allez lire ce post), je crois que j'ai enfin compris pourquoi il vaut mieux utiliser fgets() et aussi comment l'utiliser ;)

Merci beaucoup!
Alex

28/08/2008, 06h32
stephl
Quelques remarques:
- Même s'il est peu vraisemblable que l'allocation de 32 bytes échoue ici, il est préférable de tester que l'allocation de mémoire a fonctionné avant d'essayer de stocker quelque chose à cet endroit.
- La mémoire n'est pas libérée si l'ouverture de fichier échoue.
- Le fprintf() pose problème si input contient des caractères %, car fprintf() s'attendra alors à trouver des paramètres supplémentaires. La façon correcte de faire est:
  
  Code:
  
  fprintf(filename, "%s", input);
- filename n'est pas un nom très bien choisi. Le filename, selon moi, c'est le premier paramètre passé à fopen(). J'aurais plutôt appelé la variable file, ou f, ou outfile, etc...
28/08/2008, 09h12
nicolas.sitbon

Citation:

Envoyé par stephl

La façon correcte de faire est:

Code:

fprintf(filename, "%s", input);

Code:

fputs (input, filename);
28/08/2008, 13h36
oniric
Citation:

Envoyé par stephl

Même s'il est peu vraisemblable que l'allocation de 32 bytes échoue ici, il est préférable de tester que l'allocation de mémoire a fonctionné avant d'essayer de stocker quelque chose à cet endroit.

comme le dit steph si tu veux jouer la securite jusqu'au bout, tu peux te coder une fonction par dessus malloc qui protege. Personnelement pour certain projet critique j'ai besoin de le faire. Il suffit de faire une fonction du type :
Code:

1 2 3 4 5 6 7 8 9 10 11 12 13 void *xmalloc(size_t size) { void *ptr = malloc(size); if (ptr == NULL) { write(STDERR_FILENO, "Memory Exausted\n", 16); exit(1); } return ptr; }
28/08/2008, 14h37
stephl

Citation:

Envoyé par nicolas.sitbon

Code:

fputs (input, filename);

Je suis d'accord, c'est une autre solution. J'ai juste voulu indiquer la manière correcte de faire en utilisant fprintf(), car le problème ne venait pas de la fonction utilisée, mais de l'appel qui en était fait.

Merci de vos conseils.

J'ai refait le petit bout de code . . . Est-ce que c'est mieux?

Code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
 
	char *input;
	FILE *fname;
 
	input = (char *)malloc(32);
 
	if(input==NULL){
		puts("Memory allocation error");
		return 0;
	}
 
	fgets(input,32,stdin);
	fname = fopen("yourname.txt","w");
	if(!fname){
		puts("FILE ERROR");
		free(input);
		return 0;
	}
	fprintf(fname,"%s",input);
	fclose(fname);
	free(input);
 
	return 0;

Merci!
Alex

28/08/2008, 16h38
stephl

Citation:

Envoyé par Alexandreg12

Merci de vos conseils.

J'ai refait le petit bout de code . . . Est-ce que c'est mieux?

Oui, c'est mieux. Mais je ne comprends toujours pas le nom de variable choisi (fname). Le filename, c'est ce que l'on passe à fopen() pour indiquer à quel fichier on souhaite accéder (ici, yourname.txt). fopen() ne retourne pas un nom de fichier, mais plutôt une sorte de handle, comme un ID (plus précisément, c'est un pointeur vers une structure comme l'indique la définition). On peut assimiler cela au fichier si on le souhaite et ainsi nommer la variable file, outputfile, etc... Personnellement, je trouve étrange d'utiliser le nom fname. Mais bon, ce n'est qu'un détail.
28/08/2008, 16h45
Invité1

Merci!

Ahh . . . pour le fname . . . Je savais juste pas quoi mettre et puis c'est juste un code d'exemple . . . C'est vrai que c'est pas un très bon nom;)

Merci beaucoup

Alex
28/08/2008, 18h47
Obsidian
Citation:

Envoyé par Alexandreg12

Merci de vos conseils.

J'ai refait le petit bout de code . . . Est-ce que c'est mieux?

Tu progresses vite et bien, ça fait plaisir à voir ! Histoire de chipoter, puisque ton code est presque irréprochable, tu peux aussi prendre l'habitude d'initialiser tes variables et pointeurs à la déclaration :
Code:

1 2 char *input=NULL; FILE *fname=NULL;
Tel qu'il est écrit, ton code présent ne peut pas planter, mais ça te rendra de grands services lors de développements de projets de plus grande envergure.

C'est pas forcément nécessaire si tu sais ce que tu veux car cela a un coût lors d'une allocation dynamique (y compris variables locales dans la pile), car il faut alors les effacer logiciellement, mais ce coût est minime pour de si petites variables.

Code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
#include <stdio.h>
#include <stdlib.h>
 
#define MY_FILE    "yourname.txt"
#define INPUT_SIZE 32
 
int main (void)
{
   int ret = EXIT_SUCCESS;
   FILE *fname = fopen (MY_FILE,"w");
 
   if (fname != NULL)
   {
      char *input = malloc (INPUT_SIZE * sizeof *input);
 
      if (input != NULL)
      {
         if (fgets (input, INPUT_SIZE, stdin) != NULL)
         {
            fputs(input, fname);
         }
         else if (ferror (stdin) != 0)
         {
            perror ("fgets");
            ret = EXIT_FAILURE;
         }
 
         free (input), input = NULL;
      }
      else
      {
         perror ("malloc");
         ret = EXIT_FAILURE;
      }
 
      fclose (fname), fname = NULL;
   }
   else
   {
      perror ("fopen");
      ret = EXIT_FAILURE;
   }
 
   return ret;
 }

28/08/2008, 20h15
milouz123

Je pense que ce code serait un peu plus lisible avec les clauses de sortie en cas d'erreur directement après chaque test.
28/08/2008, 20h29
nicolas.sitbon

Citation:

Envoyé par milouz123

Je pense que ce code serait un peu plus lisible avec les clauses de sortie en cas d'erreur directement après chaque test.

Tu n'as jamais du faire de gros projets!
Bonne pratique de programmation : un point d'entrée, un point de sortie.
28/08/2008, 20h35
Emmanuel Delahaye
Citation:

Envoyé par Alexandreg12

J'ai refait le petit bout de code . . . Est-ce que c'est mieux?

C'est inutilement complexe et il manque l'essentiel.

1 - pourquoi allouer une taille fixe ?
2 - tous ces 'return' me donnent le tournis. Il y a une logique séquentielle à respecter pour écrire du code sérieux, sinon, un jour ou l'autre, ou oublie de libérer une ressource...

Le principe, c'est
Code:

1 2 3 4 5 6 7 8 9 10 11 12 T *p = allocation(); if (p != NULL) { /* utilisation de p */ liberation (p), p = NULL; } else { /* erreur */ }
Et si on a affaire à un deuxième objet alloué, on imbrique.

Ca donne le code qu'a posté Nicolas... qui a été à bonne école !

Par contre attention. fgets() est certes sûre si on passe une taille cohérente et 2ème paramètre, par contre, il y a 2 choses importantes à garder en mémoire :

- si il y a la place, le caractère de fin de ligne ('\n') est stocké dans la chaine saisie.
- si il n'y a pas la place, la chaine est tronquée (mais reste valide), le '\n' n'est pas stocké dans la chaine, ainsi que les caractères qui le précèdent si il y en a. Le fait que le '\n' nait pas été lu fait que l'appel suivant de fgets() n'est pas bloquant.

Selon les besoins, il faut appliquer les traitements adéquates.

Si tu veux faire une fonction vraiment intéressante et utile (je m'en sers tous les jours), écris une saisie de ligne de taille quelconque à base de fgetc() et de realloc(). Là, l'usage de l'allocation dynamique sera justifié.
Code:

1 2 3 4 5 char *get_line_dyn (void) { /* ... */ }
qui s'utilise évidemment comme ceci :
Code:

1 2 3 4 5 6 7 8 9 10 11 12 char *line = get_line_dyn(); if (line != NULL) { /* utilisation de line */ free (line), line = NULL; } else { /* erreur */ }