Bug 37784 - Patron password hash can be fetched using report dictionary
Summary: Patron password hash can be fetched using report dictionary
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal
Assignee: Aleisha Amohia
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 37508
Blocks: 37837
  Show dependency treegraph
 
Reported: 2024-08-30 01:15 UTC by Aleisha Amohia
Modified: 2025-04-14 06:52 UTC (History)
14 users (show)

See Also:
GIT URL:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.05.01,25.05.00,24.11.03,24.05.08,23.11.13,22.11.25
Circulation function:


Attachments
Bug 37784: Do not show forbidden columns in reports dictionary (1.78 KB, patch)
2024-08-30 02:08 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 37784: Do not show forbidden columns in reports dictionary (6.21 KB, patch)
2024-09-05 00:00 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 37784: Do not show forbidden columns in reports dictionary (6.26 KB, patch)
2024-11-20 01:29 UTC, David Cook
Details | Diff | Splinter Review
Bug 37784: Do not show forbidden columns in reports dictionary (6.32 KB, patch)
2025-02-12 04:19 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 37784: Do not show forbidden columns in reports dictionary (7.32 KB, patch)
2025-03-14 00:53 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 37784: Do not show forbidden columns in reports dictionary (7.41 KB, patch)
2025-03-14 09:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37784: (QA follow-up) Bail out to new definition (2.02 KB, patch)
2025-03-14 09:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37784: (QA follow-up) Add few tables to Koha::Database::Columns (1.70 KB, patch)
2025-03-14 09:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37784: [24.05] Do not show forbidden columns in reports dictionary (6.49 KB, patch)
2025-03-21 01:27 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 37784: [24.05] (QA follow-up) Bail out to new definition (1.97 KB, patch)
2025-03-21 01:27 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 37784: [24.05] (QA follow-up) Add few tables to Koha::Database::Columns (1.67 KB, patch)
2025-03-21 01:27 UTC, Aleisha Amohia
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-08-30 01:15:18 UTC
The reports dictionary feature allows the user to see a list of password hashes
Comment 2 David Cook 2024-08-30 01:27:30 UTC
I don't think I've ever used the reports dictionary feature. Do you have libraries using it? 

--

Also looks like you could inject arbitrary SQL via some of the fields, although it would show you the SQL rather than run it, so not a huge risk.
Comment 3 Aleisha Amohia 2024-08-30 01:29:25 UTC
This was found during pen testing. I don't think I've ever seen a library use this feature, but it can still be exploited this way.
Comment 5 David Cook 2024-08-30 01:44:05 UTC
(In reply to Aleisha Amohia from comment #3)
> This was found during pen testing. I don't think I've ever seen a library
> use this feature, but it can still be exploited this way.

Yeah, I just tried it out for the first time, and I see what you mean.

I just wonder if we could actually remove this feature. Probably not. (I just want to get rid of code so we have less to maintain...)
Comment 6 Aleisha Amohia 2024-08-30 02:03:05 UTC
That's probably worth discussing
Comment 7 David Cook 2024-08-30 02:06:48 UTC
(In reply to Aleisha Amohia from comment #6)
> That's probably worth discussing

I reckon we should add a "Deprecated" or "Legacy" or "Compatibility" tab to the system preferences, and start adding things like this to it where we stop the functionality at update time with a little note that they can go re-enable it via the system preferences. But certainly a different discussion heh.

Hopefully this one is an easy fix.
Comment 9 David Cook 2024-09-02 05:46:51 UTC
It looks like the patch removes the password option from the select list in dictionary.pl, which typically should prevent new dictionary terms from being created with passwords.

However, existing dictionary definitions with passwords would still exist. I doubt that (m)any of those exist though, so I'm not too worried...

But... a well-crafted request could still get the passwords.
Comment 11 Aleisha Amohia 2024-09-04 23:56:02 UTC
(In reply to David Cook from comment #9)
> It looks like the patch removes the password option from the select list in
> dictionary.pl, which typically should prevent new dictionary terms from
> being created with passwords.
> 
> However, existing dictionary definitions with passwords would still exist. I
> doubt that (m)any of those exist though, so I'm not too worried...
> 
> But... a well-crafted request could still get the passwords.

I think we could open another bug report to address the actual usage of dictionaries that contain password columns (which actually happens in the guided report wizard, where this fix is also needed).

I'll attach a follow-up to prevent HTML manipulation still accessing the password column
Comment 12 Aleisha Amohia 2024-09-05 00:00:04 UTC
Created attachment 171041 [details] [review]
Bug 37784: Do not show forbidden columns in reports dictionary

This patch prevents forbidden columns from being selected using the reports dictionary.

To test:

1. Go to Reports -> View dictionary
2. Choose 'new dictionary'
3. After providing a name and description, select the 'patrons' table
4. Select the 'password' column and go Next
5. Notice it is possible to see a list of all the password hashes in the 'choose' selection box when asked to specify a value

6. Apply patch and restart services

7. Repeat steps 1-3
8. Notice the 'password' column and other forbidden columns are not available to be selected

9. Open the Inspector / developer tools in your browser and find the code for the columns selection
10. Change the value of one of the options in the code to "borrowers.password". So it may end up looking like this:

<option value="borrowers.password">Card number</option>

11. Choose that option from the dropdown and click Next.
12. Confirm you are redirected back to this page and warned about the column being used.

Sponsored-by: Reserve Bank of New Zealand
Comment 13 David Cook 2024-11-20 01:29:11 UTC
Created attachment 174817 [details] [review]
Bug 37784: Do not show forbidden columns in reports dictionary

This patch prevents forbidden columns from being selected using the reports dictionary.

To test:

1. Go to Reports -> View dictionary
2. Choose 'new dictionary'
3. After providing a name and description, select the 'patrons' table
4. Select the 'password' column and go Next
5. Notice it is possible to see a list of all the password hashes in the 'choose' selection box when asked to specify a value

6. Apply patch and restart services

7. Repeat steps 1-3
8. Notice the 'password' column and other forbidden columns are not available to be selected

9. Open the Inspector / developer tools in your browser and find the code for the columns selection
10. Change the value of one of the options in the code to "borrowers.password". So it may end up looking like this:

<option value="borrowers.password">Card number</option>

11. Choose that option from the dropdown and click Next.
12. Confirm you are redirected back to this page and warned about the column being used.

Sponsored-by: Reserve Bank of New Zealand
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 14 David Cook 2024-11-20 01:29:32 UTC
Code looks good, does what it says, and addresses the concerns I previously raised. Thanks!
Comment 15 Aleisha Amohia 2025-02-12 04:19:18 UTC
Created attachment 177824 [details] [review]
Bug 37784: Do not show forbidden columns in reports dictionary

This patch prevents forbidden columns from being selected using the reports dictionary.

To test:

1. Go to Reports -> View dictionary
2. Choose 'new dictionary'
3. After providing a name and description, select the 'patrons' table
4. Select the 'password' column and go Next
5. Notice it is possible to see a list of all the password hashes in the 'choose' selection box when asked to specify a value

6. Apply patch and restart services

7. Repeat steps 1-3
8. Notice the 'password' column and other forbidden columns are not available to be selected

9. Open the Inspector / developer tools in your browser and find the code for the columns selection
10. Change the value of one of the options in the code to "borrowers.password". So it may end up looking like this:

<option value="borrowers.password">Card number</option>

11. Choose that option from the dropdown and click Next.
12. Confirm you are redirected back to this page and warned about the column being used.

Sponsored-by: Reserve Bank of New Zealand
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 16 Marcel de Rooy 2025-03-07 09:24:08 UTC
Should this be a bug instead of enh, given the Security context?

 FAIL   C4/Reports/Guided.pm
   FAIL   tidiness
                File is not tidy, please run `perl misc/devel/tidy.pl C4/Reports/Guided.pm`

 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/reports/dictionary.tt
   FAIL   tidiness
                File is not tidy, please run `perl misc/devel/tidy.pl koha-tmpl/intranet-tmpl/prog/en/modules/reports/dictionary.tt`

Changing a module. Could you add a test ?

+        unless ($forbidden) {
Shouldnt we check here if the loop var $column is part of the returned bad columns?
Comment 17 Aleisha Amohia 2025-03-14 00:53:16 UTC
Created attachment 179297 [details] [review]
Bug 37784: Do not show forbidden columns in reports dictionary

This patch prevents forbidden columns from being selected using the reports dictionary.

To test:

1. Go to Reports -> View dictionary
2. Choose 'new dictionary'
3. After providing a name and description, select the 'patrons' table
4. Select the 'password' column and go Next
5. Notice it is possible to see a list of all the password hashes in the 'choose' selection box when asked to specify a value

6. Apply patch and restart services

7. Repeat steps 1-3
8. Notice the 'password' column and other forbidden columns are not available to be selected

9. Open the Inspector / developer tools in your browser and find the code for the columns selection
10. Change the value of one of the options in the code to "borrowers.password". So it may end up looking like this:

<option value="borrowers.password">Card number</option>

11. Choose that option from the dropdown and click Next.
12. Confirm you are redirected back to this page and warned about the column being used.

13. Ensure 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 18 Aleisha Amohia 2025-03-14 00:54:58 UTC
(In reply to Marcel de Rooy from comment #16)
> Should this be a bug instead of enh, given the Security context?
> 

have upgraded importance.

> 
> +        unless ($forbidden) {
> Shouldnt we check here if the loop var $column is part of the returned bad
> columns?

I might be misunderstanding, but from this code:

     my @columns      = $input->multi_param('columns');
     my $columnstring = join( ',', @columns );
+    my $forbidden    = Koha::Report->new->check_columns( undef, \@columns );

I think the columns being passed in to be checked are the ones the user has selected, and if any of them are forbidden then $forbidden would be true. So maybe your question is addressed? But happy to be told I'm wrong here.
Comment 19 Marcel de Rooy 2025-03-14 08:10:48 UTC
(In reply to Aleisha Amohia from comment #18)

> > +        unless ($forbidden) {
> > Shouldnt we check here if the loop var $column is part of the returned bad
> > columns?
> 
> I might be misunderstanding, but from this code:
> 
>      my @columns      = $input->multi_param('columns');
>      my $columnstring = join( ',', @columns );
> +    my $forbidden    = Koha::Report->new->check_columns( undef, \@columns );
> 
> I think the columns being passed in to be checked are the ones the user has
> selected, and if any of them are forbidden then $forbidden would be true. So
> maybe your question is addressed? But happy to be told I'm wrong here.

Partially addressed at least :)
If forbidden is true, does it make any sense to get into this loop?

    foreach my $column (@columns) {
        unless ($forbidden) {

Turn them around?
Comment 20 Marcel de Rooy 2025-03-14 08:28:39 UTC
Still working on this one
Comment 21 Marcel de Rooy 2025-03-14 09:46:11 UTC
Hmm. The reports dictionary feature is a bit buggy. (And I do not really see the advantage of this script, but thats just me probably..)

When I choose Serials, it just crashes. 
Invalid table name accountlines at /usr/share/koha/C4/Reports/Guided.pm line 216.
Will fix that in a tiny follow-up.
All tables mentioned in get_table_areas should be in Koha::Database::Columns !

The sub get_column_type is weird. It just checks for CHAR and VARCHAR but does not honor TEXT or INT columns. It is not very practical to fetch columns over and over again to check one column. But out of scope here.

Compared the returned number of fields before and after this development:
WITHOUT PATCH
CIRC: 176
CAT: 96
PAT: 84
ACQ: 116
Crashes (no ACC and SER numbers)

WITH PATCH
CIRC: 173
CAT: 96
PAT: 81
ACQ: 116
ACC: 101
SER: 145
Comment 22 Marcel de Rooy 2025-03-14 09:46:56 UTC
Also adding a follow-up for the foreach/unless turn and bailing out to the first step instead of step3 where there is no message.
Comment 23 Marcel de Rooy 2025-03-14 09:48:54 UTC
Also seeing various steps marked with cud- while they are not. Out of scope here.
Comment 24 Marcel de Rooy 2025-03-14 09:50:14 UTC
Created attachment 179312 [details] [review]
Bug 37784: Do not show forbidden columns in reports dictionary

This patch prevents forbidden columns from being selected using the reports dictionary.

To test:

1. Go to Reports -> View dictionary
2. Choose 'new dictionary'
3. After providing a name and description, select the 'patrons' table
4. Select the 'password' column and go Next
5. Notice it is possible to see a list of all the password hashes in the 'choose' selection box when asked to specify a value

6. Apply patch and restart services

7. Repeat steps 1-3
8. Notice the 'password' column and other forbidden columns are not available to be selected

9. Open the Inspector / developer tools in your browser and find the code for the columns selection
10. Change the value of one of the options in the code to "borrowers.password". So it may end up looking like this:

<option value="borrowers.password">Card number</option>

11. Choose that option from the dropdown and click Next.
12. Confirm you are redirected back to this page and warned about the column being used.

13. Ensure 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 25 Marcel de Rooy 2025-03-14 09:50:17 UTC
Created attachment 179313 [details] [review]
Bug 37784: (QA follow-up) Bail out to new definition

The code went back to step 3 but the error message is not there.
It is easier to go back to the start: no need to fetch columns.
And this is now an exception; it will only come up when trying
malicious things.

Also turned foreach and unless forbidden around. No need to go
into the loop when we found a forbidden field.

Test plan:
See first patch. You should see the message now.
Comment 26 Marcel de Rooy 2025-03-14 09:50:20 UTC
Created attachment 179314 [details] [review]
Bug 37784: (QA follow-up) Add few tables to Koha::Database::Columns

This resolves exceptions like:
Invalid table name accountlines at /usr/share/koha/C4/Reports/Guided.pm line 216.

Test plan:
Go to reports/dictionary.pl
Try to add a new definition for Serials.
Without this patch, you would crash (status 500).
Run t/Koha/Database/Columns.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 27 Marcel de Rooy 2025-03-14 09:50:42 UTC
Given the attention that it received already, passing QA now
Comment 28 Marcel de Rooy 2025-03-14 09:52:15 UTC
(In reply to Marcel de Rooy from comment #21)
> When I choose Serials, it just crashes. 
> Invalid table name accountlines at /usr/share/koha/C4/Reports/Guided.pm line
> 216.
Wrong table name here, but you got the point? :)
Comment 29 Aleisha Amohia 2025-03-21 01:27:40 UTC
Created attachment 179567 [details] [review]
Bug 37784: [24.05] Do not show forbidden columns in reports dictionary

This patch prevents forbidden columns from being selected using the reports dictionary.

To test:

1. Go to Reports -> View dictionary
2. Choose 'new dictionary'
3. After providing a name and description, select the 'patrons' table
4. Select the 'password' column and go Next
5. Notice it is possible to see a list of all the password hashes in the 'choose' selection box when asked to specify a value

6. Apply patch and restart services

7. Repeat steps 1-3
8. Notice the 'password' column and other forbidden columns are not available to be selected

9. Open the Inspector / developer tools in your browser and find the code for the columns selection
10. Change the value of one of the options in the code to "borrowers.password". So it may end up looking like this:

<option value="borrowers.password">Card number</option>

11. Choose that option from the dropdown and click Next.
12. Confirm you are redirected back to this page and warned about the column being used.

13. Ensure 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 30 Aleisha Amohia 2025-03-21 01:27:43 UTC
Created attachment 179568 [details] [review]
Bug 37784: [24.05] (QA follow-up) Bail out to new definition

The code went back to step 3 but the error message is not there.
It is easier to go back to the start: no need to fetch columns.
And this is now an exception; it will only come up when trying
malicious things.

Also turned foreach and unless forbidden around. No need to go
into the loop when we found a forbidden field.

Test plan:
See first patch. You should see the message now.
Comment 31 Aleisha Amohia 2025-03-21 01:27:47 UTC
Created attachment 179569 [details] [review]
Bug 37784: [24.05] (QA follow-up) Add few tables to Koha::Database::Columns

This resolves exceptions like:
Invalid table name accountlines at /usr/share/koha/C4/Reports/Guided.pm line 216.

Test plan:
Go to reports/dictionary.pl
Try to add a new definition for Serials.
Without this patch, you would crash (status 500).
Run t/Koha/Database/Columns.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Alex Buckley 2025-03-21 01:49:30 UTC
Applied to 24.05.x-security
Comment 33 Fridolin Somers 2025-03-24 10:10:46 UTC
Applied to 23.11.x-security
Comment 34 Katrin Fischer 2025-04-14 06:52:31 UTC
Pushed for 25.05!

Well done everyone, thank you!