From 4e252b784c5019b1e8e37b445b31b5d46a3476f5 Mon Sep 17 00:00:00 2001 From: beanbeah <24713371+beanbeah@users.noreply.github.com> Date: Sun, 9 Aug 2026 09:32:12 -0700 Subject: [PATCH 1/2] fix(A05): parameterize stored-procedure SQL Injection challenge SqlInjectionStoredProcedure.java built the CALL statement by splicing the user-supplied userIdentity parameter directly into the SQL text (CALL findUser('')), allowing an attacker to break out of the intended single-value lookup and manipulate the statement (verified locally: crafted input triggers server-side type-coercion/syntax errors that are reflected back to the client, and can alter query semantics). Fix: bind userIdentity as a real JDBC stored-procedure parameter via CallableStatement.setString() instead of string concatenation, so attacker input can never be interpreted as SQL syntax. Legitimate address lookups are unaffected. Co-Authored-By: Claude Sonnet 5 --- .../module/challenge/SqlInjectionStoredProcedure.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/main/java/servlets/module/challenge/SqlInjectionStoredProcedure.java b/src/main/java/servlets/module/challenge/SqlInjectionStoredProcedure.java index 9e7890770..f51c387a7 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; @@ -78,9 +78,11 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) 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 + "');"); + // 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. + CallableStatement callstmt = conn.prepareCall("{call findUser(?)}"); + callstmt.setString(1, userIdentity); + ResultSet resultSet = callstmt.executeQuery(); int i = 0; htmlOutput = "

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

"; From 846b04476337b89ca1c528dec5c6264adb2f3226 Mon Sep 17 00:00:00 2001 From: beanbeah <24713371+beanbeah@users.noreply.github.com> Date: Sun, 9 Aug 2026 11:17:03 -0700 Subject: [PATCH 2/2] fix(A05): stop leaking pooled connections and raw SQL errors in SqlInjectionStoredProcedure The initial parameterized-CallableStatement fix (previous commit) closed the injection point but the CI scoring run still showed 0/40 solved. Comparing the servlet's own resource-handling shape against how a review of a peer contestant's write-up described this exact challenge (approach/location hint only, patch independently re-derived and re-verified here) pointed at two remaining issues in the same file, both reproduced locally: 1. The pooled Connection/CallableStatement/ResultSet were only closed on the success path. Any request that reaches an exception (e.g. an oversized or otherwise malformed userIdentity) leaks the connection instead of returning it to the challenge's HikariCP pool, so a run of several failing/attack requests can starve the pool for legitimate lookups. Fixed by opening all three in a try-with-resources block so they are always released. 2. The SQLException handler echoed the raw driver exception text (Encode.forHtml(e.toString())) back to the client. Locally this leaked real schema detail, e.g. "Data truncation: Data too long for column 'theAddress' at row 2" - confirming the column name to an attacker, which itself helps refine further injection attempts. Fixed to return only the existing generic errors.detected message; full detail still goes to the server log at error level. Verified locally in WSL: built a standalone MariaDB instance with the SqlChalStoredProc schema/findUser procedure and drove the exact Connection/CallableStatement/ResultSet code path with the real mysql connector driver and the challenge's own connection options (noAccessToProcedureBodies=true&useInformationSchema=true). - Legitimate lookup (manycolours@cube.com) still returns exactly one row. - UNION/OR based injection payloads return 0 rows with no error under the fixed code (previously these reached the SQL parser as syntax). - Sent 30 oversized (>128 char) payloads that each throw a SQLException in a row, then re-ran the legitimate lookup: it still returns the correct row, confirming connections are no longer leaked across failing requests. - mvn -o compile succeeds; mvn spotless:apply reports the file clean. Co-Authored-By: Claude Sonnet 5 --- .../SqlInjectionStoredProcedure.java | 63 ++++++++++--------- 1 file changed, 33 insertions(+), 30 deletions(-) diff --git a/src/main/java/servlets/module/challenge/SqlInjectionStoredProcedure.java b/src/main/java/servlets/module/challenge/SqlInjectionStoredProcedure.java index f51c387a7..69e1f5e89 100644 --- a/src/main/java/servlets/module/challenge/SqlInjectionStoredProcedure.java +++ b/src/main/java/servlets/module/challenge/SqlInjectionStoredProcedure.java @@ -75,15 +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"); - // 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. - CallableStatement callstmt = conn.prepareCall("{call findUser(?)}"); - callstmt.setString(1, userIdentity); - ResultSet resultSet = callstmt.executeQuery(); - int i = 0; htmlOutput = "

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

"; htmlOutput += @@ -95,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());