|
Description
Eric Phetteplace
2026-03-12 18:30:09 UTC
FYI I tested my proposed changed (CSP header instead of attachment header) and the results look correct. I have two identical files named malicious.svg and malicious.svg.pdf with a `console.log("hello")` script in them:
- malicious.svg: browser console shows "Executing inline script violates the following Content Security Policy directive 'script-src 'none''." and nothing is logged
- malicious.svg.pdf: browser says "Error: Failed to load PDF document" because file is not a valid PDF
Created attachment 195226 [details] [review] Bug 42080: Invoice PDFs are forced downloads Serve invoice files with a CSP header that prevents XSS instead of with an attachment header that turns them into downloads. Testing plan: - Set AcqEnableFiles preference to Do - Acquisitions > Vendor > Receive Shipment > Create invoice - Finish Receiving > Manage invoice files - Upload an SVG with a <script> tag in it - Click the SVG filename to open it - Inspect browser's console: it should say something about a script being blocked due to the CSP directive OK I added my patch and testing plan just to clarify but I leave this at status NEW until I can get some feedback. I also asked in Mattermost. For testing, you can use this as your "malicious.svg" file: <svg version="1.1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 400 400"> <title>malicious</title> <text x="200" y="200" text-anchor="middle" font-size="20" fill="black"> This SVG contains a script tag which prints "hello" to the console </text> <script>console.log("hello")</script> </svg> Hi Eric, We're actually already working on bringing Content-Security-Policy into Koha with bug 38365, so it looks like great minds think alike! That said, I think I'll need to think on this one a bit more. I love Content-Security-Policy, but it won't be turned on out of the box, and it'll be set globally so the same header will be sent for all pages sent by Starman. (I am curious though about layering Content-Security-Policy headers. Perhaps we can include the one you've provided PLUS the global one. I'm going to look into that shortly.) Another thing is while Content-Security-Policy is great (seriously I truly love it), it is only one layer of defence. Ideally it would be good to have multiple. So I'm going to think a bit more about what we could do in terms of data validation. With bug 41591 I really didn't think that it would make that much of a difference to end users, so thanks for this report. It's interesting to know that people really do want to view the file inline. Different strokes for different folks, but Koha tries to be all things to all people, so let's try to figure this one out... (In reply to David Cook from comment #4) > That said, I think I'll need to think on this one a bit more. I love > Content-Security-Policy, but it won't be turned on out of the box, and it'll > be set globally so the same header will be sent for all pages sent by > Starman. (I am curious though about layering Content-Security-Policy > headers. Perhaps we can include the one you've provided PLUS the global one. > I'm going to look into that shortly.) Fortunately, it does appear to be possible to have multiple Content-Security-Policy headers. The browser will interpret both of them and if at least one of them fails it will block, so yeah this approach could work well in this case. That said, I think it would probably be better to undo bug 41591 and re-implement with solutions from this bug report. > Another thing is while Content-Security-Policy is great (seriously I truly > love it), it is only one layer of defence. Ideally it would be good to have > multiple. > > So I'm going to think a bit more about what we could do in terms of data > validation. It looks like we store the Content-Type header in misc_files.file_type. That and the file extension are both user-supplied fields though, which cannot inherently be trusted. I think a good first step would be magic number checking. There is a library available that can help with that. So I think your patch is a good starting place, Eric. I'm just going to build a bit on top of it. Created attachment 195245 [details] [review] Bug 42080: Invoice PDFs are forced downloads Serve invoice files with a CSP header that prevents XSS instead of with an attachment header that turns them into downloads. Testing plan: - Set AcqEnableFiles preference to Do - Acquisitions > Vendor > Receive Shipment > Create invoice - Finish Receiving > Manage invoice files - Upload an SVG with a <script> tag in it - Click the SVG filename to open it - Inspect browser's console: it should say something about a script being blocked due to the CSP directive Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 195246 [details] [review] Bug 42080: (follow-up) allow some file types, checked by libmagic, to be viewed inline This patch uses File::LibMagic to determine the real file type using libmagic and magic numbers of invoice files, and allows a select group of file types to be viewed inline for user convenience. Note that a default Content-Security-Policy is still included to offer an extra layer of protection against XSS. Test plan: 0a. Apply the patch 0b. koha-plack --restart kohadev 1. Enable the syspref AcqEnableFiles (ie set to Do) 2. Find or create an active vendor 3. Create a basket if one doesn't already exist 4. Receive a shipment for the vendor 5. Create an invoice 6. Click "Finish receiving" 7. Click "Manage invoice files" 8. Upload a range of files including SVG, PDF, PNG, TXT 9. Click on the "Download" button 10. Note that the file downloads for all types 11. Click on the link in the "Name" column 12. Note that only the PDF and PNG display inline in the browser. The other file types should be downloaded same as if you clicked the "Download" button. Signed-off-by: David Cook <dcook@prosentient.com.au> |