Re: Re: [Pljava-dev] Should creating a new base type require superuser status?

Currently, you're allowed to create a new base type if you own the I/O
functions for it. That effectively restricts the command to superusers
anyway, since there's presently no way for a non-superuser to create
a function that would have the required signature. However that's a
fairly indirect protection against letting a dangerous command be
executed by malicious users; and it would fail given such apparently
innocent-looking changes as allowing PL functions to take or return
cstring. I wonder whether we shouldn't just restrict the command to
superusers explicitly, and thus affirmatively prevent possible future
security holes.

If you're not clear on why CREATE TYPE in the hands of a bad guy is
dangerous, here are a couple of reasons:

* By specifying type representation details (len/byval/align) that are
different from what the type's functions expect, you could trivially
crash the backend, and less trivially use a pass-by-reference I/O
function to read out the contents of backend memory.

* The just-added ability to specify a new type's type category and
"preferred" status could allow subverting the behavior of existing
queries that expect ambiguous operators to be resolved in a particular
way. A new preferred type could "capture" such queries and thereby
provide a trojan-horse vector for executing functions as some other
user.

There might be some other attacks I've not thought of.

Comments?

regards, tom lane

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> If you're not clear on why CREATE TYPE in the hands of a bad guy is
> dangerous, here are a couple of reasons:
>
> * By specifying type representation details (len/byval/align) that are
> different from what the type's functions expect, you could trivially
> crash the backend, and less trivially use a pass-by-reference I/O
> function to read out the contents of backend memory.

I know when I was first starting out it was a big source of frustration that
you have to get those arguments right.. Until I figured out what they all
meant and how to use them I was constantly crashing the server.

It seems to me we should be able to do better. To have some kind of struct in
the C code associated with the input/output functions from which the create
type command picks up these parameters.

As a consequence we could perhaps aim to make creating new types safe rather
than just deal with the fact that it's not safe currently? It would be nice if
non-superusers could create types which used an existing set of input/output
functions but defined new semantics.

> * The just-added ability to specify a new type's type category and
> "preferred" status could allow subverting the behavior of existing
> queries that expect ambiguous operators to be resolved in a particular
> way. A new preferred type could "capture" such queries and thereby
> provide a trojan-horse vector for executing functions as some other
> user.

Would it be enough to only require super-user to create a preferred type?

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's Slony Replication support!

Gregory Stark <stark(at)enterprisedb(dot)com> writes:
> I know when I was first starting out it was a big source of frustration that
> you have to get those arguments right.. Until I figured out what they all
> meant and how to use them I was constantly crashing the server.

> It seems to me we should be able to do better. To have some kind of struct in
> the C code associated with the input/output functions from which the create
> type command picks up these parameters.

And what are the odds that you'll get it right in a C struct if you
couldn't get it right in the SQL command? There might be some small
advantage here from a packaging standpoint, but I think the argument
that it'd help novice PG hackers is bogus.

> As a consequence we could perhaps aim to make creating new types safe rather
> than just deal with the fact that it's not safe currently? It would be nice if
> non-superusers could create types which used an existing set of input/output
> functions but defined new semantics.

Unless you're going to allow them to create new C functions, I'm not
clear on how much they're going to be able to change the semantics.

>> * The just-added ability to specify a new type's type category and
>> "preferred" status could allow subverting the behavior of existing
>> queries that expect ambiguous operators to be resolved in a particular
>> way. A new preferred type could "capture" such queries and thereby
>> provide a trojan-horse vector for executing functions as some other
>> user.

> Would it be enough to only require super-user to create a preferred type?

Well, it'd close one hole, but I think there are too many others to
take a narrow-gauge approach.

regards, tom lane

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

>> As a consequence we could perhaps aim to make creating new types safe rather
>> than just deal with the fact that it's not safe currently? It would be nice if
>> non-superusers could create types which used an existing set of input/output
>> functions but defined new semantics.
>
> Unless you're going to allow them to create new C functions, I'm not
> clear on how much they're going to be able to change the semantics.

Well there's plenty that can be done just using text or bytea as
representations. citext, or uuid for example.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's RemoteDBA services!

Tom Lane wrote:

> If you're not clear on why CREATE TYPE in the hands of a bad guy is
> dangerous, here are a couple of reasons:
>
> * By specifying type representation details (len/byval/align) that are
> different from what the type's functions expect, you could trivially
> crash the backend, and less trivially use a pass-by-reference I/O
> function to read out the contents of backend memory.

I think being able to return cstring from a user defined function is
quite dangerous already. I doubt we would ever give that capability to
non-superusers.

I do agree that creating base types should require a superuser though.
It too seems dangerous just on principle, even if today there's no
actual hole (that we already know of).

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Gregory Stark <stark(at)enterprisedb(dot)com> writes:
> "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> Unless you're going to allow them to create new C functions, I'm not
>> clear on how much they're going to be able to change the semantics.

> Well there's plenty that can be done just using text or bytea as
> representations. citext, or uuid for example.

For the sort of look-ma-no-C programming that you seem to be
envisioning, I don't think that real base types are appropriate at all.
What might make sense is to handle it as a domain over some
suitably-generic base type. The thing we'd have to fix to make that
work is the way that the ambiguous-function resolution rules
discriminate against functions that're declared to take domains.
Which is hard, but it seems a lot less likely to lead to weird
security risks than letting untrusted users mess with the details
of base-type operations.

Elein was going to go off in a corner and think about how to make that
work better, but I dunno if she's gotten anywhere yet.

regards, tom lane

On Wed, Jul 30, 2008 at 06:07:53PM -0400, Alvaro Herrera wrote:

> I do agree that creating base types should require a superuser though.
> It too seems dangerous just on principle, even if today there's no
> actual hole (that we already know of).

I agree.

--
Andrew Sullivan
ajs(at)commandprompt(dot)com
+1 503 667 4564 x104
http://www.commandprompt.com/

On Wed, 30 Jul 2008, Alvaro Herrera wrote:

> I think being able to return cstring from a user defined function is
> quite dangerous already. I doubt we would ever give that capability to
> non-superusers.
>
> I do agree that creating base types should require a superuser though.
> It too seems dangerous just on principle, even if today there's no
> actual hole (that we already know of).

pl/java already allows non-superusers to create functions returning
cstring and base types built off of these functions. It seems safe to me
if pl/java is doing the construction of cstring from a user provided
java.lang.String.

http://wiki.tada.se/display/pljava/Creating+a+Scalar+UDT+in+Java

Kris Jurka

Kris Jurka <books(at)ejurka(dot)com> writes:
> On Wed, 30 Jul 2008, Alvaro Herrera wrote:
>> I do agree that creating base types should require a superuser though.
>> It too seems dangerous just on principle, even if today there's no
>> actual hole (that we already know of).

> pl/java already allows non-superusers to create functions returning
> cstring and base types built off of these functions.

So in other words, if pl/java is installed we have a security hole
a mile wide.

regards, tom lane

It seems perfectly safe to me too for the reason that Kris mentions.

Tom, could you please elaborate where you see a security hole?

Regards,
Thomas Hallgren

Tom Lane wrote:
> Kris Jurka <books(at)ejurka(dot)com> writes:
>
>> On Wed, 30 Jul 2008, Alvaro Herrera wrote:
>>
>>> I do agree that creating base types should require a superuser though.
>>> It too seems dangerous just on principle, even if today there's no
>>> actual hole (that we already know of).
>>>
>
>
>> pl/java already allows non-superusers to create functions returning
>> cstring and base types built off of these functions.
>>
>
> So in other words, if pl/java is installed we have a security hole
> a mile wide.
>
> regards, tom lane
> _______________________________________________
> Pljava-dev mailing list
> Pljava-dev(at)pgfoundry(dot)org
> http://pgfoundry.org/mailman/listinfo/pljava-dev
>

Thomas Hallgren <thomas(at)tada(dot)se> writes:
> Tom, could you please elaborate where you see a security hole?

The problem that we've seen in the past shows up when the user lies in
the CREATE TYPE command, specifying type representation properties that
are different from what the underlying functions expect. In particular,
if it's possible to pass a pass-by-value integer to a function
that's expecting a pass-by-reference datum, you can misuse the function
to access backend memory.

I gather from looking at the example that Kris referenced that there's
some interface code in between the SQL function call and the user's Java
code, and that that interface code is itself looking at the declared
properties of the SQL type to decide what to do. So to the extent that
that code is (a) bulletproof against inconsistencies and (b) not
subvertible by the PL/Java user, it might be that there's no hole in
practice. But assumption (b) seems pretty fragile to me.

regards, tom lane

>>>>> "Tom" == Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

>> Tom, could you please elaborate where you see a security hole?

Tom> The problem that we've seen in the past shows up when the user
Tom> lies in the CREATE TYPE command, specifying type representation
Tom> properties that are different from what the underlying functions
Tom> expect. In particular, if it's possible to pass a pass-by-value
Tom> integer to a function that's expecting a pass-by-reference
Tom> datum, you can misuse the function to access backend memory.

It strikes me that type output functions are routinely invoked by
superusers (e.g. during pg_dump), and therefore if a non-superuser can
create a type, that seems to imply that there's no way for a superuser
to safely examine or dump the content of the database without risking
the execution of untrusted code, correct?

--
Andrew (irc:RhodiumToad)

Tom Lane wrote:
> Thomas Hallgren <thomas(at)tada(dot)se> writes:
>
>> Tom, could you please elaborate where you see a security hole?
>>
>
> The problem that we've seen in the past shows up when the user lies in
> the CREATE TYPE command, specifying type representation properties that
> are different from what the underlying functions expect. In particular,
> if it's possible to pass a pass-by-value integer to a function
> that's expecting a pass-by-reference datum, you can misuse the function
> to access backend memory.
>
>
This is a non-issue in PL/Java. An integer parameter is never passed by
reference and there's no way the PL/Java user can get direct access to
backend memory.

> I gather from looking at the example that Kris referenced that there's
> some interface code in between the SQL function call and the user's Java
> code, and that that interface code is itself looking at the declared
> properties of the SQL type to decide what to do. So to the extent that
> that code is (a) bulletproof against inconsistencies and (b) not
> subvertible by the PL/Java user, it might be that there's no hole in
> practice. But assumption (b) seems pretty fragile to me.
>
>
I think that assumption is without ground. Java doesn't permit you to
access memory unless you use Java classes (java.nio stuff) that is
explicitly designed to do that and you need native code to set such
things up. A PL/Java user can not do that unless he is able to link in
other shared objects or dll's to the backend process.

Based on that, I claim that your statement about a "security hole a mile
wide" is incorrect. PL/Java is not subject to issues relating to misuse
of backend memory.

Regards,
Thomas Hallgren

On Thu, 2008-07-31 at 09:39 +0100, Andrew Sullivan wrote:
> On Wed, Jul 30, 2008 at 06:07:53PM -0400, Alvaro Herrera wrote:
>
> > I do agree that creating base types should require a superuser though.
> > It too seems dangerous just on principle, even if today there's no
> > actual hole (that we already know of).
>
> I agree.

+1

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Thomas Hallgren <thomas(at)tada(dot)se> writes:
> Tom Lane wrote:
>> The problem that we've seen in the past shows up when the user lies in
>> the CREATE TYPE command, specifying type representation properties that
>> are different from what the underlying functions expect.

> This is a non-issue in PL/Java. An integer parameter is never passed by
> reference and there's no way the PL/Java user can get direct access to
> backend memory.

So what exactly does happen when the user deliberately specifies wrong
typlen/typbyval/typalign info when creating a type based on PL/Java
functions?

> I think that assumption is without ground. Java doesn't permit you to
> access memory unless you use Java classes (java.nio stuff) that is
> explicitly designed to do that and you need native code to set such
> things up. A PL/Java user can not do that unless he is able to link in
> other shared objects or dll's to the backend process.

PL/Java itself must be doing "unsafe" things in order to interface with
PG at all. So what your argument really is is that you have managed to
securely sandbox the user-written code you are calling. That might or
might not be true, but I don't think that worrying about it is without
foundation.

regards, tom lane

Tom Lane wrote:
>
>> This is a non-issue in PL/Java. An integer parameter is never passed by
>> reference and there's no way the PL/Java user can get direct access to
>> backend memory.
>>
>
> So what exactly does happen when the user deliberately specifies wrong
> typlen/typbyval/typalign info when creating a type based on PL/Java
> functions?
>
>
Everything is converted into instances of Java classes such as String,
byte[], etc.

>> I think that assumption is without ground. Java doesn't permit you to
>> access memory unless you use Java classes (java.nio stuff) that is
>> explicitly designed to do that and you need native code to set such
>> things up. A PL/Java user can not do that unless he is able to link in
>> other shared objects or dll's to the backend process.
>>
>
> PL/Java itself must be doing "unsafe" things in order to interface with
> PG at all. So what your argument really is is that you have managed to
> securely sandbox the user-written code you are calling. That might or
> might not be true, but I don't think that worrying about it is without
> foundation.
>
>
I would be presumptuous to claim that I provide the sandbox. All PL/Java
does is to provide the type mapping. The sandbox as such is implicit in
Java, much in the same way that it does it for web-browsers etc.

Regardless of that, I think there's some difference in expressing a
worry that might or might not have a foundation versus claiming that
there indeed must be a security hole a mile wide ;-)

- thomas

On Sat, 2 Aug 2008, Tom Lane wrote:

> So what exactly does happen when the user deliberately specifies wrong
> typlen/typbyval/typalign info when creating a type based on PL/Java
> functions?

I have reviewed pljava's handling of misrepresented alignment, length, and
by value parameters

1) Alignment: pljava reads and writes data a byte at a time, so all types
effectively have char alignment. Reading an integer will read
four bytes out of memory and then put those together. Therefore the
alignment cannot be misspecified.

2) Length: For fixed length types, pljava correctly detects trying to
read or write too much data and not supplying enough data on write.
Pljava does not correctly handle variable length types. It should be
setting and reading the length header itself rather than leaving that up
to the user, but it is not.

3) By value: pljava does not correctly handle passed by value types
correctly, allowing access to random memory.

So yes, pljava has a security problem, but I still object to the statement
that no PL can do this securely. I will work on fixing pljava, but I
request the change for superuser requirement for type creation be
reverted. The fact that no PL currently does it correctly is not a reason
to prohibit a PL from doing it correctly.

Kris Jurka

Kris Jurka wrote:
>
> 3) By value: pljava does not correctly handle passed by value types
> correctly, allowing access to random memory.
>
This is simply not true. There's no way a Java developer can access
random memory through PL/Java.

- thomas

Thomas Hallgren wrote:
> Kris Jurka wrote:
>>
>> 3) By value: pljava does not correctly handle passed by value types
>> correctly, allowing access to random memory.
>>
> This is simply not true. There's no way a Java developer can access
> random memory through PL/Java.

No, the point is that the Java developer can provide some data which can
convince postgresql to fetch random data for the user.

Consider the attached type which is simply an int4 equivalent.
Depending on how you define it as passed by value or passed by reference
it will or will not work (attached).

This looks like it works:

jurka=# select '1'::intbyref, '2'::intbyval;
intbyref | intbyval
----------+----------
1 | 2
(1 row)

But it doesn't really:

jurka=# create table inttest (a intbyref, b intbyval);
CREATE TABLE
jurka=# insert into inttest values ('1', '2');
INSERT 0 1
jurka=# select * from inttest;
a | b
---+------------
1 | 2139062143
(1 row)

You can also get:

jurka=# select * from inttest;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.

Kris Jurka

Kris Jurka wrote:
> Thomas Hallgren wrote:
>> Kris Jurka wrote:
>>>
>>> 3) By value: pljava does not correctly handle passed by value types
>>> correctly, allowing access to random memory.
>>>
>> This is simply not true. There's no way a Java developer can access
>> random memory through PL/Java.
>
> No, the point is that the Java developer can provide some data which
> can convince postgresql to fetch random data for the user.
>
> Consider the attached type which is simply an int4 equivalent.
> Depending on how you define it as passed by value or passed by
> reference it will or will not work (attached).
>
> This looks like it works:
>
>
> jurka=# select '1'::intbyref, '2'::intbyval;
> intbyref | intbyval
> ----------+----------
> 1 | 2
> (1 row)
>
> But it doesn't really:
>
> jurka=# create table inttest (a intbyref, b intbyval);
> CREATE TABLE
> jurka=# insert into inttest values ('1', '2');
> INSERT 0 1
> jurka=# select * from inttest;
> a | b
> ---+------------
> 1 | 2139062143
> (1 row)
It seems the pointer is confused for the actual value which means that
writing the value back will corrupt the pointer. That's bad of course
but I would classify this as a bug rather then a general security problem.

PL/Java is designed to do handle all types securely and completely hide
the concept of 'by value' or 'by reference' from the Java developer
since such concepts are meaningless in Java.

- thomas

On Wed, 18 Feb 2009, Kris Jurka wrote:

> I have reviewed pljava's handling of misrepresented alignment, length, and by
> value parameters [and it doesn't all work.]
>

I have fixed pljava to now correctly handle all of these being defined
incorrectly. So a trusted language can be used to create type input and
output functions safely. I think the restriction that only superusers can
create types should be reverted.

Kris Jurka