php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #72362 OpenSSL Blowfish encryption is incorrect for short keys
Submitted: 2016-06-08 12:35 UTC Modified: 2017-07-02 17:35 UTC
Votes:1
Avg. Score:4.0 ± 0.0
Reproduced:1 of 1 (100.0%)
Same Version:1 (100.0%)
Same OS:0 (0.0%)
From: sachavav at tut dot by Assigned: bukka (profile)
Status: Closed Package: OpenSSL related
PHP Version: 5.6.22 OS: Linux/Ubuntu
Private report: No CVE-ID: None
View Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
If you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: sachavav at tut dot by
New email:
PHP Version: OS:

 

 [2016-06-08 12:35 UTC] sachavav at tut dot by
Description:
------------
According to description of Blowfish algorithm it can use variable key starting from 32bits. If the key is shorter than block, it should be cycled over.
https://www.schneier.com/academic/archives/1994/09/description_of_a_new.html

Keys A - AA - AAA should be equivalent. This is true in openssl extension for keys, longer than 128 bits, but for shorter keys extension works incorrectly. It padds short keys with zeros up to 128 bits instead of cycling them.

Please check sample code for explanation.

If you can compare behavior of mcrypt extension - it works correctly there.

Test script:
---------------
> php -r 'echo bin2hex(openssl_encrypt("this is a test string","bf-ecb","12345678" , OPENSSL_RAW_DATA));'
0b30345b335e2ca5ba4d12c0077768c99680ca260b07d693
> php -r 'echo bin2hex(openssl_encrypt("this is a test string","bf-ecb","12345678\0\0\0\0\0\0\0\0" , OPENSSL_RAW_DATA));'
0b30345b335e2ca5ba4d12c0077768c99680ca260b07d693

> php -r 'echo bin2hex(openssl_encrypt("this is a test string","bf-ecb","1234567812345678" , OPENSSL_RAW_DATA));'
e3214d1b16e574828c8a3e222202dde81afd1ad2cb165ab3

Expected result:
----------------
Short keys are cycled over.

Actual result:
--------------
Short keys are padded with zeros

Patches

Pull Requests

Pull requests:

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2017-04-25 13:10 UTC] bukka@php.net
-Status: Open +Status: Assigned -Assigned To: +Assigned To: bukka
 [2017-04-25 13:10 UTC] bukka@php.net
So this is happening for exactly the same reason as https://bugs.php.net/bug.php?id=71917 (we don't try to set key length for variable length ciphers if the key is shorter than the default key size for the algorithm).

Unfortunately changing that as default would have exactly the same implication in terms of BC. It would stop working for people using shorter keys and relying on the fact that is filled with \0. So I would be inclined to introduce a new option for that which could possibly go to 7.1 if RM is fine with that or master (7.2) otherwise. I will create a PR.
 [2017-07-02 17:35 UTC] bukka@php.net
-Status: Assigned +Status: Closed
 [2017-07-02 17:35 UTC] bukka@php.net
A new constant OPENSSL_DONT_ZERO_PAD_KEY has been introduced to address that and it will be part of 7.1.8.

For example it can be used like:

openssl_encrypt("this is a test string","bf-ecb","12345678", OPENSSL_RAW_DATA | OPENSSL_DONT_ZERO_PAD_KEY));
 [2018-02-02 17:30 UTC] ian at oshaughnessy dot cc
Is there any way to have this fix backported to 7.0?

Since mcrypt was deprecated in 7.1, it makes it more difficult to transition to OpenSSL given this issue.
 [2018-03-09 11:46 UTC] wxiaoguang at gmail dot com
Use this function for OpenSSL to get the same key of mcrypt's blowfish:

function make_openssl_blowfish_key($key)
{
    if("$key" === '')
        return $key;

    $len = (16+2) * 4;
    while(strlen($key) < $len) {
        $key .= $key;
    }
    $key = substr($key, 0, $len);
    return $key;
}
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Sat Nov 23 08:01:28 2024 UTC