|
Description
Victor Grousset/tuxayo
2024-05-16 02:32:46 UTC
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 |