Re: [HACKERS] Proposed Patch - LDAPS support for servers on port 636 w/o TLS
Thank you all for your comments. I was unaware the ldaps: scheme was not supposed to be used for LDAP+TLS encryption, but it makes sense now that you mention it. There's a nice discussion about how the folks working on mod_ldap for Apache worked this out way back in 2005: http://mail-archives.apache.org/mod_mbox/httpd-dev/200501.mbox/[EMAIL PROTECTED] Anyway, I think we've distilled the issue down to how to best enable TLS for ldap:// connections. By my reckoning, that means we can have: 1) per-hba.conf entry configuration where the configuration can be: a) of the ldap URL extension form mentioned by David (!StartTLS). b) key=value type of param string as suggested by Magnus c) a specific URI scheme like ldap+tls:// like Tom suggested. d) a new authentication type ldaptls 2) per-postgres server configuration which can be: a) an old LDAPTLS environment variable ? needs research b) a server-wide GUC variable (along with TLSCERT specifications?) as in the current patch I'm open to other suggestions. One other thing to keep in mind is how best to map database roles to ldap Distinguished Name (dn) entries? In other words, we need to take the user jimmy in psql -U jimmy and translate into an ldap authentication request for the distinguished name that is entirely dependent on the site and ldap impl, example: uid=jimmy,ou=people,dc=example,dc=com I've racked my brain thinking of ways that this can fit cleanly in hba.conf, but I haven't found anything I _really_ like (current patch and proposal 3 below are prob my favorites.) Any other ideas/comments/suggestions? # Current Functionality for reference - no tls control hostdbname all 127.0.0.0/32ldap "ldap://ldap.example.com[:port]/ignored;uid=;ou=people,dc=example,dc=com"; # Current Functionality in patch (w/ server wide TLS control in GUC var) # GUC var causes all ldap entries to use same authentication. can be # applied to service lookup as well hostdbname all 127.0.0.0/32ldap "ldap://ldap.example.com[:port]/ou=people,dc=example,dc=com;uid="; # proposal 1 - RFC 2255 URI kind of yucky; scope, attributes, filter # not actually used in simple authentication hostdbname all 127.0.0.0/32ldap "ldap://ldap.example.com[:port]/uid=%u,ou=people,dc=example,dc=com???!StartTLS"; # proposal 1b - still RFC 2255 compliant, but semantically weird. no # filter is actually used in simple authentication hostdbname all 127.0.0.0/32ldap "ldap://ldap.example.com[:port]/ou=people,dc=example,dc=com?one??(uid=%u)!StartTLS" # proposal 2 - psuedo-URI scheme; hacky but easy hostdbname all 127.0.0.0/32ldap "ldap+tls://ldap.example.com[:port]/ou=people,dc=example,dc=com;uid=;" # proposal 3 - mod hba parsing, add new ldaptls auth type; reasonably # easy and least invasive; hostdbname all 127.0.0.0/32ldaptls "ldap://ldap.example.com[:port]/ou=people,dc=example,dc=com;uid=;"; # proposal 4 - mod hba parsing hostdbname all 127.0.0.0/32ldap "ldap://ldap.example.com[:port]/ou=people,dc=example,dc=com;uid=;"; StartTLS # proposal 5 - Magnum's key = value like idea (i'm guessing here, # Magnum. If I misinterpret, please explain) hostdbname all 127.0.0.0/32ldap "ldap://ldap.example.com[:port]/ou=people,dc=example,dc=com;prefix=uid=;start_tls=1"; I have some radical ideas as well involving completely ripping out the pg_hba.conf file but I'll leave that for another, more appropriate day. :) Thanks again for the feedback, and sorry for the verbosity. -Steve (#postgresql rockpunk) signature.asc Description: Digital signature
Re: [HACKERS] [0/4] Proposal of SE-PostgreSQL patches
Josh Berkus <[EMAIL PROTECTED]> writes: > For hackers who don't understand security frameworks, I'm going to make a > strong case for KaiGai's patch. > ... > So it would be much better to have this functionality be "mainstream" > rather than a fork. If it does get bounced, please do it becuase of code > quality and not because "nobody is asking for this". Well, let's be perfectly blunt here: this is an extremely sizable patch that will be of interest to less than 1% of our users (and I think I'm being generous in that estimate). If it's going to get in, it's going to have to not cost us anything much on reliability, maintainability, or performance --- rank those however you please, there is someone out there who will be most interested in any one of them. My look at the code today convinced me that it's not going to get applied without a whole lot of further work. I think what we must figure out in this commit-fest is whether to encourage KaiGai to start doing that work, or to decide that it probably is a dead issue and he shouldn't bother. As to reliability: the issue that worries me the most was already raised by Greg Smith --- this patch puts security checks and consequent catalog lookups into some mighty low-level places that IMHO have no business triggering catalog accesses. If I'd been able to make the patch work today, I'd have tested whether it could get through the regression tests with CLOBBER_CACHE_ALWAYS defined. I'd be happier if it were refactored to put the security checks only into executor code paths, and not try to enforce security restrictions against the system itself. (Thought experiment: what happens if SELinux denies access to a row of pg_security to the code that is supposed to be checking security? Or try this one: it looks to me like you can set up the system to pretend to some user that the pg_class rows for certain indexes don't exist, even though he has update rights on their parent table. Instant index corruption.) As to maintainability: I already posted a lot of unhappy remarks on code style and readability. I don't think there's anything unfixable there, but there's a lot of work to do to convert what was evidently written as an arms-length add-on into a sensible part of core Postgres. As to performance: frankly, I'm afraid the performance is abysmal. This was what I was mainly hoping to check out if I could have gotten it to build today. We need to at least get some rough pgbench readings before deciding whether it's worth pushing forward. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposed patch - psql wraps at window width
Tom Lane wrote: > Bruce Momjian <[EMAIL PROTECTED]> writes: > > Are others OK with $COLUMNS controlling screen output and file/pipe, or > > perhaps COLUMNS controlling only file/pipe, as GNU ls does? I have > > heard a few people say they only want \pset columns to control > > file/pipe. > > I agree with the latter. Anyone who is setting COLUMNS is going to set > it to reflect the *screen* width they want; it has zero to do with > what should happen for output going somewhere else than the screen. OK, that's what I thought, and what I think Peter wants, and what the posted patch does. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposed patch - psql wraps at window width
Bruce Momjian wrote: > Gregory Stark wrote: > > "Bruce Momjian" <[EMAIL PROTECTED]> writes: > > > > > OK, so COLUMNS should take precedence. I assume this is going to > > > require us to read the COLUMNS enviroment variable in psql _before_ > > > readline sets it, and that COLUMNS will only affect screen output, like > > > ioctl(). Is that consistent? > > > > What are you talking about? "screen output"? > > > > Are you even getting my emails? Please confirm that you receive this email. > > Yes, I am getting your emails, but I have more than you to please. > > Are others OK with $COLUMNS controlling screen output and file/pipe, or > perhaps COLUMNS controlling only file/pipe, as GNU ls does? I have > heard a few people say they only want \pset columns to control > file/pipe. > > I have outlined the GNU ls behavior in a email I just sent. I talked to Greg on IM and he had a new idea, related to my 'auto' idea of using aligned/wrapped/expanded mode automatically. His idea is that 'auto' mode (aligned/wrapped/expanded) is only for output to the screen, and 'wrapped' is always wrapped. 'Wrapped' would be affected by \pset columns, COLUMNS, or the terminal width for output to screen, file, or pipe. If no width could be derrmined for wrapped, it would default to 72. This gives us 'wrapped' that always wraps (if it can fit the headings), and most people will use 'auto' in their psqlrc, and it affects only terminal output. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposed patch - psql wraps at window width
Bruce Momjian <[EMAIL PROTECTED]> writes: > Are others OK with $COLUMNS controlling screen output and file/pipe, or > perhaps COLUMNS controlling only file/pipe, as GNU ls does? I have > heard a few people say they only want \pset columns to control > file/pipe. I agree with the latter. Anyone who is setting COLUMNS is going to set it to reflect the *screen* width they want; it has zero to do with what should happen for output going somewhere else than the screen. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposed patch - psql wraps at window width
Gregory Stark wrote: > "Bruce Momjian" <[EMAIL PROTECTED]> writes: > > > OK, so COLUMNS should take precedence. I assume this is going to > > require us to read the COLUMNS enviroment variable in psql _before_ > > readline sets it, and that COLUMNS will only affect screen output, like > > ioctl(). Is that consistent? > > What are you talking about? "screen output"? > > Are you even getting my emails? Please confirm that you receive this email. Yes, I am getting your emails, but I have more than you to please. Are others OK with $COLUMNS controlling screen output and file/pipe, or perhaps COLUMNS controlling only file/pipe, as GNU ls does? I have heard a few people say they only want \pset columns to control file/pipe. I have outlined the GNU ls behavior in a email I just sent. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposed patch - psql wraps at window width
Bryce Nesbitt wrote: > > > OK, so COLUMNS should take precedence. I assume this is going to > > require us to read the COLUMNS environment variable in psql _before_ > > readline sets it, and that COLUMNS will only affect screen output, like > > ioctl(). Is that consistent? > > > This whole thing is confusing enough at the point, I think a complete > proposal needs to be articulated. It is hard to comment on a fragment of > an idea. > > The main cases to cover are: (1) how to specify wrap for tty's In order of precedence: \pset columns $COLUMNS iotcl() > (2) how to specify wrap for pipes \pset columns > (3) how to get wrapped on platforms that don't > have the ioctl (presumably windows without cygwin) \pset columns $COLUMNS > (4) how to set up > different defaults for tty's and pipes (e.g. wrap interactive tty's, but > leave output aligned for scripts). > > And perhaps, as a bonus comment on (5) the idea of having psql NOT > source .psqlrc -X --no-psqlrc > I hope at some point someone will actually try the actual core wrapping > code, and comment on it. I tested it and it worked well once I modified it. Updated patch with clearer documentation that matches the above behavior: ftp://momjian.us/pub/postgresql/mypatches/wrap FYI, I looked into 'ls -C' hanlding a little more and ls (GNU coreutils) 5.97 honors COLUMNS _only_ in file/pipe output, not for screen output. What the C code does is to read COLUMNS, then overwrite that value with ioctl() if it works. Do we want to follow that behavior? ls has a '-w' option to specify the width, like our \pset columns. However, the manual page seems backwards: -w, --width=COLS assume screen width instead of current value The GNU 'ls' manual does not mention COLUMNS. BSD 'ls' used COLUMNS only when outputing to the screen, and ioctl fails. However, the BSD manual says: COLUMNSIf this variable contains a string representing a decimal integer, it is used as the column position width for displaying multiple-text-column output. The ls utility calculates how many pathname text columns to display based on the width pro- vided. (See -C.) Again, I think the manual is wrong. So it seem GNU ls and BSD ls are inconsistent, which I think means we should design our API the best we can, rather than rely on how others interpret COLUMNS. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [0/4] Proposal of SE-PostgreSQL patches
On Mon, 5 May 2008, Tom Lane wrote: elog() should not be used for user-facing errors. I couldn't easily tell just which of the messages are likely to be seen by users and which ones should be "can't happen" cases, but certainly there are a whole lot of these that need to be ereport()s. Likely there need to be some new ERRCODEs too. And it would be a nice step toward the scenarios I was asking about if there was a GUC variable for what level to log security violations at. I realize now the tuple-level warnings are going into the SELinux logs rather than the PostgreSQL ones, but it should be easier to change policy violations that impact the server to something other than just ERROR. -- * Greg Smith [EMAIL PROTECTED] http://www.gregsmith.com Baltimore, MD -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [0/4] Proposal of SE-PostgreSQL patches
KaiGai Kohei <[EMAIL PROTECTED]> writes: > I updated the series of SE-PostgreSQL patches for the latest pgsql-8.4devel > tree. I tried to do a bit of testing of this, but it does not work on current Fedora 8, because the policy module doesn't build: [EMAIL PROTECTED] sepgsql-policy]$ make make -f /usr/share/selinux/devel/Makefile NAME=targeted make[1]: Entering directory `/home/tgl/sepgsql/contrib/sepgsql-policy' Compiling targeted sepostgresql module /usr/bin/checkmodule: loading policy configuration from tmp/sepostgresql.tmp sepostgresql.te:349:ERROR 'syntax error' at token 'corenet_tcp_recvfrom_labeled' on line 5675: # NOTE: These changes are to be merged in the later releases. corenet_tcp_recvfrom_labeled(sepgsql_server_type, sepgsql_client_type) /usr/bin/checkmodule: error(s) encountered while parsing configuration make[1]: *** [tmp/sepostgresql.mod] Error 1 make[1]: Leaving directory `/home/tgl/sepgsql/contrib/sepgsql-policy' make: *** [sepostgresql.pp] Error 2 [EMAIL PROTECTED] sepgsql-policy]$ In the meantime, here are some random comments after my failed test build and a very fast scan through the patch: The patch tries to re-add ipcclean to the source tree --- probably a merge error? autoconf complains about the description-free AC_DEFINEs Doesn't compile warning-free if selinux not enabled ... for that matter, it doesn't compile warning-free if selinux IS enabled. No documentation updates whatsoever :-( About half of the patch seems to be conditional on #ifdef SECURITY_SYSATTR_NAME and the other half on #ifdef HAVE_SELINUX This seems bizarre: is there really any chance that there are two independently usable chunks of code here? And why is it conditional on a macro that is a field name, rather than conditional on a feature macro? That is, I'd expect to see something like #ifdef ENABLE_SEPOSTGRES throughout. For that matter, what is the point of treating SECURITY_SYSATTR_NAME as a configurable thing in the first place? I can hardly imagine a worse idea than a security-critical column having different names in different installations. The patch hasn't got a mode in which SELinux support is compiled in but not active. This is a good way to ensure that no one will ever ship standard RPMs with the feature compiled in, because they will be entirely nonfunctional for people who aren't interested in setting up SELinux. I think you need an "enable_sepostgres" GUC, or something like that. (Of course, the overhead of the per-row security column would probably discourage anyone from wanting to use such a configuration anyway, so maybe the point is moot.) sepgsql-policy has got usability problems: * It should pay attention to the configured installation PREFIX instead of hardwiring a couple of random possible installation locations * It can only support the build machine's SELINUXTYPE --- how am I supposed to produce RPMs that support all available types? The contents and use of sepgsqlGetTupleName() make it look like the entire security scheme is based on object name alone. That doesn't even account for schemas, let alone overloaded function names. Please tell me this is not as broken-by-design as it looks. I occasionally tell people "try to make the patch look like it's always been there". This is pretty far from meeting that goal. Random bits of code that are commented "PGACE:" are obviously not intended to just fit in. You've generally ignored the Postgres code layout conventions (pgindent will help to some extent but it's far from a panacea) and our commenting conventions --- eg, hardly any of the functions have header comments, and the ones that do follow conventions seen noplace in the Postgres code, like using "@" on parameter names. In general the number and quality of the comments is far below the standard for Postgres code, and the lack of any implementation documentation isn't helping. Another big problem, which I understand your motivation for but that doesn't make the code any less ugly, is that you've got trivial bits of code that're separated by two(!) levels of hook calls from where they're actually being used. Not only does that make it unreadable but the files that actually do the work combine bits of code that should be scattered across a lot of modules, causing those files to be just horrid from a modularity standpoint --- they've got their fingers stuck in everyplace. If you want this to get applied you need to start thinking of it as an integral part of the code, not an add-on. Some other bits of add-on-itis: * If you need a dedicated LWLock, declare it in lwlock.h. * If you need a node type, declare it in nodes.h (T_SEvalItem is utterly broken) Why in the world would you have security restrictions associated with TOAST tuples? Seems like all the interesting restrictions should be on the parent table. Don't randomly invent your own style of management of a postmaster child process. For one thing, this code doesn't cope with
Re: [HACKERS] Proposed patch - psql wraps at window width
"Bruce Momjian" <[EMAIL PROTECTED]> writes: > OK, so COLUMNS should take precedence. I assume this is going to > require us to read the COLUMNS enviroment variable in psql _before_ > readline sets it, and that COLUMNS will only affect screen output, like > ioctl(). Is that consistent? What are you talking about? "screen output"? Are you even getting my emails? Please confirm that you receive this email. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's On-Demand Production Tuning -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Protection from SQL injection
-BEGIN PGP SIGNED MESSAGE- Hash: RIPEMD160 > How often do people code comments into prepare statements in perl > or the equivalent in java, ruby, etc? > > Do you put comments in your perl prepare statements? Does it matter? It shouldn't. They are comments. > If comments count as a statement, at the server end, then the > multi-statement disabling also disables another attack vector - > slightly: you can no longer attack using this as your username: > "' OR 1=1;--" Using placeholders and other best practices removes such attacks completely. I mostly agree with some other people in this thread that the 'disable multi-line switch' is marginally useful at best, and provides a false sense of security. But let's not confuse the issue with examples like the above. Otherwise I'll point out yet again that this whole things a solution in search of a problem. Poorly written apps will remain poorly written apps, no matter what server-side bandaids we try to apply. - -- Greg Sabino Mullane [EMAIL PROTECTED] PGP Key: 0x14964AC8 200805051559 http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8 -BEGIN PGP SIGNATURE- iEYEAREDAAYFAkgfZzcACgkQvJuQZxSWSsjAoACg6UKhb2r94khikeOfT2cUOGhD vh0AoIY/8dSH4tkmsLxl2Jkpbn7/u3+4 =hGCo -END PGP SIGNATURE- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pgstat SRF?
Magnus Hagander wrote: > Magnus Hagander wrote: > > Tom Lane wrote: > > > Magnus Hagander <[EMAIL PROTECTED]> writes: > > > > While looking over the statistics-for-functions patch > > > > (http://archives.postgresql.org/pgsql-patches/2008-03/msg00300.php), > > > > I came back to a thought I've had before - why do we keep one > > > > function per column for pgstat functions, instead of using a set > > > > returning function? Is there some actual reason for this, or is > > > > it just legacy from a time when it was (much) harder to write > > > > SRFs? > > > > > > I think it's so that you can build your own stats views instead of > > > being compelled to select the data someone thought was good for > > > you. > > > > You can still do that if it's an SRF. You could even make the SRF > > take an optional argument to either return a single value (filtered > > the same way the individual functions are) or when this one is set > > to NULL, return the whole table. > > > > It would make the overhead a lot lower in the most common case > > ("SELECT > > * FROM pg_stat_"), while only adding a little in > > the other cases, I think. > > > > Though I'm not sure that overhead is big enough to care about in the > > first place, but if you're VIEWs are longish it could be... > > Actually, looking at this once more, the interface to the functions > sucked more than I thought. They're not actually accepting procpid as > parameters, but just an index into the current array in pgstats.. > Basically, they're not supposed to be used in any other way than > accessing all the rows at once :-) > > Attached is a version of the functions required for pg_stat_activity > implemented as a SRF instead of different functions. A quick benchmark > (grabbing the VIEW 10,000 times on a system with about 500 active > backends) shows it's about 20% faster than the function-per-value > approach, but the runtime per view is still very quick as it is today. > (And most of what overhead there is most likely comes from reading the > stats file) > > However, it also implements the lookup-by-PID functionality that IMHO > makes a lot more sense than lookup-by-backend-array-index. This is > obviously a lot more performant than querying the VIEW for all rows - > something that might be a big win for monitoring apps that look for > info about a single backend. > > Unsure if we want to go ahead and convert all functions, but I think > we can make a good argument for making *new* stats views (like the > ones about functions that in the queue) based on SRFs instead. It > also has the nice side-effect of less rows in the system tables ;) > > Comments? Unless there are any objections I'll complete this patch along with the proper documentation, and apply it for now. I will look at converting some further pgstats stuff into SRFs as well, assuming they show similar results.. //Magnus -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Exposing quals
Folks, I'd like to make it possible to see qualifiers from inside functions in PostgreSQL. For my particular case, and for dblink, it would allow user-space code to some remove bottlenecks which make currently make inter-DBMS communication extremely inefficient. The current code returns one (potentially) big string with the quals ANDed together. I'm thinking that the new code would: 1. Return just a pointer. 2. Add some functions like rsinfo_get_node_tree_str() which can turn same into (in this case, a string, but others might get tree structures) on demand from client code. 3. Add wrapper functions in each untrusted PL (trusted PLs shouldn't be doing anything that needs this). 4. Document the above. Please find attached the patch, and thanks to Neil Conway and Korry Douglas for the code, and to Jan Wieck for helping me hammer out the scheme above. Mistakes are all mine ;) Comments? Questions? Cheers, David. -- David Fetter <[EMAIL PROTECTED]> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: [EMAIL PROTECTED] Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate Index: contrib/dblink/dblink.c === RCS file: /projects/cvsroot/pgsql/contrib/dblink/dblink.c,v retrieving revision 1.73 diff -c -c -r1.73 dblink.c *** contrib/dblink/dblink.c 4 Apr 2008 17:02:56 - 1.73 --- contrib/dblink/dblink.c 8 Apr 2008 18:29:05 - *** *** 752,757 --- 752,758 char *conname = NULL; remoteConn *rconn = NULL; boolfail = true;/* default to backward compatible */ + ReturnSetInfo *rsi; /* create a function context for cross-call persistence */ funcctx = SRF_FIRSTCALL_INIT(); *** *** 826,831 --- 827,843 elog(ERROR, "wrong number of arguments"); } + if (sql && rsi->qual) + { + char *quals = rsinfo_get_qual_str(rsi); + char *qualifiedQuery = palloc(strlen(sql) + strlen(" WHERE ") + + strlen(quals) + 1); + + sprintf(qualifiedQuery, "%s WHERE %s", sql, quals); + + sql = qualifiedQuery; + } + if (!conn) DBLINK_CONN_NOT_AVAIL; Index: src/backend/executor/execQual.c === RCS file: /projects/cvsroot/pgsql/src/backend/executor/execQual.c,v retrieving revision 1.228 diff -c -c -r1.228 execQual.c *** src/backend/executor/execQual.c 25 Mar 2008 22:42:43 - 1.228 --- src/backend/executor/execQual.c 8 Apr 2008 18:29:06 - *** *** 45,50 --- 45,51 #include "funcapi.h" #include "miscadmin.h" #include "nodes/makefuncs.h" + #include "optimizer/clauses.h" #include "optimizer/planmain.h" #include "parser/parse_expr.h" #include "utils/acl.h" *** *** 1415,1420 --- 1416,1440 return result; } + /* + * + * Get either an empty string or a batch of qualifiers. + * + */ + char * + rsinfo_get_qual_str(ReturnSetInfo *rsinfo) + { + Node*qual; + List*context; + + if (rsinfo->qual == NIL) + return pstrdup(""); + + qual = (Node *) make_ands_explicit(rsinfo->qual); + context = deparse_context_for_plan(NULL, NULL, rsinfo->rtable); + + return deparse_expression(qual, context, false, false); + } /* *ExecMakeTableFunctionResult *** *** 1426,1431 --- 1446,1452 Tuplestorestate * ExecMakeTableFunctionResult(ExprState *funcexpr, ExprContext *econtext, + List *qual, List *rtable, TupleDesc expectedDesc, TupleDesc *returnDesc) { *** *** 1458,1463 --- 1479,1486 InitFunctionCallInfoData(fcinfo, NULL, 0, NULL, (Node *) &rsinfo); rsinfo.type = T_ReturnSetInfo; rsinfo.econtext = econtext; + rsinfo.qual = qual; + rsinfo.rtable = rtable; rsinfo.expectedDesc = expectedDesc; rsinfo.allowedModes = (int) (SFRM_ValuePerCall | SFRM_Materialize); rsinfo.returnMode = SFRM_ValuePerCall; Index: src/backend/executor/nodeFunctionscan.c === RCS file: /projects/cvsroot/pgsql/src/backend/executor/nodeFunctionscan.c,v retrieving revision 1.46 diff -c -c -r1.46 nodeFunctionscan.c *** src/backend/executor/nodeFunctionscan.c
Re: [HACKERS] Protection from SQL injection
Chris Browne wrote: [EMAIL PROTECTED] (Florian Weimer) writes: * Thomas Mueller: What do you think about it? Do you think it makes sense to implement this security feature in PostgreSQL as well? Can't this be implemented in the client library, or a wrapper around it? A simple approximation would be to raise an error when you encounter a query string that isn't contained in some special configuration file. This could be implemented in a client library, but that means that you're still entirely as vulnerable; any client that chooses not to use that library won't be protected. It would be a mighty attractive thing to have something at the server level to protect against the problem. If by "the problem" you mean SQL injection attacks in general, I strongly suspect you are chasing a mirage. Someone mentioned upthread that the best way to secure the database was to require that all access be via stored procedures. You can actually go quite a logn way down that road. I have seen it done quite successfully in a Fortune 50 company. In effect you are getting out of the game of catching insecure operations by not allowing your user direct access to them at all. (This approach also has huge benefits in allowing optimisation without disturbing client code). But this requires work by the DBA. I have some ideas about how we could make it a whole lot easier and more workable, but they probably don't belong in this thread. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Protection from SQL injection
Tom Lane wrote: Darren Reed <[EMAIL PROTECTED]> writes: This might seem sillly, but... ...are comments going to be considered statements for the purpose of this knob? (hoping the anwer is "yes") Are you trying to say we should forbid comments? No thank you. No. When psql (for example) parses this: COMMIT;BEGIN;-- Hi it will generate individual commands with postgres (the server) over the connection. ie. psql sends "COMMIT;" waits, then sends 'BEGIN;", waits, etc. When you do this in perl: $db->prepare("COMMIT;BEGIN;--"); one single command string (the entire string above) is sent to the server. How often do people code comments into prepare statements in perl or the equivalent in java, ruby, etc? Do you put comments in your perl prepare statements? If comments count as a statement, at the server end, then the multi-statement disabling also disables another attack vector - slightly: you can no longer attack using this as your username: "' OR 1=1;--" This raises another point - preventing muilti-statement attacks work so long as the "hacker string" isn't broken up into multiple statements by the client side. Passing said string to /bin/sh, which then passes it to psql would successfully enable the attack even with this knob turned on. But few people are likely to be using a design that is that slow. Darren -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Protection from SQL injection
[EMAIL PROTECTED] (Florian Weimer) writes: > * Thomas Mueller: > >> What do you think about it? Do you think it makes sense to implement >> this security feature in PostgreSQL as well? > > Can't this be implemented in the client library, or a wrapper around it? > A simple approximation would be to raise an error when you encounter a > query string that isn't contained in some special configuration file. This could be implemented in a client library, but that means that you're still entirely as vulnerable; any client that chooses not to use that library won't be protected. It would be a mighty attractive thing to have something at the server level to protect against the problem. -- let name="cbbrowne" and tld="linuxfinances.info" in String.concat "@" [name;tld];; http://linuxdatabases.info/info/lsf.html If you add a couple of i's to Microsoft's stock ticker symbol, you get 'misfit'. This is, of course, not a coincidence. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Protection from SQL injection
Darren Reed <[EMAIL PROTECTED]> writes: > This might seem sillly, but... > ...are comments going to be considered statements for the purpose of > this knob? > (hoping the anwer is "yes") Are you trying to say we should forbid comments? No thank you. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposed Patch - LDAPS support for servers on port 636 w/o TLS
Andreas Pflug wrote: With ldaps on port 636 STARTTLS should NEVER be issued, so the protocol identifier ldaps should be sufficient as "do not issue STARTTLS" flag. IMHO the current pg_hba.conf implementation doesn't follow the usual nomenclatura; ldap with TLS is still ldap. Using ldaps as indicator for ldap with tls over port 389 is misleading for anyone familiar with ldap. I agree. ldaps:: should mean plain SSL without StartTLS. ldap:: should mean a plain text connection, unless some additional configuration directive enables StartTLS. There has been some discussion in the past about including (or not) this configuration state in the url : http://www.openldap.org/lists/openldap-devel/200202/msg00070.html -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] statement timeout vs dump/restore
Zeugswetter Andreas OSB sIT wrote: Do we want the following: 1. pg_dump issues "set statement_timeout = 0;" to the database prior to taking its copy of data (yes/no/default-but-switchable) 2. pg_dump/pg_restore issue "set statement_timeout = 0;" in text mode output (yes/no/default-but-switchable) 3. pg_restore issues "set statement_timeout = 0;" to the database in restore mode (yes/no/default-but-switchable) I think "yes" for all three. There was some handwaving about someone maybe not wanting it, but an utter lack of convincing use-cases; so I see no point in going to the effort of providing a switch. Note that 2 and 3 are actually the same thing (if you think they are not, then you are putting the behavior in the wrong place). I thought a proper fix for 3 would not depend on 2 ? I'm sure we could separate the two if we wanted to. Since we don't it's been put in the most natural spot, which does both. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Protection from SQL injection
This might seem sillly, but... ...are comments going to be considered statements for the purpose of this knob? (hoping the anwer is "yes") Darren -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposed Patch - LDAPS support for servers on port 636 w/o TLS
Tom Lane wrote: > I think a better idea is to embed the flag in the pg_hba.conf entry > itself. Perhaps something like "ldapso:" instead of "ldaps:" to > indicate "old" secure ldap protocol, or include another parameter > in the URL body. FWIW, I'm working on a proposal to change how pg_hba.conf deals with the parameter field to make it easier to do things like this, by using a name/value pair setup instead. The LDAP url is one reason - it's hacky enough already *before* we add this kind of option to it... //Magnus -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposed Patch - LDAPS support for servers on port 636 w/o TLS
Tom Lane wrote: stephen layland <[EMAIL PROTECTED]> writes: I've written a quick patch against the head branch (8.4DEV, but it also works with 8.1.3 sources) to fix LDAP authentication support to work with LDAPS servers that do not need start TLS. I'd be interested to hear your opinions on this. Not being an LDAP user, I'm not very qualified to comment on the details here, but ... My solution was to create a boolean config variable called ldap_use_start_tls which the user can toggle whether or not start tls is necessary. ... I really don't like using a GUC variable to determine the interpretation of entries in pg_hba.conf. A configuration file exists to set configuration, it shouldn't need help from a distance. Also, doing it this way means that if several different LDAP servers are referenced in different pg_hba.conf entries, they'd all have to have the same encryption behavior. I think a better idea is to embed the flag in the pg_hba.conf entry itself. Perhaps something like "ldapso:" instead of "ldaps:" to indicate "old" secure ldap protocol, or include another parameter in the URL body. With ldaps on port 636 STARTTLS should NEVER be issued, so the protocol identifier ldaps should be sufficient as "do not issue STARTTLS" flag. IMHO the current pg_hba.conf implementation doesn't follow the usual nomenclatura; ldap with TLS is still ldap. Using ldaps as indicator for ldap with tls over port 389 is misleading for anyone familiar with ldap. Regards, Andreas -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] statement timeout vs dump/restore
> > Do we want the following: > > > 1. pg_dump issues "set statement_timeout = 0;" to the > database prior to > > taking its copy of data (yes/no/default-but-switchable) > > 2. pg_dump/pg_restore issue "set statement_timeout = 0;" in > text mode > > output (yes/no/default-but-switchable) > > 3. pg_restore issues "set statement_timeout = 0;" to the > database in > > restore mode (yes/no/default-but-switchable) > > I think "yes" for all three. There was some handwaving about someone > maybe not wanting it, but an utter lack of convincing use-cases; so > I see no point in going to the effort of providing a switch. > > Note that 2 and 3 are actually the same thing (if you think they are > not, then you are putting the behavior in the wrong place). I thought a proper fix for 3 would not depend on 2 ? Andreas -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers