New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed compatibility between encrypt() and decrypt() using openssl extention #122

Merged
merged 3 commits into from Mar 26, 2017

Conversation

Projects
None yet
2 participants
@orrisroot
Contributor

orrisroot commented Mar 26, 2017

Current XCube_Utils::decrypt() can not decrypt encrypted text using XCube_Utils::encrypt().
I'd like to request to fix this issue using this pull request!

Note 1: mcrypt based encrypted text does not have compatibility with openssl based functions. because current mcrypt based routine is not using PKCS#7 padding. All users have to update password related configuration fields after replace this file.
Note 2: Actually, in order to use openssl_encrypt() and openssl_decrypt(), we have to adjust the password length according to cipher suite. otherwise, openssl_error_string() function will notify a warning. refer: https://github.com/tom--/mcrypt2openssl/blob/master/mapping.md

Show outdated Hide outdated html/core/XCube_Utils.class.php
@@ -186,7 +186,7 @@ public static function decrypt($crypt_text, $key = null, $algorithm = 'des', $mo
mcrypt_generic_deinit($td);
mcrypt_module_close($td);
} else {
$plain_text = openssl_decrypt($crypt_text, strtoupper($algorithm.'-'.$mode), $key, OPENSSL_ZERO_PADDING);
$plain_text = openssl_decrypt($crypt_text, strtoupper($algorithm.'-'.$mode), $key);

This comment has been minimized.

@nao-pon

nao-pon Mar 26, 2017

Member

Need OPENSSL_ZERO_PADDING to decrypting encrypt data by mcrypt (old data). Why remove it?

@nao-pon

nao-pon Mar 26, 2017

Member

Need OPENSSL_ZERO_PADDING to decrypting encrypt data by mcrypt (old data). Why remove it?

This comment has been minimized.

@nao-pon

nao-pon Mar 26, 2017

Member

ん~私の理解が足らないのかかhも?他の暗号化方式を使った場合に問題があるということでしょうか・・・

@nao-pon

nao-pon Mar 26, 2017

Member

ん~私の理解が足らないのかかhも?他の暗号化方式を使った場合に問題があるということでしょうか・・・

@nao-pon

This comment has been minimized.

Show comment
Hide comment
@nao-pon

nao-pon Mar 26, 2017

Member

よくみてみたら、iv をランダムにしているので mcrypt を使う場合でも iv が必要になる暗号化方式では正常に機能していませんね。ここも合わせて修正が必要ですね。
私は詳しくないのですが、iv を取得するのに sha1 とかのハッシュ値から取得してもいいのかな?

例えば

$method = strtoupper($algorithm.'-'.$mode);
$ivLength = openssl_cipher_iv_length($method);
$iv = substr(sha1($key), 0, $ivLength);
$crypt_text = openssl_encrypt($plain_text, $method, $key, 0, $iv);
Member

nao-pon commented Mar 26, 2017

よくみてみたら、iv をランダムにしているので mcrypt を使う場合でも iv が必要になる暗号化方式では正常に機能していませんね。ここも合わせて修正が必要ですね。
私は詳しくないのですが、iv を取得するのに sha1 とかのハッシュ値から取得してもいいのかな?

例えば

$method = strtoupper($algorithm.'-'.$mode);
$ivLength = openssl_cipher_iv_length($method);
$iv = substr(sha1($key), 0, $ivLength);
$crypt_text = openssl_encrypt($plain_text, $method, $key, 0, $iv);
@orrisroot

This comment has been minimized.

Show comment
Hide comment
@orrisroot

orrisroot Mar 26, 2017

Contributor

はい、そうですね。暗号化時の IV はランダムでもいいと思います。openssl_random_pseudo_bytes() で生成してしまっていいのではないでしょうか。

えーと、私も手探りなのですが調べた限りですと、mcrypt 使って暗号化される文字列は デフォルトでブロック長分ゼロパディングされるようなので、openssl で復号する場合は OPENSSL_ZERO_PADDING をつけて mcrypt のときと同様に rtrim($plain_text, "\0") すれば良さそうですが、openssl で暗号化される文字列はデフォルトで PKCS7 パディングされるようなので、復号する場合OPENSSL_ZERO_PADDING をつけずに自動でパディング除去をさせるか、つけて自身で PKCS7 パディング除去をしてあげる必要がありそうです。

どちらにせよ現状の OPENSSL_ZERO_PADDING をつけたコードは正しく動いていなくて
・過去に mcrypt で暗号化された文字列が、openssl で復号してもゼロパディングされたまま
・新たに openssl で暗号化された文字列が、openssl で復号してもPKCS7パディングされたまま
となり、どちらもパディング文字列が残ったままの状態です。

選択肢として、Pull Request のように
1.mcrypt で暗号化されたものの互換性は捨てて、新たに openssl で暗号化されたものだけ正しく復号化できるようにする。
もしくは、
2.過去の mcrypt で暗号化されたもののうち一部(後術)もなんとか頑張ろうとして、 openssl で復号化する際に OPENSSL_ZERO_PADDING つけた後、ゼロパディングもしくは PCKS7 パディングを剥ぎ取る処理を加える。
のいずれかかと思います。

あと、2.の場合も互換性面では微妙に怪しくて、そもそも今の mcrypt ベースの処理も怪しくて encrypt() と decrypt() で異なるランダムな文字列の Initialization Vector $iv を使っていますので、ECB 以外の暗号利用モードを使っていた場合、どちらにせよ正しく復号化できなかった気がします。ということで、mcrypt で暗号化時に使った $iv を保存・復号化時に使える仕様にはなっていないので、新たに加わった openssl なコードに対して、頑張って間違って暗号化された可能性のある mcrypt に対して一部だけでも互換性を残そうとする必要もあんまりない気がしてますので1.で安易に提案した次第です。

https://en.wikipedia.org/wiki/Block_cipher_mode_of_operation

Contributor

orrisroot commented Mar 26, 2017

はい、そうですね。暗号化時の IV はランダムでもいいと思います。openssl_random_pseudo_bytes() で生成してしまっていいのではないでしょうか。

えーと、私も手探りなのですが調べた限りですと、mcrypt 使って暗号化される文字列は デフォルトでブロック長分ゼロパディングされるようなので、openssl で復号する場合は OPENSSL_ZERO_PADDING をつけて mcrypt のときと同様に rtrim($plain_text, "\0") すれば良さそうですが、openssl で暗号化される文字列はデフォルトで PKCS7 パディングされるようなので、復号する場合OPENSSL_ZERO_PADDING をつけずに自動でパディング除去をさせるか、つけて自身で PKCS7 パディング除去をしてあげる必要がありそうです。

どちらにせよ現状の OPENSSL_ZERO_PADDING をつけたコードは正しく動いていなくて
・過去に mcrypt で暗号化された文字列が、openssl で復号してもゼロパディングされたまま
・新たに openssl で暗号化された文字列が、openssl で復号してもPKCS7パディングされたまま
となり、どちらもパディング文字列が残ったままの状態です。

選択肢として、Pull Request のように
1.mcrypt で暗号化されたものの互換性は捨てて、新たに openssl で暗号化されたものだけ正しく復号化できるようにする。
もしくは、
2.過去の mcrypt で暗号化されたもののうち一部(後術)もなんとか頑張ろうとして、 openssl で復号化する際に OPENSSL_ZERO_PADDING つけた後、ゼロパディングもしくは PCKS7 パディングを剥ぎ取る処理を加える。
のいずれかかと思います。

あと、2.の場合も互換性面では微妙に怪しくて、そもそも今の mcrypt ベースの処理も怪しくて encrypt() と decrypt() で異なるランダムな文字列の Initialization Vector $iv を使っていますので、ECB 以外の暗号利用モードを使っていた場合、どちらにせよ正しく復号化できなかった気がします。ということで、mcrypt で暗号化時に使った $iv を保存・復号化時に使える仕様にはなっていないので、新たに加わった openssl なコードに対して、頑張って間違って暗号化された可能性のある mcrypt に対して一部だけでも互換性を残そうとする必要もあんまりない気がしてますので1.で安易に提案した次第です。

https://en.wikipedia.org/wiki/Block_cipher_mode_of_operation

@orrisroot

This comment has been minimized.

Show comment
Hide comment
@orrisroot

orrisroot Mar 26, 2017

Contributor

openssl と mcrypt との互換性確保を真面目にやろうとしたら結構面倒そうですね。
課題としては今後のことを考えると openssl 側の仕様を主とするのがよさそうだと思いますが、ある程度まともなコードにするには…

  1. アルゴリズムとモードのパラメータ互換性の確保
    mcrypt 側に対応変換マップを持たせる。
    https://github.com/tom--/mcrypt2openssl/blob/master/mapping.md

  2. パディング方法の統一
    mcrypt ではデフォルトでゼロパディングされてしまうので、暗号化する前に PKCS7 パディング処理をいれ、復号化後に PKCS7 パディング除去処理をいれる。

  3. Initialization Vector 値の統一
    IV 使うならば encrypt 時の IV を decrypt 時に使えるような細工が必要。encrypt 時にランダム生成して暗号化後の文字と結合してしまうか、KEY 同様 SALT あたりから一意になるようにしてしまうか。

  4. openssl 利用時の KEY 長の指定
    mcrypt のときと同様に openssl_encrypt()、 openssl_decrypt() に渡さねばならない $key の長さは暗号化アルゴリズム・モードによって異なるが、openssl 側には、mcrypt_enc_get_key_size() のような必要となるキー長を返す関数がない。自前でアルゴリズムとモードごとのキー長データを持つしかなさそう。これを怠ると openssl_error_string() でエラーが出る。

  5. これまでの互換性はどうするか
    mcrypt 側の処理も変更してしまう必要があるので、頑張って残さないといけない理由が薄れる。捨ててしまっていいか?

な感じでしょうか。

Contributor

orrisroot commented Mar 26, 2017

openssl と mcrypt との互換性確保を真面目にやろうとしたら結構面倒そうですね。
課題としては今後のことを考えると openssl 側の仕様を主とするのがよさそうだと思いますが、ある程度まともなコードにするには…

  1. アルゴリズムとモードのパラメータ互換性の確保
    mcrypt 側に対応変換マップを持たせる。
    https://github.com/tom--/mcrypt2openssl/blob/master/mapping.md

  2. パディング方法の統一
    mcrypt ではデフォルトでゼロパディングされてしまうので、暗号化する前に PKCS7 パディング処理をいれ、復号化後に PKCS7 パディング除去処理をいれる。

  3. Initialization Vector 値の統一
    IV 使うならば encrypt 時の IV を decrypt 時に使えるような細工が必要。encrypt 時にランダム生成して暗号化後の文字と結合してしまうか、KEY 同様 SALT あたりから一意になるようにしてしまうか。

  4. openssl 利用時の KEY 長の指定
    mcrypt のときと同様に openssl_encrypt()、 openssl_decrypt() に渡さねばならない $key の長さは暗号化アルゴリズム・モードによって異なるが、openssl 側には、mcrypt_enc_get_key_size() のような必要となるキー長を返す関数がない。自前でアルゴリズムとモードごとのキー長データを持つしかなさそう。これを怠ると openssl_error_string() でエラーが出る。

  5. これまでの互換性はどうするか
    mcrypt 側の処理も変更してしまう必要があるので、頑張って残さないといけない理由が薄れる。捨ててしまっていいか?

な感じでしょうか。

@orrisroot

This comment has been minimized.

Show comment
Hide comment
@orrisroot

orrisroot Mar 26, 2017

Contributor

あ、あとついでに上記 4. に関連して、 $key とかの元データにハッシュ値を使うならば、raw_output フラグ立てて BINARY データとして必要キー長取り出すようにしないと強度が激減しそうです。また、逆にアルゴリズムが AES だとキー長は 56 byte ですので、md5 や sha1 などのバイナリハッシュ値だと16byte や 20byte では足りない可能性もあるので足し加える処理も要りそうです。

Contributor

orrisroot commented Mar 26, 2017

あ、あとついでに上記 4. に関連して、 $key とかの元データにハッシュ値を使うならば、raw_output フラグ立てて BINARY データとして必要キー長取り出すようにしないと強度が激減しそうです。また、逆にアルゴリズムが AES だとキー長は 56 byte ですので、md5 や sha1 などのバイナリハッシュ値だと16byte や 20byte では足りない可能性もあるので足し加える処理も要りそうです。

@nao-pon

This comment has been minimized.

Show comment
Hide comment
@nao-pon

nao-pon Mar 26, 2017

Member

色々とややこしそうですね。私がこのメソッドを追加したのですが、動機は管理画面の設定値で生データのままデータベースに保存することに抵抗のあるデーター(例えば X-update の FTP ID やパスワードなど)を簡易的でも暗号化したほうがよかろうということで、管理画面から簡単に呼び出せるようにしたことです。

よって、現状で使用されているのはデフォルトの des-ecb で iv は必要ないため、この問題のあるコードでもたまたま動いていました。

今回の修正要件としては、openssl 系関数の使用可能な環境下で mcrypt の des-ecb でエンコードしたデータを問題なくデコードできることが必須になります。

あとは、今後使われる場合を考えて、汎用的なメソッドにすることでよいと思います。アルゴリズムとモードが合致しないものを指定する場合については、環境依存によるところが大きいので、このメソッドを呼び出す以前に環境を判定してそれぞれ指定して呼び出していただくようにしたいと思います。また Key長に関しても、必要に応じて選択した暗号化に適した Key を予め与えるということにしたいと思います。

後ほど、私なりに修正してみます。

Member

nao-pon commented Mar 26, 2017

色々とややこしそうですね。私がこのメソッドを追加したのですが、動機は管理画面の設定値で生データのままデータベースに保存することに抵抗のあるデーター(例えば X-update の FTP ID やパスワードなど)を簡易的でも暗号化したほうがよかろうということで、管理画面から簡単に呼び出せるようにしたことです。

よって、現状で使用されているのはデフォルトの des-ecb で iv は必要ないため、この問題のあるコードでもたまたま動いていました。

今回の修正要件としては、openssl 系関数の使用可能な環境下で mcrypt の des-ecb でエンコードしたデータを問題なくデコードできることが必須になります。

あとは、今後使われる場合を考えて、汎用的なメソッドにすることでよいと思います。アルゴリズムとモードが合致しないものを指定する場合については、環境依存によるところが大きいので、このメソッドを呼び出す以前に環境を判定してそれぞれ指定して呼び出していただくようにしたいと思います。また Key長に関しても、必要に応じて選択した暗号化に適した Key を予め与えるということにしたいと思います。

後ほど、私なりに修正してみます。

@nao-pon

This comment has been minimized.

Show comment
Hide comment
@nao-pon

nao-pon Mar 26, 2017

Member

こんな感じでいかがでしょうか、必要に応じて $key, $algorithm, $mode, $option, $iv を指定して使用して頂く感じです。

    /**
     * @public
     * @brief [Static] To encrypt strings.
     * @param $plain_text string
     * @param $key        string
     * @param $algorithm  string
     * @param $mode       string
     * @param $option     integer  option of openssl_encrypt()
     * @param $iv         string   initialization vector
     * @return string - Encrypted string.
     */
    public static function encrypt($plain_text, $key = null, $algorithm = 'des', $mode = 'ecb', $option = null, $iv = null)
    {
        if ($plain_text === '') {
            return $plain_text;
        }
        
        if (is_null($key) || ! is_string($key)) {
            if (! defined('XOOPS_SALT')) {
                return $plain_text;
            }
            $key = XOOPS_SALT;
        }
        $key = md5($key);

        if (! function_exists('openssl_encrypt')) {
            if (! extension_loaded('mcrypt')) {
                return $plain_text;
            }
            $td  = mcrypt_module_open($algorithm, '', $mode, '');
            $key = substr($key, 0, mcrypt_enc_get_key_size($td));
            if (is_null($iv)) {
                $iv  = substr(sha1($key), 0, mcrypt_enc_get_iv_size($td));
            }
            
            if (mcrypt_generic_init($td, $key, $iv) < 0) {
                return $plain_text;
            }
            
            $crypt_text = base64_encode(mcrypt_generic($td, $plain_text));
            
            mcrypt_generic_deinit($td);
            mcrypt_module_close($td);
        } else {
            $method = strtoupper($algorithm.'-'.$mode);
            if (is_null($option)) {
                $option = 0;
            }
            if (is_null($iv)) {
                $ivLength = openssl_cipher_iv_length($method);
                $iv = substr(sha1($key), 0, $ivLength);
            }
            $crypt_text = openssl_encrypt($plain_text, $method, $key, $option, $iv);
        }
        
        return $crypt_text === false ? $plain_text : $crypt_text;
    }
    
    /**
     * @public
     * @brief [Static] To decrypt strings.
     * @param $crypt_text string
     * @param $key        string
     * @param $algorithm  string
     * @param $$mode      string
     * @param $option     integer  option of openssl_decrypt()
     * @param $iv         string   initialization vector
     * @return string - Decrypted string.
     */
    public static function decrypt($crypt_text, $key = null, $algorithm = 'des', $mode = 'ecb', $option = null, $iv = null)
    {
        if ($crypt_text === '') {
            return $crypt_text;
        }
        
        if (is_null($key) || ! is_string($key)) {
            if (! defined('XOOPS_SALT')) {
                return $crypt_text;
            }
            $key = XOOPS_SALT;
        }
        $key = md5($key);

        if (! function_exists('openssl_decrypt')) {
            if (! extension_loaded('mcrypt')) {
                return $crypt_text;
            }
            $td  = mcrypt_module_open($algorithm, '', $mode, '');
            $key = substr($key, 0, mcrypt_enc_get_key_size($td));
            if (is_null($iv)) {
                $iv  = substr(sha1($key), 0, mcrypt_enc_get_iv_size($td));
            }
            
            if (mcrypt_generic_init($td, $key, $iv) < 0) {
                return $crypt_text;
            }
            
            $plain_text = rtrim(mdecrypt_generic($td, base64_decode($crypt_text)), "\0");
            
            mcrypt_generic_deinit($td);
            mcrypt_module_close($td);
        } else {
            $method = strtoupper($algorithm.'-'.$mode);
            if (is_null($option)) {
                $option = OPENSSL_ZERO_PADDING;
            }
            if (is_null($iv)) {
                $ivLength = openssl_cipher_iv_length($method);
                $iv = substr(sha1($key), 0, $ivLength);
            }
            $plain_text = openssl_decrypt($crypt_text, $method, $key, $option, $iv);
        }
        
        return $plain_text === false ? $crypt_text : $plain_text;
    }
Member

nao-pon commented Mar 26, 2017

こんな感じでいかがでしょうか、必要に応じて $key, $algorithm, $mode, $option, $iv を指定して使用して頂く感じです。

    /**
     * @public
     * @brief [Static] To encrypt strings.
     * @param $plain_text string
     * @param $key        string
     * @param $algorithm  string
     * @param $mode       string
     * @param $option     integer  option of openssl_encrypt()
     * @param $iv         string   initialization vector
     * @return string - Encrypted string.
     */
    public static function encrypt($plain_text, $key = null, $algorithm = 'des', $mode = 'ecb', $option = null, $iv = null)
    {
        if ($plain_text === '') {
            return $plain_text;
        }
        
        if (is_null($key) || ! is_string($key)) {
            if (! defined('XOOPS_SALT')) {
                return $plain_text;
            }
            $key = XOOPS_SALT;
        }
        $key = md5($key);

        if (! function_exists('openssl_encrypt')) {
            if (! extension_loaded('mcrypt')) {
                return $plain_text;
            }
            $td  = mcrypt_module_open($algorithm, '', $mode, '');
            $key = substr($key, 0, mcrypt_enc_get_key_size($td));
            if (is_null($iv)) {
                $iv  = substr(sha1($key), 0, mcrypt_enc_get_iv_size($td));
            }
            
            if (mcrypt_generic_init($td, $key, $iv) < 0) {
                return $plain_text;
            }
            
            $crypt_text = base64_encode(mcrypt_generic($td, $plain_text));
            
            mcrypt_generic_deinit($td);
            mcrypt_module_close($td);
        } else {
            $method = strtoupper($algorithm.'-'.$mode);
            if (is_null($option)) {
                $option = 0;
            }
            if (is_null($iv)) {
                $ivLength = openssl_cipher_iv_length($method);
                $iv = substr(sha1($key), 0, $ivLength);
            }
            $crypt_text = openssl_encrypt($plain_text, $method, $key, $option, $iv);
        }
        
        return $crypt_text === false ? $plain_text : $crypt_text;
    }
    
    /**
     * @public
     * @brief [Static] To decrypt strings.
     * @param $crypt_text string
     * @param $key        string
     * @param $algorithm  string
     * @param $$mode      string
     * @param $option     integer  option of openssl_decrypt()
     * @param $iv         string   initialization vector
     * @return string - Decrypted string.
     */
    public static function decrypt($crypt_text, $key = null, $algorithm = 'des', $mode = 'ecb', $option = null, $iv = null)
    {
        if ($crypt_text === '') {
            return $crypt_text;
        }
        
        if (is_null($key) || ! is_string($key)) {
            if (! defined('XOOPS_SALT')) {
                return $crypt_text;
            }
            $key = XOOPS_SALT;
        }
        $key = md5($key);

        if (! function_exists('openssl_decrypt')) {
            if (! extension_loaded('mcrypt')) {
                return $crypt_text;
            }
            $td  = mcrypt_module_open($algorithm, '', $mode, '');
            $key = substr($key, 0, mcrypt_enc_get_key_size($td));
            if (is_null($iv)) {
                $iv  = substr(sha1($key), 0, mcrypt_enc_get_iv_size($td));
            }
            
            if (mcrypt_generic_init($td, $key, $iv) < 0) {
                return $crypt_text;
            }
            
            $plain_text = rtrim(mdecrypt_generic($td, base64_decode($crypt_text)), "\0");
            
            mcrypt_generic_deinit($td);
            mcrypt_module_close($td);
        } else {
            $method = strtoupper($algorithm.'-'.$mode);
            if (is_null($option)) {
                $option = OPENSSL_ZERO_PADDING;
            }
            if (is_null($iv)) {
                $ivLength = openssl_cipher_iv_length($method);
                $iv = substr(sha1($key), 0, $ivLength);
            }
            $plain_text = openssl_decrypt($crypt_text, $method, $key, $option, $iv);
        }
        
        return $plain_text === false ? $crypt_text : $plain_text;
    }
@nao-pon

This comment has been minimized.

Show comment
Hide comment
@nao-pon

nao-pon Mar 26, 2017

Member

と思ったのですが、$key, $algorithm, $mode, $option, $iv を指定するならこのメソッドの利用価値はほとんどないですね。いっそのこと des-ecb による (en|de)coder にしたほうがすっきりして良いのかも知れません。

    /**
     * @public
     * @brief [Static] To encrypt strings by "DES-ECB".
     * @param $plain_text string
     * @param $key        string
     * @return string - Encrypted string.
     */
    public static function encrypt($plain_text, $key = null)
    {
        if ($plain_text === '') {
            return $plain_text;
        }
        
        if (is_null($key) || ! is_string($key)) {
            if (! defined('XOOPS_SALT')) {
                return $plain_text;
            }
            $key = XOOPS_SALT;
        }
        $key = md5($key);

        if (! function_exists('openssl_encrypt')) {
            if (! extension_loaded('mcrypt')) {
                return $plain_text;
            }
            $td  = mcrypt_module_open('des', '', 'ecb', '');
            $key = substr($key, 0, mcrypt_enc_get_key_size($td));
            $iv  = substr(sha1($key), 0, mcrypt_enc_get_iv_size($td));
            
            if (mcrypt_generic_init($td, $key, $iv) < 0) {
                return $plain_text;
            }
            
            $crypt_text = base64_encode(mcrypt_generic($td, $plain_text));
            
            mcrypt_generic_deinit($td);
            mcrypt_module_close($td);
        } else {
            $crypt_text = openssl_encrypt($plain_text, 'DES-ECB', $key);
        }
        
        return $crypt_text === false ? $plain_text : $crypt_text;
    }
    
    /**
     * @public
     * @brief [Static] To decrypt strings by "DES-ECB".
     * @param $crypt_text string
     * @param $key        string
     * @return string - Decrypted string.
     */
    public static function decrypt($crypt_text, $key = null)
    {
        if ($crypt_text === '') {
            return $crypt_text;
        }
        
        if (is_null($key) || ! is_string($key)) {
            if (! defined('XOOPS_SALT')) {
                return $crypt_text;
            }
            $key = XOOPS_SALT;
        }
        $key = md5($key);

        if (! function_exists('openssl_decrypt')) {
            if (! extension_loaded('mcrypt')) {
                return $crypt_text;
            }
            $td  = mcrypt_module_open('des', '', 'ecb', '');
            $key = substr($key, 0, mcrypt_enc_get_key_size($td));
            $iv  = substr(sha1($key), 0, mcrypt_enc_get_iv_size($td));
            
            if (mcrypt_generic_init($td, $key, $iv) < 0) {
                return $crypt_text;
            }
            
            $plain_text = rtrim(mdecrypt_generic($td, base64_decode($crypt_text)), "\0");
            
            mcrypt_generic_deinit($td);
            mcrypt_module_close($td);
        } else {
            $plain_text = openssl_decrypt($crypt_text, 'DES-ECB', $key, OPENSSL_ZERO_PADDING);
        }
        
        return $plain_text === false ? $crypt_text : $plain_text;
    }
Member

nao-pon commented Mar 26, 2017

と思ったのですが、$key, $algorithm, $mode, $option, $iv を指定するならこのメソッドの利用価値はほとんどないですね。いっそのこと des-ecb による (en|de)coder にしたほうがすっきりして良いのかも知れません。

    /**
     * @public
     * @brief [Static] To encrypt strings by "DES-ECB".
     * @param $plain_text string
     * @param $key        string
     * @return string - Encrypted string.
     */
    public static function encrypt($plain_text, $key = null)
    {
        if ($plain_text === '') {
            return $plain_text;
        }
        
        if (is_null($key) || ! is_string($key)) {
            if (! defined('XOOPS_SALT')) {
                return $plain_text;
            }
            $key = XOOPS_SALT;
        }
        $key = md5($key);

        if (! function_exists('openssl_encrypt')) {
            if (! extension_loaded('mcrypt')) {
                return $plain_text;
            }
            $td  = mcrypt_module_open('des', '', 'ecb', '');
            $key = substr($key, 0, mcrypt_enc_get_key_size($td));
            $iv  = substr(sha1($key), 0, mcrypt_enc_get_iv_size($td));
            
            if (mcrypt_generic_init($td, $key, $iv) < 0) {
                return $plain_text;
            }
            
            $crypt_text = base64_encode(mcrypt_generic($td, $plain_text));
            
            mcrypt_generic_deinit($td);
            mcrypt_module_close($td);
        } else {
            $crypt_text = openssl_encrypt($plain_text, 'DES-ECB', $key);
        }
        
        return $crypt_text === false ? $plain_text : $crypt_text;
    }
    
    /**
     * @public
     * @brief [Static] To decrypt strings by "DES-ECB".
     * @param $crypt_text string
     * @param $key        string
     * @return string - Decrypted string.
     */
    public static function decrypt($crypt_text, $key = null)
    {
        if ($crypt_text === '') {
            return $crypt_text;
        }
        
        if (is_null($key) || ! is_string($key)) {
            if (! defined('XOOPS_SALT')) {
                return $crypt_text;
            }
            $key = XOOPS_SALT;
        }
        $key = md5($key);

        if (! function_exists('openssl_decrypt')) {
            if (! extension_loaded('mcrypt')) {
                return $crypt_text;
            }
            $td  = mcrypt_module_open('des', '', 'ecb', '');
            $key = substr($key, 0, mcrypt_enc_get_key_size($td));
            $iv  = substr(sha1($key), 0, mcrypt_enc_get_iv_size($td));
            
            if (mcrypt_generic_init($td, $key, $iv) < 0) {
                return $crypt_text;
            }
            
            $plain_text = rtrim(mdecrypt_generic($td, base64_decode($crypt_text)), "\0");
            
            mcrypt_generic_deinit($td);
            mcrypt_module_close($td);
        } else {
            $plain_text = openssl_decrypt($crypt_text, 'DES-ECB', $key, OPENSSL_ZERO_PADDING);
        }
        
        return $plain_text === false ? $crypt_text : $plain_text;
    }
@orrisroot

This comment has been minimized.

Show comment
Hide comment
@orrisroot

orrisroot Mar 26, 2017

Contributor

ならば、DES-ECB の鍵長さは 8 ですので $key は 8 文字で切る処理と、decrypt() の方は、PCKS7 を削ってさらに \0 を削るコードを入れれば openssl_encrypt() で暗号化した場合の結果も復号できるようにかるかと思います。

  $crypt_text = openssl_encrypt($plain_text, 'DES-ECB', substr($key, 0, 8));
  $plain_text = openssl_decrypt($crypt_text, 'DES-ECB', substr($key, 0, 8), OPENSSL_ZERO_PADDING);
  $plain_text = rtrim(substr($plain_text, 0, strlen($plain_text) - ord(substr($plain_text, -1))), "\0");
Contributor

orrisroot commented Mar 26, 2017

ならば、DES-ECB の鍵長さは 8 ですので $key は 8 文字で切る処理と、decrypt() の方は、PCKS7 を削ってさらに \0 を削るコードを入れれば openssl_encrypt() で暗号化した場合の結果も復号できるようにかるかと思います。

  $crypt_text = openssl_encrypt($plain_text, 'DES-ECB', substr($key, 0, 8));
  $plain_text = openssl_decrypt($crypt_text, 'DES-ECB', substr($key, 0, 8), OPENSSL_ZERO_PADDING);
  $plain_text = rtrim(substr($plain_text, 0, strlen($plain_text) - ord(substr($plain_text, -1))), "\0");
@nao-pon

This comment has been minimized.

Show comment
Hide comment
@nao-pon

nao-pon Mar 26, 2017

Member

なるほど!私が素人なので専門家の方に助言いただけると助かります!
こんな感じでしょうか。

    /**
     * @public
     * @brief [Static] To encrypt strings by "DES-ECB".
     * @param $plain_text string
     * @param $key        string
     * @return string - Encrypted string.
     */
    public static function encrypt($plain_text, $key = null)
    {
        if ($plain_text === '') {
            return $plain_text;
        }
        
        if (is_null($key) || ! is_string($key)) {
            if (! defined('XOOPS_SALT')) {
                return $plain_text;
            }
            $key = XOOPS_SALT;
        }
        $key = substr(md5($key), 0, 8);

        if (! function_exists('openssl_encrypt')) {
            if (! extension_loaded('mcrypt')) {
                return $plain_text;
            }
            $td  = mcrypt_module_open('des', '', 'ecb', '');
            
            if (mcrypt_generic_init($td, $key, 'iv_dummy') < 0) {
                return $plain_text;
            }
            
            $crypt_text = base64_encode(mcrypt_generic($td, $plain_text));
            
            mcrypt_generic_deinit($td);
            mcrypt_module_close($td);
        } else {
            $crypt_text = openssl_encrypt($plain_text, 'DES-ECB', $key);
        }
        
        return $crypt_text === false ? $plain_text : $crypt_text;
    }
    
    /**
     * @public
     * @brief [Static] To decrypt strings by "DES-ECB".
     * @param $crypt_text string
     * @param $key        string
     * @return string - Decrypted string.
     */
    public static function decrypt($crypt_text, $key = null)
    {
        if ($crypt_text === '') {
            return $crypt_text;
        }
        
        if (is_null($key) || ! is_string($key)) {
            if (! defined('XOOPS_SALT')) {
                return $crypt_text;
            }
            $key = XOOPS_SALT;
        }
        $key = substr(md5($key), 0, 8);

        if (! function_exists('openssl_decrypt')) {
            if (! extension_loaded('mcrypt')) {
                return $crypt_text;
            }
            $td  = mcrypt_module_open('des', '', 'ecb', '');
            
            if (mcrypt_generic_init($td, $key, 'iv_dummy') < 0) {
                return $crypt_text;
            }
            
            $plain_text = rtrim(mdecrypt_generic($td, base64_decode($crypt_text)), "\0");
            
            mcrypt_generic_deinit($td);
            mcrypt_module_close($td);
        } else {
            $plain_text = openssl_decrypt($crypt_text, 'DES-ECB', $key, OPENSSL_ZERO_PADDING);
            $plain_text = rtrim(substr($plain_text, 0, strlen($plain_text) - ord(substr($plain_text, -1))), "\0");
        }
        
        return $plain_text === false ? $crypt_text : $plain_text;
    }
Member

nao-pon commented Mar 26, 2017

なるほど!私が素人なので専門家の方に助言いただけると助かります!
こんな感じでしょうか。

    /**
     * @public
     * @brief [Static] To encrypt strings by "DES-ECB".
     * @param $plain_text string
     * @param $key        string
     * @return string - Encrypted string.
     */
    public static function encrypt($plain_text, $key = null)
    {
        if ($plain_text === '') {
            return $plain_text;
        }
        
        if (is_null($key) || ! is_string($key)) {
            if (! defined('XOOPS_SALT')) {
                return $plain_text;
            }
            $key = XOOPS_SALT;
        }
        $key = substr(md5($key), 0, 8);

        if (! function_exists('openssl_encrypt')) {
            if (! extension_loaded('mcrypt')) {
                return $plain_text;
            }
            $td  = mcrypt_module_open('des', '', 'ecb', '');
            
            if (mcrypt_generic_init($td, $key, 'iv_dummy') < 0) {
                return $plain_text;
            }
            
            $crypt_text = base64_encode(mcrypt_generic($td, $plain_text));
            
            mcrypt_generic_deinit($td);
            mcrypt_module_close($td);
        } else {
            $crypt_text = openssl_encrypt($plain_text, 'DES-ECB', $key);
        }
        
        return $crypt_text === false ? $plain_text : $crypt_text;
    }
    
    /**
     * @public
     * @brief [Static] To decrypt strings by "DES-ECB".
     * @param $crypt_text string
     * @param $key        string
     * @return string - Decrypted string.
     */
    public static function decrypt($crypt_text, $key = null)
    {
        if ($crypt_text === '') {
            return $crypt_text;
        }
        
        if (is_null($key) || ! is_string($key)) {
            if (! defined('XOOPS_SALT')) {
                return $crypt_text;
            }
            $key = XOOPS_SALT;
        }
        $key = substr(md5($key), 0, 8);

        if (! function_exists('openssl_decrypt')) {
            if (! extension_loaded('mcrypt')) {
                return $crypt_text;
            }
            $td  = mcrypt_module_open('des', '', 'ecb', '');
            
            if (mcrypt_generic_init($td, $key, 'iv_dummy') < 0) {
                return $crypt_text;
            }
            
            $plain_text = rtrim(mdecrypt_generic($td, base64_decode($crypt_text)), "\0");
            
            mcrypt_generic_deinit($td);
            mcrypt_module_close($td);
        } else {
            $plain_text = openssl_decrypt($crypt_text, 'DES-ECB', $key, OPENSSL_ZERO_PADDING);
            $plain_text = rtrim(substr($plain_text, 0, strlen($plain_text) - ord(substr($plain_text, -1))), "\0");
        }
        
        return $plain_text === false ? $crypt_text : $plain_text;
    }
@orrisroot

This comment has been minimized.

Show comment
Hide comment
@orrisroot

orrisroot Mar 26, 2017

Contributor

ありがとうございます!
よさそうです。:-)

Contributor

orrisroot commented Mar 26, 2017

ありがとうございます!
よさそうです。:-)

@orrisroot

This comment has been minimized.

Show comment
Hide comment
@orrisroot

orrisroot Mar 26, 2017

Contributor

あ、すみません!逆に openssl で encrypt して mcrypt で decrypt する場合の互換性も追加してはどうでしょうか。同様に PKCS7 パディング除去と \0 除去のコードで両対応できそうです。

 $plain_text = mdecrypt_generic($td, base64_decode($crypt_text));
 $plain_text = rtrim(substr($plain_text, 0, strlen($plain_text) - ord(substr($plain_text, -1))), "\0");
Contributor

orrisroot commented Mar 26, 2017

あ、すみません!逆に openssl で encrypt して mcrypt で decrypt する場合の互換性も追加してはどうでしょうか。同様に PKCS7 パディング除去と \0 除去のコードで両対応できそうです。

 $plain_text = mdecrypt_generic($td, base64_decode($crypt_text));
 $plain_text = rtrim(substr($plain_text, 0, strlen($plain_text) - ord(substr($plain_text, -1))), "\0");
@orrisroot

This comment has been minimized.

Show comment
Hide comment
@orrisroot

orrisroot Mar 26, 2017

Contributor

検証用のコードをはっておきます。
https://gist.github.com/orrisroot/c446b5f5455a9134be97d8309d507297

Contributor

orrisroot commented Mar 26, 2017

検証用のコードをはっておきます。
https://gist.github.com/orrisroot/c446b5f5455a9134be97d8309d507297

@nao-pon

This comment has been minimized.

Show comment
Hide comment
@nao-pon

nao-pon Mar 26, 2017

Member

いいですね!これでコミットします。

Member

nao-pon commented Mar 26, 2017

いいですね!これでコミットします。

@orrisroot

This comment has been minimized.

Show comment
Hide comment
@orrisroot

orrisroot Mar 26, 2017

Contributor

あ、もう一つ問題がありました。
OPENSSL_ZERO_PADDING は PHP 5.3 では使えないかも。
RHEL/CentOS 6 のデフォルトの php-5.3.3-48.el6_8.x86_64 では復号に失敗します。

PHP Notice:  Use of undefined constant OPENSSL_ZERO_PADDING - assumed 'OPENSSL_ZERO_PADDING' in openssl_mcrypt_compatibility_check.php on line 34

mcrypt よりも openssl が優先なので、mcrypt 入っていてもいなくても動作しなくなってます。
この際 PHP 5.4 以降を必須ということでいいでしょうか。

Contributor

orrisroot commented Mar 26, 2017

あ、もう一つ問題がありました。
OPENSSL_ZERO_PADDING は PHP 5.3 では使えないかも。
RHEL/CentOS 6 のデフォルトの php-5.3.3-48.el6_8.x86_64 では復号に失敗します。

PHP Notice:  Use of undefined constant OPENSSL_ZERO_PADDING - assumed 'OPENSSL_ZERO_PADDING' in openssl_mcrypt_compatibility_check.php on line 34

mcrypt よりも openssl が優先なので、mcrypt 入っていてもいなくても動作しなくなってます。
この際 PHP 5.4 以降を必須ということでいいでしょうか。

[XCube_Utils] Fix encrypt/decrypt to DES-ECB
Also compatibility problem fixed.
@nao-pon

This comment has been minimized.

Show comment
Hide comment
@nao-pon

nao-pon Mar 26, 2017

Member

なんと!PHP 5.4 からパラメータが変わっているんですね。PHP 5.4 未満では openssl を使わないようにします。

Member

nao-pon commented Mar 26, 2017

なんと!PHP 5.4 からパラメータが変わっているんですね。PHP 5.4 未満では openssl を使わないようにします。

[XCube_Utils] cotrrection for PHP<5.4 of `decrypt()`
PHP < 5.4.0 can not use `OPENSSL_ZERO_PADDING`

@nao-pon nao-pon merged commit 9eb600e into xoopscube:master Mar 26, 2017

@orrisroot

This comment has been minimized.

Show comment
Hide comment
@orrisroot

orrisroot Mar 27, 2017

Contributor

すみません!もう一つ問題が発覚しました…
mcrypt 側の encrypt 時のゼロパディング付与処理についてですが、ブロックサイズ8でちょうど割り切れる入力データ長の場合、パディング文字が付かないことがわかりました。その為、今のコードですと $plain_text の長さが 8 で割り切れる場合、mcrypt で暗号化された文字の復号に失敗します。

ということで、以下の様に今のパディング除去のコードをもう少し真面目に書く必要がありそうです。

// remove \0, PKCS#7 padding
$plain_text = rtrim(substr($plain_text, 0, strlen($plain_text) - ord(substr($plain_text, -1))), "\0");

// remove \0 padding for mcrypt encrypted text
$plain_text = rtrim($plain_text, "\0");
// remove pkcs#7 padding for openssl encrypted text if padding string found
$pad_ch = substr($plain_text, -1);
$pad_len = ord($pad_ch);
if (substr_compare($plain_text, str_repeat($pad_ch, $pad_len), -$pad_len) == 0) {
    $plain_text = substr($plain_text, 0, strlen($plain_text) - $pad_len);
}
Contributor

orrisroot commented Mar 27, 2017

すみません!もう一つ問題が発覚しました…
mcrypt 側の encrypt 時のゼロパディング付与処理についてですが、ブロックサイズ8でちょうど割り切れる入力データ長の場合、パディング文字が付かないことがわかりました。その為、今のコードですと $plain_text の長さが 8 で割り切れる場合、mcrypt で暗号化された文字の復号に失敗します。

ということで、以下の様に今のパディング除去のコードをもう少し真面目に書く必要がありそうです。

// remove \0, PKCS#7 padding
$plain_text = rtrim(substr($plain_text, 0, strlen($plain_text) - ord(substr($plain_text, -1))), "\0");

// remove \0 padding for mcrypt encrypted text
$plain_text = rtrim($plain_text, "\0");
// remove pkcs#7 padding for openssl encrypted text if padding string found
$pad_ch = substr($plain_text, -1);
$pad_len = ord($pad_ch);
if (substr_compare($plain_text, str_repeat($pad_ch, $pad_len), -$pad_len) == 0) {
    $plain_text = substr($plain_text, 0, strlen($plain_text) - $pad_len);
}

nao-pon added a commit that referenced this pull request Mar 28, 2017

@nao-pon

This comment has been minimized.

Show comment
Hide comment
@nao-pon

nao-pon Mar 28, 2017

Member

@orrisroot ありがとうございます!確かに8バイトで割り切れるデータでは問題がありますね。
ということで早速修正して、XOOPS X CorePack 20170328 としました。

心強く、助かります。 👍

Member

nao-pon commented Mar 28, 2017

@orrisroot ありがとうございます!確かに8バイトで割り切れるデータでは問題がありますね。
ということで早速修正して、XOOPS X CorePack 20170328 としました。

心強く、助かります。 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment