ASVS
ASVS copied to clipboard
OWASP
[Clarification/For Discussion] Allow-list of all HTML form fields in 5.1.3
ASVS - 5.1.3 verification requirement is:
Verify that all input (HTML form fields, REST requests, URL parameters, HTTP headers, cookies, batch files, RSS feeds, etc) is validated using positive validation (allow lists). ([C5])
Not all HTML form fields can be verified through an allow-list, e.g. Free-text form field which allows html to be entered.
I propose that we add the phrasing 'where applicable/possible' to this to allow for these edge cases, otherwise they will always fail on this requirement.
Possible, or perhaps preferably allow-lists, alternatively sanitizers to emphasis that allow-lists is the preferred choice and that a sanitiser should only be used where allow-lists are not up for the task.
@vdbaan I think we may have missed the subtle points this requirement is trying to make. I think it is trying to make clear that input validation should be based on only allowing certain content or characters as opposed to blocking certain contact or characters which we discourage.
If we are allowing HTML content then the sanitizer should only be allowing safe HTML elements and attributes as opposed to blocking unsafe ones. As such, I think we need to make this clearer when we talk about sanitizers.
I understand what you are saying, however this is not how it is interpreted. I have spoken with multiple dev teams and most of them interpret this requirement as "we can only use allow-lists in order to be compliant".
I would strongly advice not to use subtlety in the requirements as they leave room for interpretation and in this case it seems that it is interpreted more strictly than intended.
I 100% agree. Can you suggest an alternative wording which makes it clear if your perspective but also makes it clear that all the violation of sanitization has to be based on an allow list style approach?
Hi @jmanico, I saw the fix and was concerned that the original intention was not clear which is why I asked @vdbaan if he had an alternative suggestion
@vdbaan - can you please give us an example, where this requirement causes some problem?
For my interpretation - if you need to "accept all" and you later sanitize it, then it just passes input validation as there is nothing to validate. Or you still have some validation, like max-length, allowed characters etc.
I'm not a fan of the idea to promote sanitizing the user input inside input validation requirement, it may cause more problems than it solves.
The teams that I have spoken with read this requirement as such that all input should always validated against allow-lists. "Accept-all" and then sanitisation was seen as unpractical. "What is the use of validation then, if you don't validate anything" was asked (This was especially asked with respect to Free-text input fields which allows HTML markup for styling, including images and tables).
I understand that these teams read this requirement perhaps too literal, but that is also the audience that we are working with and any room for mis-interpretation will be an area for mistakes (and potentially security issues).
This is why I added the "preferably allow-lists, alternatively sanitisation" in the merge request. With this I state that we recommend allow-lists, however, in those situations where allow-lists are not usable, people can use sanitisation.
Thanks @vdbaan, so when you say sanitisation, you are talking about a situation where HTML markup is allowed. In this case, you would say that a sanitiser such as DOMPurify should be used which takes a deny list approach to sanitisation which technically we are saying that they should not do.
I would be tempted to offer a carve out specifically for HTML formatting but I don't think I would want to leave a blanket exception.
How about this:
Verify that all input (HTML form fields, REST requests, URL parameters, HTTP headers, cookies, batch files, RSS feeds, etc) is validated using positive validation (allow lists). Where HTML markup must be accepted for input, a well-known and secure sanitization function may be used instead.
I like this direction!
On Mar 31, 2022, at 3:15 AM, Josh Grossman @.***> wrote:
ο»Ώ Thanks @vdbaan, so when you say sanitisation, you are talking about a situation where HTML markup is allowed. In this case, you would say that a sanitiser such as DOMPurify should be used which takes a deny list approach to sanitisation which technically we are saying that they should not do.
I would be tempted to offer a carve out specifically for HTML formatting but I don't think I would want to leave a blanket exception.
How about this:
Verify that all input (HTML form fields, REST requests, URL parameters, HTTP headers, cookies, batch files, RSS feeds, etc) is validated using positive validation (allow lists). Where HTML mark must be accepted for input, a well-known and secure sanitization function may be used instead.
β Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.
thanks @tghosth, that looks better than my proposal as it is unambiguous on what we want to achieve.
Where HTML markup must be accepted for input, a well-known and secure sanitization function may be used instead.
I think this added part is duplicating 5.2.1:
V5.2.1 Verify that all untrusted HTML input from WYSIWYG editors or similar is properly sanitized with an HTML sanitizer library or framework feature.
Great spot @elarlang!
Please can you take a look at my further change here (I added the [MODIFIED] tag in a subsequent commit) and if you agree please approve #1258
I would like to get some other opinion on that topic as well. I'm not going too strongly fight against this change, but I clearly don't like it. Arguments here: https://github.com/OWASP/ASVS/issues/1250#issuecomment-1080618045
So I would argue that the change solves your issue @elarlang since we are not promoting sanitizing the user input inside the input validation requirement but rather we are referring a specific case to a different location for the avoidance of doubt.
I am happy to wait to see if anyone else has comments though
This fix is live in the 5.0 directory.
Summary:
- You cannot use allows lists when the input type is HTML, you need to sanitize HTML input with DOMPurify or similar.
- The change is done and live and makes total sense. I'm closing this out.
As it was a clear sanitization issue and did not fit the validation requirement, we removed this part from 5.1.3.
re-open to check, if the point is covered somewhere else or do we need to improve something else.
Ugh, so I think maybe we need to make it clear that input validation is a control based on business requirements rather than security requirements, i.e. what is the business expectation for this field. i.e. if we are expecting HTML in this field then that needs to be allowed by the allow list. Maybe something like:
[MODIFIED] Verify that all input is validated using positive validation, using an allowed list of values or patterns, based on business or functionality expectations for the input.
and then maybe we also add a clarification in the explanatory text for this section
What do you think? @elarlang @vdbaan
In a way, the added part is going to duplicate 5.1.4, but I'm ok with that.
Ugh, so I think maybe we need to make it clear that input validation is a control based on business requirements rather than security requirements, i.e. what is the business expectation for this field. i.e. if we are expecting HTML in this field then that needs to be allowed by the allow list. Maybe something like:
[MODIFIED] Verify that all input is validated using positive validation, using an allowed list of values or patterns, based on business or functionality expectations for the input.
and then maybe we also add a clarification in the explanatory text for this section
What do you think? @elarlang @vdbaan
I think this is a very wise observation, Josh,
We have now security decision documentation requirements:
| # | Description | L1 | L2 | L3 | CWE |
|---|---|---|---|---|---|
| 1.5.1 | [MODIFIED, SPLIT TO 1.5.5, LEVEL L2 > L1] Verify that input validation rules define how to check the validity of data items against an expected structure. This could be common data formats such as credit card numbers, e-mail addresses, telephone numbers, or it could be an internal data format. | β | β | β | 20 |
| 1.5.5 | [ADDED, SPLIT FROM 1.5.1] Verify that input validation rules are documented and define how to ensure the logical and contextual consistency of combined data items, such as checking that suburb and zipcode match. | β | β | β | 20 |
And related implementation requirements:
| # | Description | L1 | L2 | L3 | CWE |
|---|---|---|---|---|---|
| 5.1.3 | [MODIFIED] Verify that all input is validated using positive validation, against an allowed list of values, patterns or ranges to enforce business or functional expectations for that input. | β | β | β | 20 |
| 5.1.4 | [MODIFIED, SPLIT TO 5.1.7] Verify that data items with an expected structure are validated according to the pre-defined rules. | β | β | β | 20 |
| 5.2.1 | [MODIFIED] Verify that all untrusted HTML input from WYSIWYG editors or similar is properly sanitized using a well-known and secure HTML sanitization library or framework feature. | β | β | β | 116 |
| 5.2.2 | [MODIFIED] Verify that data being passed to a potentially dangerous context is sanitized beforehand to enforce safety measures, such as only allowing characters which are safe for this context and trimming input which is too long. | β | β | β | 138 |
The "improvements" (Where HTML markup must be accepted for input, refer to requirement 5.2.1) that were made to 5.1.3 are removed.
I don't think we need this phrase about sanitization in a validation requirement, as it may cause more confusion than it can solve.
We should not fill requirements with noise to educate some misbelieving developers, if really needed, it can be covered in a chapter text (if not covered yet).
So the proposal is to close it out.
ping @tghosth