Security vulnerability in version 2.0.18.8

Lolo999

It appears 2.0.18.8, which I understand is the stable release, has one unpatched published security vulnerability and possibly another one, as documented at:

http://www.exploit-db.com/exploits/25720/

To verify, I logged in on my forum as an admin, created a new discussion and saved a draft. I then opened another browser, signed as an ordinary user, and was able to see the mentioned draft by using a simple .../index.php?p=/post/editdiscussion/0/1

I did not try the other flagging XSS vulnerability described in the same page

Adrian

I can confirm the Flag plugin vulnerability does exist. This to me is worse than someone accessing a draft I have written. I will see if can post an update fix....unless someone beats me to it

Adrian

okay, here is the fix for the flag vunerability. what you do is open up class.flagging.plugin.php in the flag plugin. On line 253

$Comment = $Sender->Form->GetValue('Plugin.Flagging.Reason');

add right below on line 254

add:

[redacted]

Maybe someone wants to take a stab at the draft issue? I can confirm it is also an issue.

UPDATE: Correct fix is here: https://github.com/vanillaforums/Garden/commit/e117af7c126944514749b2e01244392077d0ac5c

vrijvlinder

I disabled the flagging plugin in my installs, Just no need and the check mark did not go away after disabling I had to hack something to make it go away....

about the drafts , I am using forceredirect plugin and it won't allow anyone to get to that area, they must be logged in to do so. And only to the user can see their drafts ... so no I do not have that issue with drafts.

Adrian

@vrijvlinder, I had no choice with flagging, I have issues with human spammers promoting their business.

As for the drafts can you share more about how you are using force-redirect? Are you just using it as normal. Any mods?

vrijvlinder

there are only a limited amount of controllers allowed and draftscontroller is not one of them.

only the controllers you allow will be open for view to guests. when you use this plugin.

http://vanillaforums.org/addon/forceredirect-plugin

Adrian

@vrijvlinder, awesome plugin, works like a charm.

If you use the flagging plugin, just change that one line and you are good to go. So now all vulnerabilities mentioned are closed by this thread

cheers @Lolo999 for pointing them out

vrijvlinder

You should add to it mobilefriendly true so it also works on mobile. I noticed it may not have it ? I need to check.

Adrian

@vrijvlinder looks like it does work on mobile without the flag

vrijvlinder

ok cool then , I just don't remember if I added it or not, I know it works for me too. Thanks for verifying

Adrian

my pleasure I appreciate you creating such a helpful add-on to harden security.

vrijvlinder

Thanks ! A user with similar issues inspired it, but

well, I did not create it all alone, @peregrine helped me fix the errors I made and @hgtonight came up with the array idea to add or remove controllers as needed. So it was a small community effort.

It is very simple but it does the job. The only thing I would like it if there was a way to specify plugin controllers. Does not seem to work if you use say : extrapageplugincontroller or plugincontrollerextrapage, I don't think there is a way to do that is there ?

Lolo999

I'm not entirely sure about what forceredirect does. From what I read in this thread I understand it restricts contents so guests cannot view them.

If that is the case, the vulnerability is half closed, as registered users could still access the drafts of other registered users (and possibly modify them?), as I described in my example above.

vrijvlinder

as registered users could still access the drafts of other registered users (and possibly modify them?), as I described in my example above.

I tried accessing the drafts of user and it is not possible. Not using

http://www.mysite.com/forum/drafts to access someone else's drafts

and not with this either, the link to the draft

/forum/discussion/1/0#Form_Comment

The link to the drafts of other users does not appear to the admin or other users.

The way this works is it check for user id to get the correct page of drafts.

If you could possible give a structured link to be able to verify this it would be great
If this was for real, I think more people would come out and say something like "my drafts have been erased by someone else" That would suck for sure. I can't verify it because I can't reproduce the potential issue

I'm not entirely sure about what forceredirect does.

It restricts pages from guests and redirects to login.

x00

@adriansonline said:
okay, here is the fix for the flag vunerability. what you do is open up class.flagging.plugin.php in the flag plugin. On line 253

$Comment = $Sender->Form->GetValue('Plugin.Flagging.Reason');

add right below on line 254

add:

 `$Comment = preg_replace('`(?<!(\<img src=[\'"]))(([a-z-9\+\.]*)://[\@a-z0-9\x21\x23-\x27\x2a-\x2e\x3a\x3b\/;\x3f-\x7a\x7e\x3d]+)`i', '',$Comment);`

I don't want rain on anyone's parade, but this is not a viable solution at all, seems to be lifted from some unrelated question I answered. Please be careful.

If you want to be secure you should use Gdn_Format::Text() instead.

As it is already being use in one place on output then it should be applied on all the reports.

This is really @Tim 's responsibility, since he is the author.

50sQuiff

I've just confirmed the Draft vulnerability. You can also delete another person's draft (discussion or comment) using a request like:

YOURSITE/vanilla/drafts/delete/DRAFTID/TRANSIENTKEY?Target=drafts

50sQuiff

In class.draftscontroller.php, after

// Delete the draft
         $Draft = $this->DraftModel->GetID($DraftID);

You can add the following:

if ($Draft->InsertUserID !== $Session->UserID) {
            Gdn::Dispatcher()->Dispatch('DefaultPermission');
            exit();
}

and in class.postcontroller.php, after

public function EditDiscussion($DiscussionID = '', $DraftID = '') {
      if ($DraftID != '') {
         $this->Draft = $this->DraftModel->GetID($DraftID);

You can add:

if ($this->Draft->InsertUserID !== Gdn::Session()->UserID) {
            Gdn::Dispatcher()->Dispatch('DefaultPermission');
            exit();
         }

That should work as a hotfix for the issue before the Vanilla team can release a new version.

Adrian

No rain on my parade. While @Tim may be an author of a plugin, I tried to help to close the vulnerability made public in our community on a Sunday. Isn't that the point of community, to help out others? My only error was I should have credited you @x00 for your answer somewhere else, which I had read earlier and which inspired. I was trying to offer a patch as fast as I could to a vulnerability made public in this forum. Sometimes you have to do that before some can release the perfect patch. I will try to use your solution instead, but I am not sorry I offered a solution...

Adrian

The thread where @x00 the earlier solution came from was : http://vanillaforums.org/discussion/comment/186558 (for full transparency--I tracked it down)

the patch that I have worked out from his advice (above) is on line 254:

$Sender->SetData('Plugin.Flagging.Reason', Gdn_Format::Text($Comment)); //per @x00

this will ensure all flag inputs are text to your database. I works for me. Please let me know if you know better with the proper code solution

x00

@adriansonline said:
I will try to use your solution instead, but I am not sorry I offered a solution...

My point that it was not a solution. I applaud you for trying to help, however, if people believed that would close the vulnerability, they might take it in good faith and not check back.

It was actually an answer to a specific unrelated question. All it does it remove urls, except in image tags. It is not a substitute for sanitation.

x00

@adriansonline said:
The thread where x00 the earlier solution came from was : http://vanillaforums.org/discussion/comment/186558 (for full transparency--I tracked it down)

the patch that I have worked out from his advice (above) is on line 254:

$Sender->SetData('Plugin.Flagging.Reason', Gdn_Format::Text($Comment)); //per x00

this will ensure all flag inputs are text to your database. I works for me. Please let me know if you know better with the proper code solution

This is an adequate solution, until an official patch, but doesn't help if the someone already used this exploit, and that data iInow in the database, which is one of the reason why sanitation is done on ouput.