Subject: Should creating a new base type require superuser status?

From: Tom Lane <[email-masked]>
Date: Wed, 30 Jul 2008 15:50:13 -0400

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

---

From: Gregory Stark <[email-masked]>
Date: Wed, 30 Jul 2008 22:19:36 +0100

> 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.

> provide a trojan-horse vector for executing functions as some other

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!

---

From: Tom Lane <[email-masked]>
Date: Wed, 30 Jul 2008 17:29:48 -0400

> 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.

> 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.

> 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

---

From: Gregory Stark <[email-masked]>
Date: Wed, 30 Jul 2008 23:01:06 +0100

> 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!

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 30 Jul 2008 18:07:53 -0400

> 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.

---

From: Tom Lane <[email-masked]>
Date: Wed, 30 Jul 2008 18:13:10 -0400

> 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

---

From: Andrew Sullivan <[email-masked]>
Date: Thu, 31 Jul 2008 09:39:47 +0100

> actual hole (that we already know of).

I agree.

-- 
Andrew Sullivan
[email-masked]
+1 503 667 4564 x104
http://www.commandprompt.com/

---

From: Kris Jurka <[email-masked]>
Date: Thu, 31 Jul 2008 15:45:13 -0400 (EDT)

> 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

---

From: Tom Lane <[email-masked]>
Date: Thu, 31 Jul 2008 17:23:14 -0400

> 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

---

From: Thomas Hallgren <[email-masked]>
Date: Fri, 01 Aug 2008 22:43:53 +0200

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:
> _______________________________________________
> Pljava-dev mailing list
> [email-masked]
> http://pgfoundry.org/mailman/listinfo/pljava-dev
>

---

From: Tom Lane <[email-masked]>
Date: Fri, 01 Aug 2008 17:42:41 -0400

> 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

---

From: Andrew Gierth <[email-masked]>
Date: Sat, 02 Aug 2008 00:51:47 +0100

>>>>> "Tom" == Tom Lane <[email-masked]> 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)

---

From: Thomas Hallgren <[email-masked]>
Date: Sat, 02 Aug 2008 08:44:09 +0200

> 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.

> 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

---

From: Simon Riggs <[email-masked]>
Date: Sat, 02 Aug 2008 10:41:12 +0100

> > actual hole (that we already know of).

+1

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

---

From: Tom Lane <[email-masked]>
Date: Sat, 02 Aug 2008 12:12:50 -0400

Thomas Hallgren <[email-masked]> writes:
>> are different from what the underlying functions expect.

> reference and there's no way the PL/Java user can get direct access to 

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

> 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

---

From: Thomas Hallgren <[email-masked]>
Date: Sun, 03 Aug 2008 08:11:53 +0200

> typlen/typbyval/typalign info when creating a type based on PL/Java
Everything is converted into instances of Java classes such as String, 
byte[], etc.

> might not be true, but I don't think that worrying about it is without
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

---

From: Kris Jurka <[email-masked]>
Date: Wed, 18 Feb 2009 00:43:38 -0500 (EST)

> typlen/typbyval/typalign info when creating a type based on PL/Java

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

---

From: Thomas Hallgren <[email-masked]>
Date: Wed, 18 Feb 2009 07:07:11 +0100

> 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

---

From: Kris Jurka <[email-masked]>
Date: Tue, 17 Feb 2009 23:34:48 -0800

> 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
package types;

import java.io.IOException;
import java.sql.SQLData;
import java.sql.SQLException;
import java.sql.SQLInput;
import java.sql.SQLOutput;
import java.util.logging.Logger;

public class Int implements SQLData
{
	private static Logger s_logger = Logger.getAnonymousLogger();

	private int m_i;
	private String m_typeName;

	public static Int parse(String input, String typeName) throws SQLException
	{
		try
		{
			int i = Integer.parseInt(input);
			return new Int(i, typeName);
		}
		catch(NumberFormatException e)
		{
			throw new SQLException(e.getMessage());
		}
	}

	public Int()
	{
	}

	public Int(int i, String typeName)
	{
		m_i = i;
		m_typeName = typeName;
	}

	public String getSQLTypeName()
	{
		return m_typeName;
	}

	public void readSQL(SQLInput stream, String typeName) throws SQLException
	{
		s_logger.info(typeName + " from SQLInput");
		m_i = stream.readInt();
		m_typeName = typeName;
	}

	public void writeSQL(SQLOutput stream) throws SQLException
	{
		s_logger.info(m_typeName + " to SQLOutput");
		stream.writeInt(m_i);
	}

	public String toString()
	{
		s_logger.info(m_typeName + " toString");
		return Integer.toString(m_i);
	}

}
CREATE TYPE intbyval;

CREATE FUNCTION intbyval_in(cstring)
	RETURNS intbyval
	AS 'UDT[types.Int] input'
	LANGUAGE java IMMUTABLE STRICT;

CREATE FUNCTION intbyval_out(intbyval)
	RETURNS cstring
	AS 'UDT[types.Int] output'
	LANGUAGE java IMMUTABLE STRICT;

CREATE FUNCTION intbyval_recv(internal)
	RETURNS intbyval
	AS 'UDT[types.Int] receive'
	LANGUAGE java IMMUTABLE STRICT;

CREATE FUNCTION intbyval_send(intbyval)
	RETURNS bytea
	AS 'UDT[types.Int] send'
	LANGUAGE java IMMUTABLE STRICT;

CREATE TYPE intbyval (
	internallength = 4,
	input = intbyval_in,
	output = intbyval_out,
	receive = intbyval_recv,
	send = intbyval_send,
	passedbyvalue
	);

CREATE TYPE intbyref;

CREATE FUNCTION intbyref_in(cstring)
	RETURNS intbyref
	AS 'UDT[types.Int] input'
	LANGUAGE java IMMUTABLE STRICT;

CREATE FUNCTION intbyref_out(intbyref)
	RETURNS cstring
	AS 'UDT[types.Int] output'
	LANGUAGE java IMMUTABLE STRICT;

CREATE FUNCTION intbyref_recv(internal)
	RETURNS intbyref
	AS 'UDT[types.Int] receive'
	LANGUAGE java IMMUTABLE STRICT;

CREATE FUNCTION intbyref_send(intbyref)
	RETURNS bytea
	AS 'UDT[types.Int] send'
	LANGUAGE java IMMUTABLE STRICT;

CREATE TYPE intbyref (
	internallength = 4,
	input = intbyref_in,
	output = intbyref_out,
	receive = intbyref_recv,
	send = intbyref_send
	);

---

From: Thomas Hallgren <[email-masked]>
Date: Wed, 18 Feb 2009 12:23:07 +0100

Kris Jurka wrote:
> No, the point is that the Java developer can provide some data which 
> can convince postgresql to fetch random data for the user.
>
> Depending on how you define it as passed by value or passed by 
> reference it will or will not work (attached).
>
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

---

From: Kris Jurka <[email-masked]>
Date: Fri, 20 Feb 2009 16:19:11 -0500 (EST)

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