is there a known major security hole regarding sql injection since 1.0.8.7?

[loop]

Hello everyone
It doesn't have to mean anything, but I received a request today from a customer who says he only used his email with me (special email address for my store) and received spam from 2 sites 1 week ago.... hence the assumption if somehow someone was able to read the email with a SQL injection or similar.

I am currently still using 1.0.8.7, is there anything known that it had a leak and has been fixed since then?

Could of course have happened somewhere else on the internet but I would like to look into it

thanks for your help

[burt]

None known.

[14Steve14]

The only one I can find online was all about 1.0.8.20 which is detailed here, but its an old one and I believe that define language has been removed from later versions. https://advisories.checkpoint.com/defen ... -0420.html

[burt]

Even on that one, the attacker needs access to your shops admin area.
Which most shopowners would hopefully (a) have locked down and (b) not share details of passwords etc

[14Steve14]

Even on that one, the attacker needs access to your shops admin area.
Which most shopowners would hopefully (a) have locked down and (b) not share details of passwords etc

You may be surprised how many store owners may not lockdown their admin or even change the file name.

[loop]

Everywhere i see you should use preparestaement in sql, why is it not used in phoenixcart? Snyk.io suggest alot of prepare statement to prevent injections..hmm

[ecartz]

While prepared statements are one way to avoid SQL injection, they are not the only way.

Phoenix has been secure from SQL injection since the beginning and has never used prepared statements.

I'd have switched to prepared statements if there was a good way to switch some statements but not others. As it is, we'd have to switch every SQL query in the shop at once in both core and all add-on extensions. Or we'd have to create something that served the same purpose.

MySQLi added a new method. We might eventually override that in the database class, but we'd have to polyfill for older versions that didn't have it. I haven't looked into that seriously yet.

[loop]

hi ecartz
since I really want to go through my own code / addons at the moment and want to rule out that I have a leak somewhere, should I now work with prepared statemens in my codes or as you have done in your code? What is a bit annoying is that the tool snyk.io, which is quite handy to use in visualstudio, always shows “sql injection” danger when it detects that variables are made without a perpare statement.

But as far as I can see, I could easily make my addons at least with “prepare statements” and leave the core code, should be no problem, right?

What would you advise me to do? I've produced a lot of code over the years and it's handy if a tool like snyk.io shows me where to go for xss and sql injection...

[loop]

I am thinking of a function extension

Code: Select all

function tep_db_prepare_query($query, $params, $link = 'db') {
    $stmt = $GLOBALS[$link]->prepare($query);
    if ($stmt === false) {
        die('Fehler beim Vorbereiten des Statements: ' . $GLOBALS[$link]->error);
    }

    // Typen und Werte der Parameter extrahieren
    $types = '';
    $values = [];
    foreach ($params as $param) {
        $types .= $param['type'];
        $values[] = $param['value'];
    }

    // Dynamisch die Parameter binden
    $bindParams = array_merge([$types], $values);
    $bindParamsRefs = [];
    foreach ($bindParams as $key => $value) {
        $bindParamsRefs[$key] = &$bindParams[$key];
    }
    call_user_func_array(array($stmt, 'bind_param'), $bindParamsRefs);

    if (!$stmt->execute()) {
        die('Fehler beim Ausführen des Statements: ' . $stmt->error);
    }

    $result = $stmt->get_result(); // Ergebnis-Set abrufen

    return $result; // Ergebnis-Set zurückgeben
}

$query = "SELECT customers_id, customers_firstname FROM customers WHERE customers_firstname = ? AND customers_lastname = ?";
$params = [
    'customers_firstname' => ['value' => $_GET['firstname'], 'type' => 's'],
    'customers_lastname' => ['value' => $_GET['lastname'], 'type' => 's']
];

$test_query = tep_db_prepare_query($query, $params);

while ($row = $test_query->fetch_assoc()) {
    echo "Customer ID: " . $row['customers_id'] . " / " . $row['customers_firstname'] . "<br>";
}

ist this a good idea? it would be quiet easy to replace my querys....only have to change the query and the function tep_db_prepare_query and add $params to say what kind of param it is... what you think?

Code: Select all

$params = [
    'customers_firstname' => ['value' => $_GET['firstname'], 'type' => 's'],
    'customers_lastname' => ['value' => $_GET['lastname'], 'type' => 's']
];

as i already wrote, i don't ask to replace the core with my function, my idea is to replace my addons / codes and use this kind of function for my querys, so i'm save aswell...thanks!