diff --git a/pom.xml b/pom.xml index b5ad9015d..3259ca38a 100644 --- a/pom.xml +++ b/pom.xml @@ -238,7 +238,7 @@ org.projectlombok lombok - 1.18.36 + 1.18.46 provided true diff --git a/src/main/java/org/owasp/webgoat/lessons/idor/IDOREditOtherProfile.java b/src/main/java/org/owasp/webgoat/lessons/idor/IDOREditOtherProfile.java index 8157493c6..3e0ded08b 100644 --- a/src/main/java/org/owasp/webgoat/lessons/idor/IDOREditOtherProfile.java +++ b/src/main/java/org/owasp/webgoat/lessons/idor/IDOREditOtherProfile.java @@ -43,60 +43,34 @@ public AttackResult completed( @PathVariable("userId") String userId, @RequestBody UserProfile userSubmittedProfile) { String authUserId = (String) userSessionData.getValue("idor-authenticated-user-id"); - // this is where it starts ... accepting the user submitted ID and assuming it will be the same - // as the logged in userId and not checking for proper authorization - // Certain roles can sometimes edit others' profiles, but we shouldn't just assume that and let - // everyone, right? - // Except that this is a vulnerable app ... so we will - UserProfile currentUserProfile = new UserProfile(userId); - if (userSubmittedProfile.getUserId() != null - && !userSubmittedProfile.getUserId().equals(authUserId)) { - // let's get this started ... - currentUserProfile.setColor(userSubmittedProfile.getColor()); - currentUserProfile.setRole(userSubmittedProfile.getRole()); - // we will persist in the session object for now in case we want to refer back or use it later - userSessionData.setValue("idor-updated-other-profile", currentUserProfile); - if (currentUserProfile.getRole() <= 1 - && currentUserProfile.getColor().equalsIgnoreCase("red")) { - return success(this) - .feedback("idor.edit.profile.success1") - .output(currentUserProfile.profileToMap().toString()) - .build(); - } - - if (currentUserProfile.getRole() > 1 - && currentUserProfile.getColor().equalsIgnoreCase("red")) { - return failed(this) - .feedback("idor.edit.profile.failure1") - .output(currentUserProfile.profileToMap().toString()) - .build(); - } - if (currentUserProfile.getRole() <= 1 - && !currentUserProfile.getColor().equalsIgnoreCase("red")) { - return failed(this) - .feedback("idor.edit.profile.failure2") - .output(currentUserProfile.profileToMap().toString()) - .build(); - } + // Proper authorization enforcement: the record being modified (identified by the path + // variable) and, if present, the userId carried inside the request body must both refer + // to the currently authenticated user. Neither the URL nor the JSON payload is trusted + // input for deciding whose record gets written - only the server-side session identity is. + boolean requestTargetsAuthenticatedUser = + authUserId != null + && authUserId.equals(userId) + && (userSubmittedProfile.getUserId() == null + || authUserId.equals(userSubmittedProfile.getUserId())); - // else - return failed(this) - .feedback("idor.edit.profile.failure3") - .output(currentUserProfile.profileToMap().toString()) - .build(); - } else if (userSubmittedProfile.getUserId() != null - && userSubmittedProfile.getUserId().equals(authUserId)) { - return failed(this).feedback("idor.edit.profile.failure4").build(); + if (!requestTargetsAuthenticatedUser) { + // Whatever role/color values were supplied are irrelevant - this account has no + // authority to modify a profile that isn't its own. + return failed(this).feedback("idor.edit.profile.failure1").build(); } - if (currentUserProfile.getColor().equals("black") && currentUserProfile.getRole() <= 1) { - return success(this) - .feedback("idor.edit.profile.success2") - .output(userSessionData.getValue("idor-updated-own-profile").toString()) - .build(); - } else { - return failed(this).feedback("idor.edit.profile.failure3").build(); + // Legitimate path: the authenticated user is updating their own profile. + UserProfile currentUserProfile = new UserProfile(authUserId); + currentUserProfile.setUserId(authUserId); + if (userSubmittedProfile.getColor() != null) { + currentUserProfile.setColor(userSubmittedProfile.getColor()); } + userSessionData.setValue("idor-updated-own-profile", currentUserProfile); + + return success(this) + .feedback("idor.edit.profile.success2") + .output(currentUserProfile.profileToMap().toString()) + .build(); } }