Get logged in as arbitrary user via the access callback

There is no way to fix this the other way arround.

The problem is falsh without cookies. You will lose the session as the PHPSID ( SID$HASH ) hookie ist lost durin upload. Thats why a post field with a hashed sid is used to restore the user-session. After that, you need to set session_id properly, as this will be used during the toke-validation, while nearly all other modules are using user->sid

greggles first blush was right:

Simply submit the following form to a victim;

<form action="http://vulnerable.example.com/swfupload" method="POST">
<input type="hidden" name="sid" value="31"/>
<input type="submit" value="Submit" />
</form>

And poof, user 1 (31 is the hexvalue of 1).

Better title

Well actually one thing has nothin to do with the other.

What you encountered is right, that there could be a way to spoof the hexbin hash. So actually what shall be done hashin the sid, ans saving the pair in a cahed DB. This is later used to restore the sid.

the second case is HOW to restore the session. And there session_id($user->sid); will come in to make form validation work.

So actually that are 2 different issues where i see the last one already fixed, the first one though is valid

The attached patch should fix this

yes greggles is right, we should use the SID directly. Anything else can cause a potential hash colusion.
First i thought it might be a security leech when you save the SID as JS value. But actually, the user must be a victim of XSS then .. in that case it does not matter to steal its session - you already control the user :)

Subscribing.

+1

subscribing

subscribe

i wonder if you should delete all current releases until this issue is fixed? that's what we did with another security issue at the media module. it's really only a matter of time that some hacker boy or girl decides to google drupal sites using swfupload, considering the big red flag on the module page linking to explicit directions. i mean, yes, we all know that technically devs shouldn't be using beta release, but take a look at the 4700 sites using an ALPHA for views -- http://drupal.org/project/usage/views --, let alone the > 540 using this module -- http://drupal.org/project/usage/swfupload

I will take care of this issue this weekend. Somehow skilip is not able to work on this module at all :/

Guys, I'm really sorry for my absence. My life is like a roller coaster atm. I will work on this tomorrow evening for sure.

subscribing

Did anyone actually try to reproduce the bug after applying the patch @#9? I really do not see how it still would be possible to get you logged in as superuser by posting '31' as sid. (most probably while I'm a noob in security)

subscribing

Added flood_is_allowed() as well. Can anyone plz test this?

@greggles ofc I read #10. I know it is possible to retain the session with only the session id. However, if you'd send a randomly generated session id, and do the comparison on only that sid, it would be possible to find a session id in the database which doesn't belong to you. That's why I do a comparison on the users uid as well. There's no way to hack that afaik.

Just had a little talk with greggles in irc and he agreed that checking on a match between sid and uid should be more secure than checking on the sid only. As far as I can see the patch posted at #23 solves this issue and is ready to be committed to head.

hi there,

when will be the new release on drupal.org including this patch? I am planning to use the module on a client site, so it would be interesting if we should wait for the new release and update or search for another solution, because this problem won't be solved soon?

Thanx in advance.

Hey there, i think we worked through it with skilip and worked out this:

http://github.com/EugenMayer/swfupload/blob/835922/swfupload.module#L29

There is not much changed compared to the old patch, we still use uid and the md5(sid) ( a token ) for verification. This is actually even more secure as the sid only way, as you have to guess the touple, not only sid. This makes attacks much harder even.

Now first we use the uid to load all sids for the databases and we md5 hash those. Then we compare it with the provided token. If that matches, we are authenticating the user.
Just have a look at the method docs, they should explain it

The flood protection does not have anything todo with the security issue but still it gives us more security agains dictionary attacks ( on (uid,sid) touples).

I would rather not work with reviewing patches here, as its harder if the squence is statements is so imporant as it is here. Please review the whole method on github, its rather short

well...what the...well yes, this was not the patch. Cool chrome bug though. If and admin comes accross, please delete my last comment http://drupal.org/node/835922#comment-3231916.

Thats the patch:

BUMP.

Waht are we waiting for? Please unlock the module so we can release this security issue ASAP.

Thank you

Committed to HEAD, released in BETA7

Oh, and thanks guys!

skilip++

The release has been packaged. I've published the node.

Thanks a lot Heine!

Jap, thank you Heine!

For tighter security you can regenerate the session id after the SWFUpload onComplete or similar
using sess_regenerate();

or even better solution
when the SWFUpload fileQueued event is fired have an AJAX call to drupal to generate a "key" for that file and you can later match the key to the user and/or filename, you can then include the "key" in the POST parameters for that file

why this is better? because keys will always be unique for each file, while the $sid hash will remain constant. The "key" should expire after one use.

Does this make sense? Is it possible?

array_search returns the index of the searched value and what does this do?

session_id(array_search($token, $valid_sids));

and what's the point in flood checking? Is the swfupload_js path more floodable! than any other path?

ufku no - that makes it more secure as the drupal core as it checks for the user authentication trys and bans him on repeating failing.

I would like to reopen the issue, because of the comment in #40.

I was having problems uploading multiple images until I noticed that the sid gets changed. You have to make sure that the array index is set to the sid, which currently does not seem to be the case.

eg.

while ($row = db_fetch_object($result)) {
        $valid_sids[$row->sid] = md5($row->sid);
      }

(I did not test it, because I am not using the default session storage)

@44: intersting. Please open a new issue. Your problem has nothing to do with the security issue here but rather session problems in general.

dig24 could you explain a bit more detailed, what you think causes failture here? I somehow cant get it, sorry

Sorry, I should have been more verbose in the first report, I just was too tired after having spent some time hunting the problem down. It's just a minor array error, but debugging was tricky:

What happens right now:
The user loads the form and can upload one picture (hook_access evaluates to true). However, in contrast to your patch in #30 the session_id is not set to session_id($user->sid);, but following the latest bugfix commit to session_id(array_search($token, $valid_sids));.
Array search returns the key, which in this case is not the sid, but just the element number, probably zero, because of the part mentioned in my patch below.
Now if the user tries to upload another file or has selected multiple files to upload in the first place, the acess_hook will evaluate to false, as the javascript/flash file is still sending the original sid, while the server is now expecting the "broken" sid value. The flash upload will thus fail at 99%, as the flash applet receives an 403 error on the POST-Url. To make things worse, after a few attempts the flood protection kicks in, so that even the first upload after loading the form will fail.

The code to fix it, is primitive, just make sure the array_key is set. (still untested)

Index: swfupload.module
===================================================================
--- swfupload.module    (revision 1710)
+++ swfupload.module    (revision 1711)
@@ -67,7 +67,7 @@
       $valid_sids = array();
       // create our hashes we need for verification
       while ($row = db_fetch_object($result)) {
-        $valid_sids[] = md5($row->sid);
+        $valid_sids[$row->sid] = md5($row->sid);
       }
     }

ah i see, thank you very much!

Alright so I tested the code and while it appears as though it is working I seem to get a re-occuring issue. I have narrowed it down to the following code:

/**
 * Generates an unique key, used for validating  upload requests
 */
function _post_key() {
  global $user;
  $result = db_query("SELECT sid FROM {sessions} WHERE uid = %d", $user->uid);
  $sid = $_SERVER['REMOTE_ADDR'];
  // There is no user with that uid, deny permission.
  if($result != false) {
   $row = db_fetch_object($result);
   $sid = $row->sid;
  }
  return bin2hex("$user->uid*". md5($user->uid ? $user->sid : $_SERVER['REMOTE_ADDR']));
}

Essentially, when you are referencing the global $user variable it appears to carry the UID but *not* the SID. Therefore, it ends up md5'ing an empty string. I couldn't figure out what was going on at first and later through some debugging found this problem.

I'm not sure how to properly set these variables instead as a callback (which might be causing this issue). Also, one solution I had thought of was to actually query the sessions table for the SID but that won't work if you have multiple enabled sessions.

BUMP just wondering if anyone is looking into this issue. It seems like the security problem is still not fixed??

Well iam following but still iam pretty busy maintaing my other modules and it seems like skilip is not really maintaining this module anymore.

Iam not sure i will continue maintaining this module in general, though i will fix issue here. Its just to huge, 990 lines of JS code, thats simply to much. Most probably that can be reduced close to 0 using ctools and jquery in general. It simply turns out that maintaining this module is a heck of a job and without help of the creator its pretty hard and too time-consuming.

After I made some test with client side resizing I believe this module just rocks. Is there any way to make it safe_?

@cem: From my point of view, the security problems have been fixed by EugenMayer, with one patch still to be ported. Are there any security concerns besides the post by ShaneOnABike, which I have commented below?

@ShaneOnABike
Where does the code you have posted come from? The current dev and release looks differently. In the code you posted there is an obvious error in the last line, which should probably be md5($user->uid ? $sid : $_SERVER['REMOTE_ADDR'])).

If it's all fixed , Why is this module in beta status still_?

Is it all fixed?

I'm still having this issue actually and yes your correct the code posted at #53 is correct.

So, I was trying to find out what still needed to happen to this bug report...

#47 has pointed out a clear & contained bug, plus the fix. I see #48 as a confirmation.

w.r.t. #56: I do not fully get what you mean. The code in #53 is not 'correct' for 2.0-beta7/HEAD, because the definition of _post_key() is totally different there.
I debugged, and in my case / with 2.0-beta7 code, $user->sid is always filled. So I'm guessing that we can disregard #49+#56 unless more information is given on how the bug is triggered. (Plus confirm that you are running an unmodified 2.0-beta7/HEAD codebase, please)

Here's #47 as a patch. But with the above in mind, I really think we can set this to RTBC now. Next commenter?

this looks good. go for it!

I can probably test this out in about a week to confirm that it works. Is this committed in the latest HEAD? I was confirming previously that indeed the code that I posted was older (and I had upgraded afterwards and it still didn't work). But I'll double check this, download the latest head, and then test it out.

@ShaneOnABike: patch in #57 (which is code from #44/47, credit to digi24) is not committed to HEAD. HEAD still seems to be equal to beta7 (last CVS commit july 28th).

FWIW: I didn't do extensive checks on the original security issue.
I just read from #43 downwards - and confirmed that with the release plus the patch, your issue reported in #49 didn't exist for me anymore.

re #49, i'd noticed similar behavior at #796916: SID not always correctly restored and proposed a patch at http://drupal.org/node/796916#comment-2958754 -- not sure if that got committed. basically:

@@ -265,7 +265,7 @@ function swfupload_add_js($element) {
  */
 function _post_key() {
   global $user;
-  return bin2hex("$user->uid*". md5(($user->uid ? $user->sid : $_SERVER['REMOTE_ADDR'])));
+  return bin2hex("$user->uid*". md5((($user->uid && $user->sid) ? $user->sid : $_SERVER['REMOTE_ADDR'])));
 }
 
 /**

looks like that piece didn't actually get committed...

Thanks aaron. I was wondering about that condition when looking through the code... but don't know much about session handling & did not get this problem myself, when testing.

So for people who like patches, and for 'issue handholding purposes': here's the fixes which are still outstanding.
#44/47 by digi24 and #46/#796916 by aaron. Credit to both.

Tested, and well commented by both / long ago, in these issues... so still RTBC as far as I'm concerned.

And you should. Iam currently maintaing way to much modules and only co here. The maintainers seems not to care to much currently. Could you guys try to help me by starting pull requests on github?

github.com/EugenMayer/swfupload

thats the official development source and i might be able to push those chages faster. Iam sorry for that situation, normaly i tend to maintain my modules (thinking about bugs) pretty actively.

hi @EugenMayer, I might be trying this pretty soon, so what's the status of this issue, is it's fixed? Should I look at the dev version + patch or directly on your github, what are the differences there, only this patch?

And what about requesting co-maintainership or maybe changing maintainers if the original maintainers are too busy. Anyway, not very versed with git but I guess it's not going to be an issue.

Well def, go for git, current version is there. Iam not the maintainer, cant invite anybody ( but i can release )

oh, you have commit access, didn't realize, and why not committing or trying this against the cvs version here, isn't that better than an external git?

iam not using CVS of drupal for development for any projects / modules i run for d.o.

Iam not commiting because my lack of time. Iam glad to work on github and make releases on github though, rather then testing patches manually, aplying them and all the timeconsuming CVS process.

Committed to HEAD, sorry for the inhumane delay :(

I can confirm that this does resolve all the issues that I had with authentication! thanks heaps for fixingn all this.

I still get swfuplad "access denied" user Anonymous when I try to upload large files... Please help!

momper

@fiftyz that's probably another issue. Please consider creating a separate issue.

@skilip: if it's fixed, may be you should remove the warning in the module homepage:
"SWFUpload is currently in Beta and has a security flaw that needs to be sorted out. Beta releases should never be used on production sites. See issue #835922 for details."

It just made me read all this thread, and now I feel like (but not that sure) that there is no more known security issues...

Holy smokes, I just spent like 15 mins reading this thread from #1 to #74 because there is a warning on the front page of this module stating that there is a security issue with it. Whata complete waste of time that was. Please fix the front page and once fixed, re-mark this issue as fixed.