Referring to security advisory SA-CONTRIB-2014-072 - Freelinking, Freelinking Case Tracker - Access bypass:

I am working with the security team on a patch for the 7.x-3.x branch to resolve the problem.

Please test and review the patch and report the results from your tests under this issue.

In order to review the patch, you need to do the following:

Clone the HEAD of the 7.x-3.x branch of the project:

git clone --branch 7.x-3.x http://git.drupal.org/project/freelinking.git
cd freelinking

Download and save the patch attached to comment #1 below in the freelinking directory.

git apply -v  freelinking-SA-CONTRIB-2014-072-2313767-9.patch

Copy the freelinking directory, including all its subdirectories, into directory where you keep contributed modules (e.g. sites/all/modules/) on your testing site, and enable the module.

Important: If you're already running freelinking, remember to run the database update (available from the Administration » Reports » Status report page).

After installing the patch, the following problems should be repaired:

  • Freelinks to node subject to access control should not be enabled for users without the right to access the node.
  • Freelinks to users should not be enabled for users without the right to access user profiles.

Your help with reviewing the patch will help to get the project back from Unsupported status.

CommentFileSizeAuthor
#16 freelinking-SA-CONTRIB-2014-072-2313767-16-D7.patch15.66 KBgisle
#16 freelinking-SA-CONTRIB-2014-072-2313767-16-D6.patch11.42 KBgisle
#15 freelinking-SA-CONTRIB-2014-072-2313767-15-D6.patch10.46 KBmradcliffe
#15 freelinking-SA-CONTRIB-2014-072-2313767-15.patch16.54 KBmradcliffe
#11 freelinking-SA-CONTRIB-2014-072-2313767-11-D6.patch48.69 KBgisle
#9 freelinking-SA-CONTRIB-2014-072-2313767-9.patch31.37 KBgisle
#1 freelinking-SA-CONTRIB-2014-072-2313767-1.git_.patch25.71 KBgisle

Comments

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented
StatusFileSize
new freelinking-SA-CONTRIB-2014-072-2313767-1.git_.patch25.71 KB
gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented
Issue summary: View changes
vijaycs85’s picture

vijaycs85
Location London, UK
commented
Status: Active » Needs review

Let's review it.

vijaycs85’s picture

vijaycs85
Location London, UK
commented

Not very sure all these changes necessary for the security.

  1. +++ /dev/null
--- a/README.txt
+++ b/README.txt

    Looks like most of the changes here are not related to the security update. Happy to open a followup to for cleanup 

  2. +++ b/freelinking.module
@@ -158,7 +159,7 @@ function freelinking_freelinking() {
+      drupal_get_path('module', 'freelinking') . '/plugins/', '/.inc$/');

    $?

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented
  1. The changes to README.txt are there to make it compliant with the module documentation guidelines, including "The file should be formatted to hard-wrap at 80 characters." (original emphasis)
  2. A dollar sign ($) at the end of an regexp means: "The pattern ends here". The change to the regexp identifying plugins is to stop it pulling in backup files (automatically created by editors such as emacs, vi, etc., such as plugin.inc~, plugin.inc-, plugin.inc.BAK) as valid plugins. Without this change to the regexp, it keeps crashing after any plugin edit if you forget to delete these backups, because any backup file will be parsed along with the original, producing double definitions.

I can post a new patch without these changes if they bother you, but I hope it is possible to get this reviewed without undoing this.

asb’s picture

asb commented

Thanks for the patch. The instructions from above are easy to follow and the patch applies cleanly.

However, the module dos not process Wikipedia-style links as documented in README.txt ("…if you do not specity a plugin name the default one configured at Freelinking Settings page will be used").

Rendered HTML looks like this with Freelinking enabled:

…manufactured by <a href="/w/index.php?title=Creation&amp;action=edit&amp;redlink=1" class="new" title="Creation (page does not exist)">Creation</a>.</p>

The rendered HTML looks identical with 'Freelinking' disabled, so in my use case it simply does nothing.

My filter processing order at ./admin/config/content/formats/mediawiki:

  1. MediaWiki API
  2. Freelinking
  3. Footnotes

MediaWiki API needs to be first because it processes Wikitags, but claims not to handle links (unlike 'mediawiki_filter'). 'Freelinking' is recommended to process links (cf. #2197879: incorrect links), but it's not unlikely that the problem lies in the implementation of MediaWiki API. Bottomline is that I can not report anything useful about the patch, sorry.

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented

@asb, the output you show comes from MediaWiki API, not from freelinking.

Both module uses the same syntax for links (double square brackets). When the MediaWiki API filter is given a higher priority, it converts the double square brackets to the rendered output you observe, and there is nothing for freelinking to process.

Btw, the patch does not interfere with this. This incompatibility with MediaWiki API has always been there.

If you want to use freelinking to process links, you must first disable MediaWiki API.

asb’s picture

asb commented

The funny part of this is that 'MediaWiki API' requires 'Freelinking' to process links ;-)

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented
Issue summary: View changes
StatusFileSize
new freelinking-SA-CONTRIB-2014-072-2313767-9.patch31.37 KB

Here is a new version of a patch to resolve the current security issue.

The difference between this and the previous patch is that the previous version turned off caching for any text type that had the freelinking filter enabled.

The new version will allow you to enable caching, provided your site has no node access control module enabled.

Refer to the summary for instructions for how to test.

Please test and write a review in this issue queue.

mlhess’s picture

mlhess commented

@gisle What about a Drupal 6 patch? The parent issue: https://security.drupal.org/node/76508, has one on it that might be able to use.

Can you post it here after confirming it fixes the issue?

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented
StatusFileSize
new freelinking-SA-CONTRIB-2014-072-2313767-11-D6.patch48.69 KB

Here is a patch for the 6.x-3.x branch.

It is based upon the patch supplied by xurizaemon in https://security.drupal.org/comment/70483#comment-70483, but I've tested it and confirmed that it resolves the access bypass issue for this branch.

It works by turning of caching for any text format that uses the freelinking filter.

Please review this patch as well.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Status: Needs review » Needs work 
  1.  delete mode 100644 CHANGELOG.txt
...
 delete mode 100644 plugins/freelinking_oneinchframe.inc
 delete mode 100644 plugins/freelinking_path.inc

    Why are these files removed? Is this necessary for security? Wouldn't the users who use oneinchframe or path be discouraged that these features were removed? Are there additional security issues with these features? 

  2. +++ b/freelinking.module
@@ -81,7 +81,9 @@ function freelinking_filter($op, $delta = 0, $format = -1,
+            //dpm($freelinking, 'fl');

    Debug code cruft. 

  3. +++ b/plugins/freelinking_file.inc
@@ -29,11 +29,21 @@ function freelinking_file_file_callback($target, $plugin) {
-  if ($target['text']) {

    Is this related to the CVE on Drupal for file_download? Should we comment that?

I would probably use the patch as is, but I'm also confused as to the additional changes. I would think a security patch to fix it would suffice, and then a follow-up when taking ownership of the module to clean up the code standards.

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented
Status: Needs work » Needs review

1. Why are these files removed? Is this necessary for security? Wouldn't the users who use oneinchframe or path be discouraged that these features were removed? Are there additional security issues with these features?

  • CHANGELOG.txt was obviously outdated (as is easy to see if you compare it to the history recorded by git). Since we no longer require repos to have a CHANGELOG.txt and IMHO maintaining one "by hand" doesn't make much sense when we use git, I removed it instead of trying to update it.
  • freelinking_oneinchframe.inc generates freelinks to http://oif.eafarris.com/wiki, which is a wiki-page in a non-existing domain. It didn't make sense to keep it.
  • freelinking_path.inc has very obvious security problems (There is even a comment that says: "This plugin exposes page titles without regard for security."), and what it does is (IMHO) of very little use, so I removed it instead of trying to fix it. If somebody really requests it, I may try to fix it and bring it back.

2. Debug code cruft.

Agreed. Since these are all commented out, I don't think this warrants a new patch. If I get custody of this project, I'll make sure they're removed in the next release.

3. Is this related to the CVE on Drupal for file_download? Should we comment that?

If you look at the code block inserted just above the diff you cite, you'll see that it uses module_invoke_all to invoke hook_file_download to allow modules to enforce permissions on file downloads when the private file download method is selected.

The code is IMHO self-explanatory, so I am not sure if this needs a comment, but I'll add one in the next release if you think that this is too oblique.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work

The patch is too large for a security review, IMO.

Please post a patch that only deals with the security issue(s), you can perform all other changes afterwards.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new freelinking-SA-CONTRIB-2014-072-2313767-15.patch16.54 KB
new freelinking-SA-CONTRIB-2014-072-2313767-15-D6.patch10.46 KB

Here are some slimmed down patches.

- If a file had some standards corrections, I left those changes intact from the previous patches.
- Removed file deletions from patch.
- Removed files that only contained standards corrections from patch.
- Removed dpm() from Drupal 6 patch.

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented
StatusFileSize
new freelinking-SA-CONTRIB-2014-072-2313767-16-D6.patch11.42 KB
new freelinking-SA-CONTRIB-2014-072-2313767-16-D7.patch15.66 KB

Here are new set of patches for both the D6 and D7 versions with the minimum number of changes required to fix the security issue.

Bugs are left unfixed, and code standards are ignored (as they are in the original).

Please note that the plugin named freelinking_path.inc is removed from the repo. That plugin has multiple security problems and must be discontinued.

matt v.’s picture

matt v. commented

@gisle,

I tested and reviewed the new patches in comment #16 above. Both patches appeared to fix the issues, after running "drush updb".

Related to mradcliffe's question 3 in comment #12 above, it's a unclear to me how the changes to plugins/freelinking_file.inc relate to the original Access bypass issue and therefore why those changes are still included in this set of patches. Other than that, the patches look good to me.

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented

@Matt V.
the original Access bypass issue (https://security.drupal.org/node/76508 - restricted login) did not mention files (only links to nodes or users) or plugins/freelinking_file.inc, but the problem with access to files was introduced later in that issue (around comment #13) IIRC.

The patches to plugins/freelinking_file.inc is required to make the project secure. The problem is specific to this plugin, and not related to the CVE on Drupal for file_download.

matt v.’s picture

matt v. commented
Status: Needs review » Reviewed & tested by the community

@gisle,

Okay, I see the change introduced in that comment/patch now. And I understand the need for it. I just couldn't find it referenced in the comments.

I'm going ahead and marking this RTBC.

  • gisle committed a255145 on 6.x-3.x 
    #2313767 by gisle: Fixing SA-CONTRIB-2014-072 v2.

  • gisle committed ca92706 on 7.x-3.x 
    #2313767 by gisle: Fixing SA-CONTRIB-2014-072 v2.
mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented

Awesome work, gisle!

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented

It must be said that the credit for fixing this should go to xurizaemon and juampy. My part has been to steer the result of their diligent work through the security team's procedure for restoring the project's supported status.

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented
Version: » 7.x-3.3
Status: Reviewed & tested by the community » Fixed

Committed to the repo.

Status: Fixed » Closed (fixed)

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