php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Request #75804 authenticated encryption tag is broken
Submitted: 2018-01-11 22:18 UTC Modified: 2020-12-31 14:13 UTC
Votes:1
Avg. Score:3.0 ± 0.0
Reproduced:1 of 1 (100.0%)
Same Version:0 (0.0%)
Same OS:0 (0.0%)
From: sergiuthepenguin at gmail dot com Assigned: bukka (profile)
Status: Re-Opened Package: OpenSSL related
PHP Version: 7.1.13 OS: Windows and GNU/Linux
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: sergiuthepenguin at gmail dot com
New email:
PHP Version: OS:

 

 [2018-01-11 22:18 UTC] sergiuthepenguin at gmail dot com
Description:
------------
openssl_decrypt function doesn't appear to properly check authentication tag length. Not in all cases, at least.

Run the script in your browser and hit refresh a few times.

Test script:
---------------
https://gist.github.com/SergiuThePenguin/55366527804b592ed86b29c8079b697a


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2018-01-11 22:33 UTC] sergiuthepenguin at gmail dot com
Forgot to mention that I am using php-7.1.13-Win32-VC14-x86 from http://windows.php.net/download (VC14 x86 Thread Safe (2018-Jan-04 00:44:08)) on Windows x64 under UwAmp (https://www.uwamp.com/en/) with the default php.ini-development.
 [2018-01-17 18:21 UTC] sergiuthepenguin at gmail dot com
-Summary: authenticated encryption does not check for tag length +Summary: authenticated encryption tag is broken
 [2018-01-17 18:21 UTC] sergiuthepenguin at gmail dot com
Performed more tests. Apparently, openssl_decrypt does not care for the tag length in 9/10 random cases (using substr). Attempting to replace some bytes from the tag does result in expected behaviour with one exception: replacing the 1st byte at the front of the tag sometimes passes the check and decrypts the data.

This doesn't add up. Why replacing the data passes the tests but removing it doesn't?

I didn't find anything in the official documentation about this behaviour, but maybe I am blind.
 [2018-01-17 18:39 UTC] sergiuthepenguin at gmail dot com
Updated the test script: https://gist.github.com/SergiuThePenguin/55366527804b592ed86b29c8079b697a

Also, Ruby has had a similar (if not identical) issue: https://github.com/ruby/openssl/issues/63
 [2018-01-20 19:34 UTC] ab@php.net
-Status: Open +Status: Feedback
 [2018-01-20 19:34 UTC] ab@php.net
Thanks for the report. Were it possible to reduce the reproduce script to max 20 lines? Also, PHP 7.2 uses OpenSSL 1.1.0, does it make a diff? Also note, that OpenSSL is provided as a DLL, the actual version can be overridden by an actual WAMP distribution. Preferable were a test with the versions provided by the official build.

Thanks.
 [2018-01-25 22:53 UTC] sergiuthepenguin at gmail dot com
-Status: Feedback +Status: Open
 [2018-01-25 22:53 UTC] sergiuthepenguin at gmail dot com
Sorry for the slow reply. I have shortened the code as much as possible: https://gist.github.com/SergiuThePenguin/91ca9f9896882ede43684ab47555b203

If we remove all the bytes of the tag except the for the 1st one, the decryption is still successful. However, if we replace all bytes except the 1st one, the decryption fails as expected.

I tested PHP 7.1.13 and 7.2.1 on Windows (outside of wamp, using the installer) as well as in GNU/Linux. The results were identical.
 [2018-01-26 16:23 UTC] ab@php.net
Thanks for the shorter code. I can reproduce the issue with OpenSSL 1.0.2 and 1.1.0. The linked Ruby issue is actually same. Here's even a shorter reproducer

$text = 'The quick brown fox jumps over the lazy dog.';
$key = random_bytes(32);
$iv = openssl_random_pseudo_bytes(openssl_cipher_iv_length('aes-256-gcm'));
$cipherText = openssl_encrypt($text, 'aes-256-gcm', $key, OPENSSL_RAW_DATA, $iv, $tag, 'test-aad', 16);

$ret = openssl_decrypt($cipherText, 'aes-256-gcm', $key, OPENSSL_RAW_DATA, $iv, $tag[0], 'test-aad');
var_dump($ret);

This prints the decrypted text, more logic were IMO bool(false).

I'd say it's definitely not a good behavior. In how far it is a security issue, is another question. An application should assert the correct tag length in first place. However, this behavior definitely increases the security risk due to programmer mistakes. The documentation states the tag length accepted by openssl_encrypt is between 4 and 16, however the below comes through with the tag length of 3

$cipherText = openssl_encrypt($text, 'aes-256-gcm', $key, OPENSSL_RAW_DATA, $iv, $tag, 'test-aad', 3);

We need to discuss further how this should be fixed. Perhaps raising the default tag length could be a solution, something like to 12 bytes as a default for AES. This of course doesn't eliminate the need to check this in the application.

Thanks.
 [2018-01-26 17:21 UTC] sergiuthepenguin at gmail dot com
-Operating System: Windows +Operating System: Windows and GNU/Linux
 [2018-01-26 17:21 UTC] sergiuthepenguin at gmail dot com
Thanks for confirming.

IMHO, the documentation should be updated to warn developers about manually verifying the tag length using isset($tag[$chosenLength - 1]) or mb_strlen($tag, '8bit').

Speaking of which, is it not possible for openssl_decrypt to have a tag length argument for internal checks?
 [2020-02-09 19:55 UTC] bukka@php.net
-Type: Security +Type: Documentation Problem -Assigned To: +Assigned To: bukka
 [2020-02-09 19:55 UTC] bukka@php.net
This is known thing and that's how OpenSSL works as well. I agree that we should document this.

Not sure about extra parameter as that's pretty easy to do in user land. Also we would need a default value which would break the cases that use shorter tag. But I guess this could be acceptable for 8.0. Might be a good idea though.
 [2020-08-20 10:34 UTC] cmb@php.net
-Type: Documentation Problem +Type: Feature/Change Request
 [2020-08-20 10:34 UTC] cmb@php.net
This issue has now been documented[1].  Changing to feature
request.

[1] <<http://svn.php.net/viewvc?view=revision&revision=350346>>
 [2020-08-20 17:20 UTC] phpdocbot@php.net
Automatic comment on behalf of mumumu
Revision: http://git.php.net/?p=doc/ja.git;a=commit;h=244520d6ee50dfe57266368e3b24adc75eedc474
Log: Fix #75804: authenticated encryption tag is broken
 [2020-08-20 17:20 UTC] phpdocbot@php.net
-Status: Assigned +Status: Closed
 [2020-08-20 20:37 UTC] cmb@php.net
-Status: Closed +Status: Re-Opened
 [2020-08-20 20:37 UTC] cmb@php.net
phpdocbot was not supposed to close this. :)
 [2020-12-30 11:59 UTC] nikic@php.net
Automatic comment on behalf of mumumu
Revision: http://git.php.net/?p=doc/ja.git;a=commit;h=fdbe569241e6ce7930f7b2618f8448f4be2bdfa4
Log: Fix #75804: authenticated encryption tag is broken
 [2020-12-30 11:59 UTC] nikic@php.net
-Status: Re-Opened +Status: Closed
 [2020-12-30 21:21 UTC] bukka@php.net
-Status: Closed +Status: Re-Opened
 [2020-12-30 21:21 UTC] bukka@php.net
This is not fixed in EN docs.
 [2020-12-31 14:13 UTC] cmb@php.net
Isn't the cautionary note for the tag parameter[1] sufficient?
Anyhow, this should probably stay open as feature request.

[1] <https://github.com/php/doc-en/commit/a125f6643eb0d25778af068032f746f2e9432364>
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Tue Mar 19 09:01:30 2024 UTC