Bug 37508 - SQL reports should not show patron password hash if queried
Summary: SQL reports should not show patron password hash if queried
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Aleisha Amohia
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 37784 37919
  Show dependency treegraph
 
Reported: 2024-07-29 02:49 UTC by Aleisha Amohia
Modified: 2024-10-01 16:31 UTC (History)
17 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement on reports module prevents SQL queries from being run if they would return a password field from the database table.
Version(s) released in:
24.11.00,24.05.03,23.11.08,23.05.14,22.11.20
Circulation function:


Attachments
Bug 37508: Throw error if password column is detected in SQL report (2.17 KB, patch)
2024-07-29 03:55 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 37508: Throw error if password column is detected in SQL report (4.62 KB, patch)
2024-07-29 05:15 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 37508: Test for errors when returning an aliased password column (1.11 KB, patch)
2024-08-07 01:15 UTC, David Cook
Details | Diff | Splinter Review
Bug 37508: (follow-up) Throw error is password is in SQL query at all (880 bytes, patch)
2024-08-07 04:38 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 37508: Throw error if password column is detected in SQL report (4.67 KB, patch)
2024-08-07 07:15 UTC, David Cook
Details | Diff | Splinter Review
Bug 37508: Test for errors when returning an aliased password column (1.16 KB, patch)
2024-08-07 07:15 UTC, David Cook
Details | Diff | Splinter Review
Bug 37508: (follow-up) Throw error is password is in SQL query at all (932 bytes, patch)
2024-08-07 07:15 UTC, David Cook
Details | Diff | Splinter Review
Bug 37508: (follow-up) Don't pass the column or sql containing password (1.06 KB, patch)
2024-08-08 23:54 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 37508: Throw error if password column is detected in SQL report (4.76 KB, patch)
2024-08-09 10:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37508: Test for errors when returning an aliased password column (1.25 KB, patch)
2024-08-09 10:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37508: (follow-up) Throw error is password is in SQL query at all (1.01 KB, patch)
2024-08-09 10:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37508: (follow-up) Don't pass the column or sql containing password (1.15 KB, patch)
2024-08-09 10:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37508: (QA follow-up) Move check to Koha::Report, extend (4.26 KB, patch)
2024-08-09 10:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37508: (QA follow-up) Use ->check_columns (1.65 KB, patch)
2024-08-09 10:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37508: Don't return Internal server error when running report (1.09 KB, patch)
2024-08-12 12:14 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37508: Don't return Internal server error when running report (1.18 KB, patch)
2024-08-12 13:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37508: [23.11] Throw error if password column is detected in SQL report (4.70 KB, patch)
2024-08-12 21:20 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 37508: [23.11] Test for errors when returning an aliased password column (1.23 KB, patch)
2024-08-12 21:20 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 37508: [23.11] (follow-up) Throw error is password is in SQL query at all (957 bytes, patch)
2024-08-12 21:20 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 37508: [23.11] (follow-up) Don't pass the column or sql containing password (1.07 KB, patch)
2024-08-12 21:20 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 37508: [23.11] (QA follow-up) Move check to Koha::Report, extend (4.23 KB, patch)
2024-08-12 21:20 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 37508: [23.11] (QA follow-up) Use ->check_columns (1.65 KB, patch)
2024-08-12 21:20 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 37508: [23.11] Don't return Internal server error when running report (1.17 KB, patch)
2024-08-12 21:20 UTC, Aleisha Amohia
Details | Diff | Splinter Review
[23.11.x] Bug 37508: (QA follow-up) Move sth error check up (1.20 KB, patch)
2024-08-13 04:12 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Aleisha Amohia 2024-07-29 02:49:16 UTC
Users can run SQL reports on the borrowers table and be returned a password hash

Someone could theoretically export these results and crack the password hashes offline.
Comment 2 David Cook 2024-07-29 02:58:29 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...
Comment 3 Aleisha Amohia 2024-07-29 03:39:45 UTC
Thanks for the input David! I'm working on a fix for this now.
Comment 4 Aleisha Amohia 2024-07-29 03:55:15 UTC
Created attachment 169806 [details] [review]
Bug 37508: Throw error if password column is detected in SQL report

TESTS TO COME
Comment 5 Aleisha Amohia 2024-07-29 05:15:57 UTC
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
Comment 7 David Cook 2024-08-07 01:15:44 UTC
Created attachment 170121 [details] [review]
Bug 37508: Test for errors when returning an aliased password column
Comment 8 David Cook 2024-08-07 01:27:30 UTC
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.
Comment 9 Aleisha Amohia 2024-08-07 04:38:32 UTC
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
Comment 10 David Cook 2024-08-07 07:15:42 UTC
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>
Comment 11 David Cook 2024-08-07 07:15:45 UTC
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>
Comment 12 David Cook 2024-08-07 07:15:48 UTC
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>
Comment 13 David Cook 2024-08-07 07:17:38 UTC
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.
Comment 14 David Cook 2024-08-07 07:18:06 UTC
Also happy to QA this one if someone else wants to do a sign off.
Comment 15 Emily Lamancusa (emlam) 2024-08-07 14:21:18 UTC
(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.
Comment 16 Nick Clemens (kidclamp) 2024-08-07 16:09:51 UTC
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
Comment 17 Aleisha Amohia 2024-08-07 21:35:07 UTC
(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).
Comment 18 David Cook 2024-08-07 23:51:37 UTC
(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"}
Comment 19 David Cook 2024-08-07 23:54:59 UTC
(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.
Comment 20 David Cook 2024-08-08 01:45:09 UTC
(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.
Comment 21 David Cook 2024-08-08 01:47:56 UTC
(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".
Comment 22 Emily Lamancusa (emlam) 2024-08-08 14:21:13 UTC
(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 ?, ?"
Comment 23 Aleisha Amohia 2024-08-08 23:54:44 UTC
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.
Comment 24 David Cook 2024-08-08 23:55:01 UTC
(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.
Comment 25 Aleisha Amohia 2024-08-08 23:55:47 UTC
(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.
Comment 26 David Cook 2024-08-08 23:57:05 UTC
(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.
Comment 27 Marcel de Rooy 2024-08-09 06:21:42 UTC
Looking here
Comment 28 Marcel de Rooy 2024-08-09 10:09:42 UTC
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>
Comment 29 Marcel de Rooy 2024-08-09 10:09:45 UTC
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>
Comment 30 Marcel de Rooy 2024-08-09 10:09:48 UTC
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>
Comment 31 Marcel de Rooy 2024-08-09 10:09:51 UTC
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>
Comment 32 Marcel de Rooy 2024-08-09 10:09:54 UTC
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>
Comment 33 Marcel de Rooy 2024-08-09 10:09:57 UTC
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>
Comment 34 Marcel de Rooy 2024-08-09 10:17:06 UTC
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.
Comment 35 Marcel de Rooy 2024-08-09 10:17:47 UTC
Should we just squash the whole bunch?
Comment 36 Aleisha Amohia 2024-08-11 22:06:30 UTC
GREAT catches Marcel!
Comment 43 Katrin Fischer 2024-08-12 12:12:38 UTC
(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...
Comment 44 Katrin Fischer 2024-08-12 12:13:19 UTC
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.
Comment 45 Nick Clemens (kidclamp) 2024-08-12 12:14:22 UTC
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
Comment 46 Marcel de Rooy 2024-08-12 12:27:31 UTC
(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 ?
Comment 47 Nick Clemens (kidclamp) 2024-08-12 12:29:02 UTC
(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?
Comment 48 Marcel de Rooy 2024-08-12 12:30:10 UTC
(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
Comment 49 Marcel de Rooy 2024-08-12 12:33:47 UTC
What do you need to hit the !$sth condition ?
Comment 51 Marcel de Rooy 2024-08-12 13:11:54 UTC
(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 ;)
Comment 52 Marcel de Rooy 2024-08-12 13:22:31 UTC
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>
Comment 53 Marcel de Rooy 2024-08-12 13:23:11 UTC
Lets include it. It prevents a die. The exact message is not that relevant?
Comment 54 Tomás Cohen Arazi (tcohen) 2024-08-12 14:40:08 UTC
Pushed to main for 24.11.

Nice work everyone, thanks!

Looking forward to improve the area on follow-up reports!
Comment 55 Tomás Cohen Arazi (tcohen) 2024-08-12 19:23:34 UTC
This is not applying to 23.11, can we get some rebase please?
Comment 56 Aleisha Amohia 2024-08-12 21:20:05 UTC
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>
Comment 57 Aleisha Amohia 2024-08-12 21:20:09 UTC
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>
Comment 58 Aleisha Amohia 2024-08-12 21:20:12 UTC
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>
Comment 59 Aleisha Amohia 2024-08-12 21:20:16 UTC
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>
Comment 60 Aleisha Amohia 2024-08-12 21:20:19 UTC
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>
Comment 61 Aleisha Amohia 2024-08-12 21:20:22 UTC
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>
Comment 62 Aleisha Amohia 2024-08-12 21:20:26 UTC
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>
Comment 63 Tomás Cohen Arazi (tcohen) 2024-08-13 04:12:39 UTC
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>
Comment 64 Jonathan Druart 2024-08-14 09:49:13 UTC
Nobody worried that we are going to break reports?

Did we investigate a way to exclude the 'password' columns from the results instead?
Comment 65 Jonathan Druart 2024-08-14 09:50:01 UTC
What about api_keys.secret, borrowers.secret, etc.?
Comment 66 Jonathan Druart 2024-08-14 09:51:55 UTC
(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);
Comment 68 David Cook 2024-08-14 23:54:10 UTC
(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.)
Comment 69 David Cook 2024-08-14 23:55:07 UTC
(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"...
Comment 70 David Cook 2024-08-15 00:02:43 UTC
(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.)
Comment 71 Marcel de Rooy 2024-08-15 06:43:00 UTC
(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.)
Comment 72 Jonathan Druart 2024-08-28 11:45:45 UTC
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
Comment 73 Inkeri Hakulinen 2024-09-10 08:31:30 UTC
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.