search

lsgs / redcap-extended-reports

Provides various additional options to enhance REDCap's built-in reporting functionality.
GNU General Public License v3.0
1 stars 3 forks source link

Identified hotspots for potential vulnerability injection #20

Closed remifrazierucsf closed 4 months ago

remifrazierucsf commented 4 months ago

@lsgs - we did a code review and have some spots we recommend looking at, which we've flagged here with //FIXME tags.

These are basically two places where we think in theory someone could POST something in and get some bad behavior, and it's probably worth sanitizing to be safe. That said, in both of these I was hesitant to try to refactor pieces to make it work since the downstream results were less obvious to me. You may have a better sense of how easy these could be to deal with.

We also evaluated the GET calls, and there are two places which in theory would normally concern me, but they both call down to core REDCap function which should sanitize (but I'm not sure they fully do). I flagged these as //TODO if you want to review them - similarly one of these is tricky to sanitize and may not be meaningfully exploitable, and the other one is used to call down to core code.

lsgs commented 4 months ago

Thank you @remifrazierucsf. This is very much appreciated!

lsgs commented 4 months ago

Re. https://github.com/lsgs/redcap-extended-reports/pull/20/commits/0a7dca672159a9343e2219708fa5e97e5f307ea0, this is the same mechanism that the built-in (and likewise superuser-only) Database Query Tool uses for checking user-submitted SQL queries. The submitted SQL gets passed down through various core methods to a call to mysqli_query(), which will run only a single query. It means that even though it will allow you to enter and save something bad like select 1; delete * from redcap_data there is no way it will run the second, non-select statement. Thanks very much for your input. I'm happy to implement further improvements as they occur.

remifrazierucsf commented 4 months ago

Re. 0a7dca6, this is the same mechanism that the built-in (and likewise superuser-only) Database Query Tool uses for checking user-submitted SQL queries. The submitted SQL gets passed down through various core methods to a call to mysqli_query(), which will run only a single query. It means that even though it will allow you to enter and save something bad like select 1; delete * from redcap_data there is no way it will run the second, non-select statement. Thanks very much for your input. I'm happy to implement further improvements as they occur.

Yeah, totally agree. I tend to over-sanitize sometimes. I really appreciate you reviewing!