new requirement for Formula Injection

[RafaelGreen1]

Hi,
In V5.3 Output Encoding and Injection Prevention I would have expected to see something about formula injection.
For example, when a server of summer camps website genrates xlsx file with students' attendace data, last name of one the students could be something like "=HYPERLINK(url, student's last name)" or "=cmd|' /C...".
There are explanations about this attack here: CSV_Injection.
Does it appear elsewhere in the standard?
Thanks,
Rafael,
Edit: Thanks to Haim Mitrany who informed me about this vulnerability.

[elarlang]

Nope, we don't have separate requirement for that. But we should have one :)

[elarlang]

(edited)
Actually, maybe we need to cover 2 problems:

• CSV injection - if value contains "cell separator symbol" and part of the value will be in different cells when you open the file
  • CSV as Comma Separated Values is a bit misleading here, as this separator can be also ;, \t (tab) or (space)
• Formula injection - when content of the cell is executed as formula

Formula Injection may work also in xls, xlsx, odf, Google Spreadsheet etc.

CWE-1236: Improper Neutralization of Formula Elements in a CSV File

[tghosth]

2 linked problems that probably need to be solved together because the mitigation for formula injection requires the mitigation for CSV injection to also be applied in order to be effective.

Suggest single new requirement

[tghosth]

Hi @RafaelGreen1, are you able to suggest a requirement wording for this new requirement?

@set-reminder 3 weeks @tghosth to revisit

[octo-reminder]

⏰ Reminder
Wednesday, January 18, 2023 12:00 AM (GMT+01:00)

@tghosth to revisit

[RafaelGreen1]

Hi @tghosth , I created this pull request: #1482
(I tried to keep the requirement short, but I added a link in the References).
It's my first pull request here, hope it's ok.
Thanks,
Rafael,

[RafaelGreen1]

@elarlang, following your comment in the pull request, I submitted a commit to fix the reference label in 1.5.4.
I understand from your comment that we need to finish the discussion here before doing the pull request. But I saw a 'Changes requested' label so I created a fix commit (If you want, I can cancel the pull request and make a new one after we finish the discussion).

For your convenience, I put here the suggested wording:

Verify that the application protects against CSV injection in generated (or exported) spreadsheet documents (such as csv, xls, xlsx, odf, Google Spreadsheet etc). Ensure that no cells begin with any of the following characters: ['=', '+', '-', '@', '\t', 'r']. Also, take care of the field separator (e.g., ‘,’, or ‘;’) and quotes (e.g., ', or "). | ✓ | ✓ | ✓ | 1236 |

• new link in the reference: [CSV Injection] (https://owasp.org/www-community/attacks/CSV_Injection)

Thanks,
Rafael,

[elarlang]

I understand from your comment that we need to finish the discussion here before doing the pull request.

Yes, in general it would be nice to do that way - then there is readable discussion in related issue and arguments, why some change was made to the document. Discussion over PR's are hard to follow. But take it as note for the smoother life in the future :)

Let's try to finalize the requirement text now and/or wait feedback from others.

[elarlang]

First part - csv injection:

Verify that the application protects against CSV injection in generated (or exported) spreadsheet documents (such as csv, xls, xlsx, odf, Google Spreadsheet etc).

• I would remove (or exported) - it just duplicates generated and can be confusing - do application need to check the csv,xls,xslx ... content when just serving file, but this is not the case.
• "Google Spreadsheet" does not fit there. I mentioned it myself, but this is where it may be executed - there is no such export format for an application like Google Spreadsheet, it just imports all others.

Second part - formula injection

Ensure that no cells begin with any of the following characters: ['=', '+', '-', '@', '\t', '\r']. Also, take care of the field separator (e.g., ‘,’, or ‘;’) and quotes (e.g., ', or ").

• Maybe to use "Verify" instead of Ensure - just like everywhere else
• In general I can not see how we can use this requirement in practice - what if in content some value start with symbol '=', what the application need to do then? ("Ensure that no cell begins with..."). The point should be, that those symbols can not be used as formula execution on the client side, so those must be properly escaped. Maybe something like:
  Verify that cell values are not executed as formula except intended by the application, e. g. values starting with special characters such as '=', '+', '-', '@' are properly escaped.

I left out \t and \r as those are part of CSV injection already and are not directly related to formula execution (if I'm wrong, please comment)

Last part - what is it

Also, take care of the field separator (e.g., ‘,’, or ‘;’) and quotes (e.g., ', or ")

• For me it duplicates first (CSV injection) part

[RafaelGreen1]

@elarlang
Here https://owasp.org/www-community/attacks/CSV_Injection, they say that CSV Injection is known as Formula Injection.
I understand you would classify usage of the field separator to start a new cell as CSV injection and usage of '=' or '@' at the beginning of the cell as formula injection.

Here is the new suggestion (based on your comments):

Verify that the application protects against CSV/Formula injection in generated spreadsheet documents
(such as csv, xls, xlsx, odf etc) by following the next two steps:

1. Verify that the cell separator, line separator and quotes are properly handled (filtered or escaped).
2. Verify that cell values are not executed as formula unless intended by the application by
   prepending the cell field with a single quote or ensure that the cell doesn't begin with one of the characters '=', '+', '-', '@'.

[elarlang]

This proposal is a bit long. I use previously mentioned pieces and put this one as target to brainstorm and develop (language and grammar checks included) further:

Verify that the application protects against CSV injection in generated spreadsheet documents (such as csv, xls, xlsx, odf) and verify that cell values are not executed as formula except intended by the application, e. g. values starting with special characters such as '=', '+', '-', '@' are properly escaped.

[RafaelGreen1]

LGTM

[octo-reminder]

🔔 @tghosth

@tghosth to revisit

[RafaelGreen1]

ping @tghosth

[tghosth]

Sorry for the delay in response.

I think the modifications below should provide the best protection. Note that \r will be covered by the RFC4180 escaping so I don't include it on the list of special characters. It also won't be a problem in non CSV formats. I do however include \00 as IIRC that character seems to be ignored meaning if \00 is the first character and = is the second character then it will still be evaluated as a formula...

Verify that the application follows the escaping rules defined in RFC4180 2.6 and 2.7 when exporting CSV files. After that (and when exporting other spreadsheet formats such as xls, xlsx, odf) it should also escape special characters including '=', '+', '-', '@' '\t' (tab) and '\00' (null character) using a single quote, if they are the first character in a field. These mechanisms should prevent Formula Injection.

@elarlang @RafaelGreen1 what do you think?

[elarlang]

I think requirement text should be understandable without opening any external resource, including RFC. This is the reason why have avoided references to external resources so far (I can see that we have now 14.5.6 added).

Another problem with RFC - if there comes new version and obsoletes or updates referred RFC, then ASVS requirement is pointing to outdated version.

On the other hand, RFC's are useful reading and we should think, if and how to cover them in ASVS in general.

[tghosth]

@elarlang problem is that the escaping rules for CSVs are too complicated to reproduce in full. Also seems unlikely that this RFC will significantly change in the near future. I think we have to accept that people are going to have to refer to external resources in order to actually apply fixes.

As such, I stand by my wording above (reproduced below). Any other thoughts @elarlang / @jmanico?

Verify that the application follows the escaping rules defined in RFC4180 2.6 and 2.7 when exporting CSV files. After that (and when exporting other spreadsheet formats such as xls, xlsx, odf) it should also escape special characters including '=', '+', '-', '@' '\t' (tab) and '\00' (null character) using a single quote, if they are the first character in a field. These mechanisms should prevent Formula Injection.

[elarlang]

Tried to find shorter version, ended up with longer one, but idea is:

Verify that the application is protected against CSV injection (by following escaping rules defined in RFC4180 2.6 and 2.7) when generating CSV content and protected against Formula Injection when generating other spreadsheet formats such as xls, xlsx, odf) by escaping special characters including '=', '+', '-', '@' '\t' (tab) and '\00' (null character) using a single quote, if they are the first character in a field.

Note - exporting files (from previous proposal) to generating content - the problem is when application generates certain content with certain formating rules, is the content exported to files, saved to database, served as inline content - does not matter.

Should we also have matching requirement when an application imports csv, xlx, etc content to sanitize it and do not execute any formulas?

[tghosth]

@elarlang I hope you don't mind but I think your proposal which is basically one long sentence is a little hard to read. What do you think about:

Verify that the application is protected against CSV/Formula Injection. Follow the escaping rules defined in RFC4180 2.6 and 2.7 when exporting CSV files. Escape special characters including '=', '+', '-', '@' '\t' (tab) and '\00' (null character) using a single quote, if they are the first character in a field, when exporting CSV files and other spreadsheet formats such as xls, xlsx, odf.

I am not sure I am aware of specific attacks on import.

[tghosth]

@jmanico what do you think about this requirement:

Verify that the application is protected against CSV/Formula Injection. Follow the escaping rules defined in RFC4180 2.6 and 2.7 when exporting CSV files. Escape special characters including '=', '+', '-', '@' '\t' (tab) and '\00' (null character) using a single quote, if they are the first character in a field, when exporting CSV files and other spreadsheet formats such as xls, xlsx, odf.

[jmanico]

Well done Josh and others, I like it.

[tghosth]

@RafaelGreen1 do you want to create a new pull request based on this?

#	Description	L1	L2	L3	CWE
5.3.11	[ADDED] Verify that the application is protected against CSV/Formula Injection. Follow the escaping rules defined in RFC4180 2.6 and 2.7 when exporting CSV files. Escape special characters including '=', '+', '-', '@' '\t' (tab) and '\00' (null character) using a single quote, if they are the first character in a field, when exporting CSV files and other spreadsheet formats such as xls, xlsx, odf.	✓	✓	✓	1236

[elarlang]

"CSV/Formula" - / vs "or #1491

[tghosth]

Ok so let's change to:

#	Description	L1	L2	L3	CWE
5.3.11	[ADDED] Verify that the application is protected against CSV and Formula Injection. Follow the escaping rules defined in RFC4180 2.6 and 2.7 when exporting CSV files. Escape special characters including '=', '+', '-', '@' '\t' (tab) and '\00' (null character) using a single quote, if they are the first character in a field, when exporting CSV files and other spreadsheet formats such as xls, xlsx, odf.	✓	✓	✓	1236

[tghosth]

Is that ok @elarlang?

[elarlang]

I'm ok with that. "Follow" and "Escape" sound like commands to the reader not "Verify that application follows" and "Verify that application escapes"

[tghosth]

Opened #1709 to resolve this @elarlang

[elarlang]

Linking - wording got updated via #2811