-2
\$\begingroup\$

I had a login code like this:

$jmeno = $_POST['jmeno'];
$heslo = $_POST['heslo'];

$sql = mysqli_query($pripojeni, "SELECT jmeno, heslo FROM databaze WHERE jmeno = '$jmeno'");
while ($data = mysqli_fetch_assoc($sql)){
    $db_jmeno = $data['jmeno'];
    $db_heslo = $data['heslo'];
}

if ($jmeno == $db_jmeno AND $heslo == password_verify($heslo, $db_heslo)){
    $_SESSION['loggedin'] = TRUE;
    $_SESSION['jmeno'] = $jmeno;

And I found on the internet that having a user variable in sql command is really bad idea.

I read something about pdo and preparing, executing statements and I changed the code to this:

try {
    $pdo = new PDO($PDO_HOST, $DATABASE_USER, $DATABASE_PASS);
    $pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
    $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
} catch (PDOException $e) {
    die('Připojení selhalo: ' . $e->getMessage());
}

$priprava = $pdo->prepare("SELECT jmeno, heslo FROM databaze WHERE jmeno = ?");
$priprava->execute([$jmeno]);
$uzivatel = $priprava->fetch();

if ($uzivatel && password_verify($_POST['heslo'], $uzivatel['heslo']))
{
    $_SESSION['loggedin'] = TRUE;
    $_SESSION['jmeno'] = $jmeno;

Can you please just tell me if this is correctly done and much better for SQL injection?

Thank you.

-------- Second code (for example and my know)

In other script I have it like this:

$id = $_GET['polozka'];

$pripojeni = mysqli_connect($DATABASE_HOST,$DATABASE_USER,$DATABASE_PASS,$DATABASE_NAME);
mysqli_set_charset($pripojeni, "utf8mb4");

$vysledek = mysqli_query($pripojeni,"SELECT hra, polozka, pouzito FROM databaze WHERE id = '$id'");
    while($data = mysqli_fetch_assoc($vysledek)) {
        
        $hra = $data['hra'];
        $polozka = $data['polozka'];
        $uplatneno = $data['pouzito'];
        
    }

I guess this code is also really bad so I changed it to this:

$dotaz = $pdo->prepare("SELECT hra, polozka, pouzito FROM databaze WHERE id = ?");
$dotaz->execute([$id]);
$podpora_inventar = $dotaz->fetch();
$hra = $podpora_inventar['hra'];
$polozka = $podpora_inventar['polozka'];
$uplatneno = $podpora_inventar['pouzito'];

is it ok like that?

\$\endgroup\$
2

1 Answer 1

1
\$\begingroup\$

let exceptions bubble up

try {
    $pdo = new PDO($PDO_HOST, $DATABASE_USER, $DATABASE_PASS);
    ...
} catch (PDOException $e) {
    die('Připojení selhalo: ' . $e->getMessage());
}

Maybe you wrote down a Requirement in your user story so the program has to exhibit this behavior, in which case this is perfect as-is. But I suspect not. Notice that we don't really "handle" (repair / ignore) the error. We merely report it.

Rather than try / catch / die, just connect to the database and if an exception happens, it happens. Let it bubble up the call stack. If some higher-level caller wants to catch it and do something useful, great. If by default we print out an informative diagnostic stack trace, also great. Much more helpful than the one-liner you currently offer.

Maybe you have this connected to a web server. Which serves random folks you wouldn't want to trust. If so, consider writing down some Security requirements. Like not revealing many code or data details. Here, you might prefer to just say, "You lose! Send email to support for assistance.", rather than revealing the particular DB diagnostic.

extract helper

    $pdo = new PDO($PDO_HOST, $DATABASE_USER, $DATABASE_PASS);
    $pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
    $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

These three lines would fit very nicely into a helper named get_db_connection, following SRP. Maybe you re-use the code when there's multiple callers of that helper. Maybe you don't. Doesn't matter. Naming a chunk of code is valuable to future maintainers, including yourself. Cultivate that habit.

injection fixed

$priprava = $pdo->prepare("SELECT jmeno, heslo FROM databaze WHERE jmeno = ?");
$priprava->execute([$jmeno]);
$uzivatel = $priprava->fetch();

Looks good to me, ship it! This is the correct way to send in query parameters, much better than the unquoted string interpolation you had before.

I am accustomed to more compact idioms for sending a query, so I'm slightly surprised it takes three lines in this environment. Well, it is what it is. Recommend you package this up as another tiny helper, passing in at least a query string and a query value. You might also pass in $pdo, but your class might be able to find it as an object attribute, in which case use that.

Notice that we must test the fetch() return value, which you correctly do. Consider making it slightly more convenient for app-level code to call your helper and correctly consume what it returns. That is, consider putting the test within the helper, and throw an exception upon seeing an unexpected DB result.

You return jmeno / username. Possibly it's convenient during debugging. But it's not strictly necessary; consider eliding it.

use hashed passwords

Yay, you win! You're doing it right. And there's even a salt incorporated in there, which is appropriate. I am reading the documentation

password_verify(string $password, string $hash):

if ($uzivatel && password_verify($_POST['heslo'], $uzivatel['heslo']))

I was having a hard time reading that line, and it didn't look right to me. It turns out that technically it is correct, but it's not terrific at communicating your intent to collaborators.

It ends with $user['password'])), which made me nervous. Turns out the code is lying, as this is definitely not a cleartext password. Please choose a better identifier, maybe zpravaSouhrn, messageDigest, povereni, credential, or passwordHash.

An identifier like uzivatel or user is fine as it is. But think of the conventional idiom of for i in .... Often the best name for an index is i. Here, you might find it convenient to conventionally talk about a radek or row that came back from the database, letting the query context advise us that it's a row from the user database.

table encodings should match

mysqli_set_charset($pripojeni, "utf8mb4");

Oh, my! Haven't seen that one before. We have left the BMP behind. I imagine you're storing U+1F602 😂 and similar emojis. And you need O(1) constant time indexing to an offset, which ordinary UTF8 can't accommodate. I'm sure you're doing the right thing, here.

As a reminder, for both mysql and postgres it is important to use consistent on-disk encodings. That is, if you ever plan to JOIN tables A and B in a performant way, you want the rows and the indexes to all use the identical encoding and collation scheme. A mismatch may prevent the backend planner from exploiting an index, impacting performance.

So if the four-byte utf8mb4 encoding is your standard, good, so be it. For a table with lots of Czech text (BMP text) I wonder if, like utf32, that will consume a bunch more disk blocks than utf8 does.

Oh, wait, now I am reading the documentation, this is frightening.

MySQL's utf8 encoding has a maximum of 3 bytes and is unable to encode all unicode characters.

Wow! I had no idea. Guess I've been using Postgres mostly. It would never occur to me that "utf8" would denote something different from the spec. Looks like you do have to do that. A unit test which INSERTs and SELECTs an emoji would be helpful.

be boring

Choosing "boring" identifiers is actually a good thing. For example, choosing the identical names that your library's documentation uses makes your code easier to read. It helps us to trace provenance, and it asserts that "foo" over here is identical to "foo" over there, having the same semantics. Letting a DB column name dictate the naming of a local variable is another variation on the same theme.

        $uplatneno = $data['pouzito'];

Yes, I get it that there's more than one synonym for "applied".

Here, I recommend you assign $pouzito = ..., in the interest of readability for future maintainers.

And see above, where your deliberate use of heslo and heslo threw me for a loop, since they have very different semantics there.


This codebase would benefit from the addition of one or more unit tests.

I would be willing to delegate maintenance tasks on this codebase to engineers who are proficient at conversational Czech. This codebase is not currently suited to maintenance by a diverse international team.

is it ok like that?

Yes, LGTM, merge down to main.

\$\endgroup\$
3
  • \$\begingroup\$ Thank you very much for all of this. I will read it more times because it is containing so much technical stuff and english as you see and know is not my native language, but it will help me so much. Thank you once again. The charset is already gone now. I had issues with some Czech letters, but it was my bad formating. My top priority is the SQL injections that I have got with the old code and the hacker read everything in the database because of that (I think/guess). \$\endgroup\$
    – Bazim
    Mar 18 at 20:09
  • \$\begingroup\$ Oh. If there was a DB exposure then your hashes are public knowledge. Make a snapshot of them. Reset all passwords to something new. After some days go by, compare current hashes with the snapshot. You will find that some are unchanged. Disable / delete all such unchanged accounts, as they are compromised. Recreate such accounts with new passwords as the need arises. This is just standard procedure for dealing with a breach. Also, don't be surprised if a single compromised account allowed the intruder to expand to other internal systems. \$\endgroup\$
    – J_H
    Mar 18 at 20:23
  • \$\begingroup\$ I have mixed feelings about this answer. It contains very good suggestions along with just... talking to yourself? Consider leaving only useful suggestions for the OP. I don't see how the "table encodings should match" section is one. and "use hashed passwords" as well. \$\endgroup\$ Mar 19 at 10:01

Not the answer you're looking for? Browse other questions tagged or ask your own question.