¿Por qué este código es vulnerable a los ataques de desbordamiento del búfer?

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;
}

Este código es vulnerable a un ataque de desbordamiento de búfer, y estoy tratando de averiguar por qué. Estoy pensando que tiene que ver con lenser declarado un en shortlugar de un int, pero no estoy muy seguro.

¿Algunas ideas?

Jason
fuente
3
Hay múltiples problemas con este código. Recuerde que las cadenas C tienen terminación nula.
Dmitri Chubarov
44
@DmitriChubarov, la terminación no nula de la cadena será un problema solo si la cadena se usa después de la llamada a strncpy. En este caso, no lo es.
R Sahu
43
Los problemas en este código fluyen directamente del hecho de que strlense calcula, se usa para la verificación de validez y luego se calcula absurdamente de nuevo : es una falla SECA. Si el segundo strlen(str)fuera reemplazado por len, no habría posibilidad de desbordamiento del búfer, independientemente del tipo de len. Las respuestas no abordan este punto, solo logran evitarlo.
Jim Balter
3
@CiaPan: Wenn pasando una cadena no terminada en nulo, strlen mostrará un comportamiento indefinido.
Kaiserludi
3
@ JimBalter Nah, creo que los dejaré allí. Tal vez alguien más tendrá el mismo error tonto y aprenderá de él. Siéntete libre de marcarlos si te molestan, alguien podría venir y eliminarlos.
Asad Saeeduddin

Respuestas:

192

En la mayoría de los compiladores, el valor máximo de an unsigned shortes 65535.

Cualquier valor anterior que se envuelva, entonces 65536 se convierte en 0 y 65600 se convierte en 65.

Esto significa que las cadenas largas de la longitud correcta (por ejemplo, 65600) pasarán la comprobación y desbordarán el búfer.


Se usa size_tpara almacenar el resultado de strlen(), no unsigned short, y compararlo lencon una expresión que codifica directamente el tamaño de buffer. Así por ejemplo:

char buffer[100];
size_t len = strlen(str);
if (len >= sizeof(buffer) / sizeof(buffer[0]))  return -1;
memcpy(buffer, str, len + 1);
orlp
fuente
2
@PatrickRoberts Teóricamente, sí. Pero debe tener en cuenta que el 10% del código es responsable del 90% del tiempo de ejecución, por lo que no debe dejar pasar el rendimiento antes que la seguridad. Y tenga en cuenta que con el tiempo el código cambia, lo que de repente puede significar que la verificación anterior se ha ido.
orlp
3
Para evitar el desbordamiento del búfer, simplemente utilícelo lencomo el tercer argumento de strncpy. Usar strlen nuevamente es tonto en cualquier caso.
Jim Balter
15
/ sizeof(buffer[0])- tenga sizeof(char)en cuenta que en C siempre es 1 (incluso si un carácter contiene miles de millones de bits), por lo que es superfluo cuando no hay posibilidad de utilizar un tipo de datos diferente. Aún así ... felicitaciones por una respuesta completa (y gracias por responder a los comentarios).
Jim Balter
3
@ rr-: char[]y char*no son lo mismo. Hay muchas situaciones en las que a char[]se convertirá implícitamente en a char*. Por ejemplo, char[]es exactamente el mismo que char*cuando se usa como tipo para argumentos de función. Sin embargo, la conversión no ocurre para sizeof().
Dietrich Epp
44
@Controll Porque si cambia el tamaño de bufferen algún momento la expresión se actualiza automáticamente. Esto es crítico para la seguridad, porque la declaración de bufferpodría estar bastante lejos de la verificación en el código real. Por lo tanto, es fácil cambiar el tamaño del búfer, pero olvide actualizar en cada ubicación donde se use el tamaño.
orlp
28

El problema está aquí:

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

Si la cadena es mayor que la longitud del búfer de destino, strncpy aún la copiará. Está basando el número de caracteres de la cadena como el número a copiar en lugar del tamaño del búfer. La forma correcta de hacer esto es la siguiente:

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

Lo que esto hace es limitar la cantidad de datos copiados al tamaño real del búfer menos uno para el carácter de terminación nulo. Luego establecemos el último byte en el búfer al carácter nulo como salvaguarda adicional. La razón de esto es porque strncpy copiará hasta n bytes, incluido el nulo de terminación, si strlen (str) <len - 1. Si no, entonces el nulo no se copia y tiene un escenario de bloqueo porque ahora su búfer tiene un no terminado cuerda.

Espero que esto ayude.

EDITAR: Tras un examen adicional y la entrada de otros, sigue una posible codificación para la función:

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);
  }

Como ya conocemos la longitud de la cadena, podemos usar memcpy para copiar la cadena de la ubicación a la que hace referencia str en el búfer. Tenga en cuenta que según la página del manual para strlen (3) (en un sistema FreeBSD 9.3), se establece lo siguiente:

 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.

Lo que interpreto es que la longitud de la cadena no incluye el nulo. Es por eso que copio len + 1 bytes para incluir el valor nulo, y la prueba verifica para asegurar que la longitud <tamaño del búfer - 2. Menos uno porque el búfer comienza en la posición 0, y menos otro para asegurarse de que haya espacio para el nulo

EDITAR: Resulta que el tamaño de algo comienza con 1 mientras que el acceso comienza con 0, por lo que -2 antes era incorrecto porque devolvería un error para cualquier cosa> 98 bytes pero debería ser> 99 bytes.

EDITAR: Aunque la respuesta sobre un corto sin signo es generalmente correcta, ya que la longitud máxima que se puede representar es 65.535 caracteres, en realidad no importa porque si la cadena es más larga que eso, el valor se ajustará. Es como tomar 75,231 (que es 0x000125DF) y enmascarar los 16 bits superiores para obtener 9695 (0x000025DF). El único problema que veo con esto son los primeros 100 caracteres más allá de 65,535, ya que la verificación de longitud permitirá la copia, pero solo copiará hasta los primeros 100 caracteres de la cadena en todos los casos y nulo terminará la cadena . Entonces, incluso con el problema envolvente, el búfer aún no se desbordará.

Esto puede o no en sí mismo representar un riesgo de seguridad dependiendo del contenido de la cadena y para qué lo está utilizando. Si se trata solo de texto directo que es legible para los humanos, entonces generalmente no hay problema. Solo obtienes una cadena truncada. Sin embargo, si es algo así como una URL o incluso una secuencia de comandos SQL, podría tener un problema.

Daniel Rudy
fuente
2
Es cierto, pero eso está fuera del alcance de la pregunta. El código muestra claramente que la función pasa un puntero de caracteres. Fuera del alcance de la función, no nos importa.
Daniel Rudy
"el búfer en el que se almacena str" - eso no es un desbordamiento del búfer , que es el problema aquí. Y cada respuesta tiene ese "problema", que es inevitable dada la firma de func... y todas las demás funciones de C que se escriben que toman cadenas terminadas en NUL como argumentos. Mencionar la posibilidad de que la entrada no esté terminada en NUL no tiene ni idea.
Jim Balter
"Eso está más allá del alcance de la pregunta", que lamentablemente está más allá de la capacidad de algunas personas para comprender.
Jim Balter
"El problema está aquí": tienes razón, pero aún te falta el tema clave, que es que la prueba ( len >= 100) se realizó contra un valor, pero la longitud de la copia recibió un valor diferente ... esto es una violación del principio DRY. Simplemente llamar strncpy(buffer, str, len)evita la posibilidad de desbordamiento del búfer y hace menos trabajo que strncpy(buffer,str,sizeof(buffer) - 1)... aunque aquí es solo un equivalente más lento memcpy(buffer, str, len).
Jim Balter
@ JimBalter Está más allá del alcance de la pregunta, pero estoy divagando. Entiendo que los valores utilizados por la prueba y lo que se utiliza en strncpy son dos valores diferentes. Sin embargo, la práctica general de codificación dice que el límite de copia debe ser sizeof (buffer) - 1, por lo que no importa cuál sea la longitud de str en la copia. strncpy dejará de copiar bytes cuando llegue a un valor nulo o copie n bytes. La siguiente línea garantiza que el último byte en el búfer es un carácter nulo. El código es seguro, mantengo mi declaración anterior.
Daniel Rudy
11

Aunque lo esté utilizando strncpy, la longitud del corte aún depende del puntero de cadena pasado. No tiene idea de cuánto dura esa cadena (es decir, la ubicación del terminador nulo en relación con el puntero). Entonces, llamar strlensolo te abre a la vulnerabilidad. Si quieres estar más seguro, úsalo strnlen(str, 100).

El código completo corregido sería:

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
fuente
@ user3386109 ¿Entonces tampoco accedería más strlenallá del final del búfer?
Patrick Roberts
2
@ user3386109 lo que estás señalando hace que la respuesta de orlp sea tan inválida como la mía. No veo por qué strnlenno resuelve el problema si lo que sugiere orlp es supuestamente correcto de todos modos.
Patrick Roberts el
1
"No creo que strnlen resuelva nada aquí", por supuesto que sí; Evita el desbordamiento buffer. "ya que str podría apuntar a un búfer de 2 bytes, ninguno de los cuales es NUL". - eso es irrelevante, ya que es cierto para cualquier implementación de func. La pregunta aquí es sobre el desbordamiento del búfer, no UB porque la entrada no está terminada en NUL.
Jim Balter
1
"El segundo parámetro pasado a strnlen debe ser el tamaño del objeto al que apunta el primer parámetro, o strnlen no tiene valor" - esto es completo y sin sentido. Si el segundo argumento para strnlen es la longitud de la cadena de entrada, entonces strnlen es equivalente a strlen. ¿Cómo obtendría ese número, y si lo tuviera, por qué necesitaría llamar a str [n] len? Para eso no es para nada strnlen.
Jim Balter
1
1 A pesar de que esta respuesta es imperfecta porque no es equivalente al código de la OP - strncpy NUL-pads y no pongan fin NUL, mientras que strcpy NUL-finaliza y no NUL-pad, que hace resolver el problema, al contrario de la comentarios ridículos e ignorantes arriba.
Jim Balter
4

La respuesta con el envoltorio es correcta. Pero hay un problema que creo que no se mencionó si (len> = 100)

Bueno, si Len fuera 100, copiaríamos 100 elementos y no tendríamos \ 0. Eso claramente significaría que cualquier otra función que dependa de una cadena terminada correctamente iría más allá de la matriz original.

La cadena problemática de C es en mi humilde opinión irresoluble. Es mejor que tenga algunos límites antes de la llamada, pero incluso eso no ayudará. No hay verificación de límites y, por lo tanto, los desbordamientos del búfer siempre pueden y, desafortunadamente, sucederán ...

Friedrich
fuente
La cadena problemática es solucionable: solo use las funciones apropiadas. I. e. no strncpy() y amigos, pero las funciones de asignación de memoria como strdup()y amigos. Están en el estándar POSIX-2008, por lo que son bastante portátiles, aunque no están disponibles en algunos sistemas propietarios.
cmaster - reinstalar a monica el
"cualquier otra función que dependa de la cadena finalizada adecuada": bufferes local para esta función y no se usa en ningún otro lugar. En un programa real, tendríamos que examinar cómo se usa ... a veces no la terminación de NUL es correcta (el uso original de strncpy era crear entradas de directorio de 14 bytes de UNIX, rellenadas con NUL y no terminadas con NUL). "La cadena problemática de C es, en mi humilde opinión, irresoluble", mientras que C es un lenguaje increíble que ha sido superado por una tecnología mucho mejor, se puede escribir un código seguro si se usa suficiente disciplina.
Jim Balter
Tu observación me parece equivocada. if (len >= 100)es la condición para cuando falla la verificación , no cuando pasa, lo que significa que no hay un caso en el que se copien exactamente 100 bytes sin terminador NUL, ya que esa longitud se incluye en la condición de falla.
Patrick Roberts
@ cmaster. En este caso te equivocas. No es solucionable, porque uno siempre puede escribir más allá de los límites. Sí, es un comportamiento indefenso, pero no hay forma de prevenirlo por completo.
Friedrich
@Jim Balter. No importa. Potencialmente, puedo escribir sobre los límites de este búfer local, por lo que siempre será posible corromper algunas otras estructuras de datos.
Friedrich
3

Más allá de los problemas de seguridad relacionados con la llamada strlenmás de una vez, generalmente no se deben usar métodos de cadena en cadenas cuya longitud se conoce con precisión [para la mayoría de las funciones de cadena, solo hay un caso realmente estrecho en el que se deben usar, en cadenas para las cuales un máximo se puede garantizar la longitud, pero no se conoce la longitud precisa]. Una vez que se conoce la longitud de la cadena de entrada y se conoce la longitud del búfer de salida, se debe determinar qué tan grande se debe copiar una región y luego usarla memcpy()para realizar la copia en cuestión. Aunque es posible que strcpytenga un rendimiento superior memcpy()al copiar una cadena de solo 1-3 bytes más o menos, en muchas plataformas memcpy()es probable que sea más del doble de rápido cuando se trata de cadenas más grandes.

Aunque hay algunas situaciones en las que la seguridad vendría a costa del rendimiento, esta es una situación en la que el enfoque seguro también es el más rápido. En algunos casos, puede ser razonable escribir código que no sea seguro contra entradas de comportamiento extraño, si el código que suministra las entradas puede garantizar que se comportarán bien, y si protegerse contra las entradas de mal comportamiento impediría el rendimiento. Asegurarse de que las longitudes de las cadenas solo se verifiquen una vez mejora tanto el rendimiento como la seguridad, aunque se puede hacer una cosa adicional para ayudar a proteger la seguridad incluso cuando se rastrea la longitud de la cadena manualmente: para cada cadena que se espera que tenga un nulo final, escriba explícitamente el nulo final. que esperar que la cadena fuente lo tenga. Por lo tanto, si uno estuviera escribiendo un strdupequivalente:

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;
}

Tenga en cuenta que la última instrucción generalmente podría omitirse si la memoria procesaba len+1bytes, pero si otro hilo modificara la cadena de origen, el resultado podría ser una cadena de destino sin terminación NUL.

Super gato
fuente
3
¿Puede explicar los problemas de seguridad relacionados con las llamadas strlenmás de una vez ?
Bogdan Alexandru
1
@BogdanAlexandru: una vez que uno ha llamado strleny ha tomado alguna acción basada en el valor devuelto (que presumiblemente fue la razón para llamarlo en primer lugar), una llamada repetida (1) siempre dará la misma respuesta que la primera, en cuyo caso es simplemente un trabajo desperdiciado, o (2) a veces (porque algo más, quizás otro hilo, modificó la cadena mientras tanto) produce una respuesta diferente, en cuyo caso el código que hace algunas cosas con la longitud (p. ej. asignar un búfer) puede asumir un tamaño diferente que el código que hace otras cosas (copiar al búfer).
supercat