Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clarify 50.6.2 #2468

Open
elarlang opened this issue Dec 15, 2024 · 28 comments · Fixed by #2479
Open

clarify 50.6.2 #2468

elarlang opened this issue Dec 15, 2024 · 28 comments · Fixed by #2479
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet 6) PR awaiting review V50 Group issues related to Web Frontend _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@elarlang
Copy link
Collaborator

# Description L1 L2 L3 CWE
50.6.2 [ADDED, SPLIT FROM 5.3.3] Verify that JavaScript context-aware methods are used when handling untrusted data to avoid unintended content execution, such as executing content as HTML instead of displaying it as text.

Seems that this requirement needs clarification or an example.

Something like:

For example, untrusted input must not be applied via innerHTML or outerHTML properties, or not used via setHTMLUnsafe, insertAdjacentHTML or document.write functions. Instead, createTextNode should be used.

@elarlang elarlang added the V50 Group issues related to Web Frontend label Dec 15, 2024
@tghosth tghosth added 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet _5.0 - prep This needs to be addressed to prepare 5.0 labels Dec 15, 2024
@elarlang
Copy link
Collaborator Author

ping @randomstuff - I saw "in some other source" that this requirement was confusing for you, does the proposed addon make sense and make it more clear?

@randomstuff
Copy link
Contributor

Thanks, I intended to make a pass of the requirements I did not understand at some point and either try to understand them or raise an issue :)

The thing I did not understand was "context-aware methods" but with the examples I understand what we are talking about. So that's a clear improvement.

(However, maybe we could make this clearer or make explicit that (AFAIU) interpretation of the user-provided HTML may depend on where we are on the DOM. Searching "context-aware JavaScript" does not give much help. I don't have any suggestion for now, however.)

@jmanico
Copy link
Member

jmanico commented Dec 17, 2024

For example, untrusted input must not be applied via innerHTML or outerHTML properties, or not used via setHTMLUnsafe, insertAdjacentHTML or document.write functions. Instead, createTextNode should be used

As a side note, here are functions in JS that are safe besides createTextNode

  • elem.textContent = dangerVariable;
  • elem.insertAdjacentText(dangerVariable);
  • elem.className = dangerVariable;
  • elem.setAttribute(safeAttribute, dangerVariable);
  • formfield.value = dangerVariable;
  • document.createTextNode(dangerVariable);
  • document.createElement(dangerVariable);

And anything else can be made safe with DOMPurify (which is also an experimental native browser function)

  • elem.innerHTML =DOMPurify.sanitize(dangerVariable);

@elarlang
Copy link
Collaborator Author

I think we just need to pick one most widespread function as an example to explain the idea behind the requirement.

I would not recommend DOMPurify here as this does not display the input as is - it changes the value and is part of sanitization.

@jmanico
Copy link
Member

jmanico commented Dec 17, 2024

I think we just need to pick one most widespread function as an example to explain the idea behind the requirement.

That fine, but I just don't want to imply it's the only one. (And I agree on DOMPurify)

Perhaps:

Verify that untrusted input must not be applied via innerHTML, outerHTML, insertAdjacentHTML, document.write or other properties or functions that render HTML. Instead, createTextNode, textContent and similar safe functions that do not render HTML and only render content as text should be used.

@elarlang
Copy link
Collaborator Author

removed 2 examples, added few commas, should > must:

Verify that untrusted input must not be applied via innerHTML, document.write, or other properties or functions that render HTML. Instead, createTextNode, textContent, and similar safe functions that do not render HTML and only render content as text must be used.

@elarlang elarlang added the 4) proposal for review Issue contains clear proposal for add/change something label Dec 17, 2024
@jmanico
Copy link
Member

jmanico commented Dec 17, 2024

I like it Elar. One small change suggestion at the end.

Verify that untrusted input must not be applied via innerHTML, document.write, or other properties or functions that render HTML. Instead, use createTextNode, textContent, and similar safe functions that do not render HTML and only render content as text.

elarlang added a commit that referenced this issue Dec 17, 2024
@elarlang elarlang linked a pull request Dec 17, 2024 that will close this issue
@elarlang elarlang added 6) PR awaiting review and removed 4) proposal for review Issue contains clear proposal for add/change something labels Dec 17, 2024
@tghosth
Copy link
Collaborator

tghosth commented Dec 18, 2024

This seems to have resulted in a negative requirement.

Can I suggest:

# Description L1 L2 L3 CWE
50.6.2 [ADDED, SPLIT FROM 5.3.3] Verify that HTML is rendered using safe functions such as createTextNode or textContent, that do not render HTML and only render content as text. Untrusted input must not be applied via innerHTML, document.write, or other properties or functions that render HTML.

@elarlang
Copy link
Collaborator Author

Verify that HTML is rendered using safe functions such as createTextNode or textContent, that do not render HTML and only render content as text. Untrusted input must not be applied via innerHTML, document.write, or other properties or functions that render HTML.

This went a bit in wrong direction I guess.

Probably needs some wordsmithing, but alternative direction:

Verify that when applying untrusted content with JavaScript into HTML or DOM, safe functions (such as createTextNode, textContent) are used that render content as text. Properties or functions (such as innerHTML, document.write) that render content as HTML must not be used.

@tghosth
Copy link
Collaborator

tghosth commented Dec 18, 2024

"Verify that functions (such as createTextNode, textContent) which safely render content as text, are used to apply untrusted to an HTML page or the DOM. Properties or functions (such as innerHTML, document.write) that render content as HTML must not be used."

How about this?

@elarlang
Copy link
Collaborator Author

are used to apply untrusted to an HTML page or the DOM.

  • "untrusted" > untrusted content, untrusted input
  • "an HTML page" vs "the DOM" - is that correct?

@tghosth
Copy link
Collaborator

tghosth commented Dec 18, 2024

"untrusted" > untrusted content, untrusted input

Yeah content is better

"an HTML page" vs "the DOM" - is that correct?

Yeah

"Verify that functions (such as createTextNode, textContent) which safely render content as text, are used to apply untrusted content to an HTML page or the DOM. Properties or functions (such as innerHTML, document.write) that render content as HTML must not be used."

@jmanico
Copy link
Member

jmanico commented Dec 19, 2024

"Verify that functions (such as createTextNode, textContent) which safely render content as text, are used to apply untrusted content to an HTML page or the DOM. Properties or functions (such as innerHTML, document.write) that render content as HTML must not be used."

Folks, there are a lot of good use cases for innerHTML and document.write. You just need to make sure if its untrusted data, that you use DOMPurify or the browers upcoming native HTML sanitizer function.

Perhaps replace the last sentence:

Functions or properties that render content as HTML (such as innerHTML and document.write) must only be used when the content is sanitized using a trusted library like DOMPurify.

@jmanico jmanico reopened this Dec 19, 2024
@elarlang
Copy link
Collaborator Author

Can you provide a use-case that is not covered by any other requirement?

@jmanico
Copy link
Member

jmanico commented Dec 20, 2024

The point I am making is that this sentence is to extreme.

"Properties or functions (such as innerHTML, document.write) that render content as HTML must not be used."

There are many use cases for these functions to be used. To say "must not or even should not" is too extreme.

This has nothing to do with other requirements. The "must" in this one is not right, as there are use cases for using these functions safely with sanitizers.

@elarlang
Copy link
Collaborator Author

elarlang commented Dec 20, 2024

"Properties or functions (such as innerHTML, document.write) that render content as HTML must not be used."

Why to produce fake news (with out of context explanation part), if the requirement says:

Verify that functions (such as createTextNode, textContent) which safely render content as text, are used to apply untrusted content to an HTML page or the DOM.

Mentioning DOMpurify is allowing sanitization (changing the user data) instead of displaying it correctly (as the data was meant to be displayed).

If there is business need to use sanitization, including DOMPurify or similar, we have requirement 5.2.1 for that:

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.

@jmanico
Copy link
Member

jmanico commented Dec 20, 2024 via email

@elarlang
Copy link
Collaborator Author

elarlang commented Dec 20, 2024

Coming from person who complains about "personal attacks"? :)

Fake news? Are you struggling with English again? Why don’t you read my comment more carefully before you reply. Maybe AI can help you with the English.

... and such a irony.

@jmanico
Copy link
Member

jmanico commented Dec 20, 2024 via email

@elarlang
Copy link
Collaborator Author

So, there is no surprise in the fact that English is not my native language. At the same time, I can not recall any situation, where it has caused any problem to achieve the goal.

Coming out with that, it's just a lame. You should get over from the proofed vulns in your solution or not admitting you are not correct. Or whatever disturbs you.

If you want to have requirement updated, re-read comments from re-open and maybe few comments before.

If you want to have a fight, you'll get it.

@jmanico
Copy link
Member

jmanico commented Dec 20, 2024 via email

@jmanico
Copy link
Member

jmanico commented Dec 21, 2024

At first I am only suggesting one small edit.

Verify that functions (such as createTextNode, textContent) which safely render content as text, are used to apply untrusted content to an HTML page or the DOM. Properties or functions (such as innerHTML, document.write) that render content as HTML should not be used.

But even still, something is missing. As we have discussed, there are two use cases:

a) If we want to render data as content—exactly as the user typed it in strictly as text —then I agree, use textContent and similar.
b) If we want to render data as HTML, then use innerHTML and DOMPurify.

I realize for (b), we capture DOMPurify in other requirements.

My problem with this requirement as is, is that "used to apply untrusted content to an HTML page or the DOM" does not capture the idea that this applies only when you want to render user input as text, as described in (a).

So perhaps:

Verify that functions (such as createTextNode, textContent) are used to apply untrusted content to an HTML page or the DOM when content needs to be rendered strictly and safely as text and not as HTML. Properties or functions (such as innerHTML, document.write) that render content as HTML should not be used when content should be rendered strictly as text.

Again, we do not always want to render data as text. So to say "never use innerHTML" is a bit too strict.

@tghosth
Copy link
Collaborator

tghosth commented Dec 22, 2024

So I seem to recall that we discussed the "must" vs "should" thing in a cabin in Bedfordshire :) I think the decision was that as a general rule whilst "must" sounds harsh, it is more technically correct and makes things clearer given that RFC 2119 defines "should" as a recommendation.

For this requirement specifically, I am not sure that I understand the following statements @jmanico:

There are many cases where using these functions on untrusted data to modify the DOM is necessary and this requirement as it stands denies that.

My problem with this requirement as is, is that "used to apply untrusted content to an HTML page or the DOM" does not capture the idea that this applies only when you want to render user input as text, as described in (a).

The way I understand it is that if we want to render HTML based on untrusted content, we need to make it safe first which is why we have 5.2.1.

If we have done that then in principle it is not longer untrusted and therefore shouldn't really be covered by the requirement we are currently discussing.

We could make this clearer with the following small change at the end of the requirement:

[ADDED, SPLIT FROM 5.3.3] Verify that functions (such as createTextNode, textContent) which safely render content as text, are used to apply untrusted content to an HTML page or the DOM. Properties or functions (such as innerHTML, document.write) that render content as HTML must not be used for untrusted content.

Does this satisfy your concern @jmanico ?

@elarlang
Copy link
Collaborator Author

Josh, I think in your version the last "untrusted content" just the duplicates already mentioned "untrusted" and does not give anything extra.

I think Jim's version for the last proposal works, but should >must.

Properties or functions (such as innerHTML, document.write) that render content as HTML must not be used when content should be rendered strictly as text.


Jim, take this comment (#2468 (comment)) as standard how to respond. I think you have comments to regret here.

If you need help with English or understanding my comment better let me know off-line and we can zoom or similar over this.

If you need help with ASVS or understanding, what is the scope, context, requirement, or the role for co-leader, let me know off-line and we can Google Meet over this.

@jmanico
Copy link
Member

jmanico commented Dec 22, 2024 via email

@elarlang
Copy link
Collaborator Author

elarlang commented Dec 22, 2024

Elar,

I think you’re still struggling with language and I prefer Zoom over Google Meet. Happy to talk live.

This sentence is still wrong when you look at it by itself.

“Properties or functions (such as innerHTML, document.write) that render content as HTML must not be used for untrusted content.”

This is still not accurate, you only want to avoid these properties and functions when you are not trying to render HTML.

With all the attempts to clue some "you are struggling with language" to me you are just ridiculous because the sentence is written by Josh :)

#2468 (comment)

Verify that functions (such as createTextNode, textContent) which safely render content as text, are used to apply untrusted content to an HTML page or the DOM. Properties or functions (such as innerHTML, document.write) that render content as HTML must not be used.

As I agreed with your proposal (#2468 (comment)) in my comment before you said that I still struggling - so if I proposed your proposal and "I was struggling with the language", then who actually struggles?

What was the actual point to come with the rant? There was no reason, and for sure not any good intent.

Oh, and: #2468 (comment)

The level of failure on this is getting more and more epic. Know when to stop.


Also I agree with the comment by Josh, that kind of concludes the points I previously wrote: #2468 (comment)

@jmanico
Copy link
Member

jmanico commented Dec 22, 2024 via email

@tghosth
Copy link
Collaborator

tghosth commented Dec 23, 2024

Properties or functions (such as innerHTML, document.write) that render content as HTML must not be used for untrusted content.

This is still not accurate, you only want to avoid these properties and functions when you are not trying to render HTML.

I queried this statement in a comment above and said the following:

"The way I understand it is that if we want to render HTML based on untrusted content, we need to make it safe first which is why we have 5.2.1.

If we have done that then in principle it is not longer untrusted and therefore shouldn't really be covered by the requirement we are currently discussing.

So I am not sure I understand in what context you would use these functions with untrusted content."

Can you respond to this question please @jmanico

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet 6) PR awaiting review V50 Group issues related to Web Frontend _5.0 - prep This needs to be addressed to prepare 5.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants