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

Prevent clickjacking #12

Merged
merged 2 commits into from
May 16, 2022
Merged

Prevent clickjacking #12

merged 2 commits into from
May 16, 2022

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented May 13, 2022

When the page is loaded in a sub-frame, we now display a condensed warning and a link to open the page in a new tab. It is no longer possible to add a site to the safelist from an iframe. This ensures that clickjacking cannot be used to add a site to the safelist without a users knowledge or consent.

The technique used was adapted from the OWASP clickjacking defense cheat sheet. This technique is recommended for legacy browsers that do not support security headers. We are using this technique instead of security headers because GitHub Pages does not allow using security headers.

The consequence of failure for this defense is low; all it would allow is bypassing our phishing warning. The page used to bypass the warning would itself remain susceptible to blocking. As such, it's unclear what advantage such a bypass would have over moving to a new domain that is not yet blocked. This low consequence of failure is the reason I am recommending the use of this "legacy browser" technique, rather than moving to an alternate hosting platform that supports modern security headers. This technique appears to work in all browsers, and that's good enough for now. We can move to an alternative hosting platform that supports security headers at a later date if necessary.

@Gudahtt Gudahtt requested a review from a team as a code owner May 13, 2022 13:14
@Gudahtt Gudahtt force-pushed the prevent-clickjacking branch from e687020 to 4be6bef Compare May 13, 2022 13:18
When the page is loaded in a sub-frame, we now display a condensed
warning and a link to open the page in a new tab. It is no longer
possible to add a site to the safelist from an iframe. This ensures
that clickjacking cannot be used to add a site to the safelist without
a users knowledge or consent.

The technique used was adapted from the OWASP clickjacking defense
cheat sheet [1]. This technique is recommended for legacy browsers that
do not support security headers. We are using this technique instead of
security headers because GitHub Pages does not allow using security
headers.

The consequence of failure for this defense is low; all it would allow
is bypassing our phishing warning. The page used to bypass the warning
would itself remain susceptible to blocking. As such, it's unclear what
advantage such a bypass would have over moving to a new domain that is
not yet blocked. This low consequence of failure is the reason I am
recommending the use of this "legacy browser" technique, rather than
moving to an alternate hosting platform that supports modern security
headers. This technique appears to work in all browsers, and that's
good enough for now. We can move to an alternative hosting platform
that supports security headers at a later date if necessary.

[1]: https://cheatsheetseries.owasp.org/cheatsheets/Clickjacking_Defense_Cheat_Sheet.html#best-for-now-legacy-browser-frame-breaking-script
@Gudahtt Gudahtt force-pushed the prevent-clickjacking branch from 4be6bef to 27947d8 Compare May 13, 2022 13:20
This ensures that if JavaScript is disabled, we default to the subframe
view. If JavaScript is disabled, it usually indicates that the page is
in a subframe. This page can't work correctly without JavaScript
anyway.
@Gudahtt Gudahtt force-pushed the prevent-clickjacking branch from 999169d to 0e02d1d Compare May 13, 2022 16:46
Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, based on the linked clickjacking cheatsheet

@Gudahtt Gudahtt merged commit b33da36 into main May 16, 2022
@Gudahtt Gudahtt deleted the prevent-clickjacking branch May 16, 2022 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants