dcsimg
www.webdeveloper.com
Results 1 to 7 of 7

Thread: Make this code more secure

  1. #1
    Join Date
    Jan 2015
    Posts
    70

    Make this code more secure

    Hi all. I have some code which I know is a problem to SQL injection.

    PHP Code:
    {
    $sql "

    SELECT articles.id AS articleid, articles.art_title AS title, articles.art_url AS art_url, articles.art_content AS content, articles.art_campaign AS artcampaign, campaigns.id AS campid, campaigns.camp_title AS camptitle, campaigns.camp_name AS campname, campaigns.camp_url AS camp_url
    FROM articles
    LEFT JOIN campaigns ON articles.art_campaign = campaigns.id
    WHERE art_url = \"" 
    .$_GET['art_url'] . "\"
    "
    ;

    The part in question is the
    PHP Code:
    .$_GET['art_url'] . "\" 
    part.

    What do I need to do to make it secure?

  2. #2
    Join Date
    Aug 2004
    Location
    Ankh-Morpork
    Posts
    21,978
    Best option: use either the PDO (my preference) or MySQLi extension and make use of their ability to create prepared statements along with bound input parameters.

    Inferior option if using the depracated MySQL extension: use the mysql_real_escape_string() function.

    With a PDO connection, it would be something like:
    PHP Code:
    $pdo = new PDO('mysql:host=localhost;dbname=testdb'$dbUser$dbPass);
    $sql "
    SELECT
      articles.id AS articleid,
      articles.art_title AS title, 
      articles.art_url AS art_url, 
      articles.art_content AS content, 
      articles.art_campaign AS artcampaign, 
      campaigns.id AS campid, 
      campaigns.camp_title AS camptitle, 
      campaigns.camp_name AS campname, 
      campaigns.camp_url AS camp_url
    FROM articles
    LEFT JOIN campaigns ON articles.art_campaign = campaigns.id
    WHERE art_url = :art_url"
    ;
    $stmt $pdo->prepare($sql);
    $stmt->bindParam(':art_url'$_GET['art_url']); // takes care of sanitation
    $stmt->execute();
    while(
    $row $stmt->fetch(PDO::FETCH_ASSOC)) {
        
    // do stuff with result row

    "Well done....Consciousness to sarcasm in five seconds!" ~ Terry Pratchett, Night Watch

    How to Ask Questions the Smart Way (not affiliated with this site, but well worth reading)

    My Blog
    cwrBlog: simple, no-database PHP blogging framework

  3. #3
    Join Date
    Jan 2015
    Posts
    70
    my preference is mysqli. How would this look in that?
    I missed a bit out at the top which might make a difference. This is basically a results page after clicking a link if that makes a difference.
    PHP Code:
    <?php
    if(!empty($_GET['art_url']))
    {
    $sql "

    SELECT articles.id AS articleid, articles.art_title AS title, articles.art_url AS art_url, articles.art_content AS content, campaigns.id, campaigns.camp_title AS camptitle, campaigns.camp_name AS campname, campaigns.camp_url AS camp_url
    FROM articles
    LEFT JOIN campaigns ON articles.art_campaign = campaigns.id
    WHERE art_url = \"" 
    .$_GET['art_url'] . "\"
    "
    ;
    }
        
    // then do the query, etc....

    $results $db->query($sql);

    if(
    $results->num_rows) {
        While(
    $row $results->fetch_object()) {
            
    $campname nl2br(htmlentities($row->campname));
            
    $camptitle nl2br(htmlentities($row->camptitle));
            
    $title nl2br(htmlentities($row->title));
                    
    $content = ($row->content);
            
    $art_url = ($row->art_url);
            
    $sid 'Sid';
            echo 
    "
            <div class='content'>
            <h2>
    {$campname}</h2>
            <H3>
    {$camptitle}</H3>
            <H4>
    {$title}</H4>
            "
    ;

            echo 
    "
            
    {$content}
            </div>
    Last edited by TheTrooper; 07-02-2017 at 03:27 AM. Reason: missing explanation

  4. #4
    Join Date
    Aug 2004
    Location
    Ankh-Morpork
    Posts
    21,978
    It's very similar, except MySQLi only supports positional place-holders with the "?" character, which is one of the reasons I prefer PDO, so that I can use named parameters. (The other reason is that it is not DBMS-specific.)
    PHP Code:
    $sql "
    SELECT
      articles.id AS articleid,
      articles.art_title AS title, 
      articles.art_url AS art_url, 
      articles.art_content AS content, 
      articles.art_campaign AS artcampaign, 
      campaigns.id AS campid, 
      campaigns.camp_title AS camptitle, 
      campaigns.camp_name AS campname, 
      campaigns.camp_url AS camp_url
    FROM articles
    LEFT JOIN campaigns ON articles.art_campaign = campaigns.id
    WHERE art_url = ?"
    // mysqli only supports positional place-holders

    $stmt $db->prepare($sql);
    $stmt->bind_param('s'$_GET['art_url']); // assuming it's a string?
    $results $stmt->execute(); 
    "Well done....Consciousness to sarcasm in five seconds!" ~ Terry Pratchett, Night Watch

    How to Ask Questions the Smart Way (not affiliated with this site, but well worth reading)

    My Blog
    cwrBlog: simple, no-database PHP blogging framework

  5. #5
    Join Date
    Jan 2015
    Posts
    70
    That cool and makes sense. Do I need to bind_param any other information I am going to output from the database, or is the art_url just the important one here?

  6. #6
    Join Date
    Jan 2015
    Posts
    70
    I actually have a an error which says "Number of variables doesn't match number of parameters in prepared statement". To me looks as though listed correctly

    PHP Code:
     <?php
    $sql 
    "
    SELECT articles.id AS articleid, articles.art_title AS title, articles.art_url AS art_url, articles.art_content AS content, campaigns.id, campaigns.camp_title AS camptitle, campaigns.camp_name AS campname, campaigns.camp_url AS camp_url
    FROM articles
    LEFT JOIN campaigns ON articles.art_campaign = campaigns.id
    WHERE art_url = ?"
    ;

        
    $stmt $db->prepare($sql);
        
    $stmt->bind_param('ssssss'$_GET['art_url'], $campname$title$campname$camptitle$content);
        
    $results $stmt->execute();

            echo 
    "
            <h2>
    {$campname}</h2>
            <H3>
    {$camptitle}</H3>
            <H4>
    {$title}</H4>
            "
    ;
    ?>
            <div class='right-section'>
            <h5><?php echo "{$campname}"?> Links</h5>
            <ul calss='section-list'>
                <li>dummy text</li>
            </ul>
            </div>
            <?php echo "{$content}"?>
            </div>

  7. #7
    Join Date
    Aug 2004
    Location
    Ankh-Morpork
    Posts
    21,978
    The important thing security-wise is to bind input parameters, as the binding process will take care of any escaping, quoting, etc. that's needed to prevent SQL injection. Whether or not you use binding of output params, which basically assigns output fields to variable, I guess is more a style preference that I've personally never bothered with. I'm not aware of any functional benefit to it, but that could be because I never really looked into it.
    "Well done....Consciousness to sarcasm in five seconds!" ~ Terry Pratchett, Night Watch

    How to Ask Questions the Smart Way (not affiliated with this site, but well worth reading)

    My Blog
    cwrBlog: simple, no-database PHP blogging framework

Thread Information

Users Browsing this Thread

There are currently 1 users browsing this thread. (0 members and 1 guests)

Posting Permissions

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



Recent Articles