Security Issue SA-CONTRIB-2012-006

I couldn't find a description of what the security issue actually is - had to Google.
Is this it? http://seclists.org/fulldisclosure/2011/Dec/413

The drupal security issue does not seem to explain what the issue is so it is of almost no help.

The full disclosure page does explain what the issue is, but looking at the supplied patch, it has problems.
The patch uses invalid drupal 6 functions (a typo perhaps?).
That is it uses, filter_admin_xss() and should be using filter_xss_admin().
The patch alters permissions without properly using the appropriate drupal functions (such as hook_perm()).
The original code improperly uses drupals form api and directly access $_POST to process the form data.
The direct access of the $_POST variable is probably the attack point.

I remade the patch with the following changes:

• Implemented hook_perm() and replaced 'access administration pages' with 'administer supercron'.
• Added all check_plain() and filter_xss_admin() calls as supplied in original patch.
• Added the valid ip preg_match() test calls as supplied in the original patch.
• Added an additional missed check_plain() check not found in patch.
• Replaced the $_POST uses with proper usage of the drupal forms api (aka: $form_state).

This patch is again supercron 1.3.
Please Review.

I am the module writer and was advised of this issue while away on December 26th, I e-mailed back to say that I would take care of the issue upon returning from an assignment & vacations (I do *not* commit code back from unsecured locations). I got back to a module marked as "abandoned" (how is "I will take care of this" a statement of abandonment I do not know), no way to post corrections, and other circumstances that are too upsetting to mention.

In the meantime, thanks to everyone who jumped in to constructively help; this is what the Drupal community is really about.

I have looked at the proposed patch as well. There is according to the description of the issue an injection risk with the insertion of IP addresses in the database (which would not affect the generated SQL which is sanitized).

The attack scenarios are:

1. a user abuses a site that allows users to post full unsanitized HTML in nodes or comments;

2a. the administrator comes upon this code or
2b. has an otherwise compromised browser, although that scenario would also allow the compromised credentials to be taken away from that careless admin, which is a far more interesting attack surface as it blows the whole site open to assault.

3. The injected code would then post back data to the list that manages which IPs (with allowances for domain names in future versions) can and cannot issue cron calls to the site; this may include code that would also execute arbitrary JavaScript code (just like the code that first executed out of the full-HTML nodes and comments).

In other words, if you have an easy way to execute code out of nodes and comments (which one would venture are visited more often than a list of IPs to accept and refuse Cron calls from), then you also have a second means of executing that very same code through a less-trafficked section of the site. The net effect is not so much the second attack surface, but the idea that a thorough clean-up of a site so configured and under attack will require additional thoroughness to clean-up than just cleaning up the nodes and comments; to this I agree.

I think a better approach would have been to put a security warning to the effect that SuperCron, until patched, should not be used on sites that allow unsanitized HTML in their nodes and/or comments. Even when SuperCron is patched, the same situation will allow for perfectly legitimate XHR calls across all aspects of a site's administration, using code that is fully compliant, still putting the very same sites at far more extensive risk, even in the absence of this (or any) vulnerability.

I am not trying to deflect responsibility here -- yes, there is a bug -- but I question the heavy-handedness of the response when a more informative one would have been sufficient.

I like the patch supplied by thekevinday, my only change would be to make it IPv6 compliant.

To the security team:

I want access to my module back, and that mention taken out of the main page and replaced by a statement of work in progress. I believe my response to the e-mails that I have received may have justified the code not being made available until patched, but would hardly justify a statement of module abandonment and an invitation to random coders to claim ownership as they see fit; SuperCron has been a huge investment for this small shop, one we are proud of, and one which deserves better than what it got.

I'd like to address two areas.

* The claim that the module was unfairly marked abandoned
* The difficulty of the attack scenario

The actual process to mark the project abandoned

I am the module writer and was advised of this issue while away on December 26th,

You were first notified of the issue on December 8th. I then mailed you on December 26th alerting you that we hoped to make a release January 4th. You responded only by saying: "Got it; will handle. Thanks." Compare that exchange with this claim:

I e-mailed back to say that I would take care of the issue upon returning from an assignment & vacations (I do *not* commit code back from unsecured locations).

You did not mention assignment nor vacations. You claimed understanding of my email which included a stated desire to release the fix on January 4th.

I got back to a module marked as "abandoned" (how is "I will take care of this" a statement of abandonment I do not know), no way to post corrections, and other circumstances that are too upsetting to mention.

The indication of abandonment is that you said you would do something by January 4th and then did not. I e-mailed you again on January 2nd with instructions on how to fix the problem and heard nothing in response by January 11th. This led me to believe that your mail of December 27th was not genuine and that you were no longer interested in the module.

Stephane Corlosquet, of the security team who originally mailed you on December 8th, then mailed you again on January 5th. He received no reply. At that point it had been a month from the initial contact and while you had responded you had made no reliable indication of when you would fix the problem nor further response to our inquiries. So, we followed the process on http://drupal.org/node/101497 (a resource you should have seen if you accessed the issue as sent to you in Stephane's mail of December 8th) which says: "If we have not received a reply within one month after contacting you, we will publish an advisory urging users to uninstall the affected module. The relevant project will be unpublished."

SuperCron has been a huge investment for this small shop, one we are proud of, and one which deserves better than what it got.

The security team spent hours supporting your module between December 8th and January 11th (the window you knew of the problem) making sure that the safety of site-builders was balanced with the need to respect contributors to drupal.org. With limited support from our employers, we agree to support almost 16,000 projects on drupal.org in-spite of how easy it is (or isn't) to work with the module maintainers. While your contribution is huge and important, it is but one huge and important contribution in this community. Your perspective (needing your project page to sell projects) is but one competing need including the need of site builders to be aware, in a timely manner, of security issues in modules that are installed on their sites.

The security issue and how to exploit it

I have looked at the proposed patch as well. There is according to the description of the issue an injection risk with the insertion of IP addresses in the database (which would not affect the generated SQL which is sanitized).

The issue is not about SQL, but Cross Site Scripting which is an output issue. The description on http://seclists.org/fulldisclosure/2011/Dec/413 is the same as the one you were given access to on security.drupal.org. As this document explains Drupal filters on output.

The solution suggested by theKevinDay addresses some non-security related problems with Justin's patch, but Justin's description of the problem is perfectly accurate.

I look forward to your fast review/test/commit of theKevinDay's patch at which point you can:
* Create a git tag for the release
* Create a release node marked as a "security update"
* Contact the security team so we can publish the module and re-grant you edit permissions on the project node

Greggles: Your e-mail was the first one of which I was aware of on the issue; we have a dashboard here that should first be used for communications. In fact, it is the recommended form of communication when asking to take over a module so that a trace can be made of the request. I respectfully submit that using e-mail (we all get hundreds of Drupal-related e-mails every day) is not the proper method nor the community-favored method either; e-mail gets lost, goes to spam, gets muddled, etc. The non-noisy channel should be favoured, and on drupal.org it is interpersonal messaging as visible in the dashboard.

So as far as I am concerned, I learned of the issue on December 26th, and responded on the 27th. Your e-mail stated:

"I'm not sure if you've seen this issue. Please give it your immediate
attention as it has been disclosed publicly." followed by your wish to have a release out by the 4th (which I should have been more clear on the fact that I wasn't sure we could make that deadline, but the way it was phrased sounded more like an invitation than a line in the sand)

I love SuperCron, it is my baby, we've put a lot of time and money behind it, and clearly want to do so (anybody who has been on the alpha list for SuperCron 2.0 would know this immediately -- this thing is awesome and is obviously the labour of love). So read what follows in that context:

If timings do not work or responses are not quick enough for your expectations, then there are better solutions, as will be discussed in the security forum. One of them would be to say: "The risk is known, we need to unpublish the releases until the issue gets fixed, is that all right ?" which is obviously acceptable to anyone who cares about their work and users. If an argument ensues, then one may argue that the person is somehow derelict in their handling of the code. Marking the module as abandoned however presumes an intent to abandon, a dereliction, lightheartedness, an uncaring attitude, etc. (using thesaurus.com here), which sends a wholly different signal that is much broader than "there is an issue with security, we've unpublished the code, anyone who wants to send a patch should post it, the module will be made available again when the issue has been fixed" which deals objectively with the issue at hand, not with an external unvalidated judgment as to the intention of the author, painting them and their work with far too wide a brush.

The result on users and code is the same: users are protected, appropriately warned, and invited to collaborate. The impact on the contrib author though is wholly different: they are part of the process, not the object of judgment and ostracism.

I will deal with the other issues later on.

I have posted on the larger issue here:

http://groups.drupal.org/node/206033#comment-678638

we have a dashboard here that should first be used for communications.

That would be great. However, security issues - by their very nature - cannot be discussed in public until there is a fix prepared for them. If you review the policies followed by nearly every major software project/vendor they all keep security issues private until a fix has been created. If you want to debate this point please provide examples of software projects who discuss security issues in public before a fix is available.

Even if the December 26th email was the only one you received (which means that 3 out of 4 emails somehow got lost) the result is almost the same:
* You claimed you replied that you were on vacation and would attend when you got back, you did not
* You claimed you were not informed of the 1 month timeline but you were (if your response that you "got it" was honest)

I don't see how using the built-in communication system that we have here to privately get in touch with people using private messages from the Your Dashboard tab somehow makes security issues public; is it a private communications system or not ? Does anyone besides myself see messages sent to me via the dashboard ?

In any case with the issue at hand the vulnerability was first divulged publicly outside of the usual Drupal Security SA channels so the point is a red herring.

As to whether my responses are honest or not, I find that you are hugely overstepping the decency boundaries by even implying otherwise; please retract yourself at once.

Can you post a screenshot of the "private messages" feature on drupal.org? I don't think it exists, or if it does exist then it doesn't behave the way you think it does (i.e. there is no system on drupal.org for sending private messages that are stored within the site - the closest thing is the core contact.module which sends emails to users).

I feel confident in 100% of what I have said, so if you want me to retract something you'll need to state what I've said that is dishonest or indecent.

I've been here nearly 7 years. If PrivateMsg is installed, it's very, very well hidden. :) The only communication feature I'm aware of is the Contact module, as greggles mentioned, which just sends an email.

Michelle

This is a duplicate of the thread where I discussed this; it is easy to make a filter in gMail that says "star this if it comes as a primate e-mail sent from "drupal.org", not so much if it says "watch for all e-mail discussions about Drupal in case it might be about you", which it never is in my case as a lowly contrib author.

Greggles: I want to state unequivocally that every single interaction i have had thus far in this matter has been patronizing and antagonizing to the extreme; here you are stating "if your response that you "got it" was honest". This is the comment that I am obviously referring to. If it was honest ? Who are you to evaluate, and state from a high position of authority on my issue thread, my honesty ? This crosses the line. I do not know in which world you live where you can disempower, criticize, bully and insult contributors, but it is not the respectful world in which I choose to live and would invite you to communicate with the same level of courtesy and respect that you expect from the rest of the community. I will take this up and bring it out as an example of how NOT to deal with volunteers and contributors.

I haven't been privy to all of the conversation but, based on what is in this issue, I don't see where Greg is saying anything that crosses the line. He didn't call you a lier; he said if a is true then b can't be. Simple statement of fact.

Michelle

I just wanted to follow up to say that 63reasons has contacted me to talk privately and I've accepted that request. I suggested we get a 3rd person involved as a facilitator, so if anyone knowledgable of this issue and familiar with conflict resolution is interested please contact us. None of that is a blocker to this issue, of course, so hopefully the discussion here can re-focus on the technical issue in the module.

My thanks @63reasons for suggesting that idea and thanks @michelle for your perspective.

Yes, I think it's more important to focus on the technical issues (which are a bit hard for me to find amid the confusion).

If you all have it handled, great. Otherwise I'd be glad to help / help with a 7.x version of the module. Let me know / "send me a PrivMsg" or find me on IRC. :P

Thanks all.

Did I miss something while reading the comments?

We have a bug, the maintainer is back, a patch has been made available by thekevinday (thank you!) but the state of the project is still "module is abandoned"?!

What went wrong here, besides a good example of bad communication and when will 63reasons get access to his project to deploy a patched version?

Thanks all!

@adrupalguy, 63reasons has always had access to dothe necessary things to get it marked not abandoned. I outlined these at the bottom of comment 5

* review/commit a fix
* Create a git tag for the release
* Create a release node marked as a "security update"

@greggles
Thanks, I indeed missed that one.

@63reasons
Let me know if I can help create that security update.

Ditto #18.