Pourquoi ce code est-il vulnérable aux attaques par débordement de tampon?

148
int func(char* str)
{
   char buffer[100];
   unsigned short len = strlen(str);

   if(len >= 100)
   {
        return (-1);
   }

   strncpy(buffer,str,strlen(str));
   return 0;
}

Ce code est vulnérable à une attaque par débordement de tampon, et j'essaie de comprendre pourquoi. Je pense que cela a à voir avec le fait d' lenêtre déclaré un shortau lieu d'un int, mais je ne suis pas vraiment sûr.

Des idées?

Jason
la source
3
Il y a plusieurs problèmes avec ce code. Rappelez-vous que les chaînes C sont terminées par NULL.
Dmitri Chubarov
4
@DmitriChubarov, non null, terminer la chaîne ne posera problème que si la chaîne est utilisée après l'appel à strncpy. Dans ce cas, ce n'est pas le cas.
R Sahu
43
Les problèmes de ce code découlent directement du fait qu'il strlenest calculé, utilisé pour le contrôle de validité, puis il est à nouveau calculé de manière absurde - c'est un échec DRY. Si le second strlen(str)était remplacé par len, il n'y aurait aucune possibilité de débordement de tampon, quel que soit le type de len. Les réponses n'abordent pas ce point, elles parviennent simplement à l'éviter.
Jim Balter
3
@CiaPan: Si vous lui passez une chaîne non terminée par un zéro, strlen affichera un comportement non défini.
Kaiserludi
3
@JimBalter Non, je pense que je vais les laisser là. Peut-être que quelqu'un d'autre aura la même idée fausse et en tirera des leçons. N'hésitez pas à les signaler s'ils vous ennuient, quelqu'un pourrait venir les supprimer.
Asad Saeeduddin

Réponses:

192

Sur la plupart des compilateurs, la valeur maximale de an unsigned shortest 65535.

Toute valeur au-dessus de cela est bouclée, donc 65536 devient 0 et 65600 devient 65.

Cela signifie que de longues chaînes de la bonne longueur (par exemple 65600) passeront le contrôle et déborderont de mémoire tampon.


Permet size_tde stocker le résultat de strlen(), non unsigned shortet de le comparer lenà une expression qui code directement la taille de buffer. Donc par exemple:

char buffer[100];
size_t len = strlen(str);
if (len >= sizeof(buffer) / sizeof(buffer[0]))  return -1;
memcpy(buffer, str, len + 1);
orlp
la source
2
@PatrickRoberts Théoriquement, oui. Mais vous devez garder à l'esprit que 10% du code est responsable de 90% du temps d'exécution, vous ne devez donc pas laisser passer les performances avant la sécurité. Et gardez à l'esprit qu'avec le temps, le code change, ce qui peut soudainement signifier que la vérification précédente a disparu.
orlp
3
Pour éviter tout débordement de tampon, utilisez simplement lencomme troisième argument de strncpy. Utiliser à nouveau strlen est stupide dans tous les cas.
Jim Balter
16
/ sizeof(buffer[0])- notez que sizeof(char)dans C est toujours 1 (même si un caractère contient un milliard de bits) donc c'est superflu quand il n'y a pas de possibilité d'utiliser un type de données différent. Encore ... bravo pour une réponse complète (et merci d'avoir été réactif aux commentaires).
Jim Balter
4
@ rr-: char[]et char*ne sont pas la même chose. Il existe de nombreuses situations dans lesquelles un char[]will est implicitement converti en un fichier char*. Par exemple, char[]est exactement le même que char*lorsqu'il est utilisé comme type d'arguments de fonction. Cependant, la conversion ne se produit pas pour sizeof().
Dietrich Epp
4
@Controll Parce que si vous modifiez la taille de bufferà un moment donné, l'expression se met à jour automatiquement. Ceci est essentiel pour la sécurité, car la déclaration de bufferpeut être à quelques lignes de l'enregistrement du code réel. Il est donc facile de changer la taille du tampon, mais oubliez de mettre à jour à chaque emplacement où la taille est utilisée.
orlp
28

Le problème est ici:

strncpy(buffer,str,strlen(str));
                   ^^^^^^^^^^^

Si la chaîne est supérieure à la longueur du tampon cible, strncpy la copiera toujours. Vous basez le nombre de caractères de la chaîne comme le nombre à copier au lieu de la taille du tampon. La bonne façon de procéder est la suivante:

strncpy(buffer,str, sizeof(buff) - 1);
buffer[sizeof(buff) - 1] = '\0';

Cela limite la quantité de données copiées à la taille réelle de la mémoire tampon moins un pour le caractère de fin nul. Ensuite, nous définissons le dernier octet du tampon sur le caractère nul en tant que sauvegarde supplémentaire. La raison en est que strncpy copiera jusqu'à n octets, y compris le null de fin, si strlen (str) <len - 1. Sinon, le null n'est pas copié et vous avez un scénario de plantage car maintenant votre tampon a un non terminé chaîne.

J'espère que cela t'aides.

EDIT: Après un examen plus approfondi et des contributions d'autres personnes, un codage possible pour la fonction suit:

int func (char *str)
  {
    char buffer[100];
    unsigned short size = sizeof(buffer);
    unsigned short len = strlen(str);

    if (len > size - 1) return(-1);
    memcpy(buffer, str, len + 1);
    buffer[size - 1] = '\0';
    return(0);
  }

Puisque nous connaissons déjà la longueur de la chaîne, nous pouvons utiliser memcpy pour copier la chaîne de l'emplacement référencé par str dans le tampon. Notez que d'après la page de manuel de strlen (3) (sur un système FreeBSD 9.3), ce qui suit est indiqué:

 The strlen() function returns the number of characters that precede the
 terminating NUL character.  The strnlen() function returns either the
 same result as strlen() or maxlen, whichever is smaller.

Ce que j'interprète comme étant que la longueur de la chaîne n'inclut pas le null. C'est pourquoi je copie len + 1 octets pour inclure la valeur null, et le test vérifie que la longueur <taille du tampon - 2. Moins un car le tampon commence à la position 0, et moins un autre pour s'assurer qu'il y a de la place pour le nul.

EDIT: Il s'avère que la taille de quelque chose commence par 1 tandis que l'accès commence par 0, donc le -2 avant était incorrect car il renverrait une erreur pour tout ce qui> 98 octets mais il devrait être> 99 octets.

EDIT: Bien que la réponse à propos d'un court non signé soit généralement correcte car la longueur maximale pouvant être représentée est de 65 535 caractères, cela n'a pas vraiment d'importance car si la chaîne est plus longue que cela, la valeur s'enroulera. C'est comme prendre 75,231 (qui est 0x000125DF) et masquer les 16 premiers bits vous donnant 9695 (0x000025DF). Le seul problème que je vois avec ceci est les 100 premiers caractères après 65 535 car la vérification de la longueur autorisera la copie, mais elle ne copiera que les 100 premiers caractères de la chaîne dans tous les cas et la valeur null terminera la chaîne . Ainsi, même avec le problème de bouclage, le tampon ne sera toujours pas débordé.

Cela peut ou non poser en soi un risque de sécurité en fonction du contenu de la chaîne et de l'utilisation que vous en faites. S'il ne s'agit que d'un texte simple lisible par l'homme, il n'y a généralement pas de problème. Vous obtenez juste une chaîne tronquée. Cependant, si c'est quelque chose comme une URL ou même une séquence de commandes SQL, vous pourriez avoir un problème.

Daniel Rudy
la source
2
C'est vrai, mais cela dépasse le cadre de la question. Le code montre clairement la fonction passée un pointeur de caractère. En dehors de la portée de la fonction, nous ne nous soucions pas.
Daniel Rudy
"le tampon dans lequel str est stocké" - ce n'est pas un débordement de tampon , qui est le problème ici. Et chaque réponse a ce "problème", qui est inévitable étant donné la signature de func... et toutes les autres fonctions C jamais écrites qui prennent des chaînes terminées par NUL comme arguments. Évoquer la possibilité que l'entrée ne soit pas terminée par NUL est complètement inutile.
Jim Balter
«cela dépasse le cadre de la question» - ce qui, malheureusement, dépasse la capacité de certaines personnes à comprendre.
Jim Balter
"Le problème est là" - vous avez raison, mais il vous manque toujours le problème clé, qui est que le test ( len >= 100) a été effectué contre une valeur mais que la longueur de la copie a reçu une valeur différente ... est une violation du principe DRY. L'appel simple strncpy(buffer, str, len)évite la possibilité de débordement de tampon et fait moins de travail que strncpy(buffer,str,sizeof(buffer) - 1)... bien qu'ici, ce soit juste un équivalent plus lent de memcpy(buffer, str, len).
Jim Balter
@JimBalter Cela dépasse le cadre de la question, mais je m'éloigne du sujet. Je comprends que les valeurs utilisées par le test et ce qui est utilisé dans strncpy sont deux valeurs différentes. Cependant, la pratique générale de codage dit que la limite de copie doit être sizeof (buffer) - 1 donc peu importe la longueur de str sur la copie. strncpy arrêtera de copier des octets lorsqu'il atteindra une valeur nulle ou copie n octets. La ligne suivante garantit que le dernier octet du tampon est un caractère nul. Le code est sûr, je maintiens ma déclaration précédente.
Daniel Rudy
11

Même si vous utilisez strncpy, la longueur de la coupure dépend toujours du pointeur de chaîne passé. Vous n'avez aucune idée de la longueur de cette chaîne (l'emplacement du terminateur nul par rapport au pointeur, c'est-à-dire). Appeler strlenseul vous ouvre donc à la vulnérabilité. Si vous voulez être plus sûr, utilisez strnlen(str, 100).

Le code complet corrigé serait:

int func(char *str) {
   char buffer[100];
   unsigned short len = strnlen(str, 100); // sizeof buffer

   if (len >= 100) {
     return -1;
   }

   strcpy(buffer, str); // this is safe since null terminator is less than 100th index
   return 0;
}
Patrick Roberts
la source
@ user3386109 N'aurait-il pas strlenégalement accès au-delà de la fin du tampon?
Patrick Roberts
2
@ user3386109 ce que vous faites remarquer rend la réponse d'orlp aussi invalide que la mienne. Je ne vois pas pourquoi strnlenne résout pas le problème si ce que orlp suggère est supposé correct de toute façon.
Patrick Roberts
1
"Je ne pense pas que strnlen résout quoi que ce soit ici" - bien sûr qu'il le fait; il empêche les débordements buffer. "puisque str pourrait pointer vers un tampon de 2 octets, dont aucun n'est NUL." - ce n'est pas pertinent, comme c'est le cas pour toute implémentation de func. La question ici concerne le dépassement de tampon, pas UB car l'entrée n'est pas terminée par NUL.
Jim Balter
1
"Le deuxième paramètre passé à strnlen doit être la taille de l'objet vers lequel pointe le premier paramètre, sinon strnlen est sans valeur" - c'est un non-sens complet et absolu. Si le deuxième argument de strnlen est la longueur de la chaîne d'entrée, alors strnlen est équivalent à strlen. Comment obtiendriez-vous même ce numéro, et si vous l'aviez, pourquoi auriez-vous besoin d'appeler str [n] len? Ce n'est pas du tout à quoi sert strnlen.
Jim Balter
1
+1 Bien que cette réponse est imparfaite parce que ce n'est pas équivalent au code de OP - strncpy pads de NUL et ne pas NUL se terminent, alors que strcpy NUL- se termine et ne NUL-pad, il fait résoudre le problème, contrairement à la commentaires ridicules et ignorants ci-dessus.
Jim Balter
4

La réponse avec l'emballage est juste. Mais il y a un problème qui, je pense, n'a pas été mentionné si (len> = 100)

Eh bien, si Len valait 100, nous copierions 100 éléments et nous n'aurions pas de \ 0 à la fin. Cela signifierait clairement que toute autre fonction dépendant de la chaîne correctement terminée marcherait bien au-delà du tableau d'origine.

La chaîne problématique de C est insoluble à mon humble avis. Vous feriez toujours mieux d'avoir des limites avant l'appel, mais même cela n'aidera pas. Il n'y a pas de vérification des limites et donc les débordements de tampon peuvent toujours et se produiront malheureusement

Friedrich
la source
La problématique de la chaîne est résoluble: il suffit d'utiliser les fonctions appropriées. C'est à dire. pas strncpy() et amis, mais la mémoire allouant des fonctions comme strdup()et amis. Ils sont dans la norme POSIX-2008, ils sont donc assez portables, bien qu'ils ne soient pas disponibles sur certains systèmes propriétaires.
cmaster - réintégrer monica le
"toute autre fonction dépendant de la chaîne correctement terminée" - bufferest locale à cette fonction et n'est pas utilisée ailleurs. Dans un programme réel, nous devrions examiner comment il est utilisé ... parfois, la terminaison NUL n'est pas correcte (l'utilisation originale de strncpy était de créer des entrées de répertoire de 14 octets UNIX - remplies par NUL et non terminées par NUL). «La chaîne problématique de C est insoluble à mon humble avis» - alors que C est un langage génial qui a été surpassé par une technologie bien meilleure, un code sûr peut y être écrit si une discipline suffisante est utilisée.
Jim Balter
Votre observation me semble erronée. if (len >= 100)est la condition pour quand la vérification échoue , pas quand elle réussit, ce qui signifie qu'il n'y a pas de cas où exactement 100 octets sans terminateur NUL sont copiés, car cette longueur est incluse dans la condition d'échec.
Patrick Roberts
@ cmaster. Dans ce cas, vous vous trompez. Ce n'est pas résoluble, car on peut toujours écrire au-delà des limites. Oui, c'est un comportement sans défense, mais il n'y a pas moyen de l'empêcher complètement.
Friedrich
@Jim Balter. Ce n'est pas important. Je peux potentiellement écrire sur les limites de ce tampon local et il sera donc toujours possible de corrompre d'autres structures de données.
Friedrich
3

Au-delà des problèmes de sécurité liés à l'appel strlenplus d'une fois, il ne faut généralement pas utiliser de méthodes de chaîne sur des chaînes dont la longueur est précisément connue [pour la plupart des fonctions de chaîne, il n'y a qu'un cas vraiment étroit où elles doivent être utilisées - sur des chaînes pour lesquelles un maximum la longueur peut être garantie, mais la longueur précise n'est pas connue]. Une fois que la longueur de la chaîne d'entrée est connue et que la longueur du tampon de sortie est connue, il faut déterminer la taille d'une région à copier et l'utiliser ensuite memcpy()pour effectuer réellement la copie en question. Bien qu'il soit possible que strcpycela soit plus performant memcpy()lors de la copie d'une chaîne de seulement 1 à 3 octets ou plus, sur de nombreuses plates memcpy()- formes, il est susceptible d'être plus de deux fois plus rapide lorsqu'il s'agit de chaînes plus volumineuses.

Bien qu'il existe certaines situations où la sécurité se ferait au détriment des performances, c'est une situation où l'approche sécurisée est également la plus rapide. Dans certains cas, il peut être raisonnable d'écrire du code qui n'est pas sécurisé contre les entrées au comportement étrange, si le code fournissant les entrées peut garantir qu'elles se comportent correctement, et si la protection contre les entrées mal conduites entraverait les performances. S'assurer que les longueurs de chaîne ne sont vérifiées qu'une seule fois améliore à la fois les performances et la sécurité, bien qu'une chose supplémentaire puisse être faite pour aider à protéger la sécurité même lors du suivi manuel de la longueur de la chaîne: pour chaque chaîne qui devrait avoir un nul à la fin, écrivez plutôt le null à la fin. que de s'attendre à ce que la chaîne source l'ait. Ainsi, si l'on écrivait un strdupéquivalent:

char *strdupe(char const *src)
{
  size_t len = strlen(src);
  char *dest = malloc(len+1);
  // Calculation can't wrap if string is in valid-size memory block
  if (!dest) return (OUT_OF_MEMORY(),(char*)0); 
  // OUT_OF_MEMORY is expected to halt; the return guards if it doesn't
  memcpy(dest, src, len);      
  dest[len]=0;
  return dest;
}

Notez que la dernière instruction pourrait généralement être omise si le memcpy avait traité des len+1octets, mais si un autre thread devait modifier la chaîne source, le résultat pourrait être une chaîne de destination non terminée par NUL.

supercat
la source
3
Pouvez-vous expliquer les problèmes de sécurité liés à l'appel strlenplus d'une fois ?
Bogdan Alexandru
1
@BogdanAlexandru: Une fois que l'on a appelé strlenet effectué une action en fonction de la valeur renvoyée (ce qui était probablement la raison de l'appel en premier lieu), un appel répété (1) donnera toujours la même réponse que le premier, dans ce cas, c'est simplement du travail gaspillé, ou (2) peut parfois (parce que quelque chose d'autre - peut-être un autre thread - a modifié la chaîne entre-temps) donner une réponse différente, auquel cas le code qui fait certaines choses avec la longueur (par exemple allouer un tampon) peut prendre une taille différente de celle du code qui fait autre chose (copie dans le tampon).
supercat