CODEWITHSUNDEEP
javaspringsecuritysql-injection
damon
damon

Reputation: 31

Checkmarx SQL injection high severity issue

I am getting a high severity issue in this method:

    public void recordBadLogin(final String uid, final String reason, final String ip) throws DataAccessException {
    if (Utils.isEmpty(uid)) {
        throw new DataAccessException("User information needed to update , Empty user information passed");
    }
    try {
        String sql = (String) this.queries.get(IUtilDAO.queryKeyPrefix + UtilDAO.RECORD_FAILED_LOGIN);
        Map<String, Object> paramMap = new HashMap<String, Object>();
        paramMap.put("uid", uid.trim());
        paramMap.put("reason", (reason != null ? reason.trim() : "Invalid userid/password"));
        paramMap.put("ip", ip);
        this.namedJdbcTemplate.update(sql, paramMap);
    } catch (Exception e) {
        throw new DataAccessException("Failed to record bad login for user " + uid, e);
    }
}

This line of code is causing the issue:

String sql = (String) this.queries.get(IUtilDAO.queryKeyPrefix + UtilDAO.RECORD_FAILED_LOGIN);

queries is a properties object and the prepared statement is being retrieved given IUtilDAO.queryKeyPrefix + UtilDAO.RECORD_FAILED_LOGIN. And those 2 arguments are constants. Logically I don't see how this can cause an SQL injection issue as the prepared statement is being retrieved from a dictionary. Does anyone have an idea if this is a false positive or if there is an actual vulnerability present?

Upvotes: 0

Views: 2428

Answers (1)

Egret
Egret

Reputation: 799

It's hard to tell from the example given, but I'd guess that the properties object was tainted by untrusted data. Most code flow analysis tools will taint the entire data structure if any untrusted data is placed in it.

Technically this is a "false positive". But architecturally it's something that should be fixed - it's generally a bad idea to mix trusted and untrusted data together in the same data structure. It makes it easy for future developers to misunderstand the status of a particular element, and makes it harder for both humans and tools to code review for security issues.

Upvotes: 1

Related Questions