PHP Security Question

  • PolishHurricane
  • Mastermind
  • Mastermind
  • User avatar
  • Posts: 1585

Post 3+ Months Ago

I'm pretty sure this is totally fine, but for some reason I'm paranoid about it as I don't typically never throw anything unfiltered into an include or do anything like this, but does anybody know of any instance where this is not safe and can be exploited in any way?

PHP Code: [ Select ]
<?php
$VALID_PAGES = array(
'news' => 'news.php',
'statistics' => 'site_statistics.php'
);
 
if(isset($an_array[$_REQUEST['page']]))
{
require_once($an_array[$_REQUEST['page']]);
}
else
{
require_once('default.php');
}
?>
  1. <?php
  2. $VALID_PAGES = array(
  3. 'news' => 'news.php',
  4. 'statistics' => 'site_statistics.php'
  5. );
  6.  
  7. if(isset($an_array[$_REQUEST['page']]))
  8. {
  9. require_once($an_array[$_REQUEST['page']]);
  10. }
  11. else
  12. {
  13. require_once('default.php');
  14. }
  15. ?>


I normally use a switch statement for these types of things with pre-filled values, but it's too long and I don't want it for other reasons.
  • joebert
  • Fart Bubbles
  • Genius
  • User avatar
  • Posts: 13504
  • Loc: Florida

Post 3+ Months Ago

It's not checking whether $_REQUEST['page'] exists before using it. The code could potentially expose a file path via an error notice depending on the value of error_reporting.

Does something like this make you think any differently ?

PHP Code: [ Select ]
<?php
 
$p = empty($_GET['p']) ? 'index' : $_GET['p'];
 
if(strcspn($p, '1234567890-_abcdefghijklmnopqrstuvwxyz'))
{
   $p = 'index';
}
 
if( ! is_readable("/www/lib/$p.php") || ! is_readable("/www/templates/$p.html"))
{
   $p = 'index';
}
 
require_once("/www/lib/$p.php");
 
?>
  1. <?php
  2.  
  3. $p = empty($_GET['p']) ? 'index' : $_GET['p'];
  4.  
  5. if(strcspn($p, '1234567890-_abcdefghijklmnopqrstuvwxyz'))
  6. {
  7.    $p = 'index';
  8. }
  9.  
  10. if( ! is_readable("/www/lib/$p.php") || ! is_readable("/www/templates/$p.html"))
  11. {
  12.    $p = 'index';
  13. }
  14.  
  15. require_once("/www/lib/$p.php");
  16.  
  17. ?>
  • PolishHurricane
  • Mastermind
  • Mastermind
  • User avatar
  • Posts: 1585

Post 3+ Months Ago

Well yeah, it's not checking whether the request exists in this example, I omitted that line, it's pretty much theory anyway.

I've never seen that strcspn() function that I can recall, I also messed with it, it seems to just return the same int or sometimes the same input regardless of if the string contains or doesn't contain those characters, are you sure that function is a reliable filter? I find that interesting though.

Nice call on that is_readable() for file path error. I have a lot of pages so that may be good, but question... Is is_readable() slow? I'm trying to keep the page loading optimized.

Thanks a lot Joe
  • joebert
  • Fart Bubbles
  • Genius
  • User avatar
  • Posts: 13504
  • Loc: Florida

Post 3+ Months Ago

strcspn can be a little confusing. I regularly have to consult the manual after going awhile without using it because I get strcspn and strspn mixed up because they do the exact opposite thing. Though I have an idea for remembering which one is which right now and I'll get to that later.

strcspn basically looks for characters that aren't in that character mask. From the manual,

Quote:
int strcspn ( string $str1 , string $str2 [, int $start [, int $length ]] )

Returns the length of the initial segment of str1 which does not contain any of the characters in str2.


If you track down the function in the source, you come to this where you can see how the function works internally. It just walks through the string looking for something it doesn't expect, then looks for something it does expect and returns the length of that segment.

etc/standard/string.c:1571

C Code: [ Select ]
PHPAPI size_t php_strcspn(char *s1, char *s2, char *s1_end, char *s2_end)
{
   register const char *p, *spanp;
   register char c = *s1;
 
   for (p = s1;;) {
      spanp = s2;
      do {
         if (*spanp == c || p == s1_end) {
            return p - s1;
         }
      } while (spanp++ < (s2_end - 1));
      c = *++p;
   }
   /* NOTREACHED */
}
  1. PHPAPI size_t php_strcspn(char *s1, char *s2, char *s1_end, char *s2_end)
  2. {
  3.    register const char *p, *spanp;
  4.    register char c = *s1;
  5.  
  6.    for (p = s1;;) {
  7.       spanp = s2;
  8.       do {
  9.          if (*spanp == c || p == s1_end) {
  10.             return p - s1;
  11.          }
  12.       } while (spanp++ < (s2_end - 1));
  13.       c = *++p;
  14.    }
  15.    /* NOTREACHED */
  16. }


Any time strcspn returns a number other than zero, there's bad characters in the input and we overwrite the entire bad input with the default value. This is just a simple course of action after finding bad input, in reality you would probably end up looking for possible typos, check a 301 table, or something along those lines.

Now, as far as that trick for remembering which function does which I mentioned earlier, I'll have to borrow a comment from the PHP manual.

Quote:
strcspn() can also be thought of as analogous to the following regular expression:
PHP Code: [ Select ]
<?php
// where ... represents the mask of characters
preg_match('/[^ ...]/', substr($subject, $start, $length) );
?>
  1. <?php
  2. // where ... represents the mask of characters
  3. preg_match('/[^ ...]/', substr($subject, $start, $length) );
  4. ?>


If I remember strcspn and strspn as character classes from regular expressions, I just have to remember that in a negated character class there is that extra character "^" and in the function name strcspn there is also an extra character "c".

--

I really can't say much about is_readable, I use it in the example because the tone of the conversation seems to be security and mistake-prevension-wise is_readable is less accident prone than something like file_exists. I prefer to use file_exists. I'm pretty good about making sure something is going to work before it goes live, by using defaults the way I do I'm not going to get an error about an unreadable file except in a catastrauphic failure, and there's no reason for my file permissions to change later on.
  • PolishHurricane
  • Mastermind
  • Mastermind
  • User avatar
  • Posts: 1585

Post 3+ Months Ago

Damn that PHP function is confusing. I just downloaded me a copy of the PHP source code though (if I can understand half of it). I figured it must be worth it considering you keep showing it to me in threads. :)

Anyway, I think we have a problem man. Say I had the following script...

PHP Code: [ Select ]
<?php
$test = $_GET['page'];
echo 'strcspn: '.strcspn($test,'abc');
?>
  1. <?php
  2. $test = $_GET['page'];
  3. echo 'strcspn: '.strcspn($test,'abc');
  4. ?>


If page is 'abc', then the result is: 0
If page is 'dabc', then the result is: 1
If page is 'abcd', then the result is: 0 (Yes, ZERO, uh oh)

So you can't really rely on that for security sadly.
  • joebert
  • Fart Bubbles
  • Genius
  • User avatar
  • Posts: 13504
  • Loc: Florida

Post 3+ Months Ago

Well now, that is a problem. I dare say it even seems like a bug in the function.

What about using strspn in combination with strlen then ?

PHP Code: [ Select ]
if(strlen($str) == strspn($str, 'abc'))
  • PolishHurricane
  • Mastermind
  • Mastermind
  • User avatar
  • Posts: 1585

Post 3+ Months Ago

lol, I think I'm gonna stick with regex and ctype if I need or something else, thanks man. haha
  • joebert
  • Fart Bubbles
  • Genius
  • User avatar
  • Posts: 13504
  • Loc: Florida

Post 3+ Months Ago

Yeah, I did a lot of grep-ing after that post and come to find out I don't use the strcspn function anywhere on any of my sites or scripts. I think it's one of those things that sounds like a really good idea one day but I never got around to implementing.

I submitted a documentation bug report to PHP.net, it needs to be more clear in the function description that when they say "initial" it means the start of the test subject, and not the initial section of bad characters found.
  • PolishHurricane
  • Mastermind
  • Mastermind
  • User avatar
  • Posts: 1585

Post 3+ Months Ago

ah cool
  • joebert
  • Fart Bubbles
  • Genius
  • User avatar
  • Posts: 13504
  • Loc: Florida

Post 3+ Months Ago

Received a reply to my bug report for the PHP manual on strcspn, there's now a code example in the SVN copy of the manual, the online manual should follow sooner or later. :)

http://svn.php.net/viewvc/?view=revisio ... ion=302077
  • PolishHurricane
  • Mastermind
  • Mastermind
  • User avatar
  • Posts: 1585

Post 3+ Months Ago

lol, nice.

Post Information

  • Total Posts in this topic: 11 posts
  • Users browsing this forum: Liamw411 and 106 guests
  • You cannot post new topics in this forum
  • You cannot reply to topics in this forum
  • You cannot edit your posts in this forum
  • You cannot delete your posts in this forum
  • You cannot post attachments in this forum
 
 

© 1998-2014. Ozzu® is a registered trademark of Unmelted, LLC.