CVS edit link for p0deje

Hello,

I've decided to develop SafeClick module few weeks after the discussion with Drupal Security Team about possible vulnerabilities to Clickjacking attacks. Security Team replied that protection of this class of attacks won't be implemented in core, so I've decided to develop a special module for it. More information about Clickjacking can be found at http://www.sectheory.com/clickjacking.htm.

Currently, there is no similar module available for Drupal or any other CMS. Development of this module provides Drupal with new level of security, which is not comparable to any other CMS. Number of clickjacking attacks increases - Bikini worm at Facebook and huge number of attacks on Twitter, so this module is definitely needed for many websites. High level of attention to Drupal as a governmental site (like www.whitehouse.gov), a social network with Twitter integration - all of this should get the best of secure practice.

This module implements several techniques of Clickjacking prevention, which were discussed long at http://sla.ckers.org with specialists in Web Applications Security sphere. I think they got to be reviewed by Drupal Security Team, and if they have any questions, I'll certainly explain everything.

The first technique is implementation of X-Frame-Options HTTP header. This header defines, how browser should process framing of website. It has two options: SAMEORIGIN, when browser accepts framing within website domain and DENY, when browser rejects any attempt of framing. This header is currently supported by NoScript, Safari, Chrome and IE8, so it's the best way to prevent Clickjacking.

The second technique is an implementation of Javascript and CSS hack. It looks like this:

if (top === self) { document.write(" body { display: none !important; } So if site is not being framed, javascript comments style and everything is okay. If site is being framed, javascript isn't executed and body is not shown. It's the best way to prevent Clickjacking, because usage of simple framebuster is not good as long as it can be disabled via XSS filter in IE8 and Safari. The third option is the addition of tag with message, pointing user to enable JavaScript. This thing should be used with the second technique, because it's a bad idea to show blank screen for users with disabled JavaScript. However, such option may be used alone. The fourth technique is to override stylesheet for iframe, frame, object and embed selectors. It's going to be useful if users are allowed to post stated tags at website. Most Clickjacking vectors use transparent frames and this stylesheet overrides opacity of them to 100. Rare Clickjacking vectors exploits "last loaded - first focused" behavior, when last loaded iframe is being focused, regardless its visibility. Stylesheet sets z-index for selectors to 1, so last loaded frame will be always shown to user. In theory, the second and the fourth technique may break behavior of certain contributed modules, but I didn't found such. Best regards, Alex Rodionov p0deje@gmail.com
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

p0deje’s picture

FileSize
6.18 KB
p0deje’s picture

Status: Postponed (maintainer needs more info) » Needs review
apaderno’s picture

Issue tags: +Module review

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review your code.

p0deje’s picture

FileSize
6.23 KB

I've made a mistake which noticed after uploading module.
So here is the fixed version.
Please, ignore previous one

apaderno’s picture

Status: Needs review » Needs work
  1. See http://drupal.org/coding-standards for how a module should be written; in particular see the part about the namespace respect.
  2.   /**
       * Implementation of JavaScript and CSS technique.
       */
    

    Such comment should be used for hooks implementations, not for all the functions.

  3. /**
     * Clickjane.css is a user stylesheet to help in revealing Clickjacking attacks.
     * Originally developed by Meitar Moscovitz and distributed under the terms of GNU GPLv3
     */
    

    Files that are committed in CVS repository must be licensed under GPL v2.

p0deje’s picture

FileSize
6.23 KB

1. Didn't found such paragraph in Coding Standards. Can you point me?
2. Fixed
3. On http://drupal.org/node/59 it says

Your work must be licensed under the same license as Drupal, which is GPL version 2 or later.

p0deje’s picture

Status: Needs work » Needs review
aaron’s picture

I thought that it had to be >= GPL v2, as GPL v2 is compatible with all GPL after?

apaderno’s picture

Status: Needs review » Needs work

Please note that GPLv3 is not compatible with GPLv2 by itself. However, most software released under GPLv2 allows you to use the terms of later versions of the GPL as well. When this is the case, you can use the code under GPLv3 to make the desired combination. To learn more about compatibility between GNU licenses, please see our FAQ.

Licenses - Free Software Foundation

The part about the namespace is the one that tells you which names must have functions, constants, global variables, Drupal variables. That is important because allows each module to be compatible with each other.

apaderno’s picture

4: I want to release my work under GPL version 3 or under GPL version 2-only. Can I do so and host it on Drupal.org?
You can release your work under any GPL version 2 or later compatible license, however, you may only check it into Drupal's CVS repository if you are releasing it under the same license as Drupal itself, that is, GPL version 2 or later. If you are unable or unwilling to do so, do not check it into Drupal's CVS repository.

Licensing FAQ

p0deje’s picture

Status: Needs work » Needs review
FileSize
5.54 KB

1. License problems were resolved.
2. I've looked through code and it seems to me that all is written according to coding standards: functions are in lowercase with underscores, constants are in uppercase with underscores. What do I miss?

apaderno’s picture

Status: Needs review » Needs work
// Disable X-Frame-Options HTTP header
define('X_FRAME_DISABLE', 0);

// Set X-Frame-Options HTTP header to SAMEORIGIN
define('X_FRAME_SAMEORIGIN', 1);

// Set X-Frame-Options HTTP header to DENY
define('X_FRAME_DENY', 2);

Constants defined from the module should be prefixed with the module name (in upper case characters, in this case SAFECLICK_).

p0deje’s picture

Status: Needs work » Needs review
FileSize
5.56 KB

Constants were fixed

apaderno’s picture

Status: Needs review » Fixed
p0deje’s picture

Status: Fixed » Closed (fixed)
apaderno’s picture

Status: Closed (fixed) » Fixed

Thank you for your contribution! I am going to update your account.
These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.

Status: Fixed » Closed (fixed)
Issue tags: -Module review

Automatically closed -- issue fixed for 2 weeks with no activity.

apaderno’s picture

Component: Miscellaneous » new project application
apaderno’s picture

Assigned: Unassigned » apaderno
Status: Closed (fixed) » Fixed

I am giving credits to the users who participated in this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.