Fix SQL Injection - Stored Procedure challenge (parameterize CALL findUser)

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 = "<h2 class='title'>" + bundle.getString("response.searchResults") + "</h2>";
         htmlOutput +=
@@ -93,33 +86,45 @@ public void doPost(HttpServletRequest request, HttpServletResponse response)
                 + bundle.getString("response.table.comment")
                 + "</th></tr>";
 
-        log.debug("Opening Result Set from query");
-        while (resultSet.next()) {
-          log.debug("Adding Customer " + resultSet.getString(2));
-          htmlOutput +=
-              "<tr><td>"
-                  + Encode.forHtml(resultSet.getString(2))
-                  + "</td><td>"
-                  + Encode.forHtml(resultSet.getString(3))
-                  + "</td><td>"
-                  + Encode.forHtml(resultSet.getString(4))
-                  + "</td></tr>";
-          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 +=
+                  "<tr><td>"
+                      + Encode.forHtml(resultSet.getString(2))
+                      + "</td><td>"
+                      + Encode.forHtml(resultSet.getString(3))
+                      + "</td><td>"
+                      + Encode.forHtml(resultSet.getString(4))
+                      + "</td></tr>";
+              i++;
+            }
+          }
         }
-        conn.close();
         htmlOutput += "</table>";
         if (i == 0) {
           htmlOutput = "<p>" + bundle.getString("response.noResults") + "</p>";
         }
       } catch (SQLException e) {
-        log.debug("SQL Error caught - " + e.toString());
-        htmlOutput +=
-            "<p>"
-                + errors.getString("error.detected")
-                + "</p>"
-                + "<p>"
-                + Encode.forHtml(e.toString())
-                + "</p>";
+        // 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 += "<p>" + errors.getString("error.detected") + "</p>";
       } catch (Exception e) {
         out.write(errors.getString("error.funky"));
         log.fatal(levelName + " - " + e.toString());