From 3134f180660c138b820a10ca91144fd42b28ec08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Capon?= <46624375+FrancoisCapon@users.noreply.github.com> Date: Sat, 1 Jun 2024 10:50:38 +0200 Subject: [PATCH] fix: Success if only Smith earn most salary (#1744) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Update labels * Update Java * Update Test --------- Co-authored-by: René Zubcevic --- .../introduction/SqlInjectionLesson9.java | 100 +++++------ .../i18n/WebGoatLabels.properties | 4 +- .../introduction/SqlInjectionLesson9Test.java | 156 +++++------------- 3 files changed, 95 insertions(+), 165 deletions(-) diff --git a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson9.java b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson9.java index 3df08175a..1128e2fec 100644 --- a/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson9.java +++ b/src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson9.java @@ -63,36 +63,42 @@ public class SqlInjectionLesson9 extends AssignmentEndpoint { protected AttackResult injectableQueryIntegrity(String name, String auth_tan) { StringBuilder output = new StringBuilder(); - String query = + String queryInjection = "SELECT * FROM employees WHERE last_name = '" + name + "' AND auth_tan = '" + auth_tan + "'"; try (Connection connection = dataSource.getConnection()) { - try { - Statement statement = connection.createStatement(TYPE_SCROLL_SENSITIVE, CONCUR_UPDATABLE); - SqlInjectionLesson8.log(connection, query); - ResultSet results = statement.executeQuery(query); - var test = results.getRow() != 0; - if (results.getStatement() != null) { - if (results.first()) { - output.append(SqlInjectionLesson8.generateTable(results)); - } else { - // no results - return failed(this).feedback("sql-injection.8.no.results").build(); - } - } - } catch (SQLException e) { - System.err.println(e.getMessage()); - return failed(this) - .output("
" + e.getMessage() + "") - .build(); + // V2019_09_26_7__employees.sql + int oldMaxSalary = this.getMaxSalary(connection); + int oldSumSalariesOfOtherEmployees = this.getSumSalariesOfOtherEmployees(connection); + // begin transaction + connection.setAutoCommit(false); + // do injectable query + Statement statement = connection.createStatement(TYPE_SCROLL_SENSITIVE, CONCUR_UPDATABLE); + SqlInjectionLesson8.log(connection, queryInjection); + statement.execute(queryInjection); + // check new sum of salaries other employees and new salaries of John + int newJohnSalary = this.getJohnSalary(connection); + int newSumSalariesOfOtherEmployees = this.getSumSalariesOfOtherEmployees(connection); + if (newJohnSalary > oldMaxSalary + && newSumSalariesOfOtherEmployees == oldSumSalariesOfOtherEmployees) { + // success commit + connection.commit(); // need execute not executeQuery + connection.setAutoCommit(true); + output.append( + SqlInjectionLesson8.generateTable(this.getEmployeesDataOrderBySalaryDesc(connection))); + return success(this).feedback("sql-injection.9.success").output(output.toString()).build(); } - - return checkSalaryRanking(connection, output); - - } catch (Exception e) { + // failed roolback + connection.rollback(); + return failed(this) + .feedback("sql-injection.9.one") + .output( + SqlInjectionLesson8.generateTable(this.getEmployeesDataOrderBySalaryDesc(connection))) + .build(); + } catch (SQLException e) { System.err.println(e.getMessage()); return failed(this) .output("
" + e.getMessage() + "") @@ -100,29 +106,31 @@ public class SqlInjectionLesson9 extends AssignmentEndpoint { } } - private AttackResult checkSalaryRanking(Connection connection, StringBuilder output) { - try { - String query = "SELECT * FROM employees ORDER BY salary DESC"; - try (Statement statement = - connection.createStatement(TYPE_SCROLL_SENSITIVE, CONCUR_UPDATABLE); ) { - ResultSet results = statement.executeQuery(query); + private int getSqlInt(Connection connection, String query) throws SQLException { + Statement statement = connection.createStatement(TYPE_SCROLL_SENSITIVE, CONCUR_UPDATABLE); + ResultSet results = statement.executeQuery(query); + results.first(); + return results.getInt(1); + } - results.first(); - // user completes lesson if John Smith is the first in the list - if ((results.getString(2).equals("John")) && (results.getString(3).equals("Smith"))) { - output.append(SqlInjectionLesson8.generateTable(results)); - return success(this) - .feedback("sql-injection.9.success") - .output(output.toString()) - .build(); - } else { - return failed(this).feedback("sql-injection.9.one").output(output.toString()).build(); - } - } - } catch (SQLException e) { - return failed(this) - .output("
" + e.getMessage() + "") - .build(); - } + private int getMaxSalary(Connection connection) throws SQLException { + String query = "SELECT max(salary) FROM employees"; + return this.getSqlInt(connection, query); + } + + private int getSumSalariesOfOtherEmployees(Connection connection) throws SQLException { + String query = "SELECT sum(salary) FROM employees WHERE auth_tan != '3SL99A'"; + return this.getSqlInt(connection, query); + } + + private int getJohnSalary(Connection connection) throws SQLException { + String query = "SELECT salary FROM employees WHERE auth_tan = '3SL99A'"; + return this.getSqlInt(connection, query); + } + + private ResultSet getEmployeesDataOrderBySalaryDesc(Connection connection) throws SQLException { + String query = "SELECT * FROM employees ORDER BY salary DESC"; + Statement statement = connection.createStatement(TYPE_SCROLL_SENSITIVE, CONCUR_UPDATABLE); + return statement.executeQuery(query); } } diff --git a/src/main/resources/lessons/sqlinjection/i18n/WebGoatLabels.properties b/src/main/resources/lessons/sqlinjection/i18n/WebGoatLabels.properties index b702537ca..aca652d13 100644 --- a/src/main/resources/lessons/sqlinjection/i18n/WebGoatLabels.properties +++ b/src/main/resources/lessons/sqlinjection/i18n/WebGoatLabels.properties @@ -63,8 +63,8 @@ SqlStringInjectionHint.8.3=Try appending a SQL statement that always resolves to SqlStringInjectionHint.8.4=Make sure all quotes (" ' ") are opened and closed properly so the resulting SQL query is syntactically correct. SqlStringInjectionHint.8.5=Try extending the WHERE clause of the statement by adding something like: ' OR '1' = '1. -sql-injection.9.success=Well done! Now you are earning the most money. And at the same time you successfully compromised the integrity of data by changing the salary! -sql-injection.9.one=Still not earning enough! Better try again and change that. +sql-injection.9.success=Well done! Now you are earning the most money. And at the same time you successfully compromised the integrity of data by changing your salary! +sql-injection.9.one=Still not earning enough or only your own salary must be changed! Better try again and change that. SqlStringInjectionHint.9.1=Try to find a way, to chain another query to the end of the existing one. SqlStringInjectionHint.9.2=Use the ; metacharacter to do so. SqlStringInjectionHint.9.3=Make use of DML to change your salary. diff --git a/src/test/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson9Test.java b/src/test/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson9Test.java index 4cd0c368f..44438f6c0 100644 --- a/src/test/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson9Test.java +++ b/src/test/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson9Test.java @@ -37,123 +37,7 @@ import org.springframework.test.web.servlet.request.MockMvcRequestBuilders; */ public class SqlInjectionLesson9Test extends SqlLessonTest { - private String completedError = "JSON path \"lessonCompleted\""; - - @Test - public void oneAccount() throws Exception { - try { - mockMvc - .perform( - MockMvcRequestBuilders.post("/SqlInjection/attack9") - .param("name", "Smith") - .param("auth_tan", "3SL99A")) - .andExpect(status().isOk()) - .andExpect(jsonPath("lessonCompleted", is(false))) - .andExpect(jsonPath("$.feedback", is(messages.getMessage("sql-injection.9.one")))) - .andExpect(jsonPath("$.output", containsString("
"))); - } catch (AssertionError e) { - if (!e.getMessage().contains(completedError)) throw e; - - mockMvc - .perform( - MockMvcRequestBuilders.post("/SqlInjection/attack9") - .param("name", "Smith") - .param("auth_tan", "3SL99A")) - .andExpect(status().isOk()) - .andExpect(jsonPath("lessonCompleted", is(true))) - .andExpect(jsonPath("$.feedback", is(messages.getMessage("sql-injection.9.success")))) - .andExpect(jsonPath("$.output", containsString("
"))); - } - } - - @Test - public void multipleAccounts() throws Exception { - try { - mockMvc - .perform( - MockMvcRequestBuilders.post("/SqlInjection/attack9") - .param("name", "Smith") - .param("auth_tan", "3SL99A' OR '1' = '1")) - .andExpect(status().isOk()) - .andExpect(jsonPath("lessonCompleted", is(false))) - .andExpect(jsonPath("$.feedback", is(messages.getMessage("sql-injection.9.one")))) - .andExpect( - jsonPath( - "$.output", - containsString( - "
96134<\\/td>Bob<\\/td>Franco<\\/td>Marketing<\\/td>83700<\\/td>LO9S2V<\\/td><\\/tr>"))); - } catch (AssertionError e) { - if (!e.getMessage().contains(completedError)) throw e; - - mockMvc - .perform( - MockMvcRequestBuilders.post("/SqlInjection/attack9") - .param("name", "Smith") - .param("auth_tan", "3SL99A' OR '1' = '1")) - .andExpect(status().isOk()) - .andExpect(jsonPath("lessonCompleted", is(true))) - .andExpect(jsonPath("$.feedback", is(messages.getMessage("sql-injection.9.success")))) - .andExpect( - jsonPath( - "$.output", - containsString( - "
96134<\\/td>Bob<\\/td>Franco<\\/td>Marketing<\\/td>83700<\\/td>LO9S2V<\\/td><\\/tr>"))); - } - } - - @Test - public void wrongNameReturnsNoAccounts() throws Exception { - try { - mockMvc - .perform( - MockMvcRequestBuilders.post("/SqlInjection/attack9") - .param("name", "Smithh") - .param("auth_tan", "3SL99A")) - .andExpect(status().isOk()) - .andExpect(jsonPath("lessonCompleted", is(false))) - .andExpect(jsonPath("$.feedback", is(messages.getMessage("sql-injection.8.no.results")))) - .andExpect(jsonPath("$.output").doesNotExist()); - } catch (AssertionError e) { - if (!e.getMessage().contains(completedError)) throw e; - - mockMvc - .perform( - MockMvcRequestBuilders.post("/SqlInjection/attack9") - .param("name", "Smithh") - .param("auth_tan", "3SL99A")) - .andExpect(status().isOk()) - .andExpect(jsonPath("lessonCompleted", is(true))) - .andExpect(jsonPath("$.feedback", is(messages.getMessage("sql-injection.8.no.success")))) - .andExpect(jsonPath("$.output").doesNotExist()); - } - } - - @Test - public void wrongTANReturnsNoAccounts() throws Exception { - try { - mockMvc - .perform( - MockMvcRequestBuilders.post("/SqlInjection/attack9") - .param("name", "Smithh") - .param("auth_tan", "")) - .andExpect(status().isOk()) - .andExpect(jsonPath("lessonCompleted", is(false))) - .andExpect(jsonPath("$.feedback", is(messages.getMessage("sql-injection.8.no.results")))) - .andExpect(jsonPath("$.output").doesNotExist()); - } catch (AssertionError e) { - if (!e.getMessage().contains(completedError)) throw e; - - mockMvc - .perform( - MockMvcRequestBuilders.post("/SqlInjection/attack9") - .param("name", "Smithh") - .param("auth_tan", "")) - .andExpect(status().isOk()) - .andExpect(jsonPath("lessonCompleted", is(true))) - .andExpect(jsonPath("$.feedback", is(messages.getMessage("sql-injection.9.success")))) - .andExpect(jsonPath("$.output").doesNotExist()); - } - } + private final String completedError = "JSON path \"lessonCompleted\""; @Test public void malformedQueryReturnsError() throws Exception { @@ -181,6 +65,44 @@ public class SqlInjectionLesson9Test extends SqlLessonTest { } } + @Test + public void SmithIsNotMostEarning() throws Exception { + mockMvc + .perform( + MockMvcRequestBuilders.post("/SqlInjection/attack9") + .param("name", "Smith") + .param( + "auth_tan", + "3SL99A'; UPDATE employees SET salary = 9999 WHERE last_name = 'Smith")) + .andExpect(status().isOk()) + .andExpect(jsonPath("lessonCompleted", is(false))) + .andExpect(jsonPath("$.feedback", is(messages.getMessage("sql-injection.9.one")))); + } + + @Test + public void OnlySmithSalaryMustBeUpdated() throws Exception { + mockMvc + .perform( + MockMvcRequestBuilders.post("/SqlInjection/attack9") + .param("name", "Smith") + .param("auth_tan", "3SL99A'; UPDATE employees SET salary = 9999 -- ")) + .andExpect(status().isOk()) + .andExpect(jsonPath("lessonCompleted", is(false))) + .andExpect(jsonPath("$.feedback", is(messages.getMessage("sql-injection.9.one")))); + } + + @Test + public void OnlySmithMustMostEarning() throws Exception { + mockMvc + .perform( + MockMvcRequestBuilders.post("/SqlInjection/attack9") + .param("name", "'; UPDATE employees SET salary = 999999 -- ") + .param("auth_tan", "")) + .andExpect(status().isOk()) + .andExpect(jsonPath("lessonCompleted", is(false))) + .andExpect(jsonPath("$.feedback", is(messages.getMessage("sql-injection.9.one")))); + } + @Test public void SmithIsMostEarningCompletesAssignment() throws Exception { mockMvc