Apostrophe in search term kills my page

[eventide7]

I have a table of "artist" and "title" data for songs. When a user enters any search term that contains an apostrophe ("ain't" for example) it returns no text, not even the message that no matches were found. What am I doing wrong (and please use simple terms that an old technology-challenged lady can understand). Any assistance is appreciated.

    $pdo = new PDO("mysql:host=$host;dbname=$database_name", $user, $password, array(
    PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION
    ));
// Search from MySQL database table
$search=$_POST['search'];
$query = $pdo->prepare("select * from songs where artist LIKE '%$search%' OR title LIKE '%$search%'  LIMIT 0 , 100");
$query->bindValue(1, "%$search%", PDO::PARAM_STR);
$query->execute();
// Display search result
         if (!$query->rowCount() == 0) {
		 		echo "Search found :<br/>";
				echo "<table>";	
                echo "<tr><td>Artist</td><td>Title</td></tr>";				
            while ($results = $query->fetch()) {
				echo "<tr><td>";			
                echo $results['artist'];
				echo "</td><td>";
                echo $results['title'];
				echo "</td></tr>";				
            }
				echo "</table>";		
        } else {
            echo 'Your search for <b><i>' .$search . '</b></i> returned no results. <br>Please try your search again using fewer terms.';
        }

[Jacques1]

You're inserting $_POST['search'] straight into the query string, which means that a single quote will terminate the SQL string after the LIKE keyword and change the entire query. It's a self-inflicted SQL injection. What's much worse than your current bug is that anybody can manipulate the query and fetch sensitive data (passwords, e-mail addresses etc.) or even take over the server.

 

To fix this vulnerability, stop inserting PHP string into queries. The whole point of prepared statements is that you use a constant query with placeholders:

<?php

const DB_HOST = 'localhost';
const DB_USER = '...';
const DB_PASSWORD = '...';
const DB_NAME = '...';
const DB_CHARSET = 'UTF8';



/*
 * Set up the database connection
 * - the character encoding *must* be defined in the DSN string, not with a SET NAMES query
 * - be sure to turn off "emulated prepared statements" to prevent SQL injection attacks
 * - turn on exceptions so that you don't have to manually check for errors
 */
$dSN = 'mysql:host='.DB_HOST.';dbname='.DB_NAME.';charset='.DB_CHARSET;

$databaseConnection = new PDO($dSN, DB_USER, DB_PASSWORD, [
    PDO::ATTR_EMULATE_PREPARES => false,                 // activate real prepared statements
    PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,         // activate exceptions
    PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,    // fetch associative arrays by default (this isn't critical)
]);

$songStmt = $databaseConnection->prepare('
    SELECT
        artist,     -- always select *concrete* columns
        title
    FROM
        songs
    WHERE
        artist LIKE :artist_search_term
        OR title LIKE :title_search_term
    LIMIT
        100
');
$songStmt->execute([
    'artist_search_term' => $_POST['search'],
    'title_search_term' => $_POST['search'],
]);

foreach ($songStmt as $song)
{
    var_dump($song);
}

Edited March 1, 2016 by Jacques1

[Psycho]

To add a little more context to Jacques1's response. If a user entered "ain't" as the search term, the query that would be built using the current code would be

 

 

select *

from songs

where artist LIKE '%$ain't%'

OR title LIKE '%$ain't%'

LIMIT 0 , 100

 

The single quote in the word "ain't" closes the start of the search string leaving the t%' content hanging outside the search string - thus creating an invalid query. As Jacques1 was stating someone could use this security hole to do nasty things in your database by using content that would create a valid query(ies) to do as they please.

 

To prevent such a problem the content need to be escaped so the database understands that the quote mark in the word "ain't" is part of the search term and not a closing quote for the term. In the old days we would use a function that would escape certain characters based on the Database type (for MySql this would put a backslash before certain characters). Now, with prepared queries, as you are using, you  need to put a placeholder in the prepared query and then pass the value to use for that placeholder when executing the query. The database engine will escape the values automatically. Be sure to pay attention to how Jacques1 instantiates the DB connection. There is a parameter he included to make sure the prepared queries are actually doing what they are supposed to do - the default is to only emulate prepared statements which is far less secure.

[mac_gyver]

you will also need to put the % wild-card search characters around the actual data value in $_POST['search'] when you supply it for the query's execution.

[eventide7]

First of all, I really appreciate you all taking the time to try to help me. I don't speak the same language you do, but you've been very kind to offer explanations for 'whys' and 'hows', and I'm doing my best to understand.

 

Of course, I have more questions:

1) I have the database connection set to a user profile that is read-only. Is this not enough to prevent SQL injections?

2) I rewrote the code to match what Jacques posted, but got a 500 Internal Server Error. I then tried changing the $_POST['search'] to $_POST['%search%'], and the error remained.

 

If I'm too newb for this, let me know. I have a generally decent grasp of html and css, but php is like Chinese to me. For some reason I just haven't been able to wrap my head around it.

[mac_gyver]

Is this not enough to prevent SQL injections?

 

 

no. the injected sql is normally used with SELECT queries to read other tables in your database. because you are already selecting data to display, all the injected sql needs to do is satisfy the syntax of your existing query, then it can add a UNION SELECT anything it wants, to get your code to query for and output things like the content of a user table including email addresses and hashed passwords.

 

500 Internal Server Error

 

 

there's nothing syntactically wrong with the posted code, provided you are using php5.4+. you are likely getting a fatal run-time error, such as an uncaught pdo  exception. do you have php's error_reporting set to E_ALL and display_error set to ON so that any run-time errors will be reported and displayed?

 

if you are using a version of php that's lower than 5.4, you would be getting a php syntax error due to some php5.4+ specific syntax in use. what is your php version?

 

the wild-card % are around the data, not part of the array index name. you would use the following, including the double-quotes -

"%{$_POST['search']}%"

Edited March 2, 2016 by mac_gyver

[eventide7]

Okay, I got the error reporting to display. It says, "Parse error: syntax error, unexpected '[' in /hermes/bosweb26d/b43/ipg.poetskaraokecom/sample2.php on line 38."

 

That line starts here:

$databaseConnection = new PDO($dSN, DB_USER, DB_PASSWORD, [
    PDO::ATTR_EMULATE_PREPARES => false,                 // activate real prepared statements
    PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,         // activate exceptions
    PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,    // fetch associative arrays by default (this isn't critical)
]);

[eventide7]

I had already replaced 

const DB_HOST = 'localhost';
const DB_USER = '...';
const DB_PASSWORD = '...';
const DB_NAME = '...';
const DB_CHARSET = 'UTF8';

 

with my information for logging on to the read-only song database.

 

My php version is 5.3.

Edited March 2, 2016 by eventide7

[mac_gyver]

the error is because of the php version.

 

add the word array before the [

[eventide7]

$databaseConnection = new PDO($dSN, DB_USER, DB_PASSWORD, array (

    PDO::ATTR_EMULATE_PREPARES => false,                 // activate real prepared statements

    PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,         // activate exceptions

    PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,    // fetch associative arrays by default (this isn't critical)

)

 

now give me this error: Parse error: syntax error, unexpected T_VARIABLE in /hermes/bosweb26d/b43/ipg.poetskaraokecom/sample2.php on line 44

 

iPage gives me the option to update to PHP version 5.5. or 5.6. This is my only php-dependent page on my site. Are there repercussions for changing the version on my server, or is the process pretty automatic if I tell it to change to 5.6?

[mac_gyver]

you should use the highest php version that's available to you.

 

there are actually few incompatible changes moving up to higher php versions, provided code isn't using deprecated/removed features, and code doing basic/common tasks isn't likely to be affected at all.

 

edit: for the error above in post #10, it looks like you are missing the semi-colon ; on the end of that statement.

Edited March 2, 2016 by mac_gyver

[eventide7]

Woo hoo! I changed to PHP5.6, and it looks like the form at least is working. I'll play with it some and see if I can get it to return search results in a coherent manner (right now it shows nothing but the search box) and get back to you. Thanks!

[eventide7]

Current error on loading the page:

Warning: PDO::__construct(): The server requested authentication method unknown to the client [mysql_old_password] in /hermes/bosweb26d/b43/ipg.poetskaraokecom/sample2.php on line 42 Fatal error: Uncaught exception 'PDOException' with message 'SQLSTATE[HY000] [2054] The server requested authentication method unknown to the client' in /hermes/bosweb26d/b43/ipg.poetskaraokecom/sample2.php:42 Stack trace: #0 /hermes/bosweb26d/b43/ipg.poetskaraokecom/sample2.php(42): PDO->__construct('mysql:host=p***...', '****', '****', Array) #1 {main} thrown in /hermes/bosweb26d/b43/ipg.poetskaraokecom/sample2.php on line 42

 

From this code:

/*
 * Set up the database connection
 * - the character encoding *must* be defined in the DSN string, not with a SET NAMES query
 * - be sure to turn off "emulated prepared statements" to prevent SQL injection attacks
 * - turn on exceptions so that you don't have to manually check for errors
 */
$dSN = 'mysql:host='.DB_HOST.';dbname='.DB_NAME.';charset='.DB_CHARSET;

$databaseConnection = new PDO($dSN, DB_USER, DB_PASSWORD, [
    PDO::ATTR_EMULATE_PREPARES => false,                 // activate real prepared statements
    PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,         // activate exceptions
    PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,    // fetch associative arrays by default (this isn't critical)
]);

$songStmt = $databaseConnection->prepare('
    SELECT
        artist,
        title
    FROM
        songs
    WHERE
        artist LIKE :search
        OR title LIKE :search
    LIMIT
        100
');
$songStmt->execute([
    'search' => "%{$_POST['search']}%",
    'search' => "%{$_POST['search']}%",
]);

foreach ($songStmt as $song)
{
    var_dump($song);
}

// Display search result
         if (!$song->rowCount() == 0) {
		 		echo "Search found :<br/>";
				echo "<table style=\"font-family:arial;color:#333333;\">";	
                echo "<tr><td style=\"border-style:solid;border-width:1px;border-color:#98bf21;background:#98bf21;\">Artist</td><td style=\"border-style:solid;border-width:1px;border-color:#98bf21;background:#98bf21;\">Title</td></tr>";				
            while ($results = $song->fetch()) {
				echo "<tr><td style=\"border-style:solid;border-width:1px;border-color:#98bf21;\">";			
                echo $results['artist'];
				echo "</td><td style=\"border-style:solid;border-width:1px;border-color:#98bf21;\">";
                echo $results['title'];
				echo "</td></tr>";				
            }
				echo "</table>";		
        } else {
            echo 'Your search for <b><i>' .$search . '</b></i> returned no results. <br>Please try your search again using fewer terms.';
        }

?>

[Jacques1]

Your MySQL passwords are stored in an ancient, insecure format which is no longer supported. You need to keep MySQL up-to-date as well and then generate new passwords (since the old one may have been compromised).

[eventide7]

My server says my MySQL version is 5.5.44. It doesn't give me the option to update that. However, I did change my passwords and updated the script accordingly. Now upon loading the page I see this:

array(2) { ["artist"]=> string(20) "? And The Mysterians" ["title"]=> string( "96 Tears" } array(2) { ["artist"]=> string( "10 Years" ["title"]=> string(16) "Through The Iris" } array(2) { ["artist"]=> string( "10 Years" ["title"]=> string(9) "Wasteland" } array(2) { ["artist"]=> string(14) "10,000 Maniacs" ["title"]=> string(17) "Because The Night" } array(2) { ["artist"]=> string(14) "10,000 Maniacs" ["title"]=> string(21) "Candy Everybody Wants" } array(2) { ["artist"]=> string(14) "10,000 Maniacs" ["title"]=> string(14) "More Than This" } array(2) { ["artist"]=> string(14) "10,000 Maniacs" ["title"]=> string(14) "These Are Days" }

...which I am assuming is the result of:

foreach ($songStmt as $song)

{

    var_dump($song);

}

 

I appreciate your help, everyone. I'm off to figure out how to understand my new output so I can format it. If you have a direction to send this puppy, please note it here. Your patience with me has been commendable.

Many thanks.

Edited March 2, 2016 by eventide7

[ginerjm]

Try

 

foreach ($songStmt as $song)
echo $song['artist'] . " - " . $song['title'] . "<br>";

 

Much better looking output.

[Jacques1]

Make sure to HTML-escape all outgoing data. Otherwise the same problem that happened with your query may happen again with your HTML.

<?php

/**
 * HTML-escapes a string so that it can safely be included in an HTML document
 *
 * @param string $raw_input the string which should be escaped
 * @param string $encoding  the character encoding of the input string
 *
 * @return string the encoded string
 */
function html_escape($raw_input, $encoding)
{
    return htmlspecialchars($raw_input, ENT_QUOTES | ENT_SUBSTITUTE, $encoding);
}

echo html_escape($song['artist'].' - '.$song['title'], 'UTF-8');

[eventide7]

Try

 

foreach ($songStmt as $song)
echo $song['artist'] . " - " . $song['title'] . "<br>";

 

Much better looking output.

I will have your babies. I mean THANK YOU! I am so happy!

[ginerjm]

Be sure to read and follow Jacques advice - I should have done what he did. Good practice makes for safe code.

[eventide7]

Jacques1, I added the information you provided. It ended up making my output lose the breaks between the lines. i tried adding them in by rewriting 

echo html_escape($song['artist'].' - '.$song['title'].'<br>', 'UTF-8'); 

which ended up printing the <br> in my output and didn't separate the lines, and I tried adding

echo '<br>';

after that statement, but it did not separate the lines either. 

 

Here's my output with the html escape (searching for "ain't country"):

 

David Allan Coe - If That Ain't CountryTravis Tritt - Country Ain't Country

Edited March 2, 2016 by eventide7

[ginerjm]

Have you tried doing any reading on how to program in php? You aren't understanding what you have been given.

 

First - did you create the function as Jacques showed you?

 

Second - you use that function for EACH item that you want to output.

 

Third - you can't just add an echo since the foreach will only loop thru the single line I gave you. You need to enclose all lines to be executed by the foreach in curly braces.

 

A little light reading of a php source is what you really need to do.

[Jacques1]

You must exclude the actual HTML markup from the html_escape() call.

 

Actually, I'd avoid <br> and use a semantic element like an unordered list instead. This gives your markup a meaningful structure and makes it easier to read for machines (search engine bots, maybe screen readers for visually impaired users etc.).

echo '<ul>';
foreach ($songStmt as $song)
{
    echo '<li>'.html_escape($song['artist'] . " - " . $song['title'], 'UTF-8').'</li>';
}
echo '</ul>';

If you don't like the bullet points, you can hide them with CSS.

[eventide7]

I currently have 18 websites opened on my desktop. Light reading for you is heavy duty for me. As I've stated, I don't understand much of php. It's Greek to me. I've been trying for several years in my spare time. I keep finding that when I think I have something down it's become "deprecated" or I'm using it incorrectly. This php search page is totally different from the way I've been doing it using a simple query. I am trying as hard as I can to understand the syntax.

[eventide7]

You must exclude the actual HTML markup from the html_escape() call.

 

Actually, I'd avoid <br> and use a semantic element like an unordered list instead. This gives your markup a meaningful structure and makes it easier to read for machines (search engine bots, maybe screen readers for visually impaired users etc.).

echo '<ul>';
foreach ($songStmt as $song)
{
    echo '<li>'.html_escape($song['artist'] . " - " . $song['title'], 'UTF-8').'</li>';
}
echo '</ul>';

If you don't like the bullet points, you can hide them with CSS.

Now THAT I know how to write (CSS I mean). Thank you, Jacques!