php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #75185 Buffer overflow in json_decode() with JSON_INVALID_UTF8_IGNORE or JSON_INVALID
Submitted: 2017-09-11 09:57 UTC Modified: 2017-09-19 01:36 UTC
Votes:1
Avg. Score:1.0 ± 0.0
Reproduced:0 of 0 (0.0%)
From: Ciprian dot Pitis at microsoft dot com Assigned: cmb (profile)
Status: Closed Package: JSON related
PHP Version: Irrelevant OS:
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: Ciprian dot Pitis at microsoft dot com
New email:
PHP Version: OS:

 

 [2017-09-11 09:57 UTC] Ciprian dot Pitis at microsoft dot com
Description:
------------
This bug was fixed in 7.2.0-beta3, originally found on 7.2.0-beta1, and affecting beta1 and beta2. Adding it here for documenting purposes.

Problem lies within ext/json/json_scanner.c file. Variable utf8_invalid_count is used for counting the delta of the string len due to the invalid utf8 characters ignore or substitution:
	int utf8_addition = (s->options & PHP_JSON_INVALID_UTF8_SUBSTITUTE) ? 3 : 0;
	s->utf8_invalid = 1;
	s->utf8_invalid_count += utf8_addition - 1;
	PHP_JSON_CONDITION_GOTO(STR_P1);

As you can see, utf8_invalid_count will decrease by 1 for each invalid byte using _IGNORE option and increment by 2 using _SUBSTITUTE option. This is further used with allocating the string taken from JSON:

size_t len = s->cursor - s->str_start - s->str_esc - 1 + s->utf8_invalid_count;

The problem ( at least it seems ) is that utf8_invalid_count is not reinitialized to zero when entering php_json_scan. Hence, when allocating another string, the buffer will be too small due to utf8_invalid_count decrementing the len.
The size of the overflow is controllable by attack and it equals O = (L2 – L1) where L2 = length of second string allocated and L1 = amount of invalid UTF8 bytes injected into first string.

Following POC should reproduce the attack ( Im writing this off my head as I have the real POC on my other laptop, in case it’s not proper I’m going to send the proper POC tomorrow ):

<?php
$text = '"'.chr(0xC1).chr(0xC1).'""DDD"';
var_dump(json_decode($text,false,512,JSON_INVALID_UTF8_IGNORE));

What will happen is that the first string ( "#C1#C1" ) will be properly stripped of two invalid bytes and returned, however the second string will get allocated too small due to utf8_invalid_count = -2 , hence len instead of being 3 will become 1.
Then, inside the php_json_scanner_copy_string routine, buffer overflow will happen. 

_SUBSTITUTE option , while less convenient, can also be used for exploiting as to overflow the sign bit ( utf8_invalid_count is a 32 bit signed int ) we need to send around 1.1 GB of invalid bytes, and around 2.2 GB to make the counter be feasible for attack.

Fortunately, the feature was recently introduced and is not adopted wide, but we should definitely act quickly to make sure the bug doesn’t make it to 7.2.0 as there’s big potential for attack surface once adoption takes place.


Test script:
---------------
<?php
$text = '"'.chr(0xC1).chr(0xC1).'""DDD"';
var_dump(json_decode($text,false,512,JSON_INVALID_UTF8_IGNORE));


Expected result:
----------------
Deserialized json object

Actual result:
--------------
Crash

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2017-09-11 16:30 UTC] cmb@php.net
-Status: Open +Status: Verified -Assigned To: +Assigned To: cmb
 [2017-09-11 16:30 UTC] cmb@php.net
Thanks for reporting this! According to our security issue
classification this issue is a low severity issue, and since it
already has been fixed, there is no need for a private report.

Anyhow, the bug fix has to be documented in NEWS and the
changelog, and a respective regression test should be added.
 [2017-09-11 17:26 UTC] cmb@php.net
> Im writing this off my head as I have the real POC on my other
> laptop, in case it’s not proper I’m going to send the proper POC
> tomorrow

Yes, please. While I can confirm issues regarding an
uninititalized variable in beta1 and beta2, I cannot reproduce the
segfault, and actually $text looks strange, and might have been:

  '["'.chr(0xC1).chr(0xC1).'", "DDD"]'
 [2017-09-18 14:53 UTC] bukka@php.net
I have already fixed it in 

https://github.com/php/php-src/commit/41d7621f48d78034755ccd540ade850eedc838c6#diff-0d909864ac5e6d49aa3902ec594dae74

The changes has not been released and there was no bug report (just an email that Rasmus forwarded to me) so I didn't bother with NEWS but feel free to update it (will probably require updating news entries in the website - web changelog).

The test also cover the segfault (it won't work for your example as you have 3 bytes after - you need to have less to make it segfault - see the test)
 [2017-09-18 15:24 UTC] cmb@php.net
-Status: Verified +Status: Closed
 [2017-09-18 15:24 UTC] cmb@php.net
> The test also cover the segfault (it won't work for your example as you have 3
> bytes after - you need to have less to make it segfault - see the test)

Ah, that explains the issue. So since we're having a proper test already, no
need to add another one.

NEWS updated via <http://git.php.net/?p=php-src.git;a=commit;h=613bac9>.

Actually, there's no need to update the Changelog separately, because there is
none yet for PHP 7.2.
 [2017-09-19 01:36 UTC] stas@php.net
-Type: Security +Type: Bug
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Sun Nov 19 01:31:42 2017 UTC