Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add example for sql_injection_spring_jdbc with annotation #551

Merged
merged 1 commit into from Apr 29, 2020

Conversation

Marx314
Copy link
Contributor

@Marx314 Marx314 commented Apr 24, 2020

This is an open discussion as the description can be also given for @value without additional information.

It still can be vulnerable if the annotation use some SpEL voodoo but I'm not sure if how exactly.

Example of a "vulnerable" application :

public class ClientRepository {
    @Value("${jdbc.query.create.client}")
    private String reqCreateClient;  // INSERT INTO client (no_client, name, mail, date_inscrit) VALUES (?, ?, ?, ?);
    @Autowired
    private JdbcTemplate jdbcTemplate;
     
    public int createClient(Client client) {
        Assert.notNull(client, "entity cannot be null");
        try {
            return jdbcTemplate.update(reqCreateClient, ps -> {
                Assert.notNull(client.getId(), "embedded id clientId cannot be null");
                ps.setInt(1, client.getId());
                ps.setString(2, client.getName());
                ps.setString(3, client.getMail());
                ps.setTimestamp(4, client.getDateInscription());
            });
        } catch (DuplicateKeyException e) {
            return update(e);
        }
    }
}

@h3xstream
Copy link
Member

The example above should be safe because "reqCreateClient" is loaded from a properties.

(This is a false positive)

At the moment, we don't do extra analysis @Value from Spring. It would be nice I guess to add some heuristic to avoid common false positive.

Generalizing the payload

From my basic research the values stored in @Value will not change at runtime once loaded. They seem to be scoped specifically to system properties and properties files. (https://www.baeldung.com/spring-value-annotation)

I guess we could consider those properties as safe without even analyzing the expression.

@h3xstream h3xstream merged commit 59463fb into find-sec-bugs:master Apr 29, 2020
@h3xstream h3xstream added this to the version-1.11.0 milestone Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants