qifeng-bai
commented
1 year ago The reporter looked at the wrong commit.
On our current dev and master branch, https://github.com/AtlasOfLivingAustralia/specieslist-webapp/blob/develop/src/main/groovy/au/org/ala/specieslist/SecurityUtil.groovy#L66
canDelete = (list.userId == userId) || list.editors?.contains(userId) checks if the list belongs to this user or the user is in Edit group
I have reviewed and tested the code, and it works as expected
Reported from Helpdesk
Hi
Whilst testing upgrading the CAS service within our test setup I came across a security bug in specieslist-webapp which I believe is also still present in your production code. Due to the nature of the issue I spoke to Vicente who suggested emailing yourselves direct as it wasn't really appropriate to raise a public issue.
I believe that any authenticated user (i.e. no admin roles needed) is able to delete any species list from the application, private or public. Steps to reproduce:
Log in as User A Create a list (I was testing with a private one but it should make no difference) Find the int id of the list (I got it from an edit post)*, not the data resource id Log in as User B Go to (GET) /speciesList/delete/{id}
*if I'm correct someone malicious could simply iterate 1-10000 and delete all lists.
Whilst I haven't debugged fully due to time constraints I believe it is in part because the SecurityUtil ( https://github.com/AtlasOfLivingAustralia/specieslist-webapp/blob/12940e5bfe2f3558db5e30a47c9e21f9f934cc30/src/main/groovy/au/org/ala/specieslist/SecurityUtil.groovy#L29) checks by data resource id rather than int id and so doesn't find a list to check against and therefor defaults to letting the request through. I found this in our test environment which is old code however having seen that the code didn't appear to have changed I was able to repeat this on the ALA site with a test list and 2 user accounts.
That said it looks like this method was really only intended to check if a user could view a list, however as it is also used to check if a user can delete a list*, even if the above was not the case I believe it would still leave public lists susceptible to deletion by any user.
*https://github.com/AtlasOfLivingAustralia/specieslist-webapp/blob/12940e5bfe2f3558db5e30a47c9e21f9f934cc30/grails-app/controllers/au/org/ala/specieslist/SecurityInterceptor.groovy#L42