Summary: | Filepaths not validated in ZIP upload to picture-upload.pl | ||
---|---|---|---|
Product: | Koha | Reporter: | David Cook <dcook> |
Component: | Patrons | Assignee: | David Cook <dcook> |
Status: | RESOLVED FIXED | QA Contact: | Marcel de Rooy <m.de.rooy> |
Severity: | normal | ||
Priority: | P5 - low | CC: | amitddng135, dcook, didier.gautheron, fridolin.somers, gmcharlt, jonathan.druart, katrin.fischer, kyle.m.hall, laurent.ducos, lisette, lucas, m.de.rooy, martin.renvoize, nick, philippe.blouin, tomascohen, wainuiwitikapark |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Small patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: |
24.11.00,24.05.03,23.11.08,23.05.14,22.11.20
|
|
Circulation function: | |||
Bug Depends on: | 37323 | ||
Bug Blocks: | |||
Attachments: |
Bug 37488: Validate paths in datalink.txt/idlink.txt files
Bug 37488: Validate paths in datalink.txt/idlink.txt files Image file that can be used for testing Bug 37488: Validate paths in datalink.txt/idlink.txt files |
Description
David Cook
2024-07-26 03:44:44 UTC
Created attachment 169640 [details] [review] Bug 37488: Validate paths in datalink.txt/idlink.txt files This change validates the paths in datalink.txt/idlink.txt, so that only images in the unpacked archive directory are allowed Test plan: 0. Apply the patch 1. koha-plack --reload kohadev 2. Create a datalink.txt file with the following: 42,selfie.jpg 3. Create a jpeg at selfie.jpg 4. ZIP the datalink.txt and selfie.jpg files 5. Upload to the "Upload patron images" tool (after enabling the "patronimages" system preference) 6. Note that the image uploads correctly Created attachment 169704 [details] [review] Bug 37488: Validate paths in datalink.txt/idlink.txt files This change validates the paths in datalink.txt/idlink.txt, so that only images in the unpacked archive directory are allowed Test plan: 0. Apply the patch 1. koha-plack --reload kohadev 2. Create a datalink.txt file with the following: 42,selfie.jpg 3. Create a jpeg at selfie.jpg 4. ZIP the datalink.txt and selfie.jpg files 5. Upload to the "Upload patron images" tool (after enabling the "patronimages" system preference) 6. Note that the image uploads correctly Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 169707 [details]
Image file that can be used for testing
Combination of helpfulness and vanity :-) Feel free to use my lovely selfie to ease testing
Looking here Created attachment 169992 [details] [review] Bug 37488: Validate paths in datalink.txt/idlink.txt files This change validates the paths in datalink.txt/idlink.txt, so that only images in the unpacked archive directory are allowed Test plan: 0. Apply the patch 1. koha-plack --reload kohadev 2. Create a datalink.txt file with the following: 42,selfie.jpg 3. Create a jpeg at selfie.jpg 4. ZIP the datalink.txt and selfie.jpg files 5. Upload to the "Upload patron images" tool (after enabling the "patronimages" system preference) 6. Note that the image uploads correctly Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Isnt this patch only redirecting to another error message actually ? Without this patch, I have something like: [2024/08/02 08:58:38] [WARN] Opening /tmp/7PkHGkjawG//etc/selfie.jpg failed! at /usr/share/koha/tools/picture-upload.pl line 413. (Line number may be affected by a few debugs.) And with this patch: [2024/08/02 08:59:49] [WARN] Missing filename at /usr/share/koha/tools/picture-upload.pl line 435. Using a path /etc/selfie.jpg (nice picture, Nick ;) Will put a QA stamp on it, just asking.. Note that missing filename is not really the best error message? (In reply to Marcel de Rooy from comment #7) > Without this patch, I have something like: > > [2024/08/02 08:58:38] [WARN] Opening /tmp/7PkHGkjawG//etc/selfie.jpg failed! > at /usr/share/koha/tools/picture-upload.pl line 413. > (Line number may be affected by a few debugs.) You haven't crafted your exploit quite right there. You'd need to preface "/etc/selfie.jpg" with "../.." and then it would work. > And with this patch: > > [2024/08/02 08:59:49] [WARN] Missing filename at > /usr/share/koha/tools/picture-upload.pl line 435. > > Using a path /etc/selfie.jpg (nice picture, Nick ;) > > Will put a QA stamp on it, just asking.. > > Note that missing filename is not really the best error message? No, it's probably not the best error message, but I thought it was accurate enough, and kept the patch small and easy to backport. (In reply to David Cook from comment #8) > (In reply to Marcel de Rooy from comment #7) > > Without this patch, I have something like: > > > > [2024/08/02 08:58:38] [WARN] Opening /tmp/7PkHGkjawG//etc/selfie.jpg failed! > > at /usr/share/koha/tools/picture-upload.pl line 413. > > (Line number may be affected by a few debugs.) > > You haven't crafted your exploit quite right there. You'd need to preface > "/etc/selfie.jpg" with "../.." and then it would work. > Tried several variations. Retried this one: [2024/08/05 06:49:30] [WARN] Opening /tmp/ArJVJ8uzLT/../../etc/selfie.jpg failed! at /usr/share/koha/tools/picture-upload.pl line 416. Debian 11 container. There must be more to it.. (In reply to Marcel de Rooy from comment #9) > Tried several variations. Retried this one: > > [2024/08/05 06:49:30] [WARN] Opening /tmp/ArJVJ8uzLT/../../etc/selfie.jpg > failed! at /usr/share/koha/tools/picture-upload.pl line 416. > > Debian 11 container. > > There must be more to it.. It looks like you're not using ktd. I'm guessing it's with your work Koha setup? That could complicate things. Have you checked the file permissions? What permissions does the Koha user run as? (In reply to David Cook from comment #10) > (In reply to Marcel de Rooy from comment #9) > > Tried several variations. Retried this one: > > > > [2024/08/05 06:49:30] [WARN] Opening /tmp/ArJVJ8uzLT/../../etc/selfie.jpg > > failed! at /usr/share/koha/tools/picture-upload.pl line 416. > > > > Debian 11 container. > > > > There must be more to it.. > > It looks like you're not using ktd. I'm guessing it's with your work Koha > setup? That could complicate things. Have you checked the file permissions? > What permissions does the Koha user run as? No ktd. But the setup is fine, running the koha user for Apache and Plack. I just realize that I was kind of misinterpreting your test plan and the contents of Nicks test zip file. We are not preventing to write to /tmp or /etc here! We are preventing to load from /tmp or /etc. So, all good. Katrin, could you get this further please? Thx (In reply to Marcel de Rooy from comment #12) > Katrin, could you get this further please? Thx Please check my note on the Mattermost security channel. Pushed to main for 24.11. Nice work everyone, thanks! Not backporting to 23.05.x unless requested |