|
Description
Raschin Tavakoli
2015-06-18 10:19:30 UTC
Created attachment 40336 [details] [review] Bug 14408 Path traversal vulnerability /cgi-bin/koha/svc/virtualshelves/search /cgi-bin/koha/svc/members/search Are vulnerable To test: 1/ Hit /cgi-bin/koha/svc/members/search?template_path=members/tables/members_results.tt Notice you get a valid JSON response 2/ Hit /search?template_path=..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd (You may have add more ..%2f or remove them to get the correct path) Notice you can see the contents of the /etc/passwd file 3/ Apply patch 4/ Hit the first url again, notice it still works 5/ Hit the second url notice it now errors with a file not found Repeat for the other script also Chris, It is not enough, test with: /cgi-bin/koha/svc/members/search?template_path=test%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd Don't you think we should sanitize in get_template_and_user? Created attachment 40347 [details] [review] [ COUTER-PATCH - CHANGE DONE IN C4::AUTH ] Bug 14408 Path traversal vulnerability /cgi-bin/koha/svc/virtualshelves/search /cgi-bin/koha/svc/members/search Are vulnerable To test: 1/ Hit /cgi-bin/koha/svc/members/search?template_path=members/tables/members_results.tt Notice you get a valid JSON response 2/ Hit /search?template_path=..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd (You may have add more ..%2f or remove them to get the correct path) Notice you can see the contents of the /etc/passwd file 3/ Hit /cgi-bin/koha/svc/members/search?template_path=test%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd 4/ Apply patch 5/ Hit the first url again, notice it still works 6/ Hit the second url notice it now errors with a file not found 7/ Hit the third url notice it now errors with a file not found Repeat for the other script also Created attachment 40348 [details] [review] Bug 14408: Add tests to get_template_and_user Created attachment 40374 [details] [review] [SIGNED OFF] Bug 14408 Path traversal vulnerability /cgi-bin/koha/svc/virtualshelves/search /cgi-bin/koha/svc/members/search Are vulnerable To test: 1/ Hit /cgi-bin/koha/svc/members/search?template_path=members/tables/members_results.tt Notice you get a valid JSON response 2/ Hit /search?template_path=..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd (You may have add more ..%2f or remove them to get the correct path) Notice you can see the contents of the /etc/passwd file 3/ Hit /cgi-bin/koha/svc/members/search?template_path=test%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd 4/ Apply patch 5/ Hit the first url again, notice it still works 6/ Hit the second url notice it now errors with a file not found 7/ Hit the third url notice it now errors with a file not found Repeat for the other script also Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 40375 [details] [review] [SIGNED OFF] Bug 14408: Add tests to get_template_and_user Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 40386 [details] [review] [PASSED QA] Bug 14408 Path traversal vulnerability /cgi-bin/koha/svc/virtualshelves/search /cgi-bin/koha/svc/members/search Are vulnerable To test: 1/ Hit /cgi-bin/koha/svc/members/search?template_path=members/tables/members_results.tt Notice you get a valid JSON response 2/ Hit /search?template_path=..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd (You may have add more ..%2f or remove them to get the correct path) Notice you can see the contents of the /etc/passwd file 3/ Hit /cgi-bin/koha/svc/members/search?template_path=test%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd 4/ Apply patch 5/ Hit the first url again, notice it still works 6/ Hit the second url notice it now errors with a file not found 7/ Hit the third url notice it now errors with a file not found Repeat for the other script also Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 40387 [details] [review] [PASSED QA] Bug 14408: Add tests to get_template_and_user Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> main patch applies fine on 3.18, but the tests don't. I resolved the conflicts (number of tests, easy one), but the tests don't pass. I'm leaving that off for now until such time as I have a bit more time to look into it. The problem, however, is resolved, regardless of the test result. It is not ideal to sanitise input like this. Instead, we should only accept known good values. See: https://www.owasp.org/index.php/File_System#Path_traversal True, I attempted to do that in my original patch, but was too lenient I think. + $template_path =~ s/[^a-zA-Z_-]*//; The current patch leaves us open to a null byte attack (which we had in the language cookie, which we solved by accepting only valid chars) Turns out, the problem with my patch was I was missing the g on the end of the regex. But yes, we do need a patch that only accepts valid chars (which might be different depending on the use case, for instance in this case / is a valid char) This evening after work I'll resurrect my patch and fix the regexp. Unless someone beats me. Created attachment 40443 [details] [review] Bug 14408 Path Traversal error Counter counter patch Please test well, including with the null byte %00, this uses a whitelisting to only allow files ending with .tt and not allowing ../etc Note the previous patch tries to protect against /etc/passwd but //etc/passwd is now vulnerable. I do think a whitelist is safer than trying to do a blacklist /cgi-bin/koha/svc/virtualshelves/search /cgi-bin/koha/svc/members/search Are vulnerable To test: 1/ Hit /cgi-bin/koha/svc/members/search?template_path=members/tables/members_results.tt Notice you get a valid JSON response 2/ Hit /search?template_path=..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd (You may have add more ..%2f or remove them to get the correct path) Notice you can see the contents of the /etc/passwd file 3/ Hit /cgi-bin/koha/svc/members/search?template_path=test%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd 4/ Apply patch 5/ Hit the first url again, notice it still works 6/ Hit the second url notice it now errors with a file not found 7/ Hit the third url notice it now errors with a file not found Repeat for the other script also Please test thoroughly, I did the patch in a hurry, and im happy to see a better whitelisting approach if someone has one. Created attachment 40445 [details] [review] Bug 14408 Path Traversal error Counter counter patch Please test well, including with the null byte %00, this uses a whitelisting to only allow files ending with .tt and not allowing ../etc Note the previous patch tries to protect against /etc/passwd but //etc/passwd is now vulnerable. I do think a whitelist is safer than trying to do a blacklist /cgi-bin/koha/svc/virtualshelves/search /cgi-bin/koha/svc/members/search Are vulnerable To test: 1/ Hit /cgi-bin/koha/svc/members/search?template_path=members/tables/members_results.tt Notice you get a valid JSON response 2/ Hit /search?template_path=..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd (You may have add more ..%2f or remove them to get the correct path) Notice you can see the contents of the /etc/passwd file 3/ Hit /cgi-bin/koha/svc/members/search?template_path=test%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd 4/ Apply patch 5/ Hit the first url again, notice it still works 6/ Hit the second url notice it now errors with a file not found 7/ Hit the third url notice it now errors with a file not found Repeat for the other script also Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org> Created attachment 40446 [details] [review] Bug 14408: Add tests to get_template_and_user Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org> Created attachment 40447 [details] [review] Bug 14408: Allow integers in template paths Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org> Created attachment 40491 [details] [review] [PASSED QA] Bug 14408 Path Traversal error Counter counter patch Please test well, including with the null byte %00, this uses a whitelisting to only allow files ending with .tt and not allowing ../etc Note the previous patch tries to protect against /etc/passwd but //etc/passwd is now vulnerable. I do think a whitelist is safer than trying to do a blacklist /cgi-bin/koha/svc/virtualshelves/search /cgi-bin/koha/svc/members/search Are vulnerable To test: 1/ Hit /cgi-bin/koha/svc/members/search?template_path=members/tables/members_results.tt Notice you get a valid JSON response 2/ Hit /search?template_path=..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd (You may have add more ..%2f or remove them to get the correct path) Notice you can see the contents of the /etc/passwd file 3/ Hit /cgi-bin/koha/svc/members/search?template_path=test%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd 4/ Apply patch 5/ Hit the first url again, notice it still works 6/ Hit the second url notice it now errors with a file not found 7/ Hit the third url notice it now errors with a file not found Repeat for the other script also Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 40492 [details] [review] [PASSED QA] Bug 14408: Add tests to get_template_and_user Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 40493 [details] [review] [PASSED QA] Bug 14408: Allow integers in template paths Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Patches pushed to master. Thanks Chris and Jonathan! Pushed to 3.20.x will be in 3.20.1 Hi, I'm having trouble with the tests patch for 3.18.x, I've left it off for now but have tested the patch itself and it seems to correct the issue. Liz Pushed to 3.18.x will be in 3.18.08 There is a problem with some pages calling get_template_and_user with empty string in template_name : acqui/updatesupplier.pl opac/opac-ratings.pl tools/quotes/quotes_ajax.pl tools/quotes/quotes-upload_ajax.pl Should we accept empty string or correct those pages to use has_permission() ? also, minor error : dot must be espaced in regexp and I dont understand why there is a "?" at the end : ^[$safe_chars]+\.tt$ Do I create a new Bug ? (In reply to Fridolin SOMERS from comment #26) > also, minor error : > dot must be espaced in regexp and I dont understand why there is a "?" at > the end : > ^[$safe_chars]+\.tt$ > > Do I create a new Bug ? Chris, I would say this changes could do the trick: - die "bad template path" unless $in->{'template_name'} =~ m/^[$safe_chars]+.tt?$/ig; #sanitize input + die "bad template path" unless $in->{'template_name'} =~ m/^[$safe_chars]+\.tt$/ig; #sanitize input Pushed to 3.14.x, will be in 3.14.16 (In reply to Fridolin SOMERS from comment #25) > There is a problem with some pages calling get_template_and_user with empty > string in template_name : > > acqui/updatesupplier.pl > opac/opac-ratings.pl > tools/quotes/quotes_ajax.pl > tools/quotes/quotes-upload_ajax.pl > > Should we accept empty string or correct those pages to use has_permission() > ? Yes, I think so. Did you open a new bug report? (In reply to Liz Rea from comment #24) > Pushed to 3.18.x will be in 3.18.08 Correction from the last patch in this set (Bug 14408: Allow integers in template paths) seems to be missing in 3.18.8 / 3.18.x branch (not 100% sure if it is or isn't an issue in 3.18, but it may be so, as there are plenty of *.tt files in 3.18.x with numbers in their names: addorderiso2709.tt, z3950_*.tt, marc21_field_*.tt and so on). Pushed to 3.16.x, will be in 3.16.12 (In reply to Mason James from comment #31) > Pushed to 3.16.x, will be in 3.16.12 Ok, the changes are different from 3.18.x and upper. I've reverted my patches in 3.14 to copy those from 3.16.x Hi, I've created a patch for 3.18 that contains that last fix, but doesn't use the tests. It's pushed and rolled up to the download site. Liz Another side effect of this patches is that item search gets broken (3.18.x and up seems to be affected), because we have at least two templates like this: - catalogue/itemsearch.json.tt - catalogue/itemsearch.csv.tt Hi Jacob, Please report this as a separate bug, you can mark it as a blocker for this one. Cheers, Liz Jacek... I'm so sorry my contacts are getting sticky. Liz Created attachment 40564 [details] [review] bug 14408 - itemsearch no longer working To test: Click Advanced search in staff client Click the link for "Item Search" at the top of the page Do a search, you should get results. Try some combinations and make sure it works like it should. Created attachment 40610 [details] [review] Bug 14408 & 14439 - typo fixes for regexes As applied to 3.18.8 - still need tests for these but the meat is there. Pushed to 3.16.x, will be in 3.16.13 (In reply to Mason James from comment #40) > Pushed to 3.16.x, will be in 3.16.13 oops, was actually included in 3.16.12 release |