-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Updated Clickjacking Cheatsheet . Closes #1577 #1609
base: master
Are you sure you want to change the base?
Conversation
@jmanico @kwwall @szh @mackowski |
This is very new information to me, I will approve when @kwwall says go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this!
@caffeine-rohit do you have PoC of this exploit somewhere? I am wondering if COOP https://web.dev/articles/security-headers#coop and CORP https://web.dev/articles/security-headers#corp can help to mitigate that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, 2 things here that concern me:
- Why this image. I don't see anywhere in "cheatsheets/Clickjacking_Defense_Cheat_Sheet.md", that it's being referenced. (I've searched for the file name and 'img'.) It it is not referenced in this Clickjacking Cheat Sheet, why is it ever here?
- This image is straight out of Paulos Yibelo's blog (although there, it is a PNG file, rather than a WebP format; so apparently the format was converted.) If you did intend to use it, at a minimum, we at least need to acknowledge it as Yibelo's creation. And ideally, we ought to permission to use unless it unless it is clearly covered by some FOSS license that allows us to freely use it. While I don't believe you had malicious intentions, we certainly don't want to risk OWASP getting sued.
|
||
## Introduction | ||
|
||
This cheat sheet is intended to provide guidance for developers on how to defend against [Clickjacking](https://owasp.org/www-community/attacks/Clickjacking), also known as UI redress attacks. | ||
This cheat sheet is intended to provide guidance for developers on how to defend against [Clickjacking](https://owasp.org/www-community/attacks/Clickjacking) (also known as UI redress attacks) and [Double Clickjacking](https://www.paulosyibelo.com/2024/12/doubleclickjacking-what.html?m=1) (also called forced multi-click exploitation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Much better.
```javascript | ||
const observer = new IntersectionObserver(entries => { | ||
entries.forEach(entry => { | ||
if (!entry.isIntersecting) { | ||
alert("Warning: A hidden iframe may be attempting a Clickjacking attack!"); | ||
} | ||
}); | ||
}, { threshold: 0.5 }); | ||
|
||
document.querySelectorAll("iframe").forEach(iframe => observer.observe(iframe)); | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmanico - I am not enough of a JavaScript expert to be able to discern if this is an effective defense against Double Clickjacking or not. Could you please take a look at this (lines 357-367) and give us your thoughts regarding it's effectiveness? It is very different than Yibelo's client-side JS defense. And I am a little concerned that this may have false positives (the assumption seemingly being it is rare for multiple entities to overlap) and also it may be a time-consuming defense if there are a lot of entities on a page, given that you'd need to check this on every page.
```javascript | ||
if (document.referrer && !document.referrer.includes("your-website.com")) { | ||
alert("Warning: Clicks from an untrusted source detected!"); | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things:
- I'm not sure any website really should be trusted. Or at least there's a lot of cases not to do that. There are insider attackers. Also, this doesn't fit well into an "assume breach" mentality that many security conscious companies are now taking. (Also, we we're only concerned about cross-site requests SameSite cookies, CORS headers, etc. probably would be sufficient, shouldn't they?)
- I'm not confident this would work. I mean false positives would surely seem to be an issue, wouldn't it?
At a bare minimum, I will wait until @jmanico looks at this, but if it is valid, it probably needs a better explanation.
```javascript | ||
let lastClickTime = 0; | ||
document.getElementById("submitButton").addEventListener("click", function (event) { | ||
const now = Date.now(); | ||
if (now - lastClickTime < 500) { // 500ms delay | ||
event.preventDefault(); | ||
alert("Double-click detected! Please click only once."); | ||
} else { | ||
lastClickTime = now; | ||
// Proceed with the action | ||
} | ||
}); | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems feasible. Have you tried this on Yibelo's PoC on his blog post?
What do you think @jmanico ?
```javascript | ||
function confirmAction() { | ||
let confirmation = confirm("Are you sure you want to proceed?"); | ||
if (confirmation) { | ||
// Perform action | ||
} | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Um, TBH, I'm beginning to wonder if you are missing the point about how Clickjacking iis exploited in general. The targeted system is just there to entice the user to click or to garner the user's trust. The way this "UI redress" works is the attack places something dangerous (e.g., a site that might download malware) in a transparent overlay over something that the user generally thinks would be safe to click on (the underlying iframe of the targeted system). That overlaid system may or may not overlay "sensitive data". And you surely cannot do this on every page. You couldn't even use it on something like a financial or health insurance site, because there pretty much everything is sensitive on almost every page. Thus using something the above on such a site would be very intrusive. If it did work (and do you really think the average user is going to do a 'view source' or open the browser's web development tools and try to figure it out?), the defense needs to be unobtrusive and not require any special intelligence from the end user.
**Concept:** Instead of a simple click, users must perform a deliberate gesture, like dragging a slider, before confirming actions. | ||
|
||
```html | ||
<label for="secureAction">Slide to confirm:</label> | ||
<input type="range" id="secureAction" min="0" max="100" oninput="checkGesture(this.value)"> | ||
<script> | ||
function checkGesture(value) { | ||
if (value == 100) { | ||
alert("Confirmed! Action executed."); | ||
} | ||
} | ||
</script> | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seriously, a slider on every page? Really? Because prior to X-FRAME-OPTIONS and CSP frame-ancestors, we had to put the anti-frame busting JavaScript on every bloody page, so I don't see why this would any different. You can't do something that is going to require an explicit action every time a page loads.
✅ Users must actively confirm before execution. | ||
|
||
✅ Prevents forced clicks leading to unintended actions. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, what is so wrong about dropping Yibelo's suggested client-side defense here?
(function(){
if (window.matchMedia && window.matchMedia("(hover: hover)").matches) {
var buttons = document.querySelectorAll('form button, form input[type="submit"]');
buttons.forEach(button => button.disabled = true);
function enableButtons() {
buttons.forEach(button => button.disabled = false);
}
document.addEventListener("mousemove", enableButtons);
document.addEventListener("keydown", e => {
if(e.key === "Tab") enableButtons();
});
}
})();
Presumably that at least is effective against his PoC. He'd lose a lot of credibility if it wasn't? At a bear minimum, I think we should include it.
Also, is there any reason why the standard anti-frame busting script that most companies use (and it probably still mentioned for standard clickjacking in this Cheat Sheet, although no longer as a preferred mechanism), not effective against double-clickjacking as well? Based on the small part that I understand about it, I think that it would.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some of these proposed solutions that require intrusive user behavior should be removed and replace by Yibelo's client-side suggestion.
Furthermore, until @jmanico looks at this and provides his feedback, I am not approving this even if those changes are made. This is complicated and requires an AppSec person with much more expertise in JavaScript than I have.
@mackowski wrote:
See Yibelo's Poc at https://www.paulosyibelo.com/2024/12/doubleclickjacking-what.html?m=1. Search for "Proof of Concept (PoC) Code" there. I don't think that either COOP or CORP will be 100% effective as clickjacking attacks do not necessarily need to be "cross-site" even though they generally are. If one of those headers work, the more common SameSite: Lax/Strict cookies approach would probably suffice. Just my opinion. I'm waiting to hear from @jmanico on this. |
@jmanico can you also review this? |
Jim, you need not "approve" this PR, but I was hoping that perhaps that you could at least provide some specific feedback, especially regarding to the JavaScript snippets. In the meantime, please ask any expert that you know to take a look at this. I will also reach out to @jeremylong and see if he's willing to give this a look. If I could find any contact info for Paulos Yibelo, I'd reach out to him and invite his feedback. (Although I did find a promising lead, so will try it.) |
This will likely cause a lot of false alarms. There are legit reasons for an iFrame to be off screen, like when a user scrolls away. |
@jmanico , is it possible to redeem this JS by enhancing it to check if the intersecting iframe is transparent (or almost transparent)? However, even that is possible, this seems like a relatively expensive check that could be abused as an intentional DoS attack, so maybe not. Like I said, I'm not a JS expert and neither did I stay at a Holiday Inn Express last night. |
@jmanico @kwwall Thanks for your input! The original approach was meant to detect hidden iframes but, as noted, it could indeed produce false positives. A potential enhancement could be to check not only visibility but also opacity (getComputedStyle(iframe).opacity) and display properties to determine whether an iframe is intentionally hidden rather than just off-screen due to natural scrolling. However, adding transparency detection might introduce performance overhead, as mentioned. Regarding DoS concerns, continuous checks on multiple iframes could be optimized with debouncing or rate limiting to prevent abuse. Do you have suggestions on how to improve efficiency while maintaining security? |
To refine this approach, we can:
Would these refinements address the concerns? |
I managed to connect with Paulos Yibelo via LinkedIn and asked him if he could take a look at this PR and maybe comment on it. He said he is unable to respond at the moment, but will see if he can follow up in a few weeks. However, I did ask if I could pass along his comments and he agreed. Note that I have made some minimal edits to protect his privacy:
https://youtu.be/pnQQ7NQ1ZPA?si=Ywu-fl2pR5QKvZBo
|
Closes #1577 . I updated the Clickjacking cheat sheet by adding Double Clickjacking into it. I added all the necessary information about this issue and explained some good effective ways to prevent Double clickjacking .