Topic: To the dev that caught me...
[Note: This post will be slightly abstract and incomplete. The intended recipient will understand, and that is all I require. Should (s)he decide to divulge more information, then that is their decision.]
First off: Kudos for taking action so quickly. Most dont even notice, let alone it in the act...
Secondly: Excellent job in applying the correct fix, on the first try. A lot of the people I come across underestimate the simplicity but effectiveness of intval, thus, leaving their application open to other, more complex vulnerabilities.
Because of your swift response to the critical situation, I have decided to leave you (for now
)
As a show of good faith, and no hard feelings; from one developer to another:
Some other issues that have not yet been addressed:
Path disclosure:
URL: http://imperialconflict.com/rankings_ov … re&g=8
By converting the "g" get param to an array, the server tosses a fatal error which exposes a server path.
(ie. ?type=topfamilies_score&g[]=8 )
Like wise: It would be a good idea to hide/remove This topic.
Potential Target:
Why is this page even live?
I mean... Really?
http://imperialconflict.com/lib/db.lib
This should be inaccessible. Deny it!
Errors Displayed: ***ERRORS DIRECTLY FROM THE DB DRIVER***
Honestly, this has me a little worried and second thinking my kindness of leaving.
But, I am an advocate of practising good security. As such, I feel like this is an important message to note: While it is fine and dandy to have errors displayed on a dev/local copy of the site... In NO instance should an error ever be shown on a production site.
There are no exceptions...
This include the fatal error above, but this MORE SO applies to errors from poorly written (and sanitized) queries.
Converting the id param to an array on http://imperialconflict.com/profile.php?id=1 will show you an error that comes directly from the driver. This should NEVER... Ever, EVER EVER be shown on a production site.
Not only is it an error (see above), but mysql errors show partial queries.
Please fix it.
(sidenote: There is also a SQLi target that relates to this. I didnt bother checking in depth)
The fix for the errors is a simple one. Check for isset and is_numeric/is_string and/or isset/empty intval/trim combo.
(ie.
if (isset($_GET['p'])&&is_numeric($_GET['p']) { do_stuff... }
if (isset($_GET['p'])&&!empty(trim($_GET['p']) { do_stuff... }
if (isset($_GET['p'])&&!empty(intval($_GET['p']) { do_stuff... })
That should eliminate the underlying problem. However the other issue is that errors are still shown. error_reporting(0).
You should be watching error logs anyways. All errors should be transparent to end users. This is how you provide a good user experience.
Lastly, use parametrized queries. I dont know how many devs work on this site, but someone needs to spend at least a few days converting the ORM over to use parameters. Not built queries.
Developer to developer, I am in hopes that you will respond maturely to this post.
But, In the event that you dont, I am withholding a (so far) single XSS potential.
I look forward to hearing a response. I will check back at some point in the future.
Perhaps then, we can be on the "same team" next time... ![]()
Regards
(ps: You should consider reporting to the punBB people that they shouldnt rely on poor, outdated code. Especially since they *JUST* released an update...
Im talking about "preg_replace(): The /e modifier is deprecated".
Can be reproduced by forcefully crashing a reply, with text inside, and reloading it.
parser.php on lines 59, 835, and 836)
(pps: Again with the error reporting. You can turn this off in the ini...)