Am I vulnerable to SQL Injection?

user_15285537 0
Viewed 220

I have this class that extends a class that implements CustomTaskChange. Since some of the class attributes like the column name and type are passed to it by a XML file, I need to make a string concatenation. I do use PreparedStatement, but I wonder if I am still vulnerable to SQL Injection because of it and if that is the case how can I make it safe? The XML is a Liquibase ChangeSet (I will leave a sample below) .

XML File

<?xml version="1.0" encoding="UTF-8"?>
<databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.8.xsd">
  <changeSet author="" id="57ba1e10-3e3a-4be1-a7a0-7a9fb9411565">
    <customChange class="com.petapilot.migrations.customChanges.MultipleAdd" columnName="testeRegex" columnType="varchar(250)"
    regex="saft_[0-9]{4}_[0-9]{1,10}_[0-9]{1,10}_nc_transactions" after="CreditAmount" notNull="true" default="sbc"></customChange>
  </changeSet>
</databaseChangeLog>

Custom Class

@Override
    public void execute(Database database) throws CustomChangeException {

        try {
            //check if there are any errors in the changelog file
            checkArgumentsNumber();

            boolean tablesFound = false;
            boolean columnsAdded = false;
            String sqlStatement = "ALTER TABLE NAME ADD COLUMN " +this.getColumnName()+" "+this.getColumnType();

            if(notNull){
                sqlStatement+=" NOT NULL";
            }

            if(defaultValue!=null){
                sqlStatement+=" DEFAULT ?";
            }

            if (this.after != null) {
                sqlStatement += " AFTER " +this.after;
            }

            System.out.println("The statement is: "+sqlStatement);

            //get tables in the database
            JdbcConnection connection = (JdbcConnection) database.getConnection();
            DatabaseMetaData metadata;
            metadata = connection.getMetaData();
            String[] types = {"TABLE"};
            ResultSet rs = metadata.getTables(connection.getCatalog(), null, "%", types);

            //if the user chose to use a regex
            if (this.getRegex() != null) {
                Pattern pattern = Pattern.compile(this.getRegex());

                while (rs.next()) {
                    String tableName = rs.getString(3);
                    Matcher matcher = pattern.matcher(tableName);
                    boolean matches = matcher.matches();
                    if (matches) {
                        tablesFound = true;
                        String addStatement=sqlStatement.replaceAll("NAME",tableName);
                        PreparedStatement s = connection.prepareStatement(addStatement);
                        if (!checkColumnsExists(s, tableName)) {
                            if(this.defaultValue!=null){
                                s.setString(1,this.defaultValue);
                            }
                            System.out.println("REGEX:"+s.toString());
                            s.executeUpdate();
                            columnsAdded = true;
                        }
                    }
                }
            }

            checkInvalidInfo(tablesFound, columnsAdded, "All the matching tables already had that column");
        } catch (InvalidArgumentsNumberException | SQLException | InvalidInfoException | DatabaseException e) {
            throw new CustomChangeException("Error enabling trigger: " + e);
        }
    }
liquibase sql-injection sql java
1 Answers

Whether it is an SQL injection vulnerability will depend on where that XML file comes from.

If programmers are authoring such files, then you do not have an SQL injection problem. Yes, it is possible to concatenate dangerous things, but it’s no more of a risk than a developer writing dangerous things directly into the code.

However, if the XML files come from end users, or from other services, or from files that other people or services are permitted to modify, then they should be considered potentially dangerous, and your code would be at risk of an SQL injection attack.

Since all of your concatenated values are actually identifiers (column names and a data type), rather than values, you probably will be safe if you check that they consist only of alphanumeric characters:

if (!getColumnName().matches("\\p{AlNum}+")) {
    throw new CustomChangeException(
        "Invalid column name: \"" + getColumnName() + "\"");
}
if (!after.matches("\\p{AlNum}+")) {
    throw new CustomChangeException(
        "Invalid 'after' column name: \"" + after + "\"");
}
if (!getColumnType().matches("\\p{AlNum}+")) {
    throw new CustomChangeException(
        "Invalid column type: \"" + getColumnType() + "\"");
}
user_18319870
Related