php-gpg bugs

[jasonhinkle]

I received a tweet saying that there are a lot of bugs in the library, see https://twitter.com/voodooKobra/status/676121698022899713?cn=cmVwbHk%3D

Obviously the tweet alone isn't really helpful for the project unless Scott is interested in perhaps giving us some details about the bugs. I'm putting this task here so that he can reply easily if he chooses to participate. Being a security specialist, of course that would be wonderful for a community project like this.

[DanielRuf]

random_int() is now used instead of the insecure rand(). I have used the polyfill by paragonie as this provides the best security and is not compatible with older PHP versions (which are officially unsupported).

Alternatively other developers may use the random string generation from phpseclib https://github.com/phpseclib/phpseclib/blob/29b1bb7a58e171cdfa599579d02b6314855b4915/phpseclib/Crypt/Random.php#L71 or another PRNG if they really want to support old and insecure PHP versions.

About the block encryption using ECB, I know this.

Currently I use php-gpg to encrypt malware samples from my honeypots so I can send them via email to me without triggering any antivirus scanner (just for research).

About the "we wrote our own primitives": Scott, this library was created by another developer (not Jason or me) who converted C code of gpg to PHP. You are very welcome to suggest and make improvements.

I do not know where and what to change to move from ECB to CBC, any help is very welcome.

[Maikuolan]

If we wanted to ensure backwards compatibility with older PHP versions, we could use PHP's in-built version_compare(); function to determine which PHP version a user is using, and based on that determination, choose to use either the more secure PHP7 random_int() function, or the older, less secure rand() function.

Of course, if we did this, some sort of warning should probably be included in documentation somewhere, warning users of the security risks associated with older versions of PHP and encouraging them to upgrade to PHP7.

Examples:

From: $t = random_int(0, 0xff);

to: $t = (version_compare(PHP_VERSION,'7.0.0')>=0)?random_int(0,0xff):rand(0,0xff);

and from: return random_int(0, 0xff);

to: return (version_compare(PHP_VERSION,'7.0.0')>=0)?random_int(0,0xff):round(rand(0,0xff));

Though in any case, PHP7 is definitely preferable to use, due to significant security improvements, and whether actually having this compatible with older versions (due to them relying on the less secure function) is a good or bad idea, I don't know. Putting this here anyhow, though, in case it's useful.

[DanielRuf]

rand() should be completely avoided as it is predictable ;-)

There are openssl, mcrypt and others. The PHP 5 polyfill should at least solve this for now.

[Maikuolan]

Cool. :+1:

[DanielRuf]

Some information about rand and seeding rand: http://programmers.stackexchange.com/questions/76229/predicting-the-output-of-phps-rand

[jasonhinkle]

edit: disregard my comment - looks like the polyfill for random_int is more robust than my suggestion

-- original comment below --

Rather than check PHP version, we could use function_exists() to go down in order from the most preferred to the least preferred?

From what I understand, the order of preference might be:

• random_int
• openssl_random_pseudo_bytes
• random_bytes
• mt_rand (fallback purposes only)
• rand (fallback purposes only)

[paragonie-scott]

Porting from PHP to C is dangerous. Google: mbstring.func_overload.

I'll explain more when I have time to open tickets for specific examples and make specific recommendations, but I'm on a business trip at the moment and it's wiped out my energy reserves.

[raed667]

Maybe a good idea is to focus on cleaning the code, nuke the windows line feed, remove unused variables, check for PEAR standards then get a fixed version that can be audited by a 3rd party.

I can help with this next week if you want to.

[DanielRuf]

This would be very helpful.

But better keep the Windows line feed there (as I use and test the code on a Windows machine and it works here without any issues). Or does this change nothiing when I paste the result into Notepad++ and GPA?

[raed667]

It should work fine, but it would be cleaner I think. Just to prepare things for a possible audit

[Maikuolan]

The standard Notepad program included with Windows requires \r\n linebreaks, but Notepad++ can handle most types of linebreak correctly (so, \n, \r, \n\r, \r\n, etc, should be fine), so, it shouldn't be a problem if we stick to using Notepad++ (but, files mightn't appear to have any linebreaks whatsoever if they're only using \n linebreaks and they're opened using the standard Notepad program). I haven't yet tested GPA with different types of linebreaks.

PHP itself, doesn't really care what type of linebreaks we use, whether or not we use any linebreaks at all, etc, and shouldn't be affected by which type of linebreaks we use, but, for what it's worth, and if we're looking for additional reasons to back using one or the other, using \n instead of \r\n would save one extra byte per each line of code (a small saving, perhaps a negligible difference, but it's something, I suppose; doing a quick line count of the current codebase for the master branch shows this saving to be ~1.43KB). Could take it or leave it. :P

I think cleaning up code could be a good idea, too; If for no other reason, then, at least, to potentially attract other (potentially picky) developers to the project (although, I think, it already looks pretty good, there's always room for improvement).

One such example could be changing:

 require_once("GPG/Expanded_Key.php");
 require_once("GPG/Public_Key.php");
 require_once("GPG/AES.php");
 require_once("GPG/globals.php");

To:

 require_once 'GPG/Expanded_Key.php';
 require_once 'GPG/Public_Key.php';
 require_once 'GPG/AES.php';
 require_once 'GPG/globals.php';

(Due to require_once being a statement, and not a function).

The other could be changing the brackets for any declarations that don't require parsing special characters or variables (\n \r \xYY \uXX \XX \t $foo, etc) from " to ' (but, of course, leaving those that do require parsing special characters or variables as they are); This should (based on what I've read and based on the benchmarks I've seen for some other unrelated PHP projects), in theory, be faster (for those declarations, because they don't then require the extra processing for parsing these), but, php-gpg already seems fast and doesn't seem to have any problems with processing time that I'm aware of (plus, I've heard some debate as to whether or not it's true that this would increase the processing speed), so, this could be considered low priority.

[DanielRuf]

One such example could be changing:

require_once("GPG/Expanded_Key.php"); require_once("GPG/Public_Key.php"); require_once("GPG/AES.php"); require_once("GPG/globals.php"); To:

require_once 'GPG/Expanded_Key.php'; require_once 'GPG/Public_Key.php'; require_once 'GPG/AES.php'; require_once 'GPG/globals.php'; (Due to require_once being a statement, and not a function).

@Maikuolan Thanks for the heads up, please check if I forgot to change it in some files.

[DanielRuf]

@raedslab @maikuolan please test if the current master branch works for you so I can prepare a new release (1.6.1).

[Maikuolan]

This seems a little unusual.

*******\bin\php\php7.0.0>php.exe *******\php-gpg\test1.php
PHP Fatal error:  Uncaught Error: Call to undefined function mb_strlen() in *******\php-gpg\master\libs\GPG.php:147
Stack trace:
#0 *******\php-gpg\master\libs\GPG.php(154): GPG->gpg_literal('Hello World. Th...')
#1 *******\php-gpg\master\libs\GPG.php(177): GPG->gpg_data('vo\r\\\xAE\xBEBYo\xF5|>1\x1E\xF4...', 'Hello World. Th...')
#2 *******\php-gpg\test1.php(42): GPG->encrypt(Object(GPG_Public_Key), 'Hello World. Th...')
#3 {main}
  thrown in *******\php-gpg\master\libs\GPG.php on line 147

*******\bin\php\php7.0.0>

I haven't had problems with multibyte functions before, so, this is new to me, but, this is a fresh install of PHP7, verbatim, without any modifications from the Windows binaries supplied by the PHP website. Do we need to do custom compiles for multibyte function, set some sort of configuration variable or such to get these functions working?

[DanielRuf]

You are right, mb_strlen is not installed by default.

http://stackoverflow.com/questions/6419102/fatal-error-call-to-undefined-function-mb-strlen

I will look if there are any polyfills if it is not installed (if this makes sense).

The Bitnami WAMP Stack 7.0.0 setup has it installed as it seems, I did not get the fatal error here. It may have mans extensions preinstalled for better support for webdevelopers which test their apps locally.

Found a polyfill in the Symfony framework:

https://github.com/symfony/polyfill/blob/master/src/Mbstring/Mbstring.php

https://github.com/symfony/polyfill/blob/master/src/Mbstring/bootstrap.php

Should we add this or mention that mb_* functions are needed (and others too)?

[Maikuolan]

Nice finds! :-)

Definitely, these would be good to add (assuming that they work correctly and can be implemented in the way that we need; I haven't tested them yet, but I can do that later today, after I get home from work).

They seem to be covered by the MIT License, so, legally, there shouldn't be any problems for us to add these.

I think this is a better way to go, rather than simply telling users that we need mb* functions enabled, because some users won't want to go to the trouble of correctly setting up native mb* functions (or, if they're working from a shared host, it's possible they mightn't be able to do so anyhow), and by including a polyfill for this functionality, we widen the potential reach for php-gpg.

[DanielRuf]

@Maikuolan please test the following code, you can add it to the globals.php file

if (!function_exists('mb_strlen')) {
    function mb_strlen($s, $encoding = null){
        $encoding = getEncoding($encoding);
        return iconv_strlen($s, $encoding);
    }
    function getEncoding($encoding){
        if (null === $encoding) {
            return 'UTF-8';
        }
        $encoding = strtoupper($encoding);
        if ('8BIT' === $encoding || 'BINARY' === $encoding) {
            return 'CP850';
        }
        if ('UTF8' === $encoding) {
            return 'UTF-8';
        }
        return $encoding;
    }
}

By default we use UTF8 in the code, what comes after `$encoding = strtoupper($encoding);´ may be redundant or unused but we may keep the code there if there are any problems with special encodings on some servers, so we have a full polyfill.

[Maikuolan]

New issue this time:

PHP Notice:  iconv_strlen(): Detected an illegal character in input string in *******\php-gpg\master\libs\GPG\globals.php on line 22
PHP Notice:  iconv_strlen(): Detected an illegal character in input string in *******\php-gpg\master\libs\GPG\globals.php on line 22

Original input entered:

Hello World. This is a test.

[DanielRuf]

Can not reproduce, works here for me. Even if I use

error_reporting(E_ALL);
ini_set('display_errors', 1);

I get int(28) for var_dump(mb_strlen_2("Hello World. This is a test."));

What is your encoding? The code should use UTF8.

[Maikuolan]

This is very strange. :-/

I'm also using UTF8.

I get int(28) for var_dump(mb_strlen("Hello World. This is a test."));

Same for me. When I use -that exact code-, I get the same results (int(28)).

But.. When I run -this- code (as per based on the example testfiles):

$plain_text_string='Hello World. This is a test.';

// using the key, encrypt your plain text using the public key
$encrypted = $gpg->encrypt($pub_key,$plain_text_string);

I get the illegal characters detected notice.

Reading over this, I think I'll experiment with some different encoding types, to see if it produces any different results.

I'll reply here again in a few minutes.

[Maikuolan]

Huh.. Okay, still confused.

Thinking maybe it was an issue with the way that $encoding was being defined, I thought I'd try changing it, to see what results could be produced.

Changing: function mb_strlen($s, $encoding = null){

To: function mb_strlen($s, $encoding = 'UTF-8'){

Didn't do anything; Still the same notice.

But, changing it to: function mb_strlen($s, $encoding = 'ISO-8859-1'){

..Seemed to work. No more notice, and suddenly, it's able to encrypt correctly; I tried decrypting it again using Kleopatra, and it decrypted successfully, matching the original string. Interestingly, using this, even when I tried encrypting non-Latin content (firstly, 47KB worth of Lorem Ipsum in Arabic, and secondly, 94KB worth of Lorem Ipsum in Chinese), it still worked, and decrypted successfully using Kleopatra, matching the original strings (which, it probably shouldn't have, because of it being non-Latin content using the Latin-1 codepage). I also tried encrypting a 6KB PE file, and this was successful; After decrypting it, I attempted to execute it, and there were no problems (so, it seems, binary files were okay with this, too).

The same results, entirely, when using the CP850 (DOS) codepage (everything worked correctly, even the non-Latin stuff).

However, one caveat to this.. When I tried encrypting a 484KB file containing pure Lorem Ipsum, the RAM usage for my PHP installation started spiraling upwards rapidly and the instance never terminated (no error, no notice, no failure, no success, no encryption).

To test if this was a freak occurrence or something that seemed to occur for any files/content above a particular filesize (as, at least, on the surface, it immediately looked like), I attempted to encrypt a 2,349KB MP3 file; Same results (RAM usage spiraling upwards and no script termination).

Not really sure what to make of all this.

[Maikuolan]

Do we know, with PHP, whether code pages are handled internally, by PHP, or handled by the system where it's installed?

If we're getting different results for these tests, and code pages are involved, I'm wondering if it might be a problem with the code pages on my machine.

[DanielRuf]

The memory issue may be a problem with the encryption which may not be the best code. We have to test this in the future and improve the code.

Can you make a var_dump() in globals.php on line 22 to see how the input looks like?

Did you try using double quotes instead of single quotes?

[DanielRuf]

Very good question, unfortunately I don't know.

Best solution would be to advise people installing / enabling the multibyte extension.

But if we can debug this and find the problem, we can solve it.

I tried ti now with the library and you are right, there is a problem in the code. I will take a look at it.

string(15) "TESTMESSAGETEXT" string(15) "TESTMESSAGETEXT" string(71) "��#� >,MQ�!T\pZpZ��tfileTESTMESSAGETEXT�k���Biy�{x�'P��-�" 
Notice: iconv_strlen(): Detected an illegal character in input string in C:\xampp\htdocs\pgp\libs\GPG\globals.php on line 25
string(71) "��#� >,MQ�!T\pZpZ��tfileTESTMESSAGETEXT�k���Biy�{x�'P��-�" 
Notice: iconv_strlen(): Detected an illegal character in input string in C:\xampp\htdocs\pgp\libs\GPG\globals.php on line 25

[DanielRuf]

Found the problem, we have to use mb_strlen(utf8_encode()) (for UTF-8 at least). So we have to check the encoding and convert the string to it (encode it) before calling iconv_strlen.

I am not sure if we should use the code at all or just force people to enable the mbstring extensions (should be enabled and installed on most or all hosting plans).

The following code fixes the issue at least, but I had to change one line to use utf8_encode so the bytes are converted to the original unicode characters.

 if (!function_exists('mb_strlen')) {
    function mb_check_encoding($var = null, $encoding = null)
    {
        if (null === $encoding) {
            if (null === $var) {
                return false;
            }
            $encoding = 'UTF-8';
        }
        return mb_detect_encoding($var, array($encoding)) || false !== @iconv($encoding, $encoding, $var);
    }
    function mb_convert_encoding($s, $toEncoding, $fromEncoding = null)
    {
        if (is_array($fromEncoding) || false !== strpos($fromEncoding, ',')) {
            $fromEncoding = mb_detect_encoding($s, $fromEncoding);
        } else {
            $fromEncoding = getEncoding($fromEncoding);
        }
        $toEncoding = getEncoding($toEncoding);
        if ('BASE64' === $fromEncoding) {
            $s = base64_decode($s);
            $fromEncoding = $toEncoding;
        }
        if ('BASE64' === $toEncoding) {
            return base64_encode($s);
        }
        if ('HTML-ENTITIES' === $toEncoding || 'HTML' === $toEncoding) {
            if ('HTML-ENTITIES' === $fromEncoding || 'HTML' === $fromEncoding) {
                $fromEncoding = 'Windows-1252';
            }
            if ('UTF-8' !== $fromEncoding) {
                $s = iconv($fromEncoding, 'UTF-8', $s);
            }
            return preg_replace_callback('/[\x80-\xFF]+/', array(__CLASS__, 'html_encoding_callback'), $s);
        }
        if ('HTML-ENTITIES' === $fromEncoding) {
            $s = html_entity_decode($s, ENT_COMPAT, 'UTF-8');
            $fromEncoding = 'UTF-8';
        }
        return iconv($fromEncoding, $toEncoding, utf8_encode($s));
    }
    function mb_detect_encoding($str, $encodingList = null, $strict = false)
    {
        if (null === $encodingList) {
            $encodingList = array('ASCII', 'UTF-8');
        } else {
            if (!is_array($encodingList)) {
                $encodingList = array_map('trim', explode(',', $encodingList));
            }
            $encodingList = array_map('strtoupper', $encodingList);
        }
        foreach ($encodingList as $enc) {
            switch ($enc) {
                case 'ASCII':
                    if (!preg_match('/[\x80-\xFF]/', $str)) {
                        return $enc;
                    }
                    break;
                case 'UTF8':
                case 'UTF-8':
                    if (preg_match('//u', $str)) {
                        return 'UTF-8';
                    }
                    break;
                default:
                    if (0 === strncmp($enc, 'ISO-8859-', 9)) {
                        return $enc;
                    }
            }
        }
        return false;
    }
    function mb_strlen($s, $encoding = null){
        $encoding = getEncoding($encoding);
        $s=mb_convert_encoding($s,$encoding);
        return iconv_strlen($s, $encoding);
    }
    function getEncoding($encoding){
        if (null === $encoding) {
            return 'UTF-8';
        }
        $encoding = strtoupper($encoding);
        if ('8BIT' === $encoding || 'BINARY' === $encoding) {
            return 'CP850';
        }
        if ('UTF8' === $encoding) {
            return 'UTF-8';
        }
        return $encoding;
    }
}

[Maikuolan]

Ah, that makes sense. So, then, we just need to find a reliable way to detect the original encoding of input..? I'll try to look around for something, but, right now, I'm not sure how we could this reliably for all cases.

[DanielRuf]

I think the used encoding in php-gpg is 8bit / UTF-8 so maybe we can remove some parts of the polyfill. I am not really sure. We should test this and weigh if we should really use this polyfill (with less code) or mention that mbstring should be enabled / compiled.

I'm not sure how we could this reliably for all cases. Me too.

@raedslab, what do you think?

[DanielRuf]

@Maikuolan can you enable the display of all errors and check if there is a memory exhausted error message? The exact error message would be helpful to fix the memory issue.

[paragonie-scott]

Um, this isn't hard.

https://github.com/fabpot/symfony/blob/f88e600833b6822db5873e25deaefd14948e4878/src/Symfony/Component/Security/Core/Util/StringUtils.php#L78-L92

[DanielRuf]

@paragonie-scott thanks for the hint. So we can directly use this code from Anthony Ferrara (ircmaxell)?

@Maikuolan please test if it works for you (with special characters).

[paragonie-scott]

I don't know, ask him maybe? I'm pretty sure he releases most of his stuff under MIT license, and I'm also relatively sure he'll say "Massive :-1:" when he sees what you're doing here, for many of the same reasons I outlined on Twitter.

[Maikuolan]

@DanielRuf Yep; As of the latest commit (4c6ca02e0a6fc12a9e739092b7b3f3fb79d7c4f3), all my tests were working correctly (can encrypt, then decrypt, and decrypted data matches the original data from prior to being encrypted). :+1:

(I'm still currently using a verbatim copy of PHP7 for Windows).

Also, as of the latest commit, the memory issues are no longer persisting (seems to encrypt quickly, and without draining all the memory).

I ran a total of 6 encryption tests, detail as follows:

Test No.	Source of data fed to gpg->encrypt();	Source content	Data size (in bytes)
1	String ( $foo='bar' )	"Hello World. This is a test."	28
2	String ( $foo='bar' )	Multiple paragraphs of Lorem Ipsum (verbatim, ASCII/ANSI).	492,678
3	String ( $foo='bar' )	Multi paragraphs of Lorem Ipsum (Arabic variation, UTF-8 w/o BOM)	45,168
4	String ( $foo='bar' )	Multi paragraphs of Lorem Ipsum (Chinese variation, UTF-8 w/o BOM)	93,788
5	Binary file (retrieved with fopen(%%,'rb') and fread()	PE binary	5,632
6	Binary file (retrieved with fopen(%%,'rb') and fread()	MP3 audio file	2,405,007

---

@paragonie-scott

when he sees what you're doing here, for many of the same reasons I outlined on Twitter.

Could you please supply a link to what you've outlined (or alternatively, repeat it here)? I'm interested to read what you've outlined, but, the only link to twitter related to this project that I've been able to find is the one supplied by @jasonhinkle in the initial post of this issue, and I can't seem to find anything else related to this project on Twitter using the search functionality (I'm not suggesting it's not there, but, I haven't been able to find it).

[AshleyPinner]

Just three quick thoughts: 1) storing the sbox in numeric format is odd; most implementations and Sbox tables for AES are in hex. (Also make sure to format it to be 16 numbers per line, as then it's much easier to compare with a reference SBox). This is to make comparison to references easier to see, and helps with the "nothing up my sleeve".

You may use this conversion:

static $S = array (
'63', '7C', '77', '7B', 'F2', '6B', '6F', 'C5', '30', '01', '67', '2B', 'FE', 'D7', 'AB', '76',
'CA', '82', 'C9', '7D', 'FA', '59', '47', 'F0', 'AD', 'D4', 'A2', 'AF', '9C', 'A4', '72', 'C0',
'B7', 'FD', '93', '26', '36', '3F', 'F7', 'CC', '34', 'A5', 'E5', 'F1', '71', 'D8', '31', '15',
'04', 'C7', '23', 'C3', '18', '96', '05', '9A', '07', '12', '80', 'E2', 'EB', '27', 'B2', '75',
'09', '83', '2C', '1A', '1B', '6E', '5A', 'A0', '52', '3B', 'D6', 'B3', '29', 'E3', '2F', '84',
'53', 'D1', '00', 'ED', '20', 'FC', 'B1', '5B', '6A', 'CB', 'BE', '39', '4A', '4C', '58', 'CF',
'D0', 'EF', 'AA', 'FB', '43', '4D', '33', '85', '45', 'F9', '02', '7F', '50', '3C', '9F', 'A8',
'51', 'A3', '40', '8F', '92', '9D', '38', 'F5', 'BC', 'B6', 'DA', '21', '10', 'FF', 'F3', 'D2',
'CD', '0C', '13', 'EC', '5F', '97', '44', '17', 'C4', 'A7', '7E', '3D', '64', '5D', '19', '73',
'60', '81', '4F', 'DC', '22', '2A', '90', '88', '46', 'EE', 'B8', '14', 'DE', '5E', '0B', 'DB',
'E0', '32', '3A', '0A', '49', '06', '24', '5C', 'C2', 'D3', 'AC', '62', '91', '95', 'E4', '79',
'E7', 'C8', '37', '6D', '8D', 'D5', '4E', 'A9', '6C', '56', 'F4', 'EA', '65', '7A', 'AE', '08',
'BA', '78', '25', '2E', '1C', 'A6', 'B4', 'C6', 'E8', 'DD', '74', '1F', '4B', 'BD', '8B', '8A',
'70', '3E', 'B5', '66', '48', '03', 'F6', '0E', '61', '35', '57', 'B9', '86', 'C1', '1D', '9E',
'E1', 'F8', '98', '11', '69', 'D9', '8E', '94', '9B', '1E', '87', 'E9', 'CE', '55', '28', 'DF',
'8C', 'A1', '89', '0D', 'BF', 'E6', '42', '68', '41', '99', '2D', '0F', 'B0', '54', 'BB', '16'
);

Remembering of course to use hexdec where required.

2) Remove the ability to use keysizes less than 2048 (2,4,8,16,512,1024). Anything smaller is too weak for any modern usage, so it shouldn't be allowed to operate.

3) In general, mind, using your own primitives is usually a bad idea when most of what you want to do is most likely covered by the OpenSSL lib in PHP. Don't get me wrong, as a learning exercise this is quite nice, but a single mistake in this code might lead to undecryptable messages at best. I'm happy to point at what could be done with just the OpenSSL lib in PHP if you want to go down that route.

[DanielRuf]

@AshleyPinner 1: Thanks for the hint. I changed it in the last commits to use the hexadecimal format for the sbox.

2: Am I right that we have to check the keysize before we encrypt the plaintext data? Is the code mentioned here right and sufficient for a keysize check? https://github.com/jasonhinkle/php-gpg/issues/19

3: Any hints on how and what we have to change to get it working with OpenSSL (or other extensions and probably some polyfills) and much better code are very welcome. Which files would be useless if we would use extensions shipped with PHP? Just AES.php or more files (and functions)?

Thanks for taking a look at the code and suggesting improvements.

@Maikuolan Please test the latest commits. Do you think we should make a 1.6.1 release or 1.7.0 (much better CPRNG). I think 1.6.1 as this is a patch and not a new feature but provides much better security.

[Maikuolan]

I'll run the tests again against the latest commits when I get home later today (about 6 hours from now). I agree; I'm thinking 1.6.1 would be better (to better conform with Semver, due to this being a patch rather than a new feature).

[DanielRuf]

Ok, no hurry. Right, when the tests are ok, I will prepare a 1.6.1 release.

[Maikuolan]

I ran the tests again just now against the latest commits, and there weren't any apparent problems; Tests completed successfully. :+1:

[DanielRuf]

Great, thanks for doing the tests and verifying that everything still works as desired.

Will make the 1.6.1 release later.

[paragonie-scott]

Could you please supply a link to what you've outlined (or alternatively, repeat it here)?

I mentioned some of them in the linked Twitter thread:

• https://twitter.com/voodooKobra/status/676060524430409729
• https://twitter.com/voodooKobra/status/676060921127661568
• https://twitter.com/voodooKobra/status/676061088690106368 (That's assuming you upgrade from ECB to CBC.)

---

Any hints on how and what we have to change to get it working with OpenSSL (or other extensions and probably some polyfills) and much better code are very welcome.

Using OpenSSL is 1000x better than writing your own primitives in PHP. Since PHP ships with ext/openssl in all versions of PHP >= 5.3.0, you can assume that the target machine has it. And if not, that's their problem, not yours..

Even better, though, is phpseclib, which actually makes it possible to do the correct and secure thing with RSA in PHP.

Take a gander at EasyRSA. It's a wrapper I wrote around phpseclib to use secure defaults. Symmetric encryption is taken care of by defuse/php-encryption. A PHP developer that is completely crypto-illiterate has a higher chance of using EasyRSA correctly than phpseclib.

Also take a look at Crypt_GPG in PEAR.

When I first found this code, it was using substr() and strlen() which PHP's mbstring.func_overload can break, employed rand() instead of a CSPRNG, and rolled AES encryption (in ECB mode) but it was textbook AES which is vulnerable to cache-timing attacks instead of hardware AES.

Each one of these flaws could destroy the security of any messages sent with your library.

Porting C to PHP is very dangerous, and I still advise against it. Write a PHP extension and wrap it, if you must do anything at all. The only people who can't work under the constraints of "no binary dependencies" are people with shared hosting accounts, which you cannot secure anyway. Please don't give them a false sense of security.

[paragonie-scott]

Also worth reading:

• Cryptographic right answers (for RSA)
• Updated cryptographic right answers (against RSA)
• Cryptography coding rules (very important)
• Error-prone cryptographic designs (slides from a talk by Daniel Bernstein)
• How to safely implement cryptography in any application (spoiler: use libsodium)

[DanielRuf]

@paragonie-scott thanks for the information.

Using a wrapper for a library which uses openssl_encrypt makes no big sense to me. Also doesn't Crypt_GPG use gpg with system calls and require the gpg binary on the system path?

I already found the openssl_encrypt call for AES with the CBC mode. I am pesonally against addig more full blown libraries or wrappers + libraries to the project.

What I need is the right openssl_encrypt code and the line which we have to change to use AES in CBC mode.

rand() is not used anymore, so this should be already fixed.

@Maikuolan I tried some code to migrate the eixsting code to OpenSSL but does not work yet.

$cipher = openssl_encrypt($text, "aes-128-ecb", $key, OPENSSL_ZERO_PADDING);
var_dump(base64_decode($cipher));

return substr(base64_decode($cipher), 0, $len);

Some related code here in phpseclib: https://github.com/phpseclib/phpseclib/blob/e2b745dedbc370e27974be971db01aca5347ca27/phpseclib/Crypt/Base.php#L692

I know that we have to migrate to AES 128 CBC but we need at least some working code using openssl_encrypt to remove unused code and classes.

Maybe you have more luck or can ask someone for useful hints or the solution, maybe on StackOverflow. I'm out today for some hours now.

[Maikuolan]

Sorry for the delayed reply; Busier past few days than expected.

I'll reply here again more properly soon.

[oucil]

Not sure if this is the right place to note this but you're discussing the random_byte/random_int methods provided by @paragonie-scott above so feel free to move this or tell me where I should be reporting it.

This library you've chosen to require for PHP <7 generates warnings consistently for us...

is_readable(): open_basedir restriction in effect. File(/dev/urandom) is not within the allowed path(s): (blah/blah/blah:/usr/share/php/:/tmp/)

Not sure about how to deal with this. We'll be moving to PHP 7 soon enough, but for now we're running PHP 5.4.45. Is there a non-standard package / library (urandom?) that is required on the linux system in addition? I don't see anything in anyones docs about this?

Thanks in advance.

[paragonie-scott]

@oucil Does version 1.1.5 of random_compat generate these warnings? Also, can you not add /dev to your list of allowed basedirs?

[oucil]

@paragonie-scott I just installed via composer a couple days ago and it looks like the requirement in this packages composer file is 1.1.x so I'm assuming we're on 1.1.5. (is there somewhere in your package I can concretely verify that's what was installed?). As far as adding /dev to the allowed basedir's, that's my last resort right now, only because this is a SAAS system and I'll need to update our app server template, bake new AMI's, and restart all our APP servers, so just a bit of work. If that's the only way to make this work, so be it.

[paragonie-scott]

I'll open a ticket in random_compat. Please let's move the discussion there. :)