Crystal Market (SECURE)

[Guest Null]

Ive secured the crystal market to the best of my knowledge. Note: I did not use sprintf, as it is pretty much useless.

heres the code...

You're unable to view this code.

Viewing code within this forum requires registration, you can register here for free.

[Joshua]

switch($_GET['action'])

 

Secure your Switch action, Use a whitelist array. Just a bit extra :)

 

$q=$db->query("SELECT cm.*, u.* FROM crystalmarke

Zero is going to tell you not to select * From the tables, tell it what you want to select :P

 

$each= abs(intval($r['cmPRICE'] / $r['cmQTY']));

 

Not needed. If you wanted to secure that tho, try just using ".number_format( wherever it prints out ) "shrugz"

 

May be just me, i'm sure it is but

{$r['username']} I like to stripslashes usernames at bare minimum, usually will htmlentities them to but that kills <font tags

 

function crystal_buy()

{

global $db,$ir,$c,$userid,$h;

$q=$db->query("SELECT * FROM crystalmarket cm WHERE cmID={$_GET['ID']}");

 

$_GET ID isnt secure. You have to secure in Each Function. Top of the page security wont cut it here.

 

Use echo and ' instead of print and " speeds up the script a bit faster, few other output variables that could be a bit better secured, but you're trying so good job :)

[Dabomstew]

Some comments on both the original post and the corrections by Immortalthug:

You're unable to view this code.

Viewing code within this forum requires registration, you can register here for free.

This isn't going to work. Why not? The crystal market index doesn't take an ID parameter, so the index page will be unable to be viewed unless you randomly append an ID onto the end of the url.

Whitelisting:

While whitelisting is an effective security technique, it isn't needed in this case because the parameter in question (action) isn't used outside the switch statement, and isn't displayed - if anything besides "add", "remove", or "buy" is passed the index page for the market is shown, which is perfectly fine behavior in this instance.

 

$q=$db->query("SELECT cm.*, u.* FROM crystalmarke

Zero is going to tell you not to select * From the tables, tell it what you want to select

This is true, this can cut some valuable time off your scripts especially when your users table is quite large and you only want one or two fields.

 

You're unable to view this code.

Viewing code within this forum requires registration, you can register here for free.

$_GET ID isnt secure. You have to secure in Each Function. Top of the page security wont cut it here.

No, in this case it is fine. Why? $_GET is a superglobal, so the abs(intval()) applied earlier to it at the top of the page will still be in effect when it is used here - so the worst thing that can happen is it searching for a crystal market ID of 0, which will just not find anything.

However, enclosing values in your SQL queries with single quotes is always a good idea for MCCodes - since it forces all inputs (GET/POST) to be escaped with magicquotes or equivalent, having quotes around all the input variables you use in queries will block pretty much any possible SQL injection from inputs. (The IP exploit, of course, being a nasty exception - but that wasn't an input anyway, rather a stupid oversight, as SERVER variables escape such filtering)

So something like

You're unable to view this code.

Viewing code within this forum requires registration, you can register here for free.

Would be even better - just in case you forget to implement the abs(intval()) filtering!

Dabomstew

[Joshua]

Dabom you say it's fine, but I promise you the Cmarket hack would work on this because the GET isnt secured.

^_-

As for the actions being secured, it's good practice, and not all injections are sql :)

[Dabomstew]

Dabom you say it's fine, but I promise you the Cmarket hack would work on this because the GET isnt secured.

^_-

As for the actions being secured, it's good practice, and not all injections are sql :)

Definitely agreed on the principle of good practice.

However, for this particular example code, all the input variables used in queries are secured.

The ID is secured at the top of the page - which means, as I said earlier, all subsequent uses of it will also be secured since it is a superglobal.

The only action which uses other input variables, adding the listings, secures its variables inside the function - thus these are also fine.

I might have missed something here - but I'm almost certain that in this case the cmarket hack is prevented in all its facets.

It is always good practice to get into the habit of securing things as much as you can, though - for the simple reason that if you sometimes forget to implement one of the measures, you should still be fine as you will be covered by the other measures.

[Guest Null]

So... Havent seen you on here in a whileeeee... dabomstew.

[Joshua]

globals.php is a super global also

However

 

If that were the case, why do you have to define $db, $ir, etc in each function? ;-)

 

I have tested this, I promise you, you have to define it in Each Function :-)

[Dabomstew]

globals.php is a super global also

However

 

If that were the case, why do you have to define $db, $ir, etc in each function? ;-)

 

I have tested this, I promise you, you have to define it in Each Function :-)

Err.. what?

$_GET, $_POST, etc are PHP superglobals defined in PHP.

From the PHP manual:

Several predefined variables in PHP are "superglobals", which means they are available in all scopes throughout a script. There is no need to do global $variable; to access them within functions or methods.

This means that no matter what context you are using it in, values in $_GET are the same, and if you change it in one place in a script it will remain the same for any code executed after, including that within functions and methods.

$db, $ir are just variables made by MCCodes, not PHP-hardcoded superglobals and thus you must define them as globals to use them as such.

Example of $_GET as a superglobal:

 

<?php

$_GET['blah']="Fail";

blah2();

function blah2()

{

print $_GET['blah'];

}

?>

Output is

Fail

[Joshua]

Ah wasnt following properly.

Using wrong terminology.

If you slap $_GET = abs(@($_GET at the top

For reasons unbenknowns to me it is still not secured in the individual functions.

This is why you will see it secured over and over in Each function.

I used to think that you could secure it at the top of the function, till i saw differantly >,<

That's why simply slapping $_GET['ID'] = abs(@intval($_GET['ID'])); into header won't secure all the GET['ID'] s

[Dabomstew]

Ah wasnt following properly.

Using wrong terminology.

If you slap $_GET = abs(@($_GET at the top

For reasons unbenknowns to me it is still not secured in the individual functions.

This is why you will see it secured over and over in Each function.

I used to think that you could secure it at the top of the function, till i saw differantly >,<

That's why simply slapping $_GET['ID'] = abs(@intval($_GET['ID'])); into header won't secure all the GET['ID'] s

OK, that's very strange...

Pass this script a GET variable ("input") with a string and you'll see what I mean...

 

<?php

$_GET['input'] = abs(@intval($_GET['input']));

some_other_function();

function some_other_function()

{

print $_GET['input'];

}

?>

Running this script as script.php?input=SOMETHING MALICIOUS HERE gives the output:

0

Therefore something very odd must have been happening with your security at the top of the page, as it clearly works.

Although securing it again in the individual functions doesn't hurt anything, it still isn't needed and I don't have an explanation as to why it wouldn't have worked for you, as by all the rules of PHP I know it should definitely work.

[Zeggy]

Dabomstew is right. Maybe what ImmortalThug did was use $_GET = abs(int($_GET)); (from seeing his last post, but maybe he just didn't bother to finish the example), in which case it would fail because $_GET is an array.

Just wondering, why is sprintf useless?

[Coly010]

Fixed and changed for LITE users

 

You're unable to view this code.

Viewing code within this forum requires registration, you can register here for free.

[Joshua]

Tried this cmarket out, not one i did, and cmarket hack will still work on it.

I've seen this several times before on multiple scripts, for various random reasons the $_GET['blah'] should be secured in each function. Try finding a script with multiple functions using the same $_GET and try to exploit each bit using +999999999999-

[Zero-Affect]

I really am shocked if that's the real Dabs then wow your giving security tips... ARE YOU KIDDING?

Fix MC before even considering giving out tips - how about giving people their money back for rubbish products... jumped up little brat.

on topic:

Immortal shut up * is what i comment on your taking away my only pleasure :(

 

Tried this cmarket out, not one i did, and cmarket hack will still work on it.I've seen this several times before on multiple scripts, for various random reasons the $_GET['blah'] should be secured in each function. Try finding a script with multiple functions using the same $_GET and try to exploit each bit using +999999999999-

Interesting what if he did

 

You're unable to view this code.

Viewing code within this forum requires registration, you can register here for free.

instead of

You're unable to view this code.

Viewing code within this forum requires registration, you can register here for free.

[Joshua]

Bah sorry to steal your glory ^_-

 

I know that the $_GET should cover the entire page if secured at the top.

But I've found it better to secure it in each function as occasionally for reasons unbenknownst to me, it doesnt always secure every call.

 

Now Zero can have fun =p

[Zero-Affect]

Weird could you show me a example of that because in all the time i've done that it's never once failed for me, im sure there is another reason why in every function it should be done.

Immortal adding something like the classic "Click Click Boom" into header may actually be efficient but not 100% which is why it's better to secure in every file rather than just header, which is probably why it's better in every function (debatable).

[Joshua]

Try and add that to the basic Cmarket only at the top of the page

Then try hacks >,<

[Joshua]

or try the yourgang.php file

Secure the Mass Payment $_POST at the very top of Yourgang.php

Then try the hack, see if you still get the event

[Zero-Affect]

I see your point there but considering i've not used Generic of anything in almost a year i'd be a little worried, i recoded gangs like 8 month ago - now i don't even run MC.

We all know it's not worth the hassle don't we Dabs :rolleyes:

[Joshua]

You know what made me decide to fix mccodes for myself? lol

Everyone saying it couldnt be secured and "wasn't" worth the hassle

So i spent entirely to darn much time ('Were talking months") to get it where it is now >,<

 

shrugz, we all need a project =P

If you're looking for a BIGGGGGGGGGGGGg project

get mccodes

If you're looking for something that comes secure already, Horizons is nice.

[Zero-Affect]

If your looking to waste time on a lost course that people have done a million times get MC.

If you want to have trust and compassion for your customer/members get something like Horizon.

MC i have edited everything so i know how much of a waste of space it actually is, i have recoded every single feature and function and if dabs had let people from MWG/CE help him im sure MCv2 would of been greatly more secure than it is.

[Joshua]

LOL no doubt about that last comment ><

[rulerofzu]

All very well but the reason why everyone is still going for mc2 is due to the amount of mods listed. Compared to the 0 listed here for horizons then it just looks more appealing.

[bluegman991]

immortal is right

in a function you have to redefine your variables

or u can just put this instead of re-defining them

global $var1,$var2;

but im not sure if when u global a $_GET vaiable it will import the variable u secured or import it straight from the url

[bluegman991]

i was wondering tho in the sql query immortal said not to SELECT * FROM

i was wondering how to select a certain field from the table and print it without selecting them all