Page 1 of 1

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

Posted: Fri Sep 06, 2024 10:05 am
by 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

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

Posted: Fri Sep 06, 2024 8:46 pm
by burt
None known.

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

Posted: Sat Sep 07, 2024 4:06 pm
by 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

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

Posted: Sat Sep 07, 2024 4:36 pm
by 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

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

Posted: Sat Sep 07, 2024 5:22 pm
by 14Steve14
burt wrote: Sat Sep 07, 2024 4:36 pm 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.

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

Posted: Sun Sep 08, 2024 5:32 am
by 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

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

Posted: Sun Sep 08, 2024 2:47 pm
by 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.

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

Posted: Tue Sep 10, 2024 7:00 am
by 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...

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

Posted: Tue Sep 10, 2024 7:40 am
by 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!