Description
Aleisha Amohia
2024-07-29 02:49:16 UTC
100% agreed Locally, if we detect "password" anywhere in a SQL report, we'll raise an exception and not run the query. We also check locally if "password" appears in the cells (ie if you do a SELECT * FROM borrowers) and we raise an exception. I don't know if people would be open to these ideas for upstream, or if they'd want something different. -- We've actually done a number of local changes to prevent data leaks in ./reports/guided_reports.pl Unfortunately, ./reports/guided_reports.pl is a tangle of over 1000 lines of code, so I suspect this will take a long time to fix in the community... Thanks for the input David! I'm working on a fix for this now. Created attachment 169806 [details] [review] Bug 37508: Throw error if password column is detected in SQL report TESTS TO COME Created attachment 169810 [details] [review] Bug 37508: Throw error if password column is detected in SQL report This enhancement prevents SQL queries from being run if they would return a password field from the database table. To test: 1. Run tests and notice they fail t/db_dependent/Reports/Guided.t 2. Apply patch and restart services 3. Create a public report with an SQL report which would access a password column in a database table 4. Try to run the report. Notice you are met with an error and the results are not shown. 5. Access the JSON URL, you should not get the results and should be shown an error 6. Confirm tests pass t/db_dependent/Reports/Guided.t Sponsored-by: Reserve Bank of New Zealand Created attachment 170121 [details] [review] Bug 37508: Test for errors when returning an aliased password column If you look at that test I attached, I think we have to check the SQL before it's passed to the database as well, because you can alias the columns to have a different name than password. However, I think that to alias "password", you have to explicitly refer to "password" in the SQL, so if you raise an exception if you find "password" in the SQL, then you should be good. It's a bit messy, but I think it's necessary. Created attachment 170124 [details] [review] Bug 37508: (follow-up) Throw error is password is in SQL query at all Confirm tests pass t/db_dependent/Reports/Guided.t Created attachment 170125 [details] [review] Bug 37508: Throw error if password column is detected in SQL report This enhancement prevents SQL queries from being run if they would return a password field from the database table. To test: 1. Run tests and notice they fail t/db_dependent/Reports/Guided.t 2. Apply patch and restart services 3. Create a public report with an SQL report which would access a password column in a database table 4. Try to run the report. Notice you are met with an error and the results are not shown. 5. Access the JSON URL, you should not get the results and should be shown an error 6. Confirm tests pass t/db_dependent/Reports/Guided.t Sponsored-by: Reserve Bank of New Zealand Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 170126 [details] [review] Bug 37508: Test for errors when returning an aliased password column Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 170127 [details] [review] Bug 37508: (follow-up) Throw error is password is in SQL query at all Confirm tests pass t/db_dependent/Reports/Guided.t Signed-off-by: David Cook <dcook@prosentient.com.au> Great work on this one, Aleisha, and thanks for that fast follow-up. It looks like I overestimated the difficulty of this one. I think I was thinking of a different change we made locally to reports, which requires a special token for running borrower reports. (The token gets emailed to a supervisor pre-configured ahead of time.) It's not the most elegant solution, but adds another layer. I'm excited to get this password change in. Also happy to QA this one if someone else wants to do a sign off. (In reply to David Cook from comment #14) > Also happy to QA this one if someone else wants to do a sign off. I'm happy to add that signoff :) One question before I do, though: is it a good idea to pass the SQL query as the error message in the aliased password column case? When I did step #5 (Access the JSON URL) with all patches applied, that caused the page to display the full query. I don't know enough about the access controls on the svc route to know if that's an issue. I have small concern here in that we are removing the ability to do "SELECT *" from anything involving the borrowers table - so anyone exporting patrons this way may lost functionality as Koha doesn't have a way to export patrons. It adds a burden to select all the columns you want, but I don't want to block because I see the need for security. Talking to Kyle he suggests possibly moving the password hashes to their own table, then just block any reports hitting that table (borrowers_password_hashes) but that requires Auth changes and more. This could be done on a separate bug after this if we think this issue is urgent to resolve (In reply to Nick Clemens (kidclamp) from comment #16) > I have small concern here in that we are removing the ability to do "SELECT > *" from anything involving the borrowers table - so anyone exporting patrons > this way may lost functionality as Koha doesn't have a way to export patrons. > > It adds a burden to select all the columns you want, but I don't want to > block because I see the need for security. > > Talking to Kyle he suggests possibly moving the password hashes to their own > table, then just block any reports hitting that table > (borrowers_password_hashes) but that requires Auth changes and more. This > could be done on a separate bug after this if we think this issue is urgent > to resolve Definitely understand this, and the password hash table is the ideal solution. I think we should get this through in the interest of securing this vulnerability, and work on the password hash table afterwards (acknowledging the scope and size of that change). (In reply to Emily Lamancusa from comment #15) > One question before I do, though: is it a good idea to pass the SQL query as > the error message in the aliased password column case? When I did step #5 > (Access the JSON URL) with all patches applied, that caused the page to > display the full query. I don't know enough about the access controls on the > svc route to know if that's an issue. Could you elaborate on that? If I go to http://localhost:8081/cgi-bin/koha/svc/report?id=1 I just see {"passworderr":"password"} (In reply to Aleisha Amohia from comment #17) > Definitely understand this, and the password hash table is the ideal > solution. > > I think we should get this through in the interest of securing this > vulnerability, and work on the password hash table afterwards (acknowledging > the scope and size of that change). Agreed. (Btw, I'd call it "borrower_credentials" so that we could store things like passwords, one-time passwords, tokens, certificates, etc.) -- Nick, for what it's worth, we've had these changes locally in prod for... a couple years now I think, and so far I haven't had any memorable complaints. (In reply to David Cook from comment #19) > (Btw, I'd call it "borrower_credentials" so that we could store things like > passwords, one-time passwords, tokens, certificates, etc.) Bug 33254 being a prime example of how it would be nice to store a token against a borrower. (In reply to David Cook from comment #20) > (In reply to David Cook from comment #19) > > (Btw, I'd call it "borrower_credentials" so that we could store things like > > passwords, one-time passwords, tokens, certificates, etc.) > > Bug 33254 being a prime example of how it would be nice to store a token > against a borrower. Bug 27305 is also a case where it would be cool to have say an "ical_token". (In reply to David Cook from comment #18) > (In reply to Emily Lamancusa from comment #15) > > One question before I do, though: is it a good idea to pass the SQL query as > > the error message in the aliased password column case? When I did step #5 > > (Access the JSON URL) with all patches applied, that caused the page to > > display the full query. I don't know enough about the access controls on the > > svc route to know if that's an issue. > > Could you elaborate on that? > > If I go to http://localhost:8081/cgi-bin/koha/svc/report?id=1 I just see > {"passworderr":"password"} If I use: SELECT password AS stuff FROM borrowers then I see: passworderr: "SELECT password AS stuff FROM borrowers /* saved_sql_id: 13 */ LIMIT ?, ?" Created attachment 170165 [details] [review] Bug 37508: (follow-up) Don't pass the column or sql containing password This patch replaces these variables with a non-translatable message. (In reply to Emily Lamancusa from comment #22) > If I use: > SELECT password AS stuff FROM borrowers > then I see: > passworderr: "SELECT password AS stuff FROM borrowers /* saved_sql_id: 13 */ > LIMIT ?, ?" Ah I see what you're saying now. My bad. Yeah that's not good. (In reply to Emily Lamancusa from comment #22) > If I use: > SELECT password AS stuff FROM borrowers > then I see: > passworderr: "SELECT password AS stuff FROM borrowers /* saved_sql_id: 13 */ > LIMIT ?, ?" Fair enough. I'm not sure how serious this is because people can see in the schema that there is a password column in that table, but in any case I have replaced that with an error string. It's not translatable in the script so not a perfect fix, but will address this problem for now. (In reply to Aleisha Amohia from comment #25) > (In reply to Emily Lamancusa from comment #22) > > If I use: > > SELECT password AS stuff FROM borrowers > > then I see: > > passworderr: "SELECT password AS stuff FROM borrowers /* saved_sql_id: 13 */ > > LIMIT ?, ?" > > > Fair enough. I'm not sure how serious this is because people can see in the > schema that there is a password column in that table, but in any case I have > replaced that with an error string. It's not translatable in the script so > not a perfect fix, but will address this problem for now. Yeah I think it's good enough for now. Nice one with the fix. Looking here Created attachment 170171 [details] [review] Bug 37508: Throw error if password column is detected in SQL report This enhancement prevents SQL queries from being run if they would return a password field from the database table. To test: 1. Run tests and notice they fail t/db_dependent/Reports/Guided.t 2. Apply patch and restart services 3. Create a public report with an SQL report which would access a password column in a database table 4. Try to run the report. Notice you are met with an error and the results are not shown. 5. Access the JSON URL, you should not get the results and should be shown an error 6. Confirm tests pass t/db_dependent/Reports/Guided.t Sponsored-by: Reserve Bank of New Zealand Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 170172 [details] [review] Bug 37508: Test for errors when returning an aliased password column Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 170173 [details] [review] Bug 37508: (follow-up) Throw error is password is in SQL query at all Confirm tests pass t/db_dependent/Reports/Guided.t Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 170174 [details] [review] Bug 37508: (follow-up) Don't pass the column or sql containing password This patch replaces these variables with a non-translatable message. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 170175 [details] [review] Bug 37508: (QA follow-up) Move check to Koha::Report, extend Do not allow password but allow password_expiry_days etc. Do not allow token, secret and uuid too. Test plan: Run t/db_dependent/Koha/Reports.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 170176 [details] [review] Bug 37508: (QA follow-up) Use ->check_columns Add shebang to Guided.t too. Test plan: See also previous commits. Try sql like: select access_token from oauth_access_tokens Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Few comments, trying to explain follow-ups too: Needed a test on select PASSWORD from borrowers too. The regex was not case insensitive. We also blocked fields like password_expiration_date. We did not block: secret, uuid and token. As for code, the added regex was a candidate for moving into sql_is_valid which is run a bit earlier. The new check_columns checks for matches on password, token etc. but uses a whitelist to allow expiration date etc. If we would deny only with exact matches, we might forget new fields and expose them. But now we add a risk of crashing a report on a new matching field while forgetting to whitelist it. Should we just squash the whole bunch? GREAT catches Marcel! (In reply to Marcel de Rooy from comment #41) > Or the rigorous: do not allow * which probably kills thousands of existing > reports.. I don't think that's something we should rush into... I think I am happy to follow Marcel's suggestion to push this as a first step and open a new bug for covering the "sneaky" cases. Created attachment 170229 [details] [review] Bug 37508: Don't return Internal server error when running report To test: 1 - Create a report like: SELECT "a" FROM borrowers WHERE <<Test>> != '' 2 - Run report 3 - Enter "password" 4 - Internal server error / stacktrace 5 - Apply patch 6 - Repeat 7 - Get a yellow warning box (In reply to Nick Clemens (kidclamp) from comment #45) > Created attachment 170229 [details] [review] [review] > Bug 37508: Don't return Internal server error when running report > > To test: > 1 - Create a report like: > SELECT "a" > FROM borrowers > WHERE <<Test>> != '' > 2 - Run report > 3 - Enter "password" > 4 - Internal server error / stacktrace > 5 - Apply patch > 6 - Repeat > 7 - Get a yellow warning box Hmm I do not see "Report could not be run " popping up ? (In reply to Marcel de Rooy from comment #46) > (In reply to Nick Clemens (kidclamp) from comment #45) > > Created attachment 170229 [details] [review] [review] [review] > > Bug 37508: Don't return Internal server error when running report > > > > To test: > > 1 - Create a report like: > > SELECT "a" > > FROM borrowers > > WHERE <<Test>> != '' > > 2 - Run report > > 3 - Enter "password" > > 4 - Internal server error / stacktrace > > 5 - Apply patch > > 6 - Repeat > > 7 - Get a yellow warning box > > Hmm I do not see "Report could not be run " popping up ? What do you see? (In reply to Nick Clemens (kidclamp) from comment #47) > (In reply to Marcel de Rooy from comment #46) > > (In reply to Nick Clemens (kidclamp) from comment #45) > > > Created attachment 170229 [details] [review] [review] [review] [review] > > > Bug 37508: Don't return Internal server error when running report > > > > > > To test: > > > 1 - Create a report like: > > > SELECT "a" > > > FROM borrowers > > > WHERE <<Test>> != '' > > > 2 - Run report > > > 3 - Enter "password" > > > 4 - Internal server error / stacktrace > > > 5 - Apply patch > > > 6 - Repeat > > > 7 - Get a yellow warning box > > > > Hmm I do not see "Report could not be run " popping up ? > > What do you see? The following error was encountered: The column selection in this report includes a password field. The report cannot be executed due to security risks. Please edit this report and ensure no password columns have been selected. Return to previous page What do you need to hit the !$sth condition ? (In reply to Nick Clemens (kidclamp) from comment #50) > (In reply to Marcel de Rooy from comment #49) > > What do you need to hit the !$sth condition ? > > It's coming from: > 574 my ( $is_sql_valid, $errors ) = Koha::Report->new( { savedsql => > $sql } )->is_sql_valid; > 575 return ( undef, @{$errors}[0] ) unless $is_sql_valid; > > The error from is_sql_valid is: > 65 } else { > 66 push @errors, { passworderr => "Illegal column in results" } > 67 if $self->check_columns($sql); > 68 } > > > check_columns has a 'To do' about checking the whole SQL > > This error is not 100% correct, but I think it is better than completely > dying Yeah that sounds great. But somehow I miss something ;) Created attachment 170230 [details] [review] Bug 37508: Don't return Internal server error when running report To test: 1 - Create a report like: SELECT "a" FROM borrowers WHERE <<Test>> != '' 2 - Run report 3 - Enter "password" 4 - Internal server error / stacktrace 5 - Apply patch 6 - Repeat 7 - Get a yellow warning box Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Lets include it. It prevents a die. The exact message is not that relevant? Pushed to main for 24.11. Nice work everyone, thanks! Looking forward to improve the area on follow-up reports! This is not applying to 23.11, can we get some rebase please? Created attachment 170241 [details] [review] Bug 37508: [23.11] Throw error if password column is detected in SQL report This enhancement prevents SQL queries from being run if they would return a password field from the database table. To test: 1. Run tests and notice they fail t/db_dependent/Reports/Guided.t 2. Apply patch and restart services 3. Create a public report with an SQL report which would access a password column in a database table 4. Try to run the report. Notice you are met with an error and the results are not shown. 5. Access the JSON URL, you should not get the results and should be shown an error 6. Confirm tests pass t/db_dependent/Reports/Guided.t Sponsored-by: Reserve Bank of New Zealand Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 170242 [details] [review] Bug 37508: [23.11] Test for errors when returning an aliased password column Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 170243 [details] [review] Bug 37508: [23.11] (follow-up) Throw error is password is in SQL query at all Confirm tests pass t/db_dependent/Reports/Guided.t Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 170244 [details] [review] Bug 37508: [23.11] (follow-up) Don't pass the column or sql containing password This patch replaces these variables with a non-translatable message. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 170245 [details] [review] Bug 37508: [23.11] (QA follow-up) Move check to Koha::Report, extend Do not allow password but allow password_expiry_days etc. Do not allow token, secret and uuid too. Test plan: Run t/db_dependent/Koha/Reports.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 170246 [details] [review] Bug 37508: [23.11] (QA follow-up) Use ->check_columns Add shebang to Guided.t too. Test plan: See also previous commits. Try sql like: select access_token from oauth_access_tokens Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 170247 [details] [review] Bug 37508: [23.11] Don't return Internal server error when running report To test: 1 - Create a report like: SELECT "a" FROM borrowers WHERE <<Test>> != '' 2 - Run report 3 - Enter "password" 4 - Internal server error / stacktrace 5 - Apply patch 6 - Repeat 7 - Get a yellow warning box Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 170253 [details] [review] [23.11.x] Bug 37508: (QA follow-up) Move sth error check up This patch moves the error check right before the ->check_columns call. This is how main and 24.05 behave. 23.11 doesn't have bug 35907 backported so things are not exactly the same. With this patch tests pass and the only difference in behavior is logging. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Nobody worried that we are going to break reports? Did we investigate a way to exclude the 'password' columns from the results instead? What about api_keys.secret, borrowers.secret, etc.? (In reply to Jonathan Druart from comment #65) > What about api_keys.secret, borrowers.secret, etc.? forget that, Marcel dealt with it in a follow-up patch. +use constant FORBIDDEN_COLUMN_MATCHES => qw(password token uuid secret); (In reply to Jonathan Druart from comment #66) > (In reply to Jonathan Druart from comment #65) > > What about api_keys.secret, borrowers.secret, etc.? > > forget that, Marcel dealt with it in a follow-up patch. > > +use constant FORBIDDEN_COLUMN_MATCHES => qw(password token uuid secret); I'm still not sold on "uuid" here, because there are a lot of hypothetical cases where uuid isn't a secret and instead is actually just a replacement for an "id" column. For instance, using "borrowernumber" as an integer instead of a uuid hypothetically puts us at risk of enumeration attacks. (If the patrons API were not properly secured due to a bug, an attacker can just spin through from 1 to 1000000 to leak all the patron data. Whereas if we used UUIDs, it would be much much harder for them to try to enumerate/list all the patrons via automatically generated URLs.) (In reply to David Cook from comment #68) > I'm still not sold on "uuid" here, because there are a lot of hypothetical > cases where uuid isn't a secret and instead is actually just a replacement > for an "id" column. Looking at borrower_password_recovery, we probably should rename "uuid" to something like "token"... (In reply to Jonathan Druart from comment #64) > Nobody worried that we are going to break reports? Actually, you've just inspired me, Jonathan. This is a fairly dramatic change. Perhaps we should wrap it with a configurable flag. We can default it to ON, but if libraries are finding it problematic, we can allow sysadmins to turn it to OFF without rolling back Koha to a previous version or having to wait for us to make a change in a point release. Eventually, we'd want to remove the flag, since this shouldn't be configurable. But maybe in the short-term... it would be a good idea. Maybe we should have an "Experimental" configuration section in sysprefs, or "Experimental" section in koha-conf.xml, or a different experimental.yml config file or something. Because you do make a good point Jonathan... this is a breaking change and maybe we do want to be gentle with our users. (For context, I've had a local version of this change running in prod for years and it's been fine, but I know it might be different for other people.) (In reply to David Cook from comment #70) > (In reply to Jonathan Druart from comment #64) > > Nobody worried that we are going to break reports? > > This is a fairly dramatic change. Perhaps we should wrap it with a > configurable flag. > > Because you do make a good point Jonathan... this is a breaking change and > maybe we do want to be gentle with our users. > > (For context, I've had a local version of this change running in prod for > years and it's been fine, but I know it might be different for other people.) Not sure if we should add options to disable security stuff. And perhaps we should be pragmatic here. How many reports will really be broken? Probably reports where just borrowers.* is used. Which seems like a lazy (and bad) choice to get some borrower fields. (Exporting borrowers should better be done via cmdline than with reporting.) We should at the minimum have advertised a bit more this change, and provide an example to fix the error. First question on the mailing: https://lists.katipo.co.nz/pipermail/koha/2024-August/060964.html Is there already somewhere a ready-made sql query that will select all the other fields of borrower table except the password? I have to admit, that I've used a simple query of borrowers as a starting point for many of my reports. If you use: "SELECT * FROM borrowers WHERE cardnumber=<<cardnumber>>", it's quite a handy tool. You get the accurate list of all the fields in borrowers-table and you can easily see, how the data is stored there, compared to what you see in Koha intranet on that patron's page. Then it's easy to build more complex reports, when you can see what data is in which field, and which code number means what. I've also used my simple query to some bug tracking purporses. However, now, firstly, you have to write a pretty long SQL query compared to this old one, and then later you have to keep track of the possible changes to the fields of borrowers table (although I suppose that won't happen so often). I hope you find some solution so that "Select * from borrowers" will be possible in the future. The idea of passwords in a different table sounds ok for a main user like me. I understand the privacy concern behind this solution, but now this takes some tools away from superlibrarians who don't have direct access to the server. |