SitePoint Sponsor

User Tag List

Results 1 to 9 of 9
  1. #1
    SitePoint Member
    Join Date
    Jun 2006
    Posts
    6
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    Question Is my script secure?

    Let me start out by saying that I'm not using SQL or any databases...

    With that said, below is a script that uses superglobal arrays to pass data to a form using a URL like: http://www-site-com/index.php?price=100.00&title=laptop

    What would you do to make this script more secure?

    Code:
    <?php 
    function getIt($value){ 
      if($_GET[$value]) {
        switch ($value) {
          case 'price':
            $pattern = '/^[$0-9.]{1,20}$/';
            break;
          case 'title':
            $pattern = '/^[0-9a-zA-Z]{1,100}$/';
            break;
        }
        if (preg_match($pattern,$_GET[$value])) {
          return $_GET[$value];
        }
      }
      return false;
    }
    ?>
    Can you spot any security holes?

    Thanks!

  2. #2
    SitePoint Wizard silver trophy
    Join Date
    Mar 2006
    Posts
    6,132
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    there arent any security holes, but there may be unintended behavior. unintended behavior can lead to security holes in other places of your app.

    you should use the D flag unless you want to allow a newline character at the end of the string
    eg
    $pattern = '/^[0-9a-zA-Z]{1,100}$/D';

    preg_match() expects a string, use is_string() to make sure thats what the var is.

    your method of validating a price seems loose.
    first, do you _really_ want to accept the $ character?
    maybe you want something like this?

    $pattern = '/^\d{1,10}(\.\d{1,2})?$/D';

  3. #3
    Follow Me On Twitter: @djg gold trophysilver trophybronze trophy Dan Grossman's Avatar
    Join Date
    Aug 2000
    Location
    Philadephia, PA
    Posts
    20,580
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    Off Topic:

    Sooo..clamcrusher.. where do you live? When do you sleep? I'm having a hard time staying up later than you to get in on some replies here. It's almost dawn again.

    Sorry to hijack your thread Kubernes

  4. #4
    SitePoint Wizard silver trophy
    Join Date
    Mar 2006
    Posts
    6,132
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Off Topic:


    hehe bay area in california. about bedtime for me

  5. #5
    SitePoint Member
    Join Date
    Jun 2006
    Posts
    6
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by clamcrusher View Post
    there arent any security holes, but there may be unintended behavior. unintended behavior can lead to security holes in other places of your app.

    you should use the D flag unless you want to allow a newline character at the end of the string
    eg
    $pattern = '/^[0-9a-zA-Z]{1,100}$/D';

    preg_match() expects a string, use is_string() to make sure thats what the var is.

    your method of validating a price seems loose.
    first, do you _really_ want to accept the $ character?
    maybe you want something like this?

    $pattern = '/^\d{1,10}(\.\d{1,2})?$/D';

    Newline character? You mean if someone tried to insert \n?

    Also, I just realized that the title will need to accept spaces. How can I allow single spaces, and disallow anything over 1 single space in a row?

    By allowing spaces, would I also have to allow the % sign, since spaces are converted to: %20

    ...or do you think it would be better to just use underscores as spaces?

    Thanks again for your advice!


    P.S. > No problem, Dan Grossman.

  6. #6
    SitePoint Wizard silver trophy
    Join Date
    Mar 2006
    Posts
    6,132
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    yup, without the D flag your regex would approve of
    $test = "foo\n";
    http://www.php.net/manual/en/referen....modifiers.php

    to allow single spaces:
    PHP Code:
    $p '/^[0-9a-zA-Z](?=[0-9a-zA-Z ]{0,99}$)( ?[0-9a-zA-Z]+)*$/D'
    it gets complicated because you cannot easily measure total string length anymore, and must turn to using assertions, specifically lookahead. this can become cumbersome quickly if you have more conditions to test for, and personally i usually resort to using a combination of other methods, as a i feel its a bit more clear and i feel more confident in what im doing. im sure some will dissagree, i suppose it depends on how good you are with regex.

    other solutions could be
    PHP Code:
    // combine this with strpos() to check for dbl space, 
    // although this would allow leading/trailing spaces
    // not an issue if using trim()
    $p '/^[0-9a-zA-Z ]{1,100}$/D';

    // or combine this with strlen()
    $p '/^[0-9a-zA-Z]( ?[0-9a-zA-Z]+)*$/D'

    you wont need to worry about &#37;20. while thats how it may appear in the url, it wont be like that in your php value. not sure if php or the webserver does it, but the values get urldecode()'ed somewhere along the line.

    it depends on the context, but i generally like to apply trim() to most user supplied values which come from user inputs, as a convenience for them. it could be quite frustrating to a user if you tell them thier input is invalid, but they are not able to see the leading/trailing whitespace they accidently added.

  7. #7
    SitePoint Member
    Join Date
    Jun 2006
    Posts
    6
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    Thumbs up

    Quote Originally Posted by clamcrusher View Post
    yup, without the D flag your regex would approve of
    $test = "foo\n";
    http://www.php.net/manual/en/referen....modifiers.php

    to allow single spaces:
    PHP Code:
    $p '/^[0-9a-zA-Z](?=[0-9a-zA-Z ]{0,99}$)( ?[0-9a-zA-Z]+)*$/D'
    it gets complicated because you cannot easily measure total string length anymore, and must turn to using assertions, specifically lookahead. this can become cumbersome quickly if you have more conditions to test for, and personally i usually resort to using a combination of other methods, as a i feel its a bit more clear and i feel more confident in what im doing. im sure some will dissagree, i suppose it depends on how good you are with regex.

    other solutions could be
    PHP Code:
    // combine this with strpos() to check for dbl space, 
    // although this would allow leading/trailing spaces
    // not an issue if using trim()
    $p '/^[0-9a-zA-Z ]{1,100}$/D';

    // or combine this with strlen()
    $p '/^[0-9a-zA-Z]( ?[0-9a-zA-Z]+)*$/D'

    you wont need to worry about %20. while thats how it may appear in the url, it wont be like that in your php value. not sure if php or the webserver does it, but the values get urldecode()'ed somewhere along the line.

    it depends on the context, but i generally like to apply trim() to most user supplied values which come from user inputs, as a convenience for them. it could be quite frustrating to a user if you tell them thier input is invalid, but they are not able to see the leading/trailing whitespace they accidently added.
    Ok, thanks!

    Also, one last thing...how can I check to make sure an extra variable hasn't been added to the URL?

    For example, how can I tell the script to only process the variables: "price" and "title"? So if someone tried to enter something like food=hamburger, the script would detect that the variable "food" was added and ignore it?

    Someone told me that they could mess everything up by adding extra variables to the URL.

    Thanks again!

  8. #8
    Follow Me On Twitter: @djg gold trophysilver trophybronze trophy Dan Grossman's Avatar
    Join Date
    Aug 2000
    Location
    Philadephia, PA
    Posts
    20,580
    Mentioned
    1 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Kubernes View Post
    Someone told me that they could mess everything up by adding extra variables to the URL.
    Variables are automatically created out of GET/POST parameters if the PHP setting register_globals is enabled. If you have control of your environment, make sure that's disabled and you won't have to worry about this. If you don't, you'd have to loop over every key in $_GET and $_POST and unset variables by the same name except your title and price.

  9. #9
    SitePoint Member
    Join Date
    Jun 2006
    Posts
    6
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Dan Grossman View Post
    Variables are automatically created out of GET/POST parameters if the PHP setting register_globals is enabled. If you have control of your environment, make sure that's disabled and you won't have to worry about this. If you don't, you'd have to loop over every key in $_GET and $_POST and unset variables by the same name except your title and price.
    I see, thanks.

    Thank you both for your help! I really appreciate it.


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •