7
\$\begingroup\$

Here is an API key gen script for a cryptocurrency trading platform I am building.

First it checks to see if a key exists in the db for the user ID. If it does exist, it displays the key. If it doesn't, it creates one.

Next it checks to make sure the key is unique, by polling the db for identical keys. If one is found to be identical, the page is refreshed via meta refresh. If no key collision is found, it inserts the key in the database and refreshes the page via meta refresh. When the page is reloaded, there is now a key in the database for the user ID, and the key is displayed.

Any tips or feedback is greatly appreciated. I am a fairly inexperienced programmer and this is my first serious project.

require_once("models/config.php");
$id = $loggedInUser->user_id;
$api_select = mysql_query("SELECT * FROM userCake_Users WHERE `User_Id`='$id'");
while($row = mysql_fetch_assoc($api_select)) {
if($row["api_key"] !== null) {
    echo '<h3>Your Api Key is:</h3><br/>';
    echo $row["api_key"];
}else{
    $user = $row["Username"];
    $pass = $row["Password"];
    $length = 128;
    $time = date("F j, Y, g:i a");
    $salt1 = $time . hash('sha512', (sha1 .$time));
    $salt2 = substr(md5(uniqid(rand(), true)), 0, 25);
    $salt3 = substr(md5(uniqid(rand(), true)), 0, 25);
    $salt4 = hash('sha256', (md5 .$time));
    $user_hash = hash('sha512', ($salt2 . $user . $salt1));
    $pass_hash = hash('sha512', ($salt1 . $pass . $salt2));
    $keyhash_a = hash('sha512', ($user_hash . $salt3));
    $keyhash_b = hash('sha512', ($pass_hash . $salt4));
    $hash_a = str_split($keyhash_a);
    $hash_b = str_split($keyhash_b);
    foreach($hash_a as $key => $value) {
        $hashed_a[] = $salt2 . hash('sha512', ($salt3 . $value)) . $salt1 . hash('sha256', ($salt4 . $key));
    }
    foreach($hash_a as $key => $value) {
        $hashed_b[] = $salt2 . hash('sha512', ($salt3 . $value)) . $salt1 . hash('sha256', ($salt4 . $key));
    }
    $hash_merge = array_merge($hashed_b, $hashed_a);
    $from_merge = implode($hash_merge);
    $exploded_2 = str_split($from_merge);
    $key_hash_last = implode($exploded_2);
    $key0 = str_shuffle($key_hash_last);
    $key1 = str_split($key0);
    $key2 = array_unique($key1);
    $key3 = implode($key2);
    $key4 = str_shuffle($key3);
    $key5 = str_shuffle($key4);
    $api_key0 = str_shuffle($key3.$key4.$key5.$key2);
    $api_key_prepped = mysql_real_escape_string($api_key0);
    $api_check_no_collision = mysql_query("SELECT * FROM userCake_Users WHERE `api_key` = '$api_key_prepped'"); //check to see if an identical key exists
    if(mysql_num_rows($api_check_no_collision) > 0) {
        echo '<meta http-equiv="refresh" content="0; URL=index.php?page=api">';//an identical key exists in the database, refresh the page and generate a new key.
    }else{
    $api_insert = mysql_query("UPDATE  `testing`.`userCake_Users` SET  `api_key` =  '$api_key_prepped' WHERE  `userCake_Users`.`User_ID` ='$id';"); //the key is unique, submit it to the database.
    echo '<meta http-equiv="refresh" content="0; URL=index.php?page=api">';
    }
}
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Three quick comments besides answering the actual question on security... First, mysql_query is deprecated and you should use newer implementations suggested here us2.php.net/mysql_query, unless you are using a much older PHP version. Second, is $id 'clean' enough to prevent SQL injection attacks in "...WHERE User_Id = '$id'"? Third, why will you want to force a page-refresh if there is a key collision, instead of just looping within the logic? In the extremely unlikely scenario of 10 key collisions, are you expecting the users to face 10 page refreshes as well? \$\endgroup\$ – h.j.k. Dec 30 '13 at 4:18
  • \$\begingroup\$ about mysql: i know. its faster to write it though and go back and change it later. about $id. yes, it comes from userCake1.4.2 standard function library. basically the system is built on top of userCake, with modified password hashing and extra functions. by default, all of userCake functions are run through stringent sanitization. Third, as i stated, i have < 6 months experience in php/mysql. i haven't figured out a way to loop back through yet without a page refresh. can you provide me an example would i just return or something, i'm not sure exactly how to accomplish it in php. \$\endgroup\$ – r3wt Dec 30 '13 at 4:47
  • \$\begingroup\$ I'd recommend something less ambitious than a cryptocurrency trading platform as a beginner project, unless you're doing it privately just for fun. \$\endgroup\$ – 200_success Dec 30 '13 at 10:16
  • \$\begingroup\$ there's a difference between a beginner project and "my first real project". \$\endgroup\$ – r3wt Dec 30 '13 at 19:27
  • 1
    \$\begingroup\$ @.r3wt I agree with @200_success that you're not ready for such a high risk project. Choose a project where a mistake doesn't mean that cryptocurrency worth thousands of USD get stolen. \$\endgroup\$ – CodesInChaos Jan 24 '14 at 12:30
16
\$\begingroup\$

It feels like you are combining lots of hashing and string transformation in hopes that complexity will make your scheme more secure. That's not engineering or computer science, though — it's Cargo Cult Programming.

There are two ways that an API key scheme could work:

  1. To generate the API key, concatenate the username with a salt and site-wide secret, then hash it. When it is later presented, verify it by concatenating the username, salt, and secret, and see if the hashes match.

    Advantages:

    • It may be possible to verify the API key without a database lookup.
    • You can immediately revoke all API keys ever issued by simply changing the site-wide secret.

    Disadvantages:

    • It's tricky to revoke the API key for a particular user without a database lookup.
  2. Generate any distinct, unguessable string. Store it in the database, associated with the user. When it is later presented, verify it by looking it up in the database.

    Advantages:

    • It's easy to understand how it works.

    Disadvantages:

    • Verification requires a database lookup.

What you have done is a combination of the two strategies. You store the generated API key in the database, associated with the user. However, while you could have used just any random string, you used a convoluted process involving lots of randomizing, hashing, concatenating, and shuffling, all without much justification, and in a way that offers no additional security benefits.

Consider, for example:

$hash_merge = array_merge($hashed_b, $hashed_a);
$from_merge = implode($hash_merge);
$exploded_2 = str_split($from_merge);
$key_hash_last = implode($exploded_2);

That just converts an array to a string, back into an array, and reconstitutes the same string again.

If you consider the properties of cryptographic hash functions, you will notice that much of the manipulations are unnecessary. In particular, cryptographic hash functions are designed such that if inputs a and b differ by as little as one bit, hash(a) and hash(b) will not resemble each other at all.

Therefore, if you choose option (2), you can generate an API key securely using something as simple as

$api_key = hash('sha256', (time() . $id . $some_sitewide_secret . rand()));

The result will be secure because:

  1. Cryptographic hashes are one-way (irreversible) functions, so knowing the hash reveals nothing about the input that was hashed. (Unguessable)
  2. The input varies by time and includes randomness, so it is not repeatable. (Distinct)
  3. The input includes the user ID, so no two users should be able to generate the same key. (Distinct)
  4. The input includes some site-wide secret string (which you can set in your configuration file), so even if an attacker tried to generate a lot of keys using the same technique in an attempt to brute-force a match, knowing the approximate time at which the key was generated and even cracking the pseudorandom number generator would not help. Pick a very long random string for the site-wide secret (a SHA-256 of any word processing document would do). (Unguessable)
  5. The output has 256 bits of entropy, which is impossible to crack by exhaustive enumeration. (Unguessable)

Furthermore, the chance that the same API key would be generated twice is about 2-250. It's more likely that your server would be struck by an asteroid tomorrow than that it would generate the same key twice using this technique, so don't worry about accidental collisions.

\$\endgroup\$
  • 1
    \$\begingroup\$ That is a great explanation you put together, you made it very simple and easy to understand \$\endgroup\$ – bumperbox Jan 24 '14 at 21:59
  • 1
    \$\begingroup\$ +1, except for "a SHA-256 of any word processing document would do". If an attacker got access to your word processing documents, they could then find the key. It would be better to use a secure source of random numbers, such as /dev/random. \$\endgroup\$ – Gareth Rees Jan 28 '14 at 13:21
5
\$\begingroup\$

An API key only have to be unique enough to avoid collison nothing more and here you are doing to much to achieve this. For example the rehashing will incrase the chance for a collison.

I have created a class for generating unique keys for example an API key usage or simply for primary key in a database table.

<?php

final class Guid {

    private static $_empty = array("00000000", "0000", "0000", "0000", "000000000000");

    private static $_parseFormats = array(
        "D" => "/^[a-f\d]{8}(-[a-f\d]{4}){4}[a-f\d]{8}$/i",
        "N" => "/^[a-f\d]{8}([a-f\d]{4}){4}[a-f\d]{8}$/i",
        "B" => "/^(\{)?[a-f\d]{8}(-[a-f\d]{4}){4}[a-f\d]{8}(?(1)\})$/i",
        "P" => "/^(\()?[a-f\d]{8}(-[a-f\d]{4}){4}[a-f\d]{8}(?(1)\))$/i",
        "X" => "/^(\{0x)[a-f\d]{8}((,0x)[a-f\d]{4}){2}(,\{0x)[a-f\d]{2}((,0x)[a-f\d]{2}){7}(\}\})$/i"
    );

    public static function NewGuid() {
        $data = openssl_random_pseudo_bytes(16);

        $data[6] = chr(ord($data[6]) & 0x0f | 0x40); // set version to 0010
        $data[8] = chr(ord($data[8]) & 0x3f | 0x80); // set bits 6-7 to 10

        $parts = str_split(bin2hex($data), 4);
        $guid = new Guid();
        $guid->_parts = array(
            $parts[0] . $parts[1],
            $parts[2],
            $parts[3],
            $parts[4],
            $parts[5] . $parts[6] . $parts[7]
        );

        return $guid;
    }

    public static function TryParse($asString, &$out_guid) {
        $out_guid = NULL;

        foreach (self::$_parseFormats as $format) {
            if (1 == preg_match($format, $asString)) {
                $clean = strtolower(str_replace(array("-", "{", "}", "(", ")", "0x", ","), "", $asString));
                $out_guid = new Guid();
                $out_guid->_parts = array(
                    substr($clean, 0, 8),
                    substr($clean, 8, 4),
                    substr($clean, 12, 4),
                    substr($clean, 16, 4),
                    substr($clean, 20, 12),
                );

                return true;
            }
        }

        return false;
    }

    public static function Parse($asString) {
        if (self::TryParse($asString, $out_guid)) {
            return $out_guid;
        }

        throw new Exception("Invalid Guid: " . $asString);
    }

    private $_parts;

    public function __construct() {
        $this->_parts = self::$_empty;
    }

    private static function _comparer(Guid $guid1, Guid $guid2) {
        return $guid1->_parts == $guid2->_parts;
    }

    public function Equals(ObjectBase $obj) {
        return self::_comparer($this, $obj);
    }

    public function ToString($format = NULL) {
        switch ($format) {
            case "";
            case "D";
                return implode("-", $this->_parts);
            case "N";
                return implode("", $this->_parts);
            case "B";
                return "{" . implode("-", $this->_parts) . "}";
            case "P";
                return "(" . implode("-", $this->_parts) . ")";
            case "X";
                $tmp = array(
                    "0x" . $this->_parts[0],
                    "0x" . $this->_parts[1],
                    "0x" . $this->_parts[2],
                    "{0x" . implode(",0x", str_split($this->_parts[3] . $this->_parts[4], 2)) . "}"
                );

                return "{" . implode(",", $tmp) . "}";
            default:
                throw new \Exception("Invalid Guid format" . $format);
        }
    }

    public function __toString() {
        return $this->ToString("D");
    }

}

The usage is easy:

$apiKey = Guid::NewGuid();

Yes you can have different formats if you like, the default __toString() call will result in something like:

f9168c5e-ceb2-4faa-b6bf-329bf39fa1e4

And you don't have to check for collisons becouse it has a really small chance to have two identical Guid generated with openssl_random_pseudo_bytes(). The only thing you have to have is a PHP installation with version 5.3.0 or higher but it's recommended anyway to have the most recent PHP version (now 5.5.7).

\$\endgroup\$
  • \$\begingroup\$ i disagree. why would you not check for collision? that's just lazy \$\endgroup\$ – r3wt Dec 30 '13 at 19:25
  • \$\begingroup\$ Generate with my code 1 billion identifier you will see. \$\endgroup\$ – Peter Kiss Dec 30 '13 at 19:36
  • \$\begingroup\$ Ok thank's Peter Kiss. i will consider using your method for creating tradekeys. they fit the bill perfectly for that, and i can use a similar scheme of polling the db to ensure uniqueness, just to be safe. this is a trading site, margin for error == NULL. i went with 200_success's answer. your's was very good, but his provided something a little more to my liking. \$\endgroup\$ – r3wt Dec 31 '13 at 1:58
  • \$\begingroup\$ Read UUID generation techniques and probability of duplicates. A properly generated UUID is so unlikely to collide that if you want to worry about collisions, you'll also want to worry about memory/CPU defects and cosmic rays making your computer behave nondeterministically. \$\endgroup\$ – 200_success Dec 31 '13 at 2:55
  • \$\begingroup\$ i would have used it if the class worked. \$\endgroup\$ – r3wt Jan 13 '14 at 9:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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