Skip to content
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

Wrong Blowfish Symmetric Key Encryption with PHP 5.2.9 (on 32-bit Linux OS) #1038

Closed
romucifu opened this issue Sep 22, 2016 · 17 comments
Closed
Labels

Comments

@romucifu
Copy link

romucifu commented Sep 22, 2016

I'm trying to encrypt a string using symmetric key encryption (blowfish) with PHP 5.2.9 and using phpseclib 1.0.3 (running in internal mode), the sample code I'm using it's pretty simple:

include('Crypt/Blowfish.php');

$cipher = new Crypt_Blowfish();
$data = 'abcdefghijk';

$cipher->setKey('abcdef0123456789abcdefghi9876543');

echo bin2hex($cipher->encrypt($data));

the generated output is (PHP 5.2.9):

0e1651fc54dd530757fc1711b696dac5

But I've tried the same code with other servers (PHP 7.07, PHP 5.3.3, PHP 5.0.4) and ALL of them generate this:

ad7145c675b1c914bbfe379dc7293bf3

I suppose that the PHP 5.2.9 output is wrong. What could be the cause of this? Any clues?

I also submitted this question to "stackoverflow.com".

NOTE: I've attached the "phpinfo" data of the 5.2.9 PHP server (It's a Linux machine, probably CentOS distro)
phpinfo_529.txt

@romucifu
Copy link
Author

Ok, looks like a problem with this instruction on line 439 of "Crypt/Blowfish.php":

$this->bctx['p'][] = $this->parray[$i] ^ $data;

Apparently this XOR bitwise operator (^) return different results depending on the values and the type of OS (32Bit or 64Bit).

For example, this code:

$a=2242054355;
$b=1701195825;
$result = $a ^ $b;
echo $result;

Returns -523945758 on 32-bit systems or returns 3771021538 on 64-bit systems. So looks like the problem is related to the type of OS and not the version of PHP (I'll edit the title later).

@terrafrost
Copy link
Member

PHP integers can be either 32-bit or 64-bit depending on the underlying system architecture. And since ints are signed you can get weird edge cases when the leading bit is a 1 (eg. 0x80000000).

Unfortunately, idk of a way to unit test this since I don't think you can control the memory address size with Travis CI but, none-the-less, this feedback helps - thanks!

@bantu
Copy link
Member

bantu commented Sep 22, 2016

I think that if $a and $b are both 32-bit integers, then $result will be the exact same on both architectures. It is the echo statement that interprets them differently. So an interesting thing to look at is where $this->bctx['p'] is used.

@terrafrost
Copy link
Member

terrafrost commented Sep 22, 2016

I think that if $a and $b are both 32-bit integers, then $result will be the exact same on both architectures.

Yah - it should be for xor.

In the second code sample romucifu posted 2242054355 is bigger than PHP_INT_MAX on 32-bit systems (2147483647). That means it's a float that's being cast to an int as -2052912941. I'm able to duplicate that.

But I'm not able to duplicate the issue with Blowfish. I'm using internal mode on a 32-bit machine with phpseclib 1.0.3:

<?php
include('Crypt/Blowfish.php');

echo PHP_INT_MAX . "\n";

$cipher = new Crypt_Blowfish();
$cipher->setPreferredEngine(CRYPT_ENGINE_INTERNAL);
$data = 'abcdefghijk';

$cipher->setKey('abcdef0123456789abcdefghi9876543');

echo bin2hex($cipher->encrypt($data));

I get 2147483647 for PHP_INT_MAX and ad7145c675b1c914bbfe379dc7293bf3 for the ciphertext.

On this particular machine I'm running PHP 5.6.11 on Windows 10. I have other PHP versions I can test it out on but I tested out #1034 (also submitted by romucifu) on multiple PHP versions and wasn't able to duplicate that. I'm not confident I'll be able to reproduce this issue but none-the-less I'll try as time permits.

One thing that might help... if I were just given SSH access to the computer demonstrating the problem. I could be emailed the auth details to terrafrost@php.net. Being able to actually reproduce the problem will let me step through the code. I could see where the data begins to diverge. I suppose I could attempt to do this by proxy (eg. I post code, the OP runs it and tells me the output, and we keep on doing that) but that can turn a multi-hour endeavour into a multi-week one that most people aren't going to have the patients for.

@bantu
Copy link
Member

bantu commented Sep 22, 2016

This, just like #1034, might be caused by bugs in PHP versions that have been outdated for years.

@terrafrost
Copy link
Member

terrafrost commented Sep 23, 2016

I just tried it on PHP 5.2.9 and I got the same thing. I got ad7145c675b1c914bbfe379dc7293bf3 instead of 0e1651fc54dd530757fc1711b696dac5. So yah I'm not able to reproduce this, even on the version of PHP you're running and the proceed further I think I'm just going to need shell access to run CLI scripts and modify them.

Even if you're running an outdated version of PHP, phpseclib should still support it imho. So long as phpseclib 1.0 claims to work on PHP 5.2 it should. And phpseclib 1.0 should continue to work on 5.2 because changing that would be a BC break

@romucifu
Copy link
Author

Unfortunately I can't give access to this host.

Terrafrost, you said that you tested it with PHP 5.2.9 but did you tested it with a 32-Bit Linux OS?

I've attached the "phpinfo" data of the host in the original post.

@romucifu romucifu changed the title Wrong Blowfish Symmetric Key Encryption with PHP 5.2.9 Wrong Blowfish Symmetric Key Encryption with PHP 5.2.9 (on 32-bit Linux OS) Sep 28, 2016
@terrafrost
Copy link
Member

Testing arbitrary versions of PHP is a little easier with Windows. You just go to http://museum.php.net/, download the binaries for the version you want, and bam, you're good to go.

Testing arbitrary versions of PHP on Linux isn't so easy. With Linux I normally use Ubuntu. The easiest way to install a non-specific version of PHP is to do sudo apt-get install php5 but that if you want to install a specific version then you pretty much have to compile it. And if you're going to do that you'll need to install the dependencies too. But for sufficiently old versions it may even take time and effort to track down the dependencies and even then you may need to find the right versions of the dependencies. Or else modify the dependencies to be BC with the version that PHP is expecting.

Like a few years ago I tried to compile PHP 5.2.17 and here's what I had to do:

sudo apt-get update
sudo apt-get install build-essential
sudo apt-get install libxml2-dev
sudo apt-get install libgmp3-dev
wget http://museum.php.net/php5/php-5.2.17.tar.gz
tar xvzf php-5.2.17.tar.gz
cd php-5.2.17
wget https://mail.gnome.org/archives/xml/2012-August/txtbgxGXAvz4N.txt
patch -p0 < txtbgxGXAvz4N.txt
sed -i 's/__GMP_BITS_PER_MP_LIMB/GMP_LIMB_BITS/g' ext/gmp/gmp.c
./configure --prefix=/usr/local --enable-bcmath --enable-mcrypt --with-gmp
make
sudo make install

In your case GMP wouldn't be needed. It might be easier but it also might not be. None-the-less I can try to give it a shot as time permits but do realize that with each successive hoop I have to jump through the odds of me successfully reproducing the problem diminish.

@romucifu
Copy link
Author

@terrafrost Don't worry, I understand the difficulty of the task.

Just one more thing, I've just tried the same script with PHP 5.2.9 over a 64-Bit Linux OS and it works perfectly.

So clearly it looks like a very specific problem of PHP 5.2.9 over 32-Bit OS.

@terrafrost
Copy link
Member

terrafrost commented Sep 29, 2016

So in Blowfish.php there are lines like this:

            $l^= ($sb_0[$r >> 24 & 0xff]  +
                  $sb_1[$r >> 16 & 0xff]  ^
                  $sb_2[$r >>  8 & 0xff]) +
                  $sb_3[$r       & 0xff];

Personally, I think ($a + $b) ^ $c is preferable to $a + $b ^ $c but that's a different matter.

The problem is adding stuff. Even if $a and $b are int's their sum may exceed the max value for a signed 32-bit int. So in those situations it'll cast a float to an int.

On PHP 7.0.2 (32-bit) on Windows casting -2368666402 to an int gives 0x72d104de back.

On PHP 5.2.17 (32-bit) on Ubuntu casting -2368666402 to an int gives 0x80000000 back.

The only thing I can figure is maybe create a new function - safe_add:

if (is_int($sum)) {
    return $sum;
}
return $sum < 0 ? fmod($sum, 0x80000000) ^ 0x80000000 : $sum;

fmod(-2368666402, 0x80000000) isn't sufficient because that returns 0xf2d104de - not 0x72d104de.

I'll have to try this on later versions of PHP on Ubuntu..

@bantu bantu added the bug label Sep 29, 2016
@bantu
Copy link
Member

bantu commented Sep 29, 2016

If the float is beyond the boundaries of integer (usually +/- 2.15e+9 = 2^31 on 32-bit platforms and +/- 9.22e+18 = 2^63 on 64-bit platforms other than Windows), the result is undefined, since the float doesn't have enough precision to give an exact integer result. No warning, not even a notice will be issued when this happens!

http://php.net/manual/de/language.types.integer.php#language.types.integer.casting

@terrafrost
Copy link
Member

terrafrost commented Sep 30, 2016

Actually, here's the function that I think will need to be added.

function safe_intval($x) {
    // PHP 5.3, per http://php.net/releases/5_3_0.php, introduced "more consistent float rounding"
    // PHP_OS & "\xDF\xDF\xDF" == strtoupper(substr(PHP_OS, 0, 3)), but a lot faster
    if (is_int($x) || version_compare(PHP_VERSION, '5.3.0') >= 0 || (PHP_OS & "\xDF\xDF\xDF") === 'WIN') {
        return $x;
    }
    return (fmod($x, 0x80000000) & 0x7FFFFFFF) |
        ((fmod(floor($x / 0x80000000), 2) & 1) << 31);
}

The first part of the OR gets the bottom 31 bits. But because fmod can return negative numbers the first bit is reserved for signage (positive / negative). For that reason we can't do fmod with 0x1 0000 0000. The second part essentially does >> 31 and then does & 1 and then does << 31. So it's isolating the msb.

I tested intval(-2368666402) on PHP7 on 32-bit Ubuntu and got 0x72d104de back. I'll try it on PHP 5.3 tomorrow and try to figure out what the oldest version of PHP that this isn't needed on is.

@terrafrost
Copy link
Member

I think https://github.com/terrafrost/phpseclib/tree/1.0-32bit-fixes should fix the issue for you. LMK!

@bantu
Copy link
Member

bantu commented Oct 1, 2016

@terrafrost I was wondering if you could just do two additions modulo 2^16 an some shifting instead of one 2^32, but I really did not think it through.

@terrafrost
Copy link
Member

We'd have to add both halves and then, for the upper half, keep track of whether or not we needed to carry stuff over. It's definitely a valid approach but idk that it'd be any faster (or slower).

@romucifu
Copy link
Author

romucifu commented Oct 6, 2016

@terrafrost YES! it works! now I have the same result on all of my hosts 32 or 64 Bit and with any PHP version.

Thanks!

@terrafrost
Copy link
Member

Glad to hear it! The fix has been merged into 1.0 / 2.0 / master.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants