[Pljava-dev] threads left running after function end

Off list I was asked about threads left running after the main function
returns. It's a pretty general question and I found what looks to be a
pretty serious security hole, so I figured I'd include the list as well.

If a function starts up a thread and doesn't wait for it to complete
before it returns, that thread is left running. It isn't allowed to
call anything on the server because the server is off doing other work
and it is not thread safe. So if the created thread tries to do a
database query it will block until the next call into a pljava function
happens at which point it can access the database again (until that
function returns). If the thread isn't doing database work it will run
until the database connection is closed at which point it will be
stopped as the JVM shuts down.

What I've discovered is that a thread which is left running after a call
to a trusted java function will then no longer be sandboxed if a call to
an untrusted function is made. So if you've created an untrusted
function as security definer and granted execution on that to an
unprivileged user they can take advantage of that and get superuser
access by leaving a thread running in a trusted function.

The original question was about making a function wait for all threads
before returning and this is possible now by simply altering the user
code to do something like:

private static void waitForAllThreads()
{
while (Thread.activeCount() > 1) {
Thread allThreads[] = new Thread[Thread.activeCount()];
int active = Thread.enumerate(allThreads);
for (int i=0; i<active; i++) {
// Don't need to wait for ourselves.
if (allThreads[i] == Thread.currentThread())
continue;

try {
allThreads[i].join();
} catch (InterruptedException ie) {
}
}
}
}

Should pljava force all threads to complete before returning though? I
can see the utility of creating a thread to bind to a tcp/ip socket and
wait for external data and queue that up for a later function call to
pickup from the thread for database processing. If you kept the
function call active over the entire lifetime of this service, you'd
have a very long transaction that would cause other problems (vacuum).
Although I suppose with a polling caller you'd need a program to do
that, so you might as well have the client bind to a socket and wait for
data instead. But I digress, your thoughts...

Kris Jurka

Keep in mind that the JVM has at least one thread that does garbage
collection. It might have other threads as well that performs other type
of maintenance. Such thread will never access the database and it's
probably a bad thing to keep them from doing their work.

Regards,
Thomas Hallgren

Kris Jurka wrote:
> Off list I was asked about threads left running after the main function
> returns. It's a pretty general question and I found what looks to be a
> pretty serious security hole, so I figured I'd include the list as well.
>
> If a function starts up a thread and doesn't wait for it to complete
> before it returns, that thread is left running. It isn't allowed to
> call anything on the server because the server is off doing other work
> and it is not thread safe. So if the created thread tries to do a
> database query it will block until the next call into a pljava function
> happens at which point it can access the database again (until that
> function returns). If the thread isn't doing database work it will run
> until the database connection is closed at which point it will be
> stopped as the JVM shuts down.
>
> What I've discovered is that a thread which is left running after a call
> to a trusted java function will then no longer be sandboxed if a call to
> an untrusted function is made. So if you've created an untrusted
> function as security definer and granted execution on that to an
> unprivileged user they can take advantage of that and get superuser
> access by leaving a thread running in a trusted function.
>
> The original question was about making a function wait for all threads
> before returning and this is possible now by simply altering the user
> code to do something like:
>
> private static void waitForAllThreads()
> {
> while (Thread.activeCount() > 1) {
> Thread allThreads[] = new Thread[Thread.activeCount()];
> int active = Thread.enumerate(allThreads);
> for (int i=0; i<active; i++) {
> // Don't need to wait for ourselves.
> if (allThreads[i] == Thread.currentThread())
> continue;
>
> try {
> allThreads[i].join();
> } catch (InterruptedException ie) {
> }
> }
> }
> }
>
> Should pljava force all threads to complete before returning though? I
> can see the utility of creating a thread to bind to a tcp/ip socket and
> wait for external data and queue that up for a later function call to
> pickup from the thread for database processing. If you kept the
> function call active over the entire lifetime of this service, you'd
> have a very long transaction that would cause other problems (vacuum).
> Although I suppose with a polling caller you'd need a program to do
> that, so you might as well have the client bind to a socket and wait for
> data instead. But I digress, your thoughts...
>
> Kris Jurka
> _______________________________________________
> Pljava-dev mailing list
> Pljava-dev at pgfoundry.org
> http://pgfoundry.org/mailman/listinfo/pljava-dev
>

On Fri, 10 Oct 2008, Thomas Hallgren wrote:

> Keep in mind that the JVM has at least one thread that does garbage
> collection. It might have other threads as well that performs other type
> of maintenance. Such thread will never access the database and it's
> probably a bad thing to keep them from doing their work.

That's OK because the calls to Thread only look at the current (and sub)
ThreadGroups. The JVM threads are elsewhere and are not found.

Unfortunately just ensuring that all threads end before function return is
not enough to plug the security hole. If a trusted function calls an
untrusted function, as soon as the untrusted function is invoked, the
SecurityManager is replaced and another thread of the trusted function can
do nefarious things. Instead of two SecurityManagers that get switched
back and forth, we need a single SecurityManager that can tell in what
context it's being used. Unfortunately I have no idea how to do that at
the moment.

Without a fix to this I don't see any alternative to dropping the trusted
version of pljava.

Kris Jurka

Kris Jurka wrote:

> Without a fix to this I don't see any alternative to dropping the
> trusted version of pljava.
>
I think the easiest fix would be to prevent a trusted function from ever calling an untrusted
function. I don't think that would be too limiting and it should be easy enough to check. The
semantic would do something like this:

At the point when the SecurityManager is replaced, check if the invocation is nested. If it is,
check if the current SecurityManager is the one used by a trusted function. If so, throw an exception.

Regards,
Thomas Hallgren