Skip to content

Possible use-after-free of the Go backing array of extensions retained by register_extensions #2629

Description

@OvOhao

Possible use-after-free of the Go backing array of extensions retained by register_extensions

registerExtensions hands the backing array of the Go slice extensions to C, which
stores it in a static global, and then immediately sets extensions = nil. The array is
read later, from PHP startup, by which point nothing in Go references it and the garbage
collector is free to reclaim it.

ext.go:20

func registerExtensions() {
	if len(extensions) == 0 {
		return
	}

	registerOnce.Do(func() {
		C.register_extensions((**C.zend_module_entry)(unsafe.Pointer(&extensions[0])), C.int(len(extensions)))
		extensions = nil
	})
}

frankenphp.c:1839 stores the pointer rather than copying:

void register_extensions(zend_module_entry **m, int len) {
  modules = m;
  modules_len = len;

  original_php_register_internal_extensions_func =
      php_register_internal_extensions_func;
  php_register_internal_extensions_func = register_internal_extensions;
}

modules is a file-static (frankenphp.c:1817) and is only dereferenced later, from the
callback that was just installed (frankenphp.c:1821):

int register_internal_extensions(void) {
  if (original_php_register_internal_extensions_func != NULL &&
      original_php_register_internal_extensions_func() != SUCCESS) {
    return FAILURE;
  }

  for (int i = 0; i < modules_len; i++) {
    if (zend_register_internal_module(modules[i]) == NULL) {
      return FAILURE;
    }
  }

So the sequence is:

  1. registerExtensions passes &extensions[0], a pointer into Go-allocated memory.
  2. register_extensions saves it in modules and returns. C now holds a Go pointer past
    the end of the cgo call, which the cgo pointer-passing rules do not allow.
  3. extensions = nil drops the only Go reference to that array.
  4. PHP later calls register_internal_extensions, which reads modules[i].

Between 3 and 4 the array is unreachable from Go, so it can be collected. The elements
themselves are fine, since RegisterExtension takes unsafe.Pointer values that live in
C memory; it is the array of pointers that is Go-allocated.

I have not produced a crash from this, and in practice the window may be short enough that
collection does not happen. The retention itself is unconditional though, and dropping the
reference on the next line removes the only thing that would have kept it alive.

runtime.Pinner is already used elsewhere in the project (phpthread.go:19).

Fix: pin the array for as long as C holds it, and keep the slice alive.

var extPinner runtime.Pinner

registerOnce.Do(func() {
	extPinner.Pin(&extensions[0])
	C.register_extensions((**C.zend_module_entry)(unsafe.Pointer(&extensions[0])), C.int(len(extensions)))
})

Dropping extensions = nil and unpinning once register_internal_extensions has run
would be enough. Copying the array into C memory with C.malloc before the call avoids
the question entirely.

If you could credit me as a reporter for my contributions to security advisory I will be thankful.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions