feat: Add IDOR Vulnerability#504
Conversation
8522e52 to
4f1c786
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #504 +/- ##
============================================
- Coverage 49.07% 47.86% -1.21%
- Complexity 346 353 +7
============================================
Files 56 57 +1
Lines 2101 2248 +147
Branches 226 243 +17
============================================
+ Hits 1031 1076 +45
- Misses 989 1073 +84
- Partials 81 99 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4f1c786 to
558749e
Compare
|
@antriksh-9 UI is not consistent with the project theme. Please use right CSS classes.
|
| description = "IDOR_LEVEL_2_FAKE_LOGIN_NO_AUTHZ", | ||
| payload = "Logged in user is 101 but try accessing 102") | ||
| @VulnerableAppRequestMapping(value = LevelConstants.LEVEL_2, htmlTemplate = "LEVEL_2/IDOR") | ||
| public ResponseEntity<GenericVulnerabilityResponseBean<String>> idorLevel2( |
There was a problem hiding this comment.
This doesn't seems right to me. The way I am thinking is: Give a login screen, and let user get details for other user instead of logged in user.
How can we do this is? create 3 users in database (we have H2 db), you can create the user in init script, we already have scripts example: https://github.com/SasanLabs/VulnerableApp/tree/master/src/main/resources/scripts/SQLInjection/db, you can create another script under folder idor/db/ and then refer it in https://github.com/SasanLabs/VulnerableApp/blob/master/src/main/java/org/sasanlabs/configuration/VulnerableAppConfiguration.java#L130.
Login the user in level 1 and give logout button too in level 1 and then let the user access its details like salary etc.
Flow is User login -> we show all the user details like full profile.
Now level 1 would allow to access user profile like all the Personal details even for other users.
Level 2 would be little difficult where user will login again on the level (set cookie on login) but say cookie can be tampered to get other users details.
Level 3 would add a bit more difficulty like JWT or however you want to think on this.
Level 4 be secure or you can think more levels too.
Level 5 can be role based :) additional thing to learn more and introduce vulnerabilities related to RBAC
Level 6 secure RBAC
Level 7 Insecure ABAC (Attribute checks like salary is visible ot everyone)
Level 8 Secure ABAC (only if attribute matches user id)
I am just giving examples of number of Levels but you can implement as per your thought.
There was a problem hiding this comment.
Refactored implementation:
-
Level 1 – No authorization check
-
Level 2 – Cookie tampering
-
Level 3 – Base64 token tampering (userId manipulation)
-
Level 4 – Broken RBAC (role escalation via unsigned token)
-
Level 5 – Secure RBAC implementation (proper role validation from DB)
I've updated the UI keeping in mind the project theme, however, if any changes are required please suggest.. |
| vulnerabilityExposed = VulnerabilityType.INSECURE_DIRECT_OBJECT_REFERENCE, | ||
| description = "IDOR_LEVEL_2_COOKIE_TAMPERING", | ||
| payload = "IDOR_PAYLOAD_LEVEL_2") | ||
| @VulnerableAppRequestMapping(value = LevelConstants.LEVEL_2, htmlTemplate = "LEVEL_2/IDOR") |
There was a problem hiding this comment.
I would suggest using POST request as Login and GET are not considered good.
requestMethod = RequestMethod.POST can be added to Annotation to make it post endpoint.
There was a problem hiding this comment.
This is applicable to all levels.
| sql, | ||
| new Object[] {loggedInUser}, | ||
| (rs, rowNum) -> | ||
| "User: " |
There was a problem hiding this comment.
Return a User Object instead of returning string.
Create a User POJO with UserName , Id , Salary etc and return that. In UI you can read it. it is good practice.
| sql, | ||
| new Object[] {username, password}, | ||
| (rs, rowNum) -> | ||
| "{\"userId\":" |
|
|
||
| int userId = Integer.parseInt(decoded.split("userId\":")[1].split(",")[0]); | ||
|
|
||
| String role = decoded.split("role\":\"")[1].split("\"")[0]; |
There was a problem hiding this comment.
Better way is to use JSON and convert it into POJO instead of splitting as split is error prone and it is not a good practice
| sql, | ||
| new Object[] {userId}, | ||
| (rs, rowNum) -> | ||
| "User: " |
There was a problem hiding this comment.
Please return POJO and that would clean up entire code.
|
|
||
| String decoded = new String(Base64.getDecoder().decode(token)); | ||
|
|
||
| int tokenUserId = Integer.parseInt(decoded.split("userId\":")[1].split(",")[0]); |
There was a problem hiding this comment.
please use POJO and JSON Deserialize into POJO
| // Proper authorization | ||
| if ("ADMIN".equalsIgnoreCase(actualRole) || tokenUserId == id) { | ||
|
|
||
| String sql = "SELECT username, salary, role FROM idor_users WHERE id=?"; |
There was a problem hiding this comment.
I think this can be made constant string.
| // LOGIN FLOW | ||
| if (username != null && password != null) { | ||
|
|
||
| String sql = "SELECT id, role FROM idor_users WHERE username=? AND password=?"; |
There was a problem hiding this comment.
I think this could be made string constant and then reused at multiple placed.
| String role = decoded.split("role\":\"")[1].split("\"")[0]; | ||
|
|
||
| // RBAC logic | ||
| if ("ADMIN".equalsIgnoreCase(role) || tokenUserId == id) { |
There was a problem hiding this comment.
Make it a String constant as well.
| <div id="loggedInDisplay"></div> | ||
|
|
||
| <h5>Fetch Profile</h5> | ||
| <div id="profileSection"> |
There was a problem hiding this comment.
I think we should just have fetch profile button and that is based on logged in user and let user/scanner find out that there is a way to fetch other users details as well.
There was a problem hiding this comment.
I think this dropdown is not needed.
| let password = document.getElementById("password").value; | ||
|
|
||
| let url = getUrlForVulnerabilityLevel(); | ||
| let queryParams = "?username=" + username + "&password=" + password; |
|
|
||
| <div id="infoSection"> | ||
| <p> | ||
| The application uses a Base64 encoded token. |
There was a problem hiding this comment.
This is kind of a hint. let user find it out from the hint? Hint comes from AttackVector annotation
|
|
||
| <div id="infoSection"> | ||
| <p> | ||
| The application enforces role-based access. |
There was a problem hiding this comment.
This is again another hint. Let user find it out from hint section.,
| @@ -0,0 +1,50 @@ | |||
| <div id="idor_level_4"> | |||
|
|
|||
| <h3>IDOR - Level 4 (Broken RBAC)</h3> | |||
There was a problem hiding this comment.
you can remove Broken RBAC from this and let user find it out
| </div> | ||
|
|
||
| <div id="response"></div> | ||
| <div id="hint"> |
There was a problem hiding this comment.
hints come automatically from AttackVector Annotation. Use that annotation for this.
|
|
||
| <div id="infoSection"> | ||
| <p> | ||
| This version validates user role from the database. |
There was a problem hiding this comment.
This is not needed i think.
|
|
||
| <div id="response"></div> | ||
| <div id="hint"> | ||
| <p>Hint: You can use <code>atob(token)</code> and <code>btoa()</code>in the browser console to decode/encode Base64.</p> |
| import org.springframework.jdbc.core.JdbcTemplate; | ||
| import org.springframework.web.bind.annotation.RequestParam; | ||
|
|
||
| @VulnerableAppRestController(descriptionLabel = "IDOR_VULNERABILITY", value = "IDORVulnerability") |
| @@ -0,0 +1,5 @@ | |||
| IDOR_PAYLOAD_LEVEL_1=Login as one user and modify the 'id' request parameter to access another user's data. | |||
|
@antriksh-9 thanks a lot for the PR. I will refactor the code but overall logic remains same. thanks for all the help. Feel free to pick another level. |
I was working on it, would have commit changes today. |
oh sorry, Apologies for taking over the PR, Yes this PR is merged but I added another PR with refactoring. Sorry for the mess up, I will be more concisions from now onwards. |
Oh no worries, probably I should have left "Working on the changes" comment here. |





Description
This PR introduces a new IDOR (Insecure Direct Object Reference) vulnerability under Broken Access Control.
The implementation demonstrates how improper authorisation checks allow users to access resources belonging to other users.
Fixes: #498
Changes Added
Added
IDORVulnerability.javaAdded new enum entry in
VulnerabilityType:INSECURE_DIRECT_OBJECT_REFERENCE (CWE-639)Implemented 3 progressive levels:
Added templates for:
LEVEL_1LEVEL_2LEVEL_3Added corresponding:
Added entries in
messages.propertiesAdded
IDORVulnerabilityTestCovered:
Verified using:
Verification: