¿Estas inyecciones de SQL son una mala idea?

3

Leyendo Evitando ataques de inyección de SQL en procedimientos almacenados comencé este debate entre mi pareja y yo --- y buscar el enfoque correcto no es tan sencillo.

No soy un experto en seguridad, pero ¿qué hace una persona cuando observa las primeras líneas de este procedimiento almacenado ... y ve esto?

No hemos tenido ningún problema de seguridad importante, pero no asumo que este enfoque está funcionando ... Si es así, déjelo solo o hágalo mejor.

ASP:

Dim conStr = ConfigurationSettings.AppSettings("WebUser")
Dim command = ("Execute dbo.web_Insert_usr_Comments @Location," & _
                    "@userexp, @full_name, @emailAddr, @comments")

Using con As New SqlConnection(conStr)
    Using cmd As New SqlCommand(command, con)
        cmd.Parameters.AddWithValue("@Location", Request.QueryString("Location"))
        cmd.Parameters.AddWithValue("@userexp", Request.QueryString("usrexp"))
        cmd.Parameters.AddWithValue("@comments", Request.QueryString("comments"))
        cmd.Parameters.AddWithValue("@full_name", Request.QueryString("full_name"))
        cmd.Parameters.AddWithValue("@emailAddr", Request.QueryString("emailAddr"))

        con.Open()
        cmd.ExecuteNonQuery()
    End Using                           
End Using 

TSQL:

 ALTER PROCEDURE [dbo].[web_Insert_usr_Comments] 
        @Location varchar(255), 
        @usrexp int = NULL,
        @full_name varchar(255) = NULL,
        @emailAddr varchar(320) = NULL,
        @comments varchar(125) = NULL

    AS
    BEGIN
        DECLARE @securityChk varchar(255),  @haveContact INT

        SET @securityChk = (SELECT @location + CAST(@usrexp AS VARCHAR(2)) + @full_name + @emailAddr + @usr_comments )  

        IF @securityChk LIKE '%SELECT%' RETURN;
        IF @securityChk LIKE '%DROP%'   RETURN;
        IF @securityChk LIKE '%INSERT%' RETURN;
        IF @securityChk LIKE '%DELETE%' RETURN;
        IF @securityChk LIKE '%EXE%'    RETURN;


    SET @haveContact =   ( SELECT  (count(*))
                FROM dbo.contacts 
                WHERE ( @emailAddr = E_mail_address AND  @location = company))
    --> Check and see if we have this person contact info                
    IF ( @haveContact = 0 )
       BEGIN 
        INSERT INTO dbo.contacts(e_mail_address, company, nick_name)
        VALUES (@emailAddr, @location, @full_name)
       END
    ELSE
        BEGIN
          INSERT INTO web_comments_dtl(timeGMT, form_id, contact_id, comment, user_exp_ranking, IP_Addr)
          SELECT GETDATE()
           , 1 --> TODO: Need better logic for form_id  
           , ( SELECT MAX(id) 
               FROM dbo.contacts
               WHERE  ( @emailAddr = E_mail_address AND  @location = company)
            OR ( @emailAddr = E_mail_address AND @full_name  = nick_name )
            OR ( @location = company))
          , @RFXcomments
          , @usrexp
          , dbo.fnBinaryIPv4(@ip_Addr)
        END
    END

Lo que me gustaría hacer, pero no estoy seguro de qué opción es la más eficiente.

  1. Debería pasar una semana o dos e investigar los problemas de seguridad (si hay uno) y el formulario de entrada del usuario que llama a este SP
  2. Reescriba todo, incluido el SP, el código que está detrás, y reconfigure IIS
  3. Agregue más cláusulas a la declaración anterior.

No creo que haya un problema de seguridad ... pero si el tipo anterior decide poner esto en su código, ¿qué debe hacer un joven DBA?

    
pregunta AAH-Shoot 23.12.2013 - 18:10
fuente

4 respuestas

7

Hacer este tipo de lista negra de palabras en sus argumentos no es necesario con procedimientos almacenados implementados correctamente, y solo llevará a una mala experiencia del usuario en caso de que esto falle en contenido benigno.

Si está utilizando un procedimiento almacenado y está estructurando su consulta con los parámetros que se pasan al procedimiento almacenado, no necesita realizar estas comprobaciones.

Sin embargo, si está concatenando cadenas y transfiriéndolas a una llamada EXEC, se está abriendo a la inyección de SQL. Mi consejo es simplemente usar procedimientos almacenados para lo que están destinados a hacer, y confiar en eso para su protección.

(Aparte de esto, 25 caracteres para el nombre completo y la dirección de correo electrónico me parecen demasiado cortos)

    
respondido por el Stefan H 23.12.2013 - 18:56
fuente
3

Como han señalado otros, las comprobaciones de inyección de SQL en este procedimiento son una mala idea. No protegen contra la inyección de SQL y bloquean algunos datos legítimos.

Pero a su punto:

  

[¿Qué se supone que debe hacer un joven DBA?]

Plantea este problema con un DBA senior o con tu administrador. Exprese sus inquietudes sobre el diseño de este procedimiento y otros procedimientos similares.

Luego sugiera que los parámetros de consulta serían una mejor solución (más fácil, más eficaz, estándar de la industria, menos posibilidades de bloquear por error la entrada legítima). Sugiera que el equipo debería convertirlo en un estándar de codificación para utilizar los parámetros de consulta como un medio de defensa contra la inyección de SQL.

Para casos de contenido dinámico que no se puede insertar como un parámetro (por ejemplo, un nombre de columna dinámico), use lista blanca en lugar de lista negra .

La razón por la que digo que debe llevar esto a los gerentes o jefes de equipo es que volver a escribir los procedimientos y reconfigurar IIS interrumpirá su programa actual para realizar el trabajo. Como miembro junior del equipo, es posible que no esté al tanto de otras restricciones o requisitos del proyecto.

Las personas que son responsables del cronograma del proyecto toman la decisión sobre cómo y cuándo reescribir todo. Si te encargas de iniciar eso, podrías causarles problemas. La comunicación abierta es el mejor plan.

    
respondido por el Bill Karwin 23.12.2013 - 19:40
fuente
2

La forma correcta de evitar la inyección de SQL es nunca construir consultas SQL mediante la manipulación de cadenas de entrada controlada por el usuario, sino simplemente usar parámetros enlazados.

Eso está en tu aplicación nunca hazlo:

db.execute_sql("SELECT * FROM users WHERE user_name = '" + user_name + 
               "' AND password_hash = HASH('" + password + "');"
              )

donde user_name podría ser una variable con un valor como '; DROP TABLE users; -- (por lo tanto, la cadena que ejecutará la base de datos ahora es SELECT * FROM users WHERE user_name = ''; DROP TABLE users; -- , que elimina la tabla de usuarios e ignora todo lo que aparece después del símbolo de comentario.

En lugar de hacer algo como

db.execute_sql_with_params("SELECT * FROM users WHERE user_name = '?'" + 
                           " AND password_hash = HASH('?');", [user_name, password]);

donde, independientemente de los valores de las variables [user_name, password] , la estructura de la declaración SQL nunca cambiará; siempre buscará un nombre de usuario con el contenido de la variable user_name .

Entonces, sí, cualquier tipo de verificación de ciertos términos (que puede ser válida y no capturará todos los ataques) es una mala idea.

Esto es ligeramente diferente a decir que solo los procedimientos almacenados utilizados (procedimientos almacenados) son simplemente funciones SQL almacenadas. Si bien sería estúpido, un procedimiento almacenado podría generar una consulta SQL de manera insegura al manipular cadenas de información del usuario, tal como lo haría en el código de su aplicación.

    
respondido por el dr jimbob 10.01.2014 - 14:48
fuente
0

Debería manejar esto más o menos en el código de la aplicación en lugar del procedimiento almacenado. Un usuario puede tener un correo electrónico como [email protected].

Intenta usar consultas parametrizadas aquí.

    
respondido por el danish 23.12.2013 - 18:47
fuente

Lea otras preguntas en las etiquetas