feat: add challenge mode to Error-Based and Union-Based SQL Injection - #708
feat: add challenge mode to Error-Based and Union-Based SQL Injection#708Mubashir78 wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChallenge-mode metadata is added to error-based SQL injection levels 1–4 and union-based levels 1–2. Localized challenge text, hints, payload descriptions, and example values are added without changing endpoint logic. ChangesSQL Injection Challenge Mode
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #708 +/- ##
=========================================
Coverage 55.37% 55.37%
Complexity 760 760
=========================================
Files 105 105
Lines 4229 4229
Branches 452 452
=========================================
Hits 2342 2342
Misses 1683 1683
Partials 204 204 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/resources/i18n/messages.properties`:
- Around line 310-314: Align the Level 1 UNION challenge text and payload by
updating UNION_SQL_INJECTION_VULNERABILITY_LEVEL1_PAYLOAD_VALUE to select from a
distinct existing table, while preserving the expected column count and
extraction behavior described by the challenge and hints.
- Around line 291-295: The Level 2 SQL injection payload values must account for
the endpoint-appended closing quote. Update
ERROR_SQL_INJECTION_VULNERABILITY_LEVEL2_PAYLOAD_VALUE and the corresponding
Level 2 UNION payload near the related entries to include a closing quote before
the comment marker and trailing whitespace after --, using H2-compatible syntax
such as ' OR '1'='1' --.
- Around line 297-307: Revise the Level 3 and Level 4 challenge, hint, payload
description, and payload value entries to match the actual endpoint behavior,
where single quotes are stripped before constructing the query. Remove the
currently invalid injection claims and payloads, and use only a verified exploit
for the exact methods; otherwise describe the input as remaining a quoted
literal and adjust the challenge accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f307322a-9125-4493-bd64-7d325fc4fda0
📒 Files selected for processing (3)
src/main/java/org/sasanlabs/service/vulnerability/sqlInjection/ErrorBasedSQLInjectionVulnerability.javasrc/main/java/org/sasanlabs/service/vulnerability/sqlInjection/UnionBasedSQLInjectionVulnerability.javasrc/main/resources/i18n/messages.properties
|
Tip For best results, initiate chat on the files or code changes.
|
|
@coderabbitai Review latest changes pushed on this PR |
|
✅ Action performedReview finished.
|
|
@coderabbitai you need to review the latest commit, not the commit you have already reviewed. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== PR commits =="
gh pr view 708 --repo SasanLabs/VulnerableApp --json commits --jq '.commits[] | "\(.oid)\t\(.messageHeadline)"'
echo
echo "== Checked-out head and parent =="
git show -s --format='%H%n%P%n%s' HEAD
echo
echo "== Exact latest-commit diff summary =="
git diff --stat HEAD^ HEAD
echo
echo "== Exact latest-commit diff =="
git diff --find-renames --find-copies --no-ext-diff HEAD^ HEADLength of output: 6687 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== SQL injection class outlines =="
ast-grep outline src/main/java/org/sasanlabs/service/vulnerability/sqlInjection/ErrorBasedSQLInjectionVulnerability.java --items all
ast-grep outline src/main/java/org/sasanlabs/service/vulnerability/sqlInjection/UnionBasedSQLInjectionVulnerability.java --items all
echo
echo "== Endpoint query construction and challenge annotations =="
rg -n -C 12 --type java \
'LEVEL[1-4]|prepareStatement\s*\(|SELECT\s+|replaceAll\s*\(|createStatement\s*\(' \
src/main/java/org/sasanlabs/service/vulnerability/sqlInjection/ErrorBasedSQLInjectionVulnerability.java \
src/main/java/org/sasanlabs/service/vulnerability/sqlInjection/UnionBasedSQLInjectionVulnerability.java
echo
echo "== auth_users schema and column references =="
rg -n -i -C 8 \
'(CREATE\s+TABLE\s+(IF\s+NOT\s+EXISTS\s+)?auth_users\b|INSERT\s+INTO\s+auth_users\b|\bauth_users\b)' \
srcLength of output: 82546 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Database initialization / data source references =="
rg -n -i -C 6 \
'(applicationJdbcTemplate|applicationDataSource|jdbc:|schema\.sql|data\.sql|ResourceDatabasePopulator|ScriptUtils|DatabasePopulator)' \
src/main src/test
echo
echo "== Authentication database script references =="
rg -n -C 5 \
'(scripts/Authentication/db/(schema|data)\.sql|Authentication/db/(schema|data)\.sql)' \
src
echo
echo "== Changed properties keys: duplicate and continuation validation =="
python3 - <<'PY'
from collections import Counter
from pathlib import Path
path = Path("src/main/resources/i18n/messages.properties")
lines = path.read_text(encoding="utf-8").splitlines()
keys = []
for number, line in enumerate(lines, 1):
stripped = line.lstrip()
if not stripped or stripped.startswith(("#", "!")) or line.endswith("\\"):
continue
if "=" in line:
key = line.split("=", 1)[0].strip()
keys.append((key, number))
counts = Counter(key for key, _ in keys)
duplicates = [(key, number) for key, number in keys if counts[key] > 1]
print("Duplicate keys:", duplicates if duplicates else "none")
for number in range(292, 321):
print(f"{number}: {lines[number - 1]!r}")
PYLength of output: 59854
No actionable issues found. The revised UNION payloads match the three-column ✅ Action performedReview finished.
|
| BLIND_SQL_INJECTION_URL_PARAM_WRAPPED_WITH_SINGLE_QUOTE_APPENDED_TO_QUERY=Query param is wrapped around "'" and then appended to the SQL Query hence query can be manipulated. | ||
|
|
||
| # Error-Based SQL Injection Challenge Mode | ||
| ERROR_SQL_INJECTION_VULNERABILITY_LEVEL1_CHALLENGE=Extract all car information by exploiting error-based SQL injection where the input is appended directly to the query without any wrapping. |
There was a problem hiding this comment.
these doesn't seems right challenges. I suggest looking at Blind SQL Injection to see how we can really use SQL Injection to extract databases and other details
Add @ChallengeCard annotations to all unsecure levels in ErrorBasedSQLInjectionVulnerability (4 levels) and UnionBasedSQLInjectionVulnerability (2 levels), following the established pattern from BlindSQLInjectionVulnerability. Add 30 corresponding message keys to messages.properties for challenge text, hints, and payloads for each level. Closes SasanLabs#561
…yloads - Level 2 payloads: add closing quote and trailing space for H2 comment syntax - Level 3/4 challenge text: accurately describe that replaceAll single-quote stripping prevents standard injection - UNION Level 1/2 payloads: select from auth_users instead of cars to extract data from a distinct table
…rom auth_users - Error-based L1/L2: extract admin_sqli password via type conversion errors - Error-based L3/L4: analyze effectiveness of single-quote stripping - Union-based L1/L2: extract credentials from auth_users table
4bc310b to
7a940f1
Compare
|
| ERROR_SQL_INJECTION_VULNERABILITY_LEVEL3_HINT1=The application removes all single quote characters from the input using replaceAll before building the query. | ||
| ERROR_SQL_INJECTION_VULNERABILITY_LEVEL3_HINT2=Although single quotes are stripped, using PreparedStatement with concatenation is the real issue. Without parameterized binding, other injection vectors may still be exploitable. | ||
| ERROR_SQL_INJECTION_VULNERABILITY_LEVEL3_PAYLOAD_DESCRIPTION=The single-quote stripping prevents standard injection but the concatenation pattern remains insecure. | ||
| ERROR_SQL_INJECTION_VULNERABILITY_LEVEL3_PAYLOAD_VALUE=1 OR 1=1 -- |
There was a problem hiding this comment.
why not have something similar as challenge and payload for this level too as level 1 and level 2?
| ERROR_SQL_INJECTION_VULNERABILITY_LEVEL4_HINT1=A PreparedStatement object is created, but the query string is built by concatenation with user input rather than using parameterized binding. | ||
| ERROR_SQL_INJECTION_VULNERABILITY_LEVEL4_HINT2=Single quotes are stripped from input before concatenation. However, the PreparedStatement offers zero protection because the injection happens before the query is prepared, not through parameterized placeholders. | ||
| ERROR_SQL_INJECTION_VULNERABILITY_LEVEL4_PAYLOAD_DESCRIPTION=The misused PreparedStatement provides no protection since concatenation happens before preparation. | ||
| ERROR_SQL_INJECTION_VULNERABILITY_LEVEL4_PAYLOAD_VALUE=1 OR 1=1 -- |
There was a problem hiding this comment.
same as above one. this challenge should also be related to retrieving some data from other tables.
| ERROR_SQL_INJECTION_VULNERABILITY_LEVEL1_PAYLOAD_DESCRIPTION=Error-based SQL injection that forces the database to display admin_sqli's password through a type conversion error. | ||
| ERROR_SQL_INJECTION_VULNERABILITY_LEVEL1_PAYLOAD_VALUE=1 AND CAST((SELECT password FROM auth_users WHERE username='admin_sqli') AS int)=1 | ||
|
|
||
| ERROR_SQL_INJECTION_VULNERABILITY_LEVEL2_CHALLENGE=The parameter is now wrapped in single quotes. Use the same error-based technique to extract the admin_sqli password from the auth_users table. |
There was a problem hiding this comment.
as user has already retrieved password from level 1, i think in this challenges, we can ask user to find out all the tables or may be password for any other user. Scripts are there under https://github.com/SasanLabs/VulnerableApp/blob/master/src/main/resources/scripts
Example:
https://github.com/SasanLabs/VulnerableApp/blob/master/src/main/resources/scripts/Authentication/db/data.sql
https://github.com/SasanLabs/VulnerableApp/blob/master/src/main/resources/scripts/SessionManagement/db/data.sql#L2
| BLIND_SQL_INJECTION_URL_PARAM_WRAPPED_WITH_SINGLE_QUOTE_APPENDED_TO_QUERY=Query param is wrapped around "'" and then appended to the SQL Query hence query can be manipulated. | ||
|
|
||
| # Error-Based SQL Injection Challenge Mode | ||
| ERROR_SQL_INJECTION_VULNERABILITY_LEVEL1_CHALLENGE=Using only the error messages returned by the database, discover the password stored for admin_sqli in the auth_users table. The application never displays table data directly - only errors. |
There was a problem hiding this comment.
Ask for any other user from https://github.com/SasanLabs/VulnerableApp/blob/master/src/main/resources/scripts/Authentication/db/data.sql
| ERROR_SQL_INJECTION_VULNERABILITY_LEVEL4_PAYLOAD_VALUE=1 OR 1=1 -- | ||
|
|
||
| # Union-Based SQL Injection Challenge Mode | ||
| UNION_SQL_INJECTION_VULNERABILITY_LEVEL1_CHALLENGE=Using a UNION-based injection, extract the username and password for admin_sqli from the auth_users table. The input is appended directly with no wrapping. |
There was a problem hiding this comment.
we should not use same challenge for each level. please think of something else. you can start the app and access the database and write the challenges. Accessing ways for database: https://github.com/SasanLabs/VulnerableApp/tree/master#connecting-to-embedded-h2-database
|
Hey @Mubashir78, was also working on #561 and only saw this after. Sorry for the overlap, should have checked open PRs first. Ran your payloads against a live instance before deciding what to do. Level 1 works well and leaks the real password through the CAST error, nice technique, better than what I had. The other five don't reproduce for me though. Level 2 (' AND CAST(...)=1 -- ) fails because id='' breaks the INT conversion before the AND even runs. Level 3 and 4 (1 OR 1=1 --) give the same generic conversion error as any bad input, quotes stripped or not. Union Level 1 (1 UNION SELECT...) doesn't work because id=1 also matches the real Audi row, and since it only reads the first result, the union row never surfaces. Needs a nonexistent id like -1. Union Level 2 hits the same id='' issue as Level 2, and since UnionBasedSQLInjectionVulnerability isn't wrapped in a try/catch it comes back as a raw 500. Opened #761 with different payloads for the same 6 levels, verified end to end. Couldn't find a real bypass for levels 3/4 at all, tried quote variants, backslash, unicode look-alikes, dollar quoting, so I built those around the verbose error leaking the table and engine instead. Not trying to step on this, just want whichever one gets merged to actually work. Happy to fold your Level 1 idea into either PR. |
Addresses @preetkaran20's review on SasanLabs#761: - Error-Based levels 1-2 now target extracting admin_sqli's real password from auth_users via CAST-based error exfiltration, instead of a boolean OR 1=1 tautology - matches the technique from SasanLabs#708's Level 1 (independently confirmed working) and is a better fit for what "Error-Based" is meant to teach. - Error-Based levels 3-4 keep the verbose-error-disclosure angle (full data access genuinely isn't reachable through this INT-typed, quote-stripped id parameter - verified again against the redesigned levels 1-2 payloads too) but are now framed as a goal to reach (identify the database engine) rather than a description of the mechanism. - Union-Based levels 1-2 now target the same specific admin_sqli record via UNION SELECT against auth_users, rather than a generic row from an unrelated table. - Every challenge is now self-contained: no level references another level's filtering by name, and quote/mechanism details that were in the challenge text moved into the hints instead. All six payloads re-verified end-to-end against a running instance.
…#761) * feat: add Challenge mode to Error-Based and Union-Based SQL Injection (#561) Completes the remaining two SQL injection classes referenced in #561 - Blind SQL Injection already has Challenge mode from a prior PR. Follows that PR's precedent of only annotating UNSECURE levels, keeping all three SQL injection modules consistent with each other. Error-Based levels 1-2 and Union-Based levels 1-2 use classic boolean tautology and UNION SELECT bypasses, verified end-to-end against a running instance (Union-Based leaks a real row from the unrelated "users" table into the car response). Error-Based levels 3-4 strip every single quote from the input and compare it against a strictly INT-typed column, which closes off actual data access - verified this holds against quote variants, backslash escaping, and dollar-quoting, not just the literal apostrophe. Their challenge instead targets the verbose SQL error the app relays back, which leaks the table name, column layout, and database engine - a real CWE-209 information disclosure distinct from full data exfiltration, and an honest depiction of what those two levels actually expose. * fix: redesign SQL injection challenges per maintainer review feedback Addresses @preetkaran20's review on #761: - Error-Based levels 1-2 now target extracting admin_sqli's real password from auth_users via CAST-based error exfiltration, instead of a boolean OR 1=1 tautology - matches the technique from #708's Level 1 (independently confirmed working) and is a better fit for what "Error-Based" is meant to teach. - Error-Based levels 3-4 keep the verbose-error-disclosure angle (full data access genuinely isn't reachable through this INT-typed, quote-stripped id parameter - verified again against the redesigned levels 1-2 payloads too) but are now framed as a goal to reach (identify the database engine) rather than a description of the mechanism. - Union-Based levels 1-2 now target the same specific admin_sqli record via UNION SELECT against auth_users, rather than a generic row from an unrelated table. - Every challenge is now self-contained: no level references another level's filtering by name, and quote/mechanism details that were in the challenge text moved into the hints instead. All six payloads re-verified end-to-end against a running instance. * fix: point Union-Based challenges at idor_users instead of auth_users Error-Based already targets auth_users for admin_sqli's password, so Union-Based now targets a different table (idor_users) to avoid teaching the same lesson twice. Leaks the ADMIN role's username and salary instead. Verified against a running instance.
4 participants
Summary
Add
@ChallengeCardannotations to all unsecure levels inErrorBasedSQLInjectionVulnerabilityandUnionBasedSQLInjectionVulnerability, following the established pattern fromBlindSQLInjectionVulnerability.Changes
ErrorBasedSQLInjectionVulnerability.java@ChallengeCardannotations to 4 unsecure levels (Level1-Level4)UnionBasedSQLInjectionVulnerability.java@ChallengeCardannotations to 2 unsecure levels (Level1-Level2)messages.propertiesCloses #561
Summary by CodeRabbit