Closed (fixed)
Project:
Excel Serialization
Version:
8.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
7 Mar 2022 at 13:11 UTC
Updated:
18 Sep 2024 at 13:59 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
michael.acampora commentedAttached patch will escape unwanted formulas.
Comment #3
lendude#2 didn't go far enough for the error we saw, I needed to quote the whole thing to get it past the value triggering the errors
Comment #4
anneke_vde commentedI tested the last patch, the batch export now runs ok and the exported data looks good.
Comment #5
rosk0I'm curious if the double quote character is the correct one to be used here.
We were recommended to use a single quote character to escape field data starting with equals sign by the security company to avoid potential remote code execution.
Comment #6
rosk0When setting a cell value,
PhpOffice\PhpSpreadsheetwill use default value binder (\PhpOffice\PhpSpreadsheet\Cell\DefaultValueBinder), if none provided. This binder is responsible for guessing data type based on its value and this is where string starting from equals sign is getting a formula data type assigned.I don't like current implementation because it modifies original data. I believe there is a better way, or even two.
During testing, I noticed that most office suites would prepend the data with a single quote to treat the value as a plain string and prevent formula processing. At the same time this data would be rendered in a cell without the prepended single quote. We could use this option if we really want to modify original data.
A better option, I believe, would be to tell
PhpOffice\PhpSpreadsheet, and consuming office software, to treat values starting from equals sign as string and prevent formula processing. The user would than need to explicitly enable formula processing for the affected cell. This is far better position from the security point of view, in my opinion.Tested in:
Latter option is implemented in this patch.
Comment #7
larowlanGoing to unpublish this as its a security issue - https://owasp.org/www-community/attacks/CSV_Injection
Can we get a private issue for this on security.drupal.org?
Comment #8
larowlanComment #9
larowlanThe security team agreed to keep this in public
Comment #10
feyp commentedNote that escaping the equal sign is not enough to prevent CSV injection. OWASP has a list of characters to escape, @larowlan already posted that one. This list is also used by Smyfony in its serialization component: https://symfony.com/blog/cve-2021-41270-prevent-csv-injection-via-formulas So the patch most likely needs to be updated to include the additional characters. Setting to "Needs work" for that.
Comment #11
solideogloria commentedQuoting relevant portion here:
Comment #12
solideogloria commentedI think it would be best to follow the guidelines from OWASP, and to prefix such data with
', rather than doing the approach in #6, though I don't really know much on the subject matter or about this module.Comment #15
ericgsmith commentedMoved patch from #6 to MR and rebased against 8.x-1.x.
Setting MR as draft and leaving as needs work as have not looked at suggested changes in above comments 10 to 12 yet.
Comment #18
theemstra commentedComment #19
solideogloria commentedRegarding #11, are there any plans to format values starting with the other recommended characters?