I have a querystring variable whitelist checker on my website. It checks the given url against permissible/allowed querystring variables (key and value). The first part checks the querystring key against allowed keys - if the key is not allowed (not in the whitelist) it will reject the querystring. the second part checks the "value" part of the querystring key/value to see if the value includes a bad word from a blacklist - if the value is in the blacklist it will reject the querystring.

This seems to work well but I notice from my server logs that the line that converts the querystring value to lowercase causes a PHP warning:

PHP Warning: strtolower() expects parameter 1 to be string, array given

This is the code:

$rejectqstr = "N";

//loop through the querystring, check each querystring KEY against the three whitelist arrays. Any key found which is not in the list will set the reject variable to Y

foreach ($_GET as $key => $value){

    if(in_array($key, $cmsnumparams) || in_array($key, $cmsstrparams) || in_array($key, $cmsboolparams)){

        //do nothing if it found

    } else {

        $rejectqstr = "Y";

    }

    //loop through the blacklist values and check each querystring value against the list

    $value = strtolower($value);

    foreach($cmsblklstparams as $blklist){

        if(strpos($value, $blklist) !== false){

            $rejectqstr = "Y";

        }

    }

}

its the line $value = strtolower($value); that is logged as a warning in the server logs.

I can't see what is wrong with this. My blacklist array (cmsblklstparams) is all lowercase so I convert the querystring value to lowercase before seeing if it is in the blacklist array.

This warning is not listed all the time in the server log so I guess it could be caused if a user tried to "inject" something in the querystring (changing it from a string to an array)?

Is there a better way of doing this or should I check if $value is an array (if so, reject the querystring) before converting it to lowercase?

I've tried testing the code with normal querystrings and it seems to work ok so I'm not sure what it being added to the querystring to cause this warning in the server logs.

Many thanks.

CodePudding user response：

I guess it could be caused if a user tried to "inject" something in the querystring (changing it from a string to an array)?

I would agree, either intentionally or unintentionally. Take for example the following HTML form:

<form action="my-request.php" method="GET">
  <label for="checkbox-setting1">
    Setting 1
    <input type="checkbox" id="checkbox-setting1" name="setting[]" value="setting1" />
  </label>
  <label for="checkbox-setting2">
    Setting 2
    <input type="checkbox" id="checkbox-setting2" name="setting[]" value="setting2" />
  </label>
  <label for="checkbox-setting3">
    Setting 3
    <input type="checkbox" id="checkbox-setting3" name="setting[]" value="setting3" />
  </label>
  <button type="submit">Search</button>
</form>

Assume this form is submitted with every checkbox checked, it will make a request using the URL .../my-request.php?setting[]=setting1&setting[]=setting2&setting[]=setting3

This is a valid GET request and PHP will interpret the setting query string parameter as an array. The way your current code is setup is that it always assumes that the incoming query string parameters will be a String.

It sounds like you want to do a type check to check if the currently iterated query string parameter is an array. If it is and you do not allow for arrays, then do your rejection. If it is and you do allow for arrays, then setup an inner loop:

// do not allow query string parameter values
if (is_array($value))
{
  $rejectqstr = "Y";
}

// or allow them
foreach($cmsblklstparams as $blklist)
{
  if (is_array($value))
  {
    foreach($value as $innerValue)
    {
      if(strpos($innerValue, $blklist) !== false)
      {
        $rejectqstr = "Y";
      }
    }
  }
  else if(strpos($value, $blklist) !== false)
  {
    $rejectqstr = "Y";
  }
}