pH7Software / pH7-Social-Dating-CMS

😻 pH7Builder (formerly pH7CMS) is a Professional & Open Source Social Dating CMS written in PHP 8 🚀 This Social Dating Script aims to be low resource-intensive, powerful and secure. pH7Builder includes over 40 modules. It is the first Professional, Free & Open Source Social Dating Site Builder Software and the first choice for enterprise level Da
https://pH7Builder.com
MIT License
961 stars 578 forks source link

Cross Site Scripting - Reflected #795

Closed ghost closed 4 years ago

ghost commented 4 years ago

There is a XSS vulnerability without need for any authentication at all. It seems to be also present in the newest versions.

To reproduce: 1)If you created sample users such as "Avis", "Clark", "Raphael", etc , click on their photos on the home page. 2)The endpoint "/signup/" has parameters that are vulnerable to Cross Site Scripting. Here is an example: http://example.com/pH7-Social-Dating-CMS/signup/?ref=main&a=index&u=chaya-ward&f_n=Avis">

I hope it will be fixed because I really like this project. It should not be too difficult to sanitize those parameters.

Undefined-Variables commented 4 years ago

This is still possible but not directly on the page it gets cached by nginx then executes when any nginx action is preformmed

with this...

no

Which is

<script>alert('FUCK')</script>

URL Encoded

then

HTML `Encoded....

Undefined-Variables commented 4 years ago

Still a problem, can we stop it?

ghost commented 4 years ago

Still a problem, can we stop it?

This is still possible but not directly on the page it gets cached by nginx then executes when any nginx action is preformmed

with this...

no

Which is

<script>alert('FUCK')</script>

URL Encoded

then

HTML `Encoded....

Damn, well I guess this is Stored XSS now. Definitely going to take some tweaking to fix. Unfortunately there are a million ways to skin the cat with XSS. Using "onmouseover", tags, tags,

ghost commented 4 years ago

Still a problem, can we stop it?

This is still possible but not directly on the page it gets cached by nginx then executes when any nginx action is preformmed

with this...

no

Which is

<script>alert('FUCK')</script>

URL Encoded

then

HTML `Encoded....

Damn, well I guess this is Stored XSS now. Definitely going to take some tweaking to fix. Unfortunately there are a million ways to skin the cat with XSS. Using "onmouseover", tags,

not sure why GitHub cut out the rest of my reply...anyway, I hope there is a CSRF token or else this XSS can lead to account hijacking.

ghost commented 4 years ago

Still a problem, can we stop it?

This is still possible but not directly on the page it gets cached by nginx then executes when any nginx action is preformmed

with this...

no

Which is

<script>alert('FUCK')</script>

URL Encoded

then

HTML `Encoded....

Damn, well I guess this is Stored XSS now. Definitely going to take some tweaking to fix. Unfortunately there are a million ways to skin the cat with XSS. Using "onmouseover", tags,

Also I'm not too knowledgeable on Cache Poisoning, but it sounds like what's happening here.

ghost commented 4 years ago

Still a problem, can we stop it?

This is still possible but not directly on the page it gets cached by nginx then executes when any nginx action is preformmed

with this...

no

Which is

<script>alert('FUCK')</script>

URL Encoded

then

HTML `Encoded....

Damn, well I guess this is Stored XSS now. Definitely going to take some tweaking to fix. Unfortunately there are a million ways to skin the cat with XSS. Using "onmouseover", tags,

Also it might be worth trying this header: "Pragma: no-cache" to prevent any more cache problems.

Undefined-Variables commented 4 years ago

@squelch0 Yeah to be honest I was waiting for an upload to finish so was just messing about with Burp and didn't think it would work in a million years. If you just URL encode it it gets escaped, if you just HTML encode it again it gets escaped but if you do one on top of the other it gets missed and yeah like you say its stored in the cache and in a worst case scenario could end up being able to hijack an account if CSRF is not being used. You would be silly though to allow any form on the public internet to not be protected by a token. I'm not sure what the solution to this is!

Undefined-Variables commented 4 years ago

Plus it may still work directly on the page as thinking about it the original did the same thing on my live server. I haven't got a local copy to hand to test it. My production server is protected by mod security which stops it directly on pages.

ghost commented 4 years ago

@squelch0 Yeah to be honest I was waiting for an upload to finish so was just messing about with Burp and didn't think it would work in a million years. If you just URL encode it it gets escaped, if you just HTML encode it again it gets escaped but if you do one on top of the other it gets missed and yeah like you say its stored in the cache and in a worst case scenario could end up being able to hijack an account if CSRF is not being used. You would be silly though to allow any form on the public internet to not be protected by a token. I'm not sure what the solution to this is

Plus it may still work directly on the page as thinking about it the original did the same thing on my live server. I haven't got a local copy to hand to test it. My production server is protected by mod security which stops it directly on pages.

Hmm, unfortunately I haven't been able to get a local copy running either. I have FastCGI and everything else I think but it still doesn't want to work. It also throws an error mentioning cache and Smarty. Anyway, a cached payload could execute any time a victim loads the page. You could easily steal everyone's session cookie and then log in as them. You could even use window.location.replace() to redirect them to another site, for example a replica site for phishing.

ghost commented 4 years ago

@squelch0 Yeah to be honest I was waiting for an upload to finish so was just messing about with Burp and didn't think it would work in a million years. If you just URL encode it it gets escaped, if you just HTML encode it again it gets escaped but if you do one on top of the other it gets missed and yeah like you say its stored in the cache and in a worst case scenario could end up being able to hijack an account if CSRF is not being used. You would be silly though to allow any form on the public internet to not be protected by a token. I'm not sure what the solution to this is

Plus it may still work directly on the page as thinking about it the original did the same thing on my live server. I haven't got a local copy to hand to test it. My production server is protected by mod security which stops it directly on pages.

Hmm, unfortunately I haven't been able to get a local copy running either. I have FastCGI and everything else I think but it still doesn't want to work. It also throws an error mentioning cache and Smarty. Anyway, a cached payload could execute any time a victim loads the page. You could easily steal everyone's session cookie and then log in as them. You could even use window.location.replace() to redirect them to another site, for example a replica site for phishing.

Of course, that's only a risk if it's stored XSS. If it is reflected we only have to worry about someone being tricked into opening a malicious link..

Undefined-Variables commented 4 years ago

Could always be worse you could own Twitter or be one of the greedy people trying to double their money out of thin air 😂

Too good to be true springs to mind!

ghost commented 4 years ago

Could always be worse you could own Twitter or be one of the greedy people trying to double their money out of thin air

Too good to be true springs to mind!

True that! Luckily there are still some good people out there, we need more of them though. Right now is a terrible time for web-app bugs..but anyway if you have any free time maybe we can try to fix this. I think there is a pretty large attack surface for this parameter, unfortunately, but I think this bug can be fixed easily. If anything you can simply filter any symbols, and only allow uppercase/lowercase letters. That would fix it.

Undefined-Variables commented 4 years ago

Removing the & symbol would break PHP

The # symbol is used in HTML so would break things like tabs and profile page.

I really don't see an easy fix for this as the problem is made up from 3 must have symbols and numbers.

ghost commented 4 years ago

Removing the & symbol would break PHP

Couldn't you use some regex to filter dangerous symbols? You could maybe use Ajax to prevent using symbols client-side..

Undefined-Variables commented 4 years ago

You could use regex to match offending URL's I suppose.

Undefined-Variables commented 4 years ago

Haha we both thought that at the exact same time.

ghost commented 4 years ago

You could use regex to match offending URL's I suppose.

There are "best practices" for these kinds of things, but I haven't read about them yet. I'm not really used to being a blue teamer haha.

Undefined-Variables commented 4 years ago

You could use regex to match offending URL's I suppose.

There are "best methods" for these kinds of things, but I haven't read about them yet. I'm not really used to being a blue teamer haha.

The very best hackers start out blue team but that evil money stuff corrupts them!

ghost commented 4 years ago

You could use regex to match offending URL's I suppose.

There are "best methods" for these kinds of things, but I haven't read about them yet. I'm not really used to being a blue teamer haha.

The very best hackers start out blue team but that evil money stuff corrupts them!

Definitely, I think blue teaming is the best experience. I know it is stressful when something like this is found, though, as the blue teamers have to rush to fix it. For me it's bittersweet, I like finding these bugs because it is fun for me and I can get paid to hack legally, but I know that it's not fun for the blue teamers. In this case, I have my own production website of this, so in a way it is kind of funny that we are all on the same boat here, haha.

Undefined-Variables commented 4 years ago

This deals with something similar and is in

 /_protected/framework/Security/Validate/Filter.class.php
         /*
         * URL Decode
         *
         * Just in case stuff like this is submitted:
         *
         * <a href="http://%77%77%77%2E%67%6F%6F%67%6C%65%2E%63%6F%6D">Google</a>
         *
         * Note: Use rawurldecode() so it does not remove plus signs
         *
         */
        $str = rawurldecode($str);

So its just a matter of tweaking this file... Leave it with me till im more awake! Its nearly bed time here nd im half asleep... Not best time for this type of thing!

Undefined-Variables commented 4 years ago

You could use regex to match offending URL's I suppose.

There are "best methods" for these kinds of things, but I haven't read about them yet. I'm not really used to being a blue teamer haha.

The very best hackers start out blue team but that evil money stuff corrupts them!

Definitely, I think blue teaming is the best experience. I know it is stressful when something like this is found, though, as the blue teamers have to rush to fix it. For me it's bittersweet, I like finding these bugs because it is fun for me and I can get paid to hack legally, but I know that it's not fun for the blue teamers. In this case, I have my own production website of this, so in a way it is kind of funny that we are all on the same boat here, haha.

Yeah I have spent sometime on HackerOne and found a load of issues but somehow the company who own the site always seem to get away with paying the reward as say things like its outside the scope but fix it anyway :/ Think Blue team is more about the satisfaction after you fix something where red team are happy when they break something. Two different types of people!

ghost commented 4 years ago

This deals with something similar and is in

 /_protected/framework/Security/Validate/Filter.class.php
         /*
         * URL Decode
         *
         * Just in case stuff like this is submitted:
         *
         * <a href="http://%77%77%77%2E%67%6F%6F%67%6C%65%2E%63%6F%6D">Google</a>
         *
         * Note: Use rawurldecode() so it does not remove plus signs
         *
         */
        $str = rawurldecode($str);

So its just a matter of tweaking this file... Leave it with me till im more awake! Its nearly bed time here nd im half asleep... Not best time for this type of thing!

Don't burn yourself out! It's afternoon here in U.S.

ghost commented 4 years ago

You could use regex to match offending URL's I suppose.

There are "best methods" for these kinds of things, but I haven't read about them yet. I'm not really used to being a blue teamer haha.

The very best hackers start out blue team but that evil money stuff corrupts them!

Definitely, I think blue teaming is the best experience. I know it is stressful when something like this is found, though, as the blue teamers have to rush to fix it. For me it's bittersweet, I like finding these bugs because it is fun for me and I can get paid to hack legally, but I know that it's not fun for the blue teamers. In this case, I have my own production website of this, so in a way it is kind of funny that we are all on the same boat here, haha.

Yeah I have spent sometime on HackerOne and found a load of issues but somehow the company who own the site always seem to get away with paying the reward as say things like its outside the scope but fix it anyway :/ Think Blue team is more about the satisfaction after you fix something where red team are happy when they break something. Two different types of people!

For me, I enjoy finding them before a cyber-criminal does. Even if I don't get paid I will report it. If it's really severe, I won't report it publicly, obviously.

polynamaude commented 4 years ago

Please stop reinventing the wheel. There's nice OWASP sheet on preventing XSS on PHP. There's already made library that work to sanitize input. They are much more tested than what we can do here.

ghost commented 4 years ago

Please stop reinventing the wheel. There's nice OWASP sheet on preventing XSS on PHP. There's already made library that work to sanitize input. They are much more tested than what we can do here.

Exactly. It can probably be fixed using one of the many tried and true methods used by every secure PHP site that has ever existed. Maybe I should have been more blunt in the beginning and said this: 1) Google "XSS Prevention" 2) Click first Google link 3) Use any of the many methods listed boom there you go, simple as that. Thanks, Google!

TL;DR this can be fixed using any of the well-documented functions and libraries, especially the OWASP cheat sheet as mentioned by @polynamaude. I'm sure there is also a common fix for the multiple-encoded payloads @Undefined-Variables was talking about.

Undefined-Variables commented 4 years ago

@polynamaude Why is this not already in place here then?

polynamaude commented 4 years ago

@Undefined-Variables Good question because there's already half of the Internet as dependecies for library... There's a sanitizer in the framework itself that is itself a copy of a already written library.

I'll show you where it lives...

polynamaude commented 4 years ago

https://github.com/pH7Software/pH7-Social-Dating-CMS/blob/master/_protected/framework/Security/Validate/Xss.class.php https://github.com/pH7Software/pH7-Social-Dating-CMS/blob/master/_protected/framework/Security/Validate/Purifier.class.php https://github.com/pH7Software/pH7-Social-Dating-CMS/blob/master/_protected/framework/Security/Validate/Validate.class.php https://github.com/pH7Software/pH7-Social-Dating-CMS/blob/master/_protected/framework/Security/Validate/Filter.class.php

3 of the classes are close to empty and are only extension of the preceeding one, adding some function.

That's what we call a city's contractor job. Looks nice on the plan but when you look in the inside, it gets pretty strange with some strange colored wire.

polynamaude commented 4 years ago

The main problem is that they use the allow all that's not rejected when it shall instead use the "accept what we are looking for only". Example, if one parameter is a number then you reject all that is not a number. By trying to find "specific xss spices" you risk missing the new ones.

ghost commented 4 years ago

The main problem is that they use the allow all that's not rejected when it shall instead use the "accept what we are looking for only". Example, if one parameter is a number then you reject all that is not a number. By trying to find "specific xss spices" you risk missing the new ones.

Right, it's just a problem with what is blacklisted and whitelisted.

polynamaude commented 4 years ago

@squelch0 When doing input sanitizing it is not recommended to use the blacklist bad stuff method. Because doing so will require to do update everytime a new method is discovered. When you limit to accepting only what can be logically used and rejecting all else, then you have less to watch for.

Too often people wanna go what they think is the safe route and code all by themselves. But the more you test, the more chance you have to find both bugs and breach. So by using external library you can easily get the advantages of having a bigger base for testing. As for this software, if we use a in-house code to do sanitizing then only the user of this software will be testing it. If we use a external library, we may have hundres, even thousands of people doing the tests.

https://owasp.org/www-community/xss-filter-evasion-cheatsheet

https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html

https://owasp.org/www-project-cheat-sheets/

This look like a pretty active library...

https://github.com/leizongmin/js-xss

polynamaude commented 4 years ago

https://cheatsheetseries.owasp.org/Glossary.html

polynamaude commented 4 years ago

@squelch0 A simple example of why to use whitelisting is the grave accent escaping for XSS. Also when you are working with specific type of data, you can always play safe by doing type casting. example : x = (int) data

ghost commented 4 years ago

@squelch0 A simple example of why to use whitelisting is the grave accent escaping for XSS. Also when you are working with specific type of data, you can always play safe by doing type casting. example : x = (int) data

Right, as you said, it's always best to whitelist, I imagine a script would be quite lengthy if you blacklist everything. Then, whitelisting/allowlisting is more effective than blacklisting/blocklisting because it's more simple and matches uppercase, lowercase, digits, etc.

polynamaude commented 4 years ago

@squelch0 I'd say simply that this is part of good programming. When you work on big project, with many developers, you have some coding standard that are enforced by team manager and code revisor. The following are example of rules that must be followed (those are common rules that most project enforce)

  1. Your variables must be named following to specifications (CamelCase, if possible a descriptive name).
  2. Your code must be documented.
  3. Your variables must have the least visibility possible that allow the algorithm to work properly.
  4. Your variables must have the most limited type possible that allow the algorithm to work properly.
  5. Your code must check for bound violation when doing operation on variable that are subject to such problem (go over 65535 and go back to 1).
  6. Everything that is allocated must be freed.
  7. Don't cast alphanumerics into integer or the opposite. ... etc

So the whitelisting approach is just part of the global.

Many years ago we didn't have much to think about security so some programming tricks we're done to make thing go faster. Now those same trick can cause us problem because there's not only the user in front of the computer that can give order to the software, there's also thousands / millions of other user on the network.

If you want a nice story relating to the risk of bad software verification procedure https://en.wikipedia.org/wiki/Therac-25

There was dead people in this one... And the story about the Ariane 5 first launch is pretty bad too. They re-used part of the code of the previous launcher. But Ariane 5 does a different trajectory than Arianne 4 and is a bit heavier. So the computer did a overflow on one variable, went thinking that the rocket was off course when it was going exactly where supposed. So it engaged the "self destruct" mode and blew itself.... The story is a bit more complex about the self-destruction but it's something related to reusing old code without validating...

polynamaude commented 4 years ago

https://cheatsheetseries.owasp.org/cheatsheets/Input_Validation_Cheat_Sheet.html

ghost commented 4 years ago

@squelch0 I'd say simply that this is part of good programming. When you work on big project, with many developers, you have some coding standard that are enforced by team manager and code revisor. The following are example of rules that must be followed (those are common rules that most project enforce)

  1. Your variables must be named following to specifications (CamelCase, if possible a descriptive name).
  2. Your code must be documented.
  3. Your variables must have the least visibility possible that allow the algorithm to work properly.
  4. Your variables must have the most limited type possible that allow the algorithm to work properly.
  5. Your code must check for bound violation when doing operation on variable that are subject to such problem (go over 65535 and go back to 1).
  6. Everything that is allocated must be freed.
  7. Don't cast alphanumerics into integer or the opposite. ... etc

So the whitelisting approach is just part of the global.

Many years ago we didn't have much to think about security so some programming tricks we're done to make thing go faster. Now those same trick can cause us problem because there's not only the user in front of the computer that can give order to the software, there's also thousands / millions of other user on the network.

If you want a nice story relating to the risk of bad software verification procedure https://en.wikipedia.org/wiki/Therac-25

There was dead people in this one... And the story about the Ariane 5 first launch is pretty bad too. They re-used part of the code of the previous launcher. But Ariane 5 does a different trajectory than Arianne 4 and is a bit heavier. So the computer did a overflow on one variable, went thinking that the rocket was off course when it was going exactly where supposed. So it engaged the "self destruct" mode and blew itself.... The story is a bit more complex about the self-destruction but it's something related to reusing old code without validating...

Thank you for the excellent information & tips, those are things that every programmer needs to learn. My first programming language was C, so what you said about freeing anything allocated is music to my ears. Is it correct to say 5 is concerning stack-overflow while 6 is concerning heap-overflow?

Very sad tragedy, but also fascinating. All it takes is one insecure, outdated function to cause these problems. And all too often these insecure, outdated functions are used for decades before anyone realizes they have privilege escalation or code execution vulnerabilities. Of course, with Ariane 5, it was catastrophic failure most likely due to weird memory bugs.

Undefined-Variables commented 4 years ago

Sorry guys I have been super busy! I still don't see a solution to this so will have a proper look today!

Undefined-Variables commented 4 years ago

Okay... Just installed a local version to test this properly and the good news is this doesn't work directly on pages (reflected). The only way I can get this to do anything worth worrying about is on a live server running Apache that is using nginx engintron which integrates nginx as a reverse caching proxy sitting in front Apache.

When used with this set up you get the 404 as normal and only has effect if you purge cache from cmd line as root. You then get the alert pop up.

Think this is cached and not escaped when you are purging as system assumes you are not trying to hack it as you are root. I can see this being something that may catch someone out if a malicious script is set and awaiting for an admin to purge cache so I have reported this to nginx's dev team.

Most important thing is the software is secure for the time being :)

Undefined-Variables commented 4 years ago

@squelch0 just had the same issue with version 16.0.0. beta 2 There is a template error in _install I just swapped _install for an earlier version.

ghost commented 4 years ago

@squelch0 just had the same issue with version 16.0.0. beta 2 There is a template error in _install I just swapped _install for an earlier version.

I use 15.4, but it won't work on my local server. I could try an earlier one than that

Undefined-Variables commented 4 years ago

@squelch0 yeah 16 has a lot of bugs just hit another one.

Undefined-Variables commented 4 years ago

Turns out the problem is when you unzip on windows all the folders are moved but the files are left behind, I have 16.0.0 working now, maybe you done the same thing?

pH-7 commented 4 years ago

@squelch0 just had the same issue with version 16.0.0. beta 2 There is a template error in _install I just swapped _install for an earlier version.

What exact error did you get during the installation? I tried to reproduce it and did a fresh installation, but everything went well. It would be so great if you can just tell me a little bit more the error you got 🤗

Undefined-Variables commented 4 years ago

You get A smarty template error. I will see if I can reproduce it and upload a screenshot.

polynamaude commented 4 years ago

Can we please stop talking about Smarty ? This doesn't use smarty even if it has a similar syntax for templating. It's like saying the Spanish is Portuguese or that French is English because they share same words sometime.

Undefined-Variables commented 4 years ago

@polynamaude the install script 100% still uses smarty templates.

pH-7 commented 4 years ago

Can we please stop talking about Smarty ? This doesn't use smarty even if it has a similar syntax for templating. It's like saying the Spanish is Portuguese or that French is English because they share same words sometime.

I know it can be quite confusing... @Undefined-Variables is absolutely right.
Only the installation wizard uses Smarty because it used to be a standalone tool I initially built separately for different projects and then reused for pH7CMS. That's why the installer uses Smarty (for legacy reasons mentioned above), but apart this, the rest of the software uses pH7Tpl (you right @polynamaude). The installer is usually removed once the software is installed. At that point, we can say that there is no Smarty anymore!

pH-7 commented 4 years ago

You get A smarty template error. I will see if I can reproduce it and upload a screenshot.

This would be awesome @Undefined-Variables I reinstalled a new version from scratch and there was no error :( Since I cannot replicate the problem, a screenshot or an error log would be really helpful!!

pH-7 commented 4 years ago

@Undefined-Variables And sorry, I have been really busy lately, so I couldn't help much around for the past weeks.