D6 fix of csrf security advisory SA-CONTRIB-2013-051 and discussion of the services D6 situation

Fixing some meetadata. Before marking it as "needs review" it should be a full patch.

Also I think it would make sense to patch 6.x-3.x.

Be sure to check your code, because you have disabled the caching.

So I am currently testing this patch after I have double checked the patch and the changes in the D7 version (commit )

So ill let you know when I'm done.

So for as far as I can see everything is working fine on my local development. The Patch & also the token request.

Im just adding some dummy code for maybe other people trying to figure it out etc.

Note this is only required for (POST, PUT, DELETE). BUT, GET trace ... are SAFE so you do need to request a CSRF token

// Note : be sure you are on the same domain otherwise you will have Cross Domain problems ;-)
$.get('http://LOCAL/services/session/token', function(data) {
   if (data) {
   // do your thing
   }
 });

And

$.ajax({
            type: "POST",
            url: "http://LOCAL-UPDATE.json,
            dataType: "json",
            contentType: "application/json",
            data: JSON.stringify({"url": "user_test.png"}),
            beforeSend: function (request) {
              request.setRequestHeader("X-CSRF-Token", xcrsftoken);
            },
            success: function(data){

So it is only required for these kinds POST, PUT, DELETE.

I think this is clear for everyone now the maintainer can add it ?

review by me , other reviews are welcome. Maybe this will increase the priority

The patch does not meet drupal's coding standards - looks like you may have used tabs instead of spaces.

Patch will break all tests as there are no changes to how services do non-safe requests during testing. Please do changes in tests as well plus add a test to ensure token functionality works as expected.

Tests need to be incorporated. Unfortunately the 6.x version of test has a few bad apples that I dont think ever got fixed.

This is why we need a d6 maintainer.

Since when tests are required for modules?

Im sorry, I just meant to reiterate what Yuri said, the tests will break unless you add tests to fix them.

I dont think its necessary, but its really helped with the development of Services making sure we dont break shit, and everything works as intended.

@NadimBak so the services module has tests (see the tests folder), these tests need to work otherwise the patch will not get committed.

And to be honest unless we get a maintainer for D6 I am of the view that support for the module should no longer exist and this patch should not even be committed to the code base, because the next time a security issue emerges the exact same things will happen. In reality services d6 is not supported as it stands.

@Erich Schulz : tested it and it looks ok, so far. And thanks for the effort

@marcingy : Let's just not mix up everything: tests are one things, the security issue is an other one. As kylebrowning already said: tests were already having problems since before AND they are not necessary. So why should they prevent from fixing this security issue? We can simply remove tests from this release until someone fixes it. I don't see why delaying more this security patch.
You are basically saying that if there is no D6 maintainer, we don't fix this problem NOW because... maybe in the future, we might have an other problem to solve!! Waw!!

And what do yo do with the thousands of d6 web sites using services and having a security breach publicly exposed?

So I disagree we need tests to prove at the very least that the patch works same as drupal 7 (I am also a maintainer for this module as is Yuriy). So while not all tests passed in D6 many did so all the tests that used to pass should pass still.

So step up and be a maintainer if you care that much :) And yes I am saying that because hey since the security issue raised its head it is amazing how many people seem willing to tell us how to do our job as maintainers but no one is willing to step upto the plate and ask to take over d6 maintainership. So that means the D6 version of the module is unsupported which means that it goes the same way as the d5 version of the module ie no patches etc. Harsh maybe but that is how it goes sorry.

Well as per the security advisory turn off the module, it is same as people still running drupal sites on old versions you accept the risk if you ignore security advisories.

well then sign me up as a maintainer and I'll commit the patch myself.

I have a D6 module based on services and I believe there some others with similar problems. I am very interested in a future for the D6 version and want to thank everybody who is helping on this. I am currently too busy to help testing but I know I have to help here to get some modules and customer D6 projects open. Currently the official support of D6 is not at the end and if this happens there will be some initiatives to extend security support. Maybe my LTS-Group is interesting for somebody: https://groups.drupal.org/drupal-lts

@marcingy: As I understand you are open to accept an additional maintainer if somebody wants to help. But it's not so easy to take the job. But this module including it's D6 version is so important that there are some people who want and who can do this. Currently I am not sure if I can do but when I am back from holidays in next month I will try if nobody else is found.

@marcingy: Yeah sure, I can be a maintainer for d6. Add the patch and remove tests so security team can review it. And if you care that much for tests in d6 why don't you fix it? :)

I haven't used d6 for over 3 years, hence why we need a D6 maintainer because none of the existing maintainers are willing to support D6 (sadly). And cool that we have a couple of offers to become a maintainer and no it isn't an easy job, but most of the time it is a fun task. Anyone that is interested drop into #drupal-services and say hi and we will get sorted out with commit rights asap.

From a quick look it appears that the folks offering to be a maintainer don't yet have much experience committing code to drupal.org (with the exception of C_Logemann). I suggest one interested person should create a sandbox project for services-d6, commit the current d6 code there and then focus the work on this patch and the security tests etc. into that branch. It will be a good way to demonstrate how the module might be maintained to give confidence to the current services maintainers on what to do next.

The problem that I have right now is that we have a supported 6.x release on the project page, and update status will tell you everything is alright with a yellow status. That totally is not the case, that's why we have update status: to indicate that there is a problem and users should be informed about it.

I don't care who assumes maintainership and whether they fix the tests or remove them. But they should be at least competent enough to post a new patch here that fixes the tab vs. spaces issue. greggles' idea with the sandbox is also nice, so please step up and keep the ball moving.

If there is no maintainer and no security release in 2 weeks then I'm going to consult with the security team and I will propose to unpublish all 6.x releases again.

I tentatively marked the 6.x-3.x branch as not supported which should make update status complain about it. I agree with klausi that it should be readily obvious that the branch is not in a good state.

I updated https://drupal.org/node/2018369 to add links and give more context to the comments there.

The 2 weeks from now seems like a fair cut off in my view. I have already expressed my concerns about a continuing d6 version on a number of threads given the lack of a maintainer. And no one has reached out as far as I know in the services channel.

I like the idea of #30 for 6.x branch to live in sandbox if someone would like to step in and maintain it there. After a while we can move to services repository itself.

@Erich Schulz - re #34 - most code editors have a mode to show whitespace characters. There is a section in the drupal.org handbooks about configuring different editors for Drupal's standards - https://drupal.org/node/147789

Hi Eric I think the "problem" was in your hook_menu... so here it is attached, let's hope it's going to work "better" now :)

Maybe now some more people could test it?

I just looked at the files and noticed that indeed it's not a tab issue, but an 'extra spaces' issue. I aree NadimBak's patch fixes that, though I didn't review it for the security issue.

At this point I suggest that someone who wants to maintain the module should create a sandbox. I proposed that in #30 and ygerasimov agreed with the idea in #35 and nobody has said they are opposed to the idea.

Personnaly I don't see the point of making a sandbox, as it contains nothing more then latest d6 version with @Erich Schulz patch applied (and tests removed) but anyways here is my sandbox https://drupal.org/sandbox/NadimBak/2022341

BTW I went a few times to the irc channel #drupal-services over the last 4 days but I never got any answer...

Any news on this patch?

Unfortunately I am not able to test the patch as we are still stuck on services 2.4 due to the amfphp module ("Fear not, newer AMFPHP modules are in development"). But I would try to make a backport to the 2.4-version once the 6.x-3.0 version is final.

I'm using drush make/aegir and the unpublished 3.1 version is breaking my production platforms. Where can I retrieve a copy of 3.1 release so I can mirror it locally? I have also checked into the #drupal-services chatroom a few times and there is nobody there.

That doesn´t sound very good :-( So everybody who is stuck on drupal 6 has to live with this security issue?

Can anybody put the consequence of the threat in a nutshell?

As far as I understood this issue it´s like: You can call methods from services via cross site scripting. But this is limited to the permissions of the user who was affected by the XSS (hopefully!).

That would mean: If I restrict the access callbacks for non-admin user roles to safe methods I could "survive" this security issue - if I avoid being affected by a XSS-attack while being logged in with my admin account?

Would that be a workaround? Or are there other ways to confine the threat to an acceptable risk?

@devkinetic you can always checkout 6.x-3.x branch from git repo. It is still there.

@meyerrob vulnerability can be exposed if you allow other users to set up a flash application on your website. They can make it doing requests to services on behalf of the username that opens the page. If user with administrative permissions will open the page that flash app will do the request on his behalf and can do some damage.

This is basic scenario of the problem.

So if you do not allow flash apps on your site you are in safe.

No, you are not. That flash app can be on any external page and can still make requests on behalf of the user. Note that the exploit kit at https://github.com/TheRook/CSRF-Request-Builder does not work for my flash plugin anymore, but I guess there are still lots of outdated flash installations out there.

Ok - thanks for the answer.

But most important for me is the confirmation (please correct me if I´m wrong):

The exploit is only able to call methods that are within the permissions of the user who was attacked by the exploit.

I think the trick about describing it as "The exploit is only able to do call methods that are within the permissions of the user who was attacked by the exploit." is that it's easy to trick someone into executing the malicious script (and it's not xss, btw). The malicious script can come from any page on the internet.

We have had no reviews of the patch. My view given this is that we contact the security team and ask them to again unpublish the drupal 6 rc for service and let services D6 die.

So a sandbox was opened and since then no comments about testing etc........or maintainership offers. We only got a response now because I am saying close d6 branch down. And still no tests.....

@marcingy: Please wait with closing down the d6 branch.
My time is limited but I spent some time the next days to solve the problems. I asked a friend to help with some of the tasks I am not so experienced with especially with the test problem. We open a sandbox soon and work on the patch.
Maybe it's a little bit difficult to meet in IRC with you because of different time zones but my friend is also in Germany so we can coordinate our work.

I suggest the reverse course of action from #54: remove support now and reinstate it if the sandbox gets to a point that satisfies the maintainers of services (i.e. has automated tests for this new security fix, has a review on the code that is more in depth than "works for me"). Having the release available/supported gives a false sense of hope.

If it's possible to open again the D6 branch it's OK for me to close down. Maybe this will bring some attention and with this some more help to get this fixed soon.

@greggles that sounds the best approach, do I need to create an issue with security team or can you do it without that?

Once that is done I will convert this to a feature request and change the status to normal.

@marcingy - done. This issue is formal enough.

And

Adding tag

This module is not supported for drupal 6......no one wants to step up and support it. No one wants to add tests to the d6 sandbox. So it is a feature request to recreate a d6 branch because one no longer exists. As per the drupal SA the fix is to disable the module.

+1: Eric

That's not true. 3 or 4 people have steped up to support it. I even made a sandbox as you requested.
And as you requested, went (and I'm not the only one) to irc channel many time but without any answer.

I really don't get why you keep trying to kill the D6 branch and specially this issue.

The sandbox needs tests and until that happens there is no way that the code can even be considered for committing.

Also as per when we unpublished the RC branch in this issue no one is giving the code reviews. We don't have time to support the d6 branch plain and simple. The d6 branch was effectively dead before the SA was published the SA simply made that truly the reality.

And what about people like @C_Logemann offering to support it?
what about having nobody on your irc channel to help those willing to do that support?

You don't have time to support d6 branch. Ok. But why do you keep other from doing it?
You clearly have lots of time to post comments on this D6 issue...

Please leave the status as is this is not a bug because there is no d6 branch, and yes I most definitely do keep on top of the issue queue.

Chiming in again, Ill gladly commit anything with tests that validate the issues are fixed. the Drupal 6 branch HAS TESTS ALREADY. Model the code from them if you want but just get the tests in there that check for this issue.

We've said it multiple times, when the tests are there, we'll commit. There's no need to continue arguing over the semantic of this feature/bug whatever.

Now I cant speak for everyone on the services team but I don't want to support Drupal 6, and last I checked, this is all voluntary work.

That being said, WE address security issues very seriously and as the SA team decided the D6 branch cannot exist until that bug is fixed. We decided to no longer support that version. Were any of us happy? No, I sure as hell wasn't, but I don't have the free time to fix an outdated system, fix the current system, and re-write for the new symphony version(I know I say I sound like its all me doing the work, but I guess I am kind of speaking for all of the maintainers who are actively working). I'm not going to do something I don't love in the little free time I have, because some people are having to maintain legacy systems, sorry. Upgrade to Drupal 7....... or 8 soon.

Besides I'm really getting off topic here, but thats how I feel. We asked, in the various means we had, to drum up support for someone to maintain that branch, and to this point, we still havnt seen tests. We want the tests to prove that the bug is actually fixed, and so that going forward we can see whenever it breaks. These tests were done in Drupal 7, I wouldn't be surprised if you copy and paste them and it worked. There are too many features in services to simply say, "Yup, I tested it myself, it works."

I'm so glad so many people worked to get that patch ready to go, and all that it needs is tests.

Just do that, or have someone do it, or hell pay someone to do it, but once it has tests, it'll be committed and then you can maintain it. Nobody is holding you guys back from taking over the Drupal 6 branch, its just the policy of tests that seem to concern people, I'm not really sure why.

If you're unwilling to do that, then I cant help you because I'm not going to maintain it. Once we commit that code without tests, people will expect us to fix the things that break and I'm pretty sure I can say nobody currently on the services team is willing to support Drupal 6. Much like the rest of the community when Drupal 8 comes out.

Hi,

The patches above are all in a non-standard format. So for a start, I have re-rolled them with the standard format so that users like myself, who urgently need this to be fixed for a D6 sites can just apply the patch and test first? Will be great if someone can fix the tests in this module for the changes too.

Thanks Erich Schulz for the original patch.

Cheers,
Victoria

Are there any details about how the tests are broken? Also, does another test need to be added to address this specific security issue? If someone can point me in the right direction I'll try and follow through.

The d7 commit had tests added as part of it see http://drupalcode.org/project/services.git/commitdiff/7287ee6. The d6 patch needs to include these and said tests need to pass.

devkinetic there are no tests patch for Drupal 6, which is why it won't be committed.

You can see the tests for D7 here, http://drupalcode.org/project/services.git/blob/7287ee6:/tests/functiona...

As this issue is still not solved, I would like to ask how much a experienced developer would charge to fix this bug for 6.x 3.0 as well as for 6.x 2.4 with tests and so on - up to a status where the maintainers of the services module are willing to commit it and activate the 6.x-branch again?

Maybe I can raise some money for this task. And maybe some others waiting for this patch want to join me :-)

Please keep discussions about payment to https://drupal.org/paid-services. Note the patch will never be committed to drupal 6.x-2.4 that branch has not been supported for years.

Just added the file that we need for this patch.

The tests run, but fail.

@djdevin well done, thank you... keep going!

I ported everything from 7287ee6febac in 7.x-3.x except the XMLRPC test case which doesn't exist in D6.

I got the tests to work but I had a few issues worth noting.

1. When CSRF fails in the test, the user gets logged out and I had to re-login in the test as the privileged user or otherwise the next call would be made as anonymous.
2. drupalGet() in D6 returns the full HTTP headers including the message. I had to parse it out to get the CSRF token. I'm not completely sure why.

People interested in this issue may also be interested in #2125265: Login resource can login users that user login forms prevent which is another security issue in the 6.x branch. It was just closed out in the private queue and brought public since it only affects 6.x and the 6.x branch is not supported.

Grr, when I added the tag, it forced itself to version 8.x.

We also need a maintainer for the 6.x branch :/

And there is another outstanding security issue so nothing has changed from 6 months ago, so even once this is committed there can't be a release. To be honest let just admit a drupal 6 release is never coming back for services.

If the maintainers of this project doesn't want the services d6 included in their project, should people who are willing to maintain d6 version create another Services project exclusively for d6 version?

@agungsuyono100: I hope we can avoid a forking situation. Currently there I think there isn't a good reason for this like a complete different coding strategy. In any case we should collaborate following the Drupal Code of Conduct (https://drupal.org/dcoc).
My current personal barrier is the implementation of tests. I understand how important they are but I am not experienced with.
But the most important thing is to solve the current and future upcoming security problems in any official branch. And about this it doesn't matter in which project we are working on this.

should people who are willing to maintain d6 version create another Services project exclusively for d6 version?

No. Those folks should review the patch above.

Also, https://drupal.org/node/2189509 got released today. I'm not sure if that needs a backport or not, but people interested in the D6 version of the module should confirm if it needs to be backported and do it, if so.

I applied the patch of #78 and checked with local simpletest without errors.
It seems that the patch added the same token check as the official D7 security fix.
Except of XMLRPC-Part (I didn't checked by myself if this is needed) I found a difference in "services.test" where in the D7 changes "$this->addCSRFHeader($headers);" was added to D7 version. I didn't know if we also need this.

@djdevin notes at comment #78:

• (1) Maybe the missing $this->addCSRFHeader($headers) causes the CSRF fails problem you have? By the way I didn't have this error.
• (2) If "drupalGet()" in D6 is working in this way we have to solve this here. Maybe there is a better way to to this, but the one chosen parsing strategy seems OK to me.

@greggels (#86): I started to analyse the affected function "_user_resource_update" and currently believe that there is also a vulnerability in D6.
I assigned myself to #2189919: D6 port of "SA-CONTRIB-2014-010 - Services - Access Bypass and Privilege Escalation"

@maintainers: It would be helpful we can search and assign issues to D6 again. D5 is also not supported but still there for this.

I think we need support of contrib modules as long as the core is supported for any module.
Meanwhile there is a discussion about a longer support of D6 core:
#2136029: Decide if and how to extend D6 security support 3 months past an 8.0.0 release

@greggles: You were right to mention this issue as an argument for missing „maintaining power“ in the contrib world.

I think dropping a branch of an officially supported core version as a security fix is a bad idea. This was a very abrupt situation for me. But I was more shocked that there is so less support from the community for this important module even since it’s been marked as unsupported in D6 branch. Because of this we should find new ways in the community to get support for contrib modules. I think it’s not good for drupal that such things are depending on the engagement of only some people. Especially the security team needs more support of the whole community I think.

I have currently only a one person company and I was very busy the last months including organizing the drupalcamp frankfurt (the first time for me and the first time in my home town). But finally I found some time to learn more about testing and now I feel ready to step in D6 maintainership for the services module.

I'm already involved in getting also the following security problems fixed:
#2189919: D6 port of "SA-CONTRIB-2014-010 - Services - Access Bypass and Privilege Escalation"
#2189921: D6 Backport of "SA-CONTRIB-2014-007 - Services - Multiple access bypass vulnerabilities"

We currently need more testing and also a security review of this 3 issues. Or is there more we have to fix?

@maintainers: I created a sandbox for testing patches, but I didn’t find a possibility to activate "Enable automated testing" as on my full projects. So can you please activate the possibility for tagging and testing again for the D6 branch?

See #32 about why we can't have a release (even unstable) on this module.

@greggles: I understand why the D6 branch was closed. I try to fix the problems to get this open again.

WE can't add the possibility of tagging and testing until we resolve the security issues, unfortunately.

Run Tests locally and provide screenshot output of passing.

Once all SA's are done, we can open the branch up again.

Thank you Kyle for committing my patch (or merging my pull request) for the other security issue:
#2189919: D6 port of "SA-CONTRIB-2014-010 - Services - Access Bypass and Privilege Escalation"

Now we can move on to get the D6 version running again.
I will test the actual patch of this issue locally as soon as possible.

Closing this as d6 is no longer supported by the community in a month so services will never have a d6 version.