Is concatenating values directly into the query problematic?

Asked

Viewed 312 times

1

These days a guy told me: "avoid forming Sqls by concatenating pieces of Strings. To do this is to ask to have security problems with SQL injection, and for this reason is considered a bad programming practice."

And as for merging variables to a String to mount an SQL, like this: 

 con.atualizar("INSERT INTO CLIENTES (nome, sexo, nascimento, cpf, renda"
                    + " VALUES: ('"+nome+"', '"+sexo+"', '"+nasc+"', '"+cpf+"', '"+renda+"');");

And the update method:

public void atualizar(String sql)throws SQLException{

        PreparedStatement st = con.prepareStatement(sql);

        try{
        st.executeUpdate(sql);

        }catch (SQLException e){
            System.out.println("Erro na atualização. RollBack será efetuado.");
            con.rollback();
        }
    }

Can be problematic too?

java sql
  • 1

    Use Preparedstatement, that there is besides leaving the bad code to read, still from the gap to this failure.

  • @Article Yes, in the update method I get this String passed by parameter and with it I create a Preparedstatement: public void update(String sql)throws Sqlexception{ Preparedstatement st = con.prepareStatement(sql);

  • 1

    Then edit the question and add the full snippet, because in this snippet there, the gap still continues.

1 answer

2


Yes, it can cause problems as it would allow the user to manipulate the query. In this answer there is a simple yet dangerous demonstration of how an attacker could exploit this. That’s why there is a class PreparedStatement, to provide security when treating data that comes directly from the user. In this other answer there is a good explanation of why to use this class.

The PreparedStatement has the feature to allow parameterize user entries to be concatenated with the query. In the code presented, despite using this class, you only created the variable, but did not parameterize anything and is passing directly in the query.

To parameterize, you need to isolate the data received from the query and leave it to the PreparedStatement concatenation. Consider the type of data received in order to call the correct equivalent set method. Assuming some of the types of its variables, it would look something like this:

con.atualizar(nome, sexo, nasc, cpf, renda);

[...]           

public void atualizar(String nome,char sexo, java.util.Date nasc, String cpf, double renda)throws SQLException{

    String query = "INSERT INTO CLIENTES (nome, sexo, nascimento, cpf, renda)"
                + " VALUES: (?, ?, ?, ?, ?);"

    try{

      PreparedStatement st = con.prepareStatement(query);

      st.setString(1, nome);
      st.setString(2, sexo);
      st.setDate(3, new java.sql.Date(nasc.getTime()));
      st.setString(4, cpf);
      st.setDouble(5, renda);

      st.executeQuery();

    }catch (SQLException e){
        System.out.println("Erro na atualização. RollBack será efetuado.");
        con.rollback();
    }
}

Of course, one cannot rely on this alone as the only way to prevent injection breaches, it is only one layer of several that one should have to protect your application.

  • I just didn’t understand which variable "sql" is that in your code. In this excerpt: Preparedstatement st = con.prepareStatement(sql);

  • 1

    @Lucaspletsch well noticed, I had forgotten to change. executeUpdate is only for update statements, for Insert and select is executeQuery.

  • Another thing: If I did it the way you showed, I couldn’t have a generic method of "Update" the tables. You know? My method accepts any Insert, or delete, drop, etc. But from what you’re telling me, it’s also not safe to have such a method, right? Or I may have, unless you put other layers of security?

  • 1

    @Lucaspletsch so your modeling is wrong. The query should not be known outside the model classes, as I understood from the initial code, you are passing the query to the model, which breaks the MVC, because the model classes are the only ones that should know how to talk to the database. If you need variants for this same method, use method overload.

  • Actually I was talking about an excerpt above: Try{ Preparedstatement st = con.Statement(sql); What variable "sql" is this?

  • I understand. So for a model class, or "entity" in ECB, a best practice is just to pass the parameters, and it performs the query?

  • 1

    @Lucaspletsch in a simpler modeling scenario, yes. Of course there are other scenarios where you can generalize this with other methods, but I don’t have much practice with others, otherwise I would explain in the answer. But overloading methods would help in your case.

Show 2 more comments

Browser other questions tagged java sql

You are not signed in. Login or sign up in order to post.