Discussion :

[NooD]

Bonjour à tous,

J'ai récupéré le code d'une fonction extraite du code du logiciel libre xrdp et je tente désormais de corriger la faille de sécurité qui y est présente.

Voici le code de la fonction :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
int add_char_at(char* text, char ch, int index)
{
  int len;
  int i;
 
  len = g_strlen(text);
 
  if (index >= len || index < 0)
  {
    text[len] = ch;
    text[len + 1] = 0;
    return 0;
  }
  for (i = len - 1; i >= index; i--)
  {
    text[i + 1] = text[i];
  }
  text[i + 1] = ch;
  text[len + 1] = 0;
 
  return 0;
}

Le code suivant suffit à provoquer une "Erreur de segmentation" :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
int main()
{
	char text[3];
 
	strcpy(text, "ac");
 
	add_char_at(text, 'f', 1);
 
	return 0;
}

Selon moi le problème se situe au niveau de la taille véritable de la chaîne text. Cette taille devrait être comparée à la taille récupérée par strlen afin de savoir si la chaîne est pleine. Auquel cas, si on décale les caractères ou qu'on ajoute un caractère en fin de chaîne, alors il y a un débordement.

Il me semble qu'il n'est pas possible de récupéré la taille réelle de la chaîne étant donné qu'il s'agit d'un char * récupéré en paramètre.
Selon moi, la seule solution serait de modifier le code en profondeur afin d'ajouter comme nouveau paramètre la taille réelle de ce fameu char *.

Avez vous d'autres suggestions ou des critiques à faire vis à vis de mon explication ?

Bonne journée

[Médinoc]

Tu as raison, il faut passer la taille de buffer en paramètre.

Mais tant qu'on y est, n'oublie pas que ce code-ci est déjà faux:

Code C :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
	char text[3];
 
	strcpy(text, "abc");

Le buffer est trop petit pour y mettre le caractère nul terminal...

[NooD]

Oups L'erreur est corrigée.

Par contre pour le débordement n'y a-t-il pas un moyen qui évite d'avoir à réécrire tous les appels ? Car cela nécessite de trouver la taille de chaque chaîne qui est passée en paramètre.
En réalité les tailles sont inscrites au niveau des malloc (nombres magiques)...

Voici la correction :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

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
int add_char_at(char* text, int size, char ch, int index)
{
  int len;
  int i;
 
  len = g_strlen(text);
 
  if(size-1 == len)
    return 0;
 
  if (index >= len || index < 0)
  {
    text[len] = ch;
    text[len + 1] = 0;
    return 0;
  }
  for (i = len - 1; i >= index; i--)
  {
    text[i + 1] = text[i];
  }
  text[i + 1] = ch;
  text[len + 1] = 0;
 
  return 0;
}
 
int main()
{
	char text[3];
 
	strcpy(text, "ab");
 
	add_char_at(text, 3, 'f', 1);
 
	return 0;
}

[Médinoc]

Il y a un moyen en C++ quand il s'agit de tableaux statiques (ça implique des templates), mais pour des tableaux alloués dynamiquement, tu vas devoir réécrire tous les appels. Et si possible, transformer ces nombres magiques en defines.

Et déclare size en tant que size_t, c'est un conseil.

[NooD]

Il y a un moyen en C++ quand il s'agit de tableaux statiques (ça implique des templates), mais pour des tableaux alloués dynamiquement, tu vas devoir réécrire tous les appels. Et si possible, transformer ces nombres magiques en defines.

Et déclare size en tant que size_t, c'est un conseil.

En réalité voici le seul appel :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
 
xrdp_bitmap.c
int APP_CC xrdp_bitmap_def_proc(struct xrdp_bitmap* self, int msg, int param1, int param2)
{
...
add_char_at(self->caption1, 256, c, self->edit_pos);
...
}

J'ai écrit la taille en dur pour bien montrer la modification.
Comme je le présentais, caption1 est un attribut d'une structure dont le pointeur est passé en paramètre.

Et pour ne rien arranger, voici la structure :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
 
xrdp_types.h
struct xrdp_bitmap
{
...
char* caption1;
...
}

Et voici le malloc :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
 
xrpd_login_wnd.c
...
b->caption1 = (char*)g_malloc(256, 1);
...

Je vais tenter les #DEFINE et modifier l'int en size_t.

[jabbounet]

en C/C++

En general, un tableau de N élément vois ses indices aller de 0 à N-1,

1/ si g_strlen est equivalent a strlen,

--> alors ce if n'est pas correct... je prendrais plutot un '>' au lieu de '>='
--> alors text[len+1] est forcément hors de la taille de ton tableau

je ne comprend pas le but de ce bloc

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
 
if (index >= len || index < 0) {
    text[len] = ch;
    text[len + 1] = 0;
    return 0;
}

[souviron34]

J'ai récupéré le code d'une fonction extraite du code du logiciel libre xrdp et je tente désormais de corriger la faille de sécurité qui y est présente.

je dirais : un très bon exemple de la dangerosité du "libre"

[jowo]

je dirais : un très bon exemple de la dangerosité du "libre"

Que faut-il comprendre par cette remarque?

Que les softs payants ou "closed source" sont sans danger et "libre" de toute faille sécuritaire?

[souviron34]

Que faut-il comprendre par cette remarque?

Que les softs payants ou "closed source" sont sans danger et "libre" de toute faille sécuritaire?

pas forcément, mais qu'entre le travail d'un professionnel et celui de "n'importe qui", surtout si c'est inclus après dans un logiciel de professionnel, il y a plus de risques...



On fait beaucoup de foin de la contrefaçon et des choses "pas chères" qui viennent de Chine, mais c'est strictement pareil pour le libre et l'open-source...

[NooD]

Concernant ce logiciel, il faut savoir qu'il est en version beta (0.41 à l'heure actuelle) et qu'il est développé par une seule personne.
Au regard de la taille des sources, on peut comprendre qu'il y ait quelques erreurs. Une erreur humaine qui se retrouve dans tous les logiciels.

Un avantage du libre, justement, j'ai trouvé dans la version source du paquet debian un patch ([xrdp_0.4.0~dfsg.orig.tar.gz]) qui corrige ces failles et peut être d'autres, donc finalement, dans ce cas, le libre a bien joué son rôle