Re: import/export of large objects on server-side
-------- Ursprüngliche Nachricht --------
Betreff: Re: [HACKERS] import/export of large objects on server-side
Von: "Klaus Reger" <K.Reger@twc.de>
An: <tgl@sss.pgh.pa.us>
Use the client-side LO import/export functions, instead.
ok, i've read the config.h and the sources. I agree that this can be a
security hole. But for our application we need lo-access from
PL/PGSQL-Procedures (explicitly on the server). We have to check out
documents, work with them and then check the next version in.Whats about an configuration-file entry, in the matter
LO_DIR=/directory or none (which is the default).
For our product we want to be compatible with the original sources of Pg,
avoiding own patches in every new version.
Hi,
I've made a patch, that introduces an entry in the PostgreSQL-config file.
You can set a drirectory, where all imports/exports can happen. If nothing
is set (the default), no imports/exports on the server-side are allowed.
To enhance the security, no reading/writung is allowed from/to non-regular
files (block-devs, symlinks, etc.)
I hope, that this patch is secure enough and will be integrated.
Regards, Klaus
Attachments:
lo_imp_exp.diffapplication/octet-stream; name=lo_imp_exp.diffDownload
? src/include/stamp-h
Index: doc/src/sgml/runtime.sgml
===================================================================
RCS file: /projects/cvsroot/pgsql/doc/src/sgml/runtime.sgml,v
retrieving revision 1.94
diff -w -b -i -B -u -r1.94 runtime.sgml
--- doc/src/sgml/runtime.sgml 2001/11/12 19:19:39 1.94
+++ doc/src/sgml/runtime.sgml 2001/11/16 11:03:05
@@ -1148,6 +1148,48 @@
</varlistentry>
<varlistentry>
+ <term><varname>LO_IMP_EXP_DIR</varname> (<type>string</type>)</term>
+ <indexterm><primary>LO_IMP_EXP_DIR</></>
+ <indexterm><primary>large objects</></>
+ <listitem>
+ <para>
+ Importing and exporting large objects on the server-side is
+ always a security-problem, because it may allow to read and
+ write files on the server. But in some cases it is neccesary
+ to handle large objects on the server, e.g. when this is done
+ in stored procedures.
+ </para>
+
+ <para>
+ To minimize the risk, the administrator may pass a directory
+ as parameter, which is the root of all of the files, that
+ can be im- or exported. The passed argument MUST be a directory.
+ To enhance the security, the files to import from or to export
+ to must be regular files. Any other types (directories,
+ block-devices, symbolic-links, etc.) are rejected by the server.
+
+ <informalexample>
+<programlisting>
+lo_imp_exp_dir = '/tmp'
+</programlisting>
+ </informalexample>
+ </para>
+
+ <para>
+ The default value for this parameter is
+ <literal>'' (empty string)</literal>. If the value is set to the empty
+ string, importing or exporting is forbidden on server-side.
+ </para>
+
+ <para>
+ This parameter can be changed in the
+ <filename>postgresql.conf</filename>
+ configuration file.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<indexterm>
<primary>fsync</primary>
</indexterm>
Index: src/backend/libpq/be-fsstubs.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/libpq/be-fsstubs.c,v
retrieving revision 1.59
diff -w -b -i -B -u -r1.59 be-fsstubs.c
--- src/backend/libpq/be-fsstubs.c 2001/06/13 21:44:41 1.59
+++ src/backend/libpq/be-fsstubs.c 2001/11/16 11:03:06
@@ -38,6 +38,7 @@
#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>
+#include <errno.h>
#include "libpq/be-fsstubs.h"
#include "libpq/libpq-fs.h"
@@ -47,6 +48,7 @@
/* [PA] is Pascal Andr� <andre@via.ecp.fr> */
+/* [KR] is Klaus Reger <k.reger@wwwdb.org> */
/*#define FSDB 1*/
#define BUFSIZE 8192
@@ -64,10 +66,16 @@
static MemoryContext fscxt = NULL;
+#define IE_IMPORT 1
+#define IE_EXPORT 2
static int newLOfd(LargeObjectDesc *lobjCookie);
static void deleteLOfd(int fd);
+static int isImpExpOK(char *filename, int ie_type);
+/* externally defined via config-file */
+char *lo_imp_exp_dir;
+
/*****************************************************************************
* File Interfaces for Large Objects
*****************************************************************************/
@@ -359,13 +367,6 @@
LargeObjectDesc *lobj;
Oid lobjOid;
-#ifndef ALLOW_DANGEROUS_LO_FUNCTIONS
- if (!superuser())
- elog(ERROR, "You must have Postgres superuser privilege to use "
- "server-side lo_import().\n\tAnyone can use the "
- "client-side lo_import() provided by libpq.");
-#endif
-
/*
* open the file to be read in
*/
@@ -374,6 +375,16 @@
nbytes = MAXPGPATH - 1;
memcpy(fnamebuf, VARDATA(filename), nbytes);
fnamebuf[nbytes] = '\0';
+
+#ifndef ALLOW_DANGEROUS_LO_FUNCTIONS
+
+ if (!superuser() && !isImpExpOK(fnamebuf, IE_IMPORT))
+ elog(ERROR, "You must have Postgres superuser privilege to use "
+ "server-side lo_import().\n\tAnyone can use the "
+ "client-side lo_import() provided by libpq.");
+#endif
+
+
fd = PathNameOpenFile(fnamebuf, O_RDONLY | PG_BINARY, 0666);
if (fd < 0)
elog(ERROR, "lo_import: can't open unix file \"%s\": %m",
@@ -422,8 +433,16 @@
LargeObjectDesc *lobj;
mode_t oumask;
+
+ nbytes = VARSIZE(filename) - VARHDRSZ;
+ if (nbytes >= MAXPGPATH)
+ nbytes = MAXPGPATH - 1;
+ memcpy(fnamebuf, VARDATA(filename), nbytes);
+ fnamebuf[nbytes] = '\0';
+
#ifndef ALLOW_DANGEROUS_LO_FUNCTIONS
- if (!superuser())
+
+ if (!superuser() && !isImpExpOK(fnamebuf, IE_EXPORT))
elog(ERROR, "You must have Postgres superuser privilege to use "
"server-side lo_export().\n\tAnyone can use the "
"client-side lo_export() provided by libpq.");
@@ -443,11 +462,6 @@
* 022. This code used to drop it all the way to 0, but creating
* world-writable export files doesn't seem wise.
*/
- nbytes = VARSIZE(filename) - VARHDRSZ;
- if (nbytes >= MAXPGPATH)
- nbytes = MAXPGPATH - 1;
- memcpy(fnamebuf, VARDATA(filename), nbytes);
- fnamebuf[nbytes] = '\0';
oumask = umask((mode_t) 0022);
fd = PathNameOpenFile(fnamebuf, O_CREAT | O_WRONLY | O_TRUNC | PG_BINARY, 0666);
umask(oumask);
@@ -566,3 +580,63 @@
{
cookies[fd] = NULL;
}
+
+
+
+/*
+ * isImpExpOK -
+ * checks if file matches the criteria for import/export [KR, 2001-11-15]
+ */
+static int
+isImpExpOK(char *filename,
+ int ie_type)
+{
+ struct stat file_stat;
+ int result = 0;
+ int stat_result;
+
+ /*
+ * When lo_imp_exp_dir is set explicitly in the config-file, then
+ * we are allowed to do this dangerous function
+ */
+ if((strncmp(lo_imp_exp_dir,
+ filename,
+ strlen(lo_imp_exp_dir)) == 0) &&
+ (strlen(lo_imp_exp_dir) > 0))
+ result = 1;
+
+ /*
+ * lo_imp_exp_dir has to be a directory
+ */
+ stat_result = lstat(lo_imp_exp_dir, &file_stat);
+ if(!S_ISDIR(file_stat.st_mode))
+ {
+ result = 0;
+#if FSDB
+ elog(NOTICE, "lo_imp_exp_dir is not a dir");
+#endif
+ }
+
+ /*
+ * filename has to be a regular file
+ */
+ stat_result = lstat(filename, &file_stat);
+ if(!S_ISREG(file_stat.st_mode) || S_ISLNK(file_stat.st_mode))
+ {
+
+ /*
+ * if we want to create a new file, but it does not exist,
+ * everything is OK (for export only)
+ */
+ if(!(ie_type == IE_EXPORT && stat_result != 0 && errno == ENOENT))
+ {
+ result = 0;
+#if FSDB
+ elog(NOTICE, "%s is not a regular file", filename);
+#endif
+ }
+ }
+
+ return result;
+}
+
Index: src/backend/utils/misc/guc.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/misc/guc.c,v
retrieving revision 1.58
diff -w -b -i -B -u -r1.58 guc.c
--- src/backend/utils/misc/guc.c 2001/10/30 05:38:56 1.58
+++ src/backend/utils/misc/guc.c 2001/11/16 11:03:09
@@ -25,6 +25,7 @@
#include "fmgr.h"
#include "libpq/auth.h"
#include "libpq/pqcomm.h"
+#include "libpq/be-fsstubs.h"
#include "miscadmin.h"
#include "optimizer/cost.h"
#include "optimizer/geqo.h"
@@ -556,6 +557,11 @@
{
"dynamic_library_path", PGC_SUSET, &Dynamic_library_path,
"$libdir", NULL, NULL
+ },
+
+ {
+ "lo_imp_exp_dir", PGC_POSTMASTER, &lo_imp_exp_dir,
+ "", NULL, NULL
},
{
Index: src/backend/utils/misc/postgresql.conf.sample
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/misc/postgresql.conf.sample,v
retrieving revision 1.26
diff -w -b -i -B -u -r1.26 postgresql.conf.sample
--- src/backend/utils/misc/postgresql.conf.sample 2001/09/30 18:57:45 1.26
+++ src/backend/utils/misc/postgresql.conf.sample 2001/11/16 11:03:09
@@ -174,6 +174,7 @@
#
# Misc
#
+#lo_imp_exp_dir = '' # absolute dirname or ''
#dynamic_library_path = '$libdir'
#australian_timezones = false
#authentication_timeout = 60 # min 1, max 600
Index: src/include/libpq/be-fsstubs.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/libpq/be-fsstubs.h,v
retrieving revision 1.15
diff -w -b -i -B -u -r1.15 be-fsstubs.h
--- src/include/libpq/be-fsstubs.h 2001/11/05 17:46:33 1.15
+++ src/include/libpq/be-fsstubs.h 2001/11/16 11:03:12
@@ -47,4 +47,6 @@
*/
extern void lo_commit(bool isCommit);
+extern char *lo_imp_exp_dir;
+
#endif /* BE_FSSTUBS_H */
"Klaus Reger" <K.Reger@twc.de> writes:
I've made a patch, that introduces an entry in the PostgreSQL-config file.
You can set a drirectory, where all imports/exports can happen. If nothing
is set (the default), no imports/exports on the server-side are allowed.
To enhance the security, no reading/writung is allowed from/to non-regular
files (block-devs, symlinks, etc.)
This is trivially defeatable, assuming that the "import/export"
directory is world writable (if it isn't, importing will be tough).
Example: say imp/exp directory is
/var/spool/impexp
Bad guy wants to read/write Postgres-owned file, say
/usr/local/pgsql/data/pg_hba.conf
All he need do is
ln -s /usr/local/pgsql/data /var/spool/impexp/link
and then ask to lo_read or lo_write
/var/spool/impexp/link/pg_hba.conf
which will be allowed since it's a regular file.
Or, even simpler, ask to read/write
/var/spool/impexp/../../../usr/local/pgsql/data/pg_hba.conf
While you could patch around these particular attacks by further
restricting the filenames, the bottom line is that server-side LO
operations are just inherently insecure.
regards, tom lane
"Klaus Reger" <K.Reger@twc.de> writes:
I've made a patch, that introduces an entry in the PostgreSQL-config
file. You can set a drirectory, where all imports/exports can happen.
If nothing is set (the default), no imports/exports on the server-side
are allowed. To enhance the security, no reading/writung is allowed
from/to non-regular files (block-devs, symlinks, etc.)This is trivially defeatable, assuming that the "import/export"
directory is world writable (if it isn't, importing will be tough).
...
While you could patch around these particular attacks by further
restricting the filenames, the bottom line is that server-side LO
operations are just inherently insecure.regards, tom lane
Ok, you're right, but is it acceptable, to configure this, using the
configfile, rather than with a compile-option?
Regards, Klaus
"Klaus Reger" <K.Reger@twc.de> writes:
Ok, you're right, but is it acceptable, to configure this, using the
configfile, rather than with a compile-option?
The patch as given isn't any more secure than just enabling
ALLOW_DANGEROUS_LO_FUNCTIONS, so I for one will vote against
applying it.
I'm still unconvinced that there's any need to create a server-side LO
import/export loophole. Client-side LO operations are inherently safer,
and that's the direction you should be looking in for a solution.
regards, tom lane
On Fri, Nov 16, 2001 at 05:02:13PM +0100, Klaus Reger wrote:
"Klaus Reger" <K.Reger@twc.de> writes:
I've made a patch, that introduces an entry in the PostgreSQL-config
file. You can set a drirectory, where all imports/exports can happen.
If nothing is set (the default), no imports/exports on the server-side
are allowed. To enhance the security, no reading/writung is allowed
from/to non-regular files (block-devs, symlinks, etc.)This is trivially defeatable, assuming that the "import/export"
directory is world writable (if it isn't, importing will be tough)....
While you could patch around these particular attacks by further
restricting the filenames, the bottom line is that server-side LO
operations are just inherently insecure.regards, tom lane
Ok, you're right, but is it acceptable, to configure this, using the
configfile, rather than with a compile-option?
You can always use client-site LO operations without this restriction.
IMHO server-site LO operations is needless and a little dirty feature.
May by add to our privilege system support for LO operations too. But
our current privilege system is very inflexible for changes1...
Karel
--
Karel Zak <zakkr@zf.jcu.cz>
http://home.zf.jcu.cz/~zakkr/
C, PostgreSQL, PHP, WWW, http://docs.linux.cz, http://mape.jcu.cz