1
\$\begingroup\$

I'm looking for best practices for writing secure session managers.

I'm making a table for the authorization token (UUID OR GUIDv4) with autoincrement, user_id, token, status (enum) then update status onPause() onResume() so when I update a token I get a new autoincrement to store with the token in shared preference. Then on token update I just insert the old token with the user_id in another table for history.

The problems that I'm having:

  1. it's good for a single device but when I want to update like password and update token I will have to end all user sessions and only the one I'm using will be updated.

  2. should I add timestamp and keep the autoincrement inserted for a certain time of token updates or days or should I let it insert new autoincerment?

Is it good practice or there is a better way?

I already read about:

  1. shared preference exploit on root. Is Shared Preferences safe for private datas?

  2. shared preference not secured even if I encrypt data. Android SharedPreference security

  3. ANDROID_ID can be null and can change upon factory reset.

  4. data will be not safe if sensitive data is exploited I came across this but could not find any other articles that approve the method he used SQL injection

I already use a prepared statement:

<?php

$db_name = "mysql:host=localhost;dbname=DATABSE;charset=utf8";
$db_username = "root";
$db_password = "";
try {
    $PDO = new PDO($db_name, $db_username, $db_password);
    //echo "connection success";
} catch (PDOException $error) {
    //echo "Error: " . $error->getMessage();
    //echo "connection error";
    exit;
}
$user_id = $_POST['user_id'];
$stmt = $PDO->prepare("
                  SELECT
                  tableA.name AS name,
                  TABLEB.logo AS logo 
                  FROM tableA
                  LEFT JOIN TABLEB ON TABLEB.id = tableA.id 
                  WHERE tableA.id = :USERID ;
                    ");
$stmt->bindParam(':USERID', $user_id);

$stmt->execute();
$row = $stmt->fetchAll(PDO::FETCH_ASSOC);

if (!empty($user_id)) {
    $returnApp = new stdClass();
    $returnApp->LOADPROFILE = 'LOAD_SUCCESSFUL';
    $returnApp->LOAD_SUCCESSFUL = $row;
    echo json_encode($returnApp);
} else {
    $returnApp = array('LOADPROFILE' => 'LOAD_FAILED');
    echo json_encode($returnApp);
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

I don't see code for some of your items of concern, so I'll focus on reviewing the provided code.

  • Your code should consistently respond with the same general formatted json string so that the receiving code can remain simple and elegant. I think your business logic indicates that an empty array as a response is somehow a failure. A non-empty array will be success.
  • You are not validating the submitted value. If the submission is anything other than a positive integer, then you can safely give the failure response -- even before bothering with a database connection.
  • I recommend eliminating single-use variables in any script unless they provide a valuable benefit.
  • Unconditionally return the empty array or array of associative arrays that is provided by fetchAll().

New code:

if (empty($_POST['user_id']) || !ctype_digit($_POST['user_id'])) {
    exit('[]');
}
$pdo = new PDO(
    "mysql:host=localhost;dbname=DATABSE;charset=utf8",
    "root",
    ""
);
$stmt = $pdo->prepare(
    "SELECT tableA.name AS name,
            TABLEB.logo AS logo 
     FROM tableA
     LEFT JOIN TABLEB ON TABLEB.id = tableA.id 
     WHERE tableA.id = ?"
);
$stmt->execute([$_POST['user_id']]);
echo json_encode($stmt->fetchAll(PDO::FETCH_ASSOC));

In terms of security, I am concerned about the fact that anyone can anonymously/trivially fire a bunch of integers at this code and get a direct feed of result set data. Maybe this is why you mentioned UUIDs -- you might think that obscuring the identifier may improve security -- but it won't, it will only potentially slow down a persistent attacker.

\$\endgroup\$
9
  • \$\begingroup\$ I'm sorry i didn't explain it well . i only provided a sample of how i usually get data because shared preference nothing i can do about it except I store a token(UUID) that changes instead of constant user_id . i miss typed the execution and fetch outside if condition and i only do this when i try to insert something and dont want the generate a row with empty variable so i choose random from all available .. thank you so much for updating my php.. if you can update with how often should i update token and if i should let it insert new autoincrement with each update ill be very grateful \$\endgroup\$
    – Taa Lee
    Jan 29, 2022 at 11:25
  • 1
    \$\begingroup\$ Perhaps you should ask a new question and show new, working code to be reviewed. If you want behavior to be changed, we don't do this on CodeReview. \$\endgroup\$ Jan 29, 2022 at 11:35
  • \$\begingroup\$ well that was my two problems and my only question was if doing that is a good practice to secure session manager you already gave an answer that UUID will slow attacker but that wasnt the main problem \$\endgroup\$
    – Taa Lee
    Jan 29, 2022 at 11:44
  • 3
    \$\begingroup\$ Please take the tour and read the Help pages on asking a question. \$\endgroup\$ Jan 29, 2022 at 22:41
  • 3
    \$\begingroup\$ Just to be clear, on CodeReview, we are only permitted to review posted code. I reviewed the only code posted. I am confident that this does not make me a bad reviewer. \$\endgroup\$ Jan 30, 2022 at 5:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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