fix(security): verify a student belongs to the run before changing their password#332
Open
Isaries wants to merge 1 commit into
Open
Conversation
…eir password The change student password endpoint checked that the signed-in teacher has write permission on the run taken from the path, but then loaded the target user directly from the student id without checking that the two are related. The id was neither confined to the run nor to students. Verify that the target is a student of that run before changing the password.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
POST /api/teacher/run/{runId}/student/{studentId}/change-passwordchecks that the signed-in teacher has write permission on the run taken from the path, but then loads the target user straight fromstudentIdwithout ever checking that the two are related. The id is confined neither to the run nor to students.A teacher who owns any one run can therefore set the password of any account in the system by id: another teacher's student, another teacher, or an administrator.
The class-level
@Secured({"ROLE_TEACHER"})and theteacherPasswordre-authentication both only establish who the caller is; neither constrains whom they may act on.isTeacherPasswordCorrectalso returns true immediately for Google users, so the re-authentication is skipped entirely for them.This is the same confused-deputy pattern as #325 (authorize object A, then mutate object B), but on the endpoint the Angular client actually calls, and without even the group scoping the legacy controller had.
Changes
Run.isStudentAssociatedToThisRun. One check covers all three cases: a student from another run is not in any of this run's periods, and neither is a teacher or an administrator.I used
isStudentAssociatedToThisRunrather thanUserService.isUserAssociatedWithRun, because the latter also returns true for a teacher associated with the run, which would still let a shared owner change the run owner's password.Testing
mvnw test -Dtest=ChangeStudentPasswordControllerTestpasses (6 tests).mvnw testshows the same 3 failures / 21 errors as a cleandevelopon my machine (database, encoding and filesystem related), with the 2 added tests passing.Note
RemoveStudentRunControllerhas the same shape, and I have opened #333 for it separately. It turned out to be a much smaller problem than this one: both steps ofremoveStudentFromRunare already scoped to the run, so passing an id from outside the run does nothing at all. The only real gap there is the missing role check, and #333 has the details.