|
Description
Ryan Henderson
2023-11-08 15:10:13 UTC
Can easily be fixed as it's only used in a couple of places: cataloguing/value_builder/marc21_field_260b.pl: source: '/cgi-bin/koha/cataloguing/ysearch.pl?table=biblioitems&field=publishercode', koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/unimarc_field_210c_bis.tt: source: "/cgi-bin/koha/cataloguing/ysearch.pl?table=biblioitems&field=publishercode", koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/unimarc_field_225a_bis.tt: source: "/cgi-bin/koha/cataloguing/ysearch.pl?table=biblioitems&field=collectiontitle", Sanitizing the input should be trivial. Created attachment 158664 [details] [review] Bug 35291: Prevent leaks from FS This should fix the problem, please test. Created attachment 158684 [details] [review] Bug 35291: Prevent leaks from FS Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 158685 [details] [review] Bug 35291: Don't allow symlinks for link files in cover image ZIP There's no reason to allow symlinks for link files in cover image ZIP files. Preventing their use prevents someone from uploading a symlink pointing to an existing file on the Koha server. Test plan: 0. Apply patch and restart/reload Koha 1. Create a PNG cover image 2. Create a datalink.txt file that contains something like the following: 29,Untitled.PNG 3. Turn on "LocalCoverImages" system preference 4. Upload via http://localhost:8081/cgi-bin/koha/tools/upload-cover-image.pl 5. Go to http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=29 6. Note the cover image has been uploaded (In reply to Jonathan Druart from comment #3) > This should fix the problem, please test. This seems to prevent the data leak, so I've signed off. I've also added a patch to disallow symlinks, so that the Perl script can't be pointed at a file on the server via a symlink. (Note that it just silently fails at the moment. I'm not sure if that is preferable to a noisy fail.) -- Of course, looking at the code, we're not validating the filename from the file. It shouldn't really cause a problem per se, but it's not good. I might add something on top of that just while we're looking at it... Created attachment 158686 [details] [review] Bug 35291: Validate filepaths in linking files Validate that the files in linking files are contained within the ZIP. Actually, I'm not confident that I've sewn up the symlink/relative filepath issue, as I'm probably handling it too far down in the code, and not testing every possible permutation... We're allowing a strange nested structure in the ZIP file... Created attachment 159151 [details] [review] Bug 35291: Prevent leaks from FS Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 159152 [details] [review] Bug 35291: Don't allow symlinks for link files in cover image ZIP There's no reason to allow symlinks for link files in cover image ZIP files. Preventing their use prevents someone from uploading a symlink pointing to an existing file on the Koha server. Test plan: 0. Apply patch and restart/reload Koha 1. Create a PNG cover image 2. Create a datalink.txt file that contains something like the following: 29,Untitled.PNG 3. Turn on "LocalCoverImages" system preference 4. Upload via http://localhost:8081/cgi-bin/koha/tools/upload-cover-image.pl 5. Go to http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=29 6. Note the cover image has been uploaded Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 159153 [details] [review] Bug 35291: Validate filepaths in linking files Validate that the files in linking files are contained within the ZIP. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> This is weird. 3 QA team members involved, signing. Yet we don't allow ourselves to do the step of marking it PQA. I'll try to reproduce the exploit and check how this solves it. Other than that, the code looks correct, makes sense and the QA script is (of course because of those involved) happy. Created attachment 159215 [details] [review] Bug 35291: Prevent leaks from FS Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com> Created attachment 159216 [details] [review] Bug 35291: Don't allow symlinks for link files in cover image ZIP There's no reason to allow symlinks for link files in cover image ZIP files. Preventing their use prevents someone from uploading a symlink pointing to an existing file on the Koha server. Test plan: 0. Apply patch and restart/reload Koha 1. Create a PNG cover image 2. Create a datalink.txt file that contains something like the following: 29,Untitled.PNG 3. Turn on "LocalCoverImages" system preference 4. Upload via http://localhost:8081/cgi-bin/koha/tools/upload-cover-image.pl 5. Go to http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=29 6. Note the cover image has been uploaded Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com> Created attachment 159217 [details] [review] Bug 35291: Validate filepaths in linking files Validate that the files in linking files are contained within the ZIP. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com> Created attachment 159218 [details] [review] Bug 35291: (QA follow-up) Tidying script for QA test tools Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com> (In reply to Aleisha Amohia from comment #16) > Created attachment 159218 [details] [review] [review] > Bug 35291: (QA follow-up) Tidying script for QA test tools > > Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com> Thanks Aleisha! I forgot to upload! This is interesting: kohadev-koha@kohadevbox:koha(master)$ qa -c 3 testing 3 commit(s) (applied to b6f350e '759 Bug 35374: Remove ES6 template li') Processing files before patches |========================>| 1 / 1 (100.00%) Processing files after patches |========================>| 1 / 1 (100.00%) OK tools/upload-cover-image.pl Processing additional checks OK! Created attachment 159220 [details] [review] Bug 35291: Prevent leaks from FS Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 159221 [details] [review] Bug 35291: Don't allow symlinks for link files in cover image ZIP There's no reason to allow symlinks for link files in cover image ZIP files. Preventing their use prevents someone from uploading a symlink pointing to an existing file on the Koha server. Test plan: 0. Apply patch and restart/reload Koha 1. Create a PNG cover image 2. Create a datalink.txt file that contains something like the following: 29,Untitled.PNG 3. Turn on "LocalCoverImages" system preference 4. Upload via http://localhost:8081/cgi-bin/koha/tools/upload-cover-image.pl 5. Go to http://localhost:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=29 6. Note the cover image has been uploaded Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 159222 [details] [review] Bug 35291: Validate filepaths in linking files Validate that the files in linking files are contained within the ZIP. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 159223 [details] [review] Bug 35291: (QA follow-up) Tidying script for QA test tools Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Pushed to master for 23.11. Nice work everyone, thanks! Pushed to 21.11.x for 21.11.26 |