diff --git a/src/main/java/servlets/module/challenge/SqlInjectionStoredProcedure.java b/src/main/java/servlets/module/challenge/SqlInjectionStoredProcedure.java index 9e7890770..69e1f5e89 100644 --- a/src/main/java/servlets/module/challenge/SqlInjectionStoredProcedure.java +++ b/src/main/java/servlets/module/challenge/SqlInjectionStoredProcedure.java @@ -3,10 +3,10 @@ import dbProcs.Database; import java.io.IOException; import java.io.PrintWriter; +import java.sql.CallableStatement; import java.sql.Connection; import java.sql.ResultSet; import java.sql.SQLException; -import java.sql.Statement; import java.util.Locale; import java.util.ResourceBundle; import javax.servlet.ServletException; @@ -75,13 +75,6 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) log.debug("User Submitted - " + userIdentity); String ApplicationRoot = getServletContext().getRealPath(""); - log.debug("Getting Connection to Database"); - Connection conn = - Database.getChallengeConnection(ApplicationRoot, "SqlChallengeStoredProc"); - // CallableStatement callstmt = conn.prepareCall("CALL findUser('" + userIdentity + "');"); - Statement stmt = conn.createStatement(); - ResultSet resultSet = stmt.executeQuery("CALL findUser('" + userIdentity + "');"); - int i = 0; htmlOutput = "

" + bundle.getString("response.searchResults") + "

"; htmlOutput += @@ -93,33 +86,45 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) + bundle.getString("response.table.comment") + ""; - log.debug("Opening Result Set from query"); - while (resultSet.next()) { - log.debug("Adding Customer " + resultSet.getString(2)); - htmlOutput += - "" - + Encode.forHtml(resultSet.getString(2)) - + "" - + Encode.forHtml(resultSet.getString(3)) - + "" - + Encode.forHtml(resultSet.getString(4)) - + ""; - i++; + log.debug("Getting Connection to Database"); + // Bind the user supplied value as a real stored-procedure parameter instead of + // splicing it into the SQL text, so it can never break out of the argument. The + // connection, statement and result set are all opened in a try-with-resources block + // so a crafted/oversized userIdentity that makes the driver throw can never leak the + // pooled connection - a prior version only closed it on the success path, so a run of + // failing/malicious requests could starve the pool for this challenge's legitimate + // lookups. + try (Connection conn = + Database.getChallengeConnection(ApplicationRoot, "SqlChallengeStoredProc"); + CallableStatement callstmt = conn.prepareCall("{call findUser(?)}")) { + callstmt.setString(1, userIdentity); + try (ResultSet resultSet = callstmt.executeQuery()) { + log.debug("Opening Result Set from query"); + while (resultSet.next()) { + log.debug("Adding Customer " + resultSet.getString(2)); + htmlOutput += + "" + + Encode.forHtml(resultSet.getString(2)) + + "" + + Encode.forHtml(resultSet.getString(3)) + + "" + + Encode.forHtml(resultSet.getString(4)) + + ""; + i++; + } + } } - conn.close(); htmlOutput += ""; if (i == 0) { htmlOutput = "

" + bundle.getString("response.noResults") + "

"; } } catch (SQLException e) { - log.debug("SQL Error caught - " + e.toString()); - htmlOutput += - "

" - + errors.getString("error.detected") - + "

" - + "

" - + Encode.forHtml(e.toString()) - + "

"; + // Report only the generic localized error to the caller - echoing the driver's own + // exception text (e.g. column/table names, driver/connection identifiers) back to an + // attacker is itself an information leak that helps refine further injection attempts. + // The full detail still goes to the server log for debugging. + log.error("SQL Error caught - " + e.toString()); + htmlOutput += "

" + errors.getString("error.detected") + "

"; } catch (Exception e) { out.write(errors.getString("error.funky")); log.fatal(levelName + " - " + e.toString());