need security advice

[AviNahum]

hey,

I wrote this function to handle and secure all GET and POST inputs.

    //-----------------------------------------------------------------------
    // Make all inputs and incoming data safe
    //-----------------------------------------------------------------------

    public function secure_incoming()
    {
          global $HTTP_GET_VARS, $HTTP_POST_VARS;
      
        // Get vars array
        if( is_array($HTTP_GET_VARS) )
        {
            foreach ($HTTP_GET_VARS as $k => $v)
            {
                $return = $this->clean_value($v);
            }
        }
    
        // Post vars array
        if( is_array($HTTP_POST_VARS) )
        {
            foreach ($HTTP_POST_VARS as $k => $v)
            {
                $return = $this->clean_value($v);
            }
        }
    
        // Return the language array
        return $return;
    }

    
    //-----------------------------------------------------------------------
    // Clean value from injects
    //-----------------------------------------------------------------------
    
    public function clean_value($val)
    {
        // If the value are empty return now to save some CPU
        if ($val == "")
        {
            return "";
        }
        
        $val = str_replace( " ", " ", $val );
        
        $val = str_replace( "&"            , "&"         , $val );
        $val = str_replace( "<!--"         , "<!--"  , $val );
        $val = str_replace( "-->"          , "-->"       , $val );
        $val = preg_replace( "/<script/i"  , "<script"   , $val );
        $val = str_replace( ">"            , ">"          , $val );
        $val = str_replace( "<"            , "<"          , $val );
        $val = str_replace( """           , """        , $val );
        $val = preg_replace( "/ /"        , "<br>"          , $val );
        $val = preg_replace( "/\$/"      , "$"        , $val );
        $val = preg_replace( "/ /"        , ""              , $val );
        $val = str_replace( "!"            , "!"         , $val );
        $val = str_replace( "'"            , "'"         , $val );
        
        // Swop user inputted backslashes
        
        $val = preg_replace( "/\(?!&#|?#)/", "\", $val ); 
        
        return $val;
    }

 

I need it to be very secure without any gaps, I'd be happy if someone would help me improve it and make in safer....

 

Thanks in advance!

[waynew]

1: You're using old style HTTP_GET_VARS and HTTP_POST_VARS

Use $_POST instead of HTTP_POST_VARS and $_GET instead of HTTP_GET_VARS

 

2: You're creating your own custom cleaning? I wouldn't do that.

Use htmlentities() to turn HTML characters into safer characters that can't cause XSS. Use strip_tags() if you're wanting to strip HTML tags out. Use mysql_real_escape_string() to make variables safe for insertion into queries.

 

3: I'm guessing that this is apart of a class? If so, get rid of the use of global. You don't need it.

[Adam]

I agree with waynewex, the HTTP_*_VARS arrays are deprecated and you shouldn't be using them, especially within what seems to be a PHP5 class. I also don't understand why you're replacing a load of strings, with the exact same string? On the other hand I don't necessarily agree with waynewex that creating your own custom cleaning is wrong, but I don't think it should be applied to all inputs as a whole. You often need data for different things and so the cleaning / filtering of it should be done on a per-input basis.

 

I'd recommend a good read up on security to be honest.

[AviNahum]

thank you both,

but i tried this script now, and it's wont work!

it's returns the array key.

 

$core->input = $core->secure_incoming();
echo $core->input['s'];

 

it's shows me 's' (the key) even if the value is somthing else...

any ideas?

 

my new code:

    //-----------------------------------------------------------------------
    // Make all inputs and incoming data safe
    //-----------------------------------------------------------------------

    public function secure_incoming()
    {
        // Get vars array
        if( is_array($_GET) )
        {
            foreach ($_GET as $k => $v)
            {
                $return = $this->clean_value($v);
            }
        }
    
        // Post vars array
        if( is_array($_POST) )
        {
            foreach ($_POST as $k => $v)
            {
                $return = $this->clean_value($v);
            }
        }
    
        // Return the language array
        return $return;
    }

    
    //-----------------------------------------------------------------------
    // Clean value from injects
    //-----------------------------------------------------------------------
    
    public function clean_value($val)
    {
        // If the value are empty return now to save some CPU
        if ($val == "")
        {
            return "";
        }
        
        $val = str_replace( " ", " ", $val );
        
        $val = str_replace( "&"            , "&"         , $val );
        $val = str_replace( "<!--"         , "<!--"  , $val );
        $val = str_replace( "-->"          , "-->"       , $val );
        $val = preg_replace( "/<script/i"  , "<script"   , $val );
        $val = str_replace( ">"            , ">"          , $val );
        $val = str_replace( "<"            , "<"          , $val );
        $val = str_replace( "\""           , """        , $val );
        $val = preg_replace( "/\n/"        , "<br>"          , $val );
        $val = preg_replace( "/\\\$/"      , "$"        , $val );
        $val = preg_replace( "/\r/"        , ""              , $val );
        $val = str_replace( "!"            , "!"         , $val );
        $val = str_replace( "'"            , "'"         , $val );
        
        // Swop user inputted backslashes
        
        $val = preg_replace( "/\\\(?!&#|\?#)/", "\", $val ); 
        
        return $val;
    }

[mraiur]

i think u are doing two things wrong.

1. str_replace accepts arrays also. so there is no need to have 10-20-100 lines of code str_replace

You can do it like str_replace( $replacement_array, $matched_array, $fromWhere);

2. u do loop the get and post but u replace every time $return.

I thing its bether to make an empty array and fill it whit the cleared values and then return it.

[waynew]

I agree with waynewex, the HTTP_*_VARS arrays are deprecated and you shouldn't be using them, especially within what seems to be a PHP5 class. I also don't understand why you're replacing a load of strings, with the exact same string? On the other hand I don't necessarily agree with waynewex that creating your own custom cleaning is wrong, but I don't think it should be applied to all inputs as a whole. You often need data for different things and so the cleaning / filtering of it should be done on a per-input basis.

 

I'd recommend a good read up on security to be honest.

 

Using custom cleaning as a replacement is bad in my opinion. Sure, you can add extra layers on top of those fundamental PHP functions, but I wouldn't throw them aside and use my own.

[AviNahum]

i tried this script without the clean_value function and i got the same result... but now it's dispaly the first character of the string.

for example $core->input['s'] = "abcd";

it's returns only the first character, "a"

i think the problem is on my foreach loop, but can't find it...

 

i dont use htmlspecialchars for some reason. i need it just to replace only a few characters...   

[mraiur]

And yet again i repeat " // Return the language array

        return $return;" u dont return a array but the last value u assign to the "$return"

[AviNahum]

thank you all!

it's works great now!

the working script:

    //-----------------------------------------------------------------------
    // Make all inputs and incoming data safe
    //-----------------------------------------------------------------------

    public function secure_incoming()
    {
        // Get vars array
        if( is_array($_GET) )
        {
            foreach ($_GET as $k => $v)
            {
                $return[$k] = $this->clean_value($v);
            }
        }
    
        // Post vars array
        if( is_array($_POST) )
        {
            foreach ($_POST as $k => $v)
            {
                $return[$k] = $this->clean_value($v);
            }
        }
    
        // Return an array
        return $return;
    }

    
    //-----------------------------------------------------------------------
    // Clean value from injects
    //-----------------------------------------------------------------------
    
    public function clean_value($val)
    {
        // If the value are empty return now to save some CPU
        if ($val == "")
        {
            return "";
        }
        
        $val = str_replace( " ", " ", $val );
        
        $val = str_replace( "&"            , "&"         , $val );
        $val = str_replace( "<!--"         , "<!--"  , $val );
        $val = str_replace( "-->"          , "-->"       , $val );
        $val = preg_replace( "/<script/i"  , "<script"   , $val );
        $val = str_replace( ">"            , ">"          , $val );
        $val = str_replace( "<"            , "<"          , $val );
        $val = str_replace( """           , """        , $val );
        $val = preg_replace( "/ /"        , "<br>"          , $val );
        $val = preg_replace( "/\$/"      , "$"        , $val );
        $val = preg_replace( "/ /"        , ""              , $val );
        $val = str_replace( "!"            , "!"         , $val );
        $val = str_replace( "'"            , "'"         , $val );
        
        // Swop user inputted backslashes
        
        $val = preg_replace( "/\(?!&#|?#)/", "\", $val ); 
        
        return $val;
    }

 

Thanks again!

 

but you got any ideas to make it more "safe"?

oh and btw, sorry for poor english...

[Adam]

Ah no I didn't mean that, more so what you've just said; building upon them.