The reports dictionary feature allows the user to see a list of password hashes
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.
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.
(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...)
That's probably worth discussing
(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.
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.
(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
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
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>
Code looks good, does what it says, and addresses the concerns I previously raised. Thanks!
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>
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?
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>
(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.
(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?
Still working on this one
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
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.
Also seeing various steps marked with cud- while they are not. Out of scope here.
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>
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.
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>
Given the attention that it received already, passing QA now
(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? :)
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>
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.
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>
Applied to 24.05.x-security
Applied to 23.11.x-security
Pushed for 25.05! Well done everyone, thank you!