views:

89

answers:

3

Hi all,

The code below is from SAMATE Reference Dataset. I used it to test a static analysis tool. As you can see the code should prevent SQL-Injection both by using a sanitization method as well as using a prepared statement.

Since SCA tools cannot know custom santitzation methods, the will not detect that the allowed method is used to prevent the injection.

public class SQLInjection_good_089 extends HttpServlet
{
    private static final long serialVersionUID = 1L;

    public SQLInjection_good_089()
    {
        super();
    }

    // Table of allowed names to use
    final String allowed_names[] = { "Mickael", "Mary", 
            "Peter", "Laura", "John"};

    // Function to check if the current name takes part of the allowed ones
    public boolean allowed( String in )
    {
        boolean bool = false;

        for( int i = 0; i < 5; i++ )
        {
            if( in.equals( allowed_names[i] ) )
            {
                // the current name is allowed to use
                bool = true;
                break;
            }
        }
        return bool;
    }

    // Method which will be called to handle HTTP GET requests
    protected void doGet( HttpServletRequest req, HttpServletResponse resp )
        throws ServletException, IOException
    {
        // Initialize the output stream
        resp.setContentType("text/html");
        ServletOutputStream out = resp.getOutputStream();
        out.println("<HTML><BODY><blockquote><pre>");
        Connection conn = null;

        // Get the parameter "name" from the data provided by the user
        String name = req.getParameter( "name" );

        if ( (name != null) && (allowed(name) == true) )
        {
            try
            {
                // Set the context factory to use to create the initial context
                System.setProperty (Context.INITIAL_CONTEXT_FACTORY, "your.ContextFactory");

                // Create the initial context and use it to lookup the data source
                InitialContext ic = new InitialContext ();
                DataSource dataSrc = (DataSource) ic.lookup ("java:comp/env/jdbc:/mydb");

                // Create a connection to the SQL database from the data source
                conn = dataSrc.getConnection();

                // Send a SQL request to the database
                PreparedStatement ps = conn.prepareStatement( "SELECT * FROM users WHERE firstname LIKE ?" );
                // replace the first parameter by name
                ps.setString(1, name);
                ps.executeQuery();
            }
            catch( NamingException e )
            {
                out.println( "Naming exception");
            }
            catch( SQLException e )
            {
                out.println( "SQL exception");
            }
            finally
            {
                try
                {
                    if (conn != null)
                        conn.close ();
                }
                catch (SQLException se)
                {
                    out.println("SQL Exception");
                }
            }
        }
        else
            return;

        out.println( "</pre></blockquote></body></html>" );
    }

    protected void doPost(HttpServletRequest request, HttpServletResponse response)
        throws ServletException, IOException
    {
    }
}

Nevertheless I think that the use of a prepared statement should prevent the injection here anyway. Am I mistaken?

+2  A: 

You are correct. The prepared statement will take care of 'illegal' SQL input.

The allowed(...) function is kind of user input validation in terms of business rules and not to prevent SQL injection.

MicSim
Acutally this was a glitch of myself. I accidentally looked in the results for the wrong project. Nevermind. Thanks
er4z0r
+1  A: 

Just the prepared statement should be enough to prevent SQL injection...

However, if you intend to write user messages to the 'out' parameter (such as out.printf( "Invalid username %s", name )), watch out for javascript injection. I could enter my name as <script>alert('hi')</script>

tim_yates
+1  A: 

It appears to prevent SQL Injection. You are of course correct that the allowed() function helps, but is not exactly the preferred method. Since your code is just a sample, I would assume in the real world most programs would allow more than 5 possible options.

MJB