Bug 28409

Summary: Category should be validated in opac-shelves.pl
Product: Koha Reporter: David Cook <dcook>
Component: Architecture, internals, and plumbingAssignee: David Cook <dcook>
Status: Pushed to oldoldoldstable --- QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: aleisha, fridolin.somers, jonathan.druart+koha, joy, katrin.fischer, kyle, lucas, martin.renvoize, nick, victor, wainuiwitikapark
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.01,20.11.07,20.05.13,19.11.19
Bug Depends on: 20982    
Bug Blocks:    
Attachments: Bug 28409: Comprehensively validate category in opac-shelves.pl
Bug 28409: Simplify data validation
Bug 28409: Comprehensively validate category in opac-shelves.pl
Bug 28409: Simplify data validation
Bug 28409: Simplify data validation
Bug 28409: Comprehensively validate category in opac-shelves.pl
Bug 28409: Simplify data validation
Bug 28409: Adjust regression test
Bug 28409: Adjust regression test

Description David Cook 2021-05-21 06:41:28 UTC
Bug 20982 shows that not validating $category can lead to XSS. While that bug fixes the XSS problem, it doesn't validate $category completely.

Create a DBI error:
http://localhost:8080/cgi-bin/koha/opac-shelves.pl?op=add&category=function(){window.location.href%20=%20%27https://git.koha-community.org/stats/koha-master/authors.html%27}()

Create a shelf with bad data:
http://localhost:8080/cgi-bin/koha/opac-shelves.pl?op=add&shelfname=blah2&category=9

This isn't really a security bug per se, but the patch for this bug will have a dependency on bug 20982. Plus, it's arguably a security bug in terms of the system accepting non-validated data against the system's intention.
Comment 1 David Cook 2021-05-21 06:45:41 UTC
Actually changed my mind... going to just update 20982...

*** This bug has been marked as a duplicate of bug 20982 ***
Comment 2 Jonathan Druart 2021-05-21 07:11:45 UTC
Created attachment 121267 [details] [review]
Bug 28409: Comprehensively validate category in opac-shelves.pl

Default to a category of 1 (ie Private). Only allow input of 1
or 2 (ie Public)

== Test plan ==
1. Go to http://localhost:8080/cgi-bin/koha/opac-shelves.pl?category=function(){window.location.href%20=%20%27https://git.koha-community.org/stats/koha-master/authors.html%27}()
2. Note that you are redirected to another website
3. Go to http://localhost:8080/cgi-bin/koha/opac-shelves.pl?op=add&shelfname=foo&category=9
4. Note that you can't see this list in the Lists (but it has been added to the database)
5. Apply the patch & restart services
6. Go to http://localhost:8080/cgi-bin/koha/opac-shelves.pl?category=function(){window.location.href%20=%20%27https://git.koha-community.org/stats/koha-master/authors.html%27}()
7. Note that you are not redirected to another website
8. Go to http://localhost:8080/cgi-bin/koha/opac-shelves.pl?op=add&shelfname=bar&category=9
9. Note that "bar" has been added as a Private list
Comment 3 Jonathan Druart 2021-05-21 07:11:50 UTC
Created attachment 121268 [details] [review]
Bug 28409: Simplify data validation

Simplify the affectation then trust it.
Comment 4 Jonathan Druart 2021-05-21 07:12:36 UTC
Then shouldn't we apply the same in shelves.pl (staff)?
Comment 5 Victor Grousset/tuxayo 2021-05-21 18:14:33 UTC
Created attachment 121281 [details] [review]
Bug 28409: Comprehensively validate category in opac-shelves.pl

Default to a category of 1 (ie Private). Only allow input of 1
or 2 (ie Public)

== Test plan ==
1. Go to http://localhost:8080/cgi-bin/koha/opac-shelves.pl?category=function(){window.location.href%20=%20%27https://git.koha-community.org/stats/koha-master/authors.html%27}()
2. Note that you are redirected to another website
3. Go to http://localhost:8080/cgi-bin/koha/opac-shelves.pl?op=add&shelfname=foo&category=9
4. Note that you can't see this list in the Lists (but it has been added to the database)
5. Apply the patch & restart services
6. Go to http://localhost:8080/cgi-bin/koha/opac-shelves.pl?category=function(){window.location.href%20=%20%27https://git.koha-community.org/stats/koha-master/authors.html%27}()
7. Note that you are not redirected to another website
8. Go to http://localhost:8080/cgi-bin/koha/opac-shelves.pl?op=add&shelfname=bar&category=9
9. Note that "bar" has been added as a Private list

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 6 Victor Grousset/tuxayo 2021-05-21 18:14:37 UTC
Created attachment 121282 [details] [review]
Bug 28409: Simplify data validation

Simplify the affectation then trust it.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 7 Victor Grousset/tuxayo 2021-05-21 18:22:59 UTC
It work! :D
Comment 10 Victor Grousset/tuxayo 2021-05-22 14:02:27 UTC
Hum, why is step 1 the same as bug 20982 ? The fix of it also fixes this case.
Comment 11 Victor Grousset/tuxayo 2021-05-22 14:06:47 UTC
Current version applies and work on 19.11.x
Comment 12 David Cook 2021-05-24 00:22:47 UTC
Comment on attachment 121282 [details] [review]
Bug 28409: Simplify data validation

Review of attachment 121282 [details] [review]:
-----------------------------------------------------------------

::: opac/opac-shelves.pl
@@ +109,4 @@
>              $shelf = Koha::Virtualshelf->new(
>                  {   shelfname          => scalar $query->param('shelfname'),
>                      sortfield          => scalar $query->param('sortfield'),
> +                    category           => $category,,

Little syntax error here
Comment 14 Jonathan Druart 2021-05-24 07:37:52 UTC
Created attachment 121308 [details] [review]
Bug 28409: Simplify data validation

Simplify the affectation then trust it.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

JD Amended patch: remove duplicate comma
Comment 15 Kyle M Hall 2021-05-28 14:40:57 UTC
Created attachment 121487 [details] [review]
Bug 28409: Comprehensively validate category in opac-shelves.pl

Default to a category of 1 (ie Private). Only allow input of 1
or 2 (ie Public)

== Test plan ==
1. Go to http://localhost:8080/cgi-bin/koha/opac-shelves.pl?category=function(){window.location.href%20=%20%27https://git.koha-community.org/stats/koha-master/authors.html%27}()
2. Note that you are redirected to another website
3. Go to http://localhost:8080/cgi-bin/koha/opac-shelves.pl?op=add&shelfname=foo&category=9
4. Note that you can't see this list in the Lists (but it has been added to the database)
5. Apply the patch & restart services
6. Go to http://localhost:8080/cgi-bin/koha/opac-shelves.pl?category=function(){window.location.href%20=%20%27https://git.koha-community.org/stats/koha-master/authors.html%27}()
7. Note that you are not redirected to another website
8. Go to http://localhost:8080/cgi-bin/koha/opac-shelves.pl?op=add&shelfname=bar&category=9
9. Note that "bar" has been added as a Private list

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall 2021-05-28 14:41:30 UTC
Created attachment 121488 [details] [review]
Bug 28409: Simplify data validation

Simplify the affectation then trust it.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

JD Amended patch: remove duplicate comma

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Victor Grousset/tuxayo 2021-06-21 23:16:17 UTC
Ready for 20.05.x, pushed to the security branch.

It works. Note comment 10, this hasn't changed so only the 2nd test case was reproducible and fixed with the patch. The 1st test case already worked.
Comment 18 Victor Grousset/tuxayo 2021-06-24 01:21:02 UTC
Trouble! (also in master)

After running the full test suite on 20.05.x-security, there is this failure:
It's doesn't fail without these patches.
But with them:

t/db_dependent/selenium/regressions.t .. 4/6 Use of uninitialized value $charge in multiplication (*) at /kohadevbox/koha/C4/Circulation.pm line 3267.
t/db_dependent/selenium/regressions.t .. 5/6 
    #   Failed test 'The second page should display the variables and attributes correctly URI escaped'
    #   at t/db_dependent/selenium/regressions.t line 257.
    #                   'http://koha:8080/cgi-bin/koha/opac-shelves.pl?page=2&category=2&op=list'
    #     doesn't match '(?^u:category=2%22%3E%3Cscript%3Ealert%28%27booh%21%27%29%3C%2Fscript%3E)'
    # Looks like you failed 1 test of 3.
t/db_dependent/selenium/regressions.t .. 6/6 
#   Failed test 'XSS vulnerabilities in pagination'
#   at t/db_dependent/selenium/regressions.t line 262.
# Looks like you failed 1 test of 6.
t/db_dependent/selenium/regressions.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/6 subtests 

============ master =================
It's doesn't fail without these patches.
But with them:


t/db_dependent/selenium/regressions.t .. 4/7 item-level_itypes set but no itemtype set for item (973) at /kohadevbox/koha/Koha/Schema/Result/Item.pm line 877.
item-level_itypes set but no itemtype set for item (973) at /kohadevbox/koha/Koha/Schema/Result/Item.pm line 877.
Use of uninitialized value $charge in multiplication (*) at /kohadevbox/koha/C4/Circulation.pm line 3394.
item-level_itypes set but no itemtype set for item (973) at /kohadevbox/koha/Koha/Schema/Result/Item.pm line 877.
item-level_itypes set but no itemtype set for item (973) at /kohadevbox/koha/Koha/Schema/Result/Item.pm line 877.
item-level_itypes set but no itemtype set for item (973) at /kohadevbox/koha/Koha/Schema/Result/Item.pm line 877.
t/db_dependent/selenium/regressions.t .. 5/7 
    #   Failed test 'The second page should display the variables and attributes correctly URI escaped'
    #   at t/db_dependent/selenium/regressions.t line 254.
    #                   'http://koha:8080/cgi-bin/koha/opac-shelves.pl?page=2&op=list&category=2'
    #     doesn't match '(?^u:category=2%22%3E%3Cscript%3Ealert%28%27booh%21%27%29%3C%2Fscript%3E)'
    # Looks like you failed 1 test of 3.
t/db_dependent/selenium/regressions.t .. 6/7 
#   Failed test 'XSS vulnerabilities in pagination'
#   at t/db_dependent/selenium/regressions.t line 258.
Wide character in print at /usr/share/perl/5.32/Test2/Formatter/TAP.pm line 156.
Wide character in print at /usr/share/perl/5.32/Test2/Formatter/TAP.pm line 156.
Wide character in print at /usr/share/perl/5.32/Test2/Formatter/TAP.pm line 156.
Wide character in print at /usr/share/perl/5.32/Test2/Formatter/TAP.pm line 156.
Wide character in print at /usr/share/perl/5.32/Test2/Formatter/TAP.pm line 156.
Wide character in print at /usr/share/perl/5.32/Test2/Formatter/TAP.pm line 156.
t/db_dependent/selenium/regressions.t .. 7/7 # Looks like you failed 1 test of 7.
t/db_dependent/selenium/regressions.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/7 subtests 


====== unrelated side note =======
master compared to 20.05.x has more warnings. It that worth opening a ticket?

item-level_itypes set but no itemtype set for item (975) at /kohadevbox/koha/Koha/Schema/Result/Item.pm line 877.

Wide character in print at /usr/share/perl/5.32/Test2/Formatter/TAP.pm line 156.
Comment 19 Jonathan Druart 2021-06-24 07:52:49 UTC
(In reply to Victor Grousset/tuxayo from comment #18)
> Trouble! (also in master)

Nothing to worry, it's just the test that is no longer valid.
Comment 20 Jonathan Druart 2021-06-24 07:57:13 UTC
Created attachment 122358 [details] [review]
Bug 28409: Adjust regression test

We are no longer expecting an URI escaped value but a corrected category
value, either 1 or 2.
Comment 21 Jonathan Druart 2021-06-24 07:59:28 UTC
Created attachment 122359 [details] [review]
Bug 28409: Adjust regression test

We are no longer expecting an URI escaped value but a corrected category
value, either 1 or 2.
Comment 22 Victor Grousset/tuxayo 2021-06-24 11:58:43 UTC
It works, thanks! :D
Comment 23 wainuiwitikapark 2021-06-25 01:56:28 UTC
Backported to 19.11.x for 19.11.19
Comment 24 Victor Grousset/tuxayo 2021-06-29 15:03:42 UTC
Backported: Pushed to 20.05.x branch for 20.05.13
Backported: Pushed to 20.11.x branch for 20.11.07
Comment 25 Jonathan Druart 2021-07-06 09:06:25 UTC
Pushed to master for 21.11, thanks to everybody involved!