Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm not a dedicated cryptographer, so I'm looking for someone to look over these functions I wrote and let me know if there are any implementation errors leading to security vulnerabilities or just anything I should improve. Any other comments about improving performance, etc. would be great too.

This is used in a PHP-based file server I'm writing, where users can optionally encrypt their files individually through their web browser. These are the filesystem functions that are called in the middle of Ajax request.

I had to separate the decryption into 3 parts because it's used not only for statically decrypting files but also for doing it on the fly during a file download. So it's not always used the same way.

It's designed to be very modular so that ciphers and other settings can be changed readily. I also took a cautious approach to security, using cascading ciphers, a 512-bit salt, 4000-iteration whirlpool-based key derivation and multiple keys and initialization vectors per file, which also saves on memory usage.

public static function EncryptFile($owner, $id, $password) { // This requires PHP 5.5.0 or higher!
    global $mysqli, $settings;
    if (empty($owner) || empty($id) || !isset($password)) {
        return false;
    }
    $crypto_settings = unserialize($settings['crypto']);
    $secure = false;
    openssl_random_pseudo_bytes(1, $secure);
    if (!$secure) {
        return false;
    }
    if (!($file = mysqli_fetch_assoc(mysqli_query($mysqli, 'SELECT * FROM content_files WHERE owner="' . $owner . '" AND id="' . $id . '"')))) {
        return false;
    }
    if ($file['crypto_binary'] || $file['crypto_settings']) {
        return false;
    }
    $chunks = ceil($file['size'] / $crypto_settings['chunk_size']);
    $keys = array();
    $vectors = array();
    $keys_string = "";
    $vectors_string = "";
    for ($i = 0; $i < $chunks; $i++) {
        $keys[$i] = array();
        for ($n = 0; $n < count($crypto_settings['ciphers']); $n++) {
            $key = openssl_random_pseudo_bytes($crypto_settings['key_lengths'][$n] / 8);
            $keys[$i][$n] = $key;
            $keys_string. = $key;
        }
        $vectors[$i] = array();
        for ($n = 0; $n < count($crypto_settings['ciphers']); $n++) {
            $vector = openssl_random_pseudo_bytes($crypto_settings['vector_lengths'][$n] / 8);
            $vectors[$i][$n] = $vector;
            $vectors_string. = $vector;
        }
    }
    $salt = openssl_random_pseudo_bytes($crypto_settings['salt_length'] / 8);
    $master_key_length = 0;
    for ($i = 0; $i < count($crypto_settings['ciphers']); $i++) {
        $master_key_length += ($crypto_settings['key_lengths'][$i] + $crypto_settings['vector_lengths'][$i]) / 8;
    }
    $master_key = openssl_pbkdf2($password, $salt, $master_key_length, $crypto_settings['pbkdf2_iterations'], $crypto_settings['pbkdf2_algorithm']);
    $master_key_used = 0;
    $crypto_binary = "TRUE".$keys_string.$vectors_string;
    for ($i = 0; $i < count($crypto_settings['ciphers']); $i++) {
        $crypto_binary = openssl_encrypt($crypto_binary, $crypto_settings['ciphers'][$i], substr($master_key, $master_key_used, $crypto_settings['key_lengths'][$i] / 8), OPENSSL_RAW_DATA, substr($master_key, $master_key_used + $crypto_settings['key_lengths'][$i] / 8, $crypto_settings['vector_lengths'][$i] / 8));
        $master_key_used += ($crypto_settings['key_lengths'][$i] + $crypto_settings['vector_lengths'][$i]) / 8;
    }
    $crypto_binary = $salt.$crypto_binary;
    if (!mysqli_query($mysqli, 'UPDATE content_files SET crypto_binary="'.mysqli_real_escape_string($mysqli, $crypto_binary).
        '", crypto_settings="'.mysqli_real_escape_string($mysqli, serialize($crypto_settings)).
        '" WHERE id="'.$id.
        '" AND owner="'.$owner.
        '"')) {
        return false;
    }
    for ($chunk = 0; $chunk < $chunks; $chunk++) {
        $file = fopen("content/$id", 'rb+');
        if (!$file) {
            return false;
        }
        fseek($file, $chunk * $crypto_settings['chunk_size']);
        $data = fread($file, $crypto_settings['chunk_size']);
        for ($i = 0; $i < count($crypto_settings['ciphers']); $i++) {
            $data = openssl_encrypt($data, $crypto_settings['ciphers'][$i], $keys[$chunk][$i], OPENSSL_RAW_DATA, $vectors[$chunk][$i]);
        }
        fseek($file, $chunk * $crypto_settings['chunk_size']);
        fwrite($file, $data);
        fclose($file);
    }
    return true;
}
public static function DecryptHeader($crypto_settings, $crypto_binary, $filesize, $password) {
    $chunks = ceil($filesize / $crypto_settings['chunk_size']);
    $salt = substr($crypto_binary, 0, $crypto_settings['salt_length'] / 8);
    $header = substr($crypto_binary, $crypto_settings['salt_length'] / 8);
    $master_key_length = 0;
    for ($i = 0; $i < count($crypto_settings['ciphers']); $i++) {
        $master_key_length += ($crypto_settings['key_lengths'][$i] + $crypto_settings['vector_lengths'][$i]) / 8;
    }
    $master_key = openssl_pbkdf2($password, $salt, $master_key_length, $crypto_settings['pbkdf2_iterations'], $crypto_settings['pbkdf2_algorithm']);
    $master_key_used = 0;
    for ($i = count($crypto_settings['ciphers']) - 1; $i >= 0; $i--) {
        $header = openssl_decrypt($header, $crypto_settings['ciphers'][$i], substr($master_key, -1 * $master_key_used - ($crypto_settings['key_lengths'][$i] + $crypto_settings['vector_lengths'][$i]) / 8, $crypto_settings['key_lengths'][$i] / 8), OPENSSL_RAW_DATA, substr($master_key, -1 * $master_key_used - ($crypto_settings['vector_lengths'][$i] / 8), $crypto_settings['vector_lengths'][$i] / 8));
        $master_key_used += ($crypto_settings['key_lengths'][$i] + $crypto_settings['vector_lengths'][$i]) / 8;
    }
    if (substr($header, 0, 4) != "TRUE") {
        return false;
    } else {
        $header = substr($header, 4);
    }
    $keys = array();
    $vectors = array();
    $header_position = 0;
    for ($c = 0; $c < $chunks; $c++) {
        $keys[$c] = array();
        for ($n = 0; $n < count($crypto_settings['ciphers']); $n++) {
            $keys[$c][$n] = substr($header, $header_position, $crypto_settings['key_lengths'][$n] / 8);
            $header_position += $crypto_settings['key_lengths'][$n] / 8;
        }
    }
    for ($c = 0; $c < $chunks; $c++) {
        $vectors[$c] = array();
        for ($n = 0; $n < count($crypto_settings['ciphers']); $n++) {
            $vectors[$c][$n] = substr($header, $header_position, $crypto_settings['vector_lengths'][$n] / 8);
            $header_position += $crypto_settings['vector_lengths'][$n] / 8;
        }
    }
    return array('keys' = > $keys, 'vectors' = > $vectors);
}
public static function DecryptChunk($file, $crypto_settings, $header, $chunk) {
    fseek($file, $chunk * $crypto_settings['chunk_size']);
    $data = fread($file, $crypto_settings['chunk_size']);
    for ($i = count($crypto_settings['ciphers']) - 1; $i >= 0; $i--) {
        $data = openssl_decrypt($data, $crypto_settings['ciphers'][$i], $header['keys'][$chunk][$i], OPENSSL_RAW_DATA, $header['vectors'][$chunk][$i]);
    }
    fseek($file, $chunk * $crypto_settings['chunk_size']);
    return $data;
}
public static function DecryptFile($owner, $id, $password) { // This requires PHP 5.5.0 or higher!
    global $mysqli;
    if (empty($owner) || empty($id) || !isset($password)) {
        return false;
    }
    if (!($file = mysqli_fetch_assoc(mysqli_query($mysqli, 'SELECT size, crypto_settings, crypto_binary FROM content_files WHERE owner="'.$owner.
        '" AND id="'.$id.
        '"')))) {
        return false;
    }
    if (!$file['crypto_settings'] || !$file['crypto_binary']) {
        return false;
    }
    $crypto_settings = unserialize($file['crypto_settings']);
    $crypto_binary = $file['crypto_binary'];
    $header = Filesystem::DecryptHeader($crypto_settings, $crypto_binary, $file['size'], $password);
    if (!$header) {
        return false;
    }
    for ($chunk = 0; $chunk < count($header['keys']); $chunk++) {
        $handle = fopen("content/$id", 'rb+');
        if (!$handle) {
            return false;
        }
        $data = Filesystem::DecryptChunk($handle, $crypto_settings, $header, $chunk);
        fwrite($handle, $data);
        fclose($handle);
    }
    if (!mysqli_query($mysqli, 'UPDATE content_files SET crypto_binary="", crypto_settings="" WHERE id="'.$id.
        '" AND owner="'.$owner.
        '"')) {
        return false;
    }
    return true;
}

Here's what I currently have in the DB as the starting cipher settings:

a:7:{s:7:"ciphers";a:2:{i:0;s:16:"CAMELLIA-256-CFB";i:1;s:11:"AES-256-CFB";}s:11:"key_lengths";a:2:{i:0;i:256;i:1;i:256;}s:14:"vector_lengths";a:2:{i:0;i:128;i:1;i:128;}s:11:"salt_length";i:512;s:16:"pbkdf2_algorithm";s:9:"whirlpool";s:17:"pbkdf2_iterations";i:3996;s:10:"chunk_size";i:20971520;}
share|improve this question
 
mysqli_query() input must be properly escaped. Can you guarantee this happens outside the EncryptFile() function? –  ldrumm Dec 5 '13 at 4:26
 
Oh no, I haven't actually put any effort at all into that yet, should've mentioned that! Right now $password comes directly from the outside, but I will validate it (it just won't happen here, it'll go in the ajax wrapper around these!) –  user3068322 Dec 5 '13 at 4:27
 
Actually the $id and $owner both originally come from the client as well, but those will be easy to validate since they're both 32-character alphanumerics. –  user3068322 Dec 5 '13 at 4:30
2  
Definitely need to look into prepared statements: php.net/manual/en/mysqli.quickstart.prepared-statements.php - or better yet, use PDO: php.net/manual/en/book.pdo.php. –  Tieson T. Dec 5 '13 at 4:48
 
Aside from the obvious swap to prepared statements (don't use PDO unless you plan on swapping from mysql in future using mysqli prepares is just fine and pdo is horrible) the rest of the code relating to the crypto side of things looks ok to me, formatting seems nice and I can't see any obvious security holes or reasons for failure. Just makesure you validate all your inputs properly if they come from anywhere a user can provide them. –  Dave Dec 6 '13 at 11:19
show 4 more comments

Know someone who can answer? Share a link to this question via email, Google+, Twitter, or Facebook.

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Browse other questions tagged or ask your own question.