Thread: Re: [PATCHES] createdb/dropdb fixes
> All I really wanted to do is fix TODO item
> * database names with spaces fail
> but that is already taken care of, they work fine. Please check it off.
> Meanwhile, database names with single quotes in names don't work very well
> at all, and because of shell quoting rules this can't be fixed, so I put
> in error messages to that end.
That seems to be a bit heavy handed; why bother disallowing things in
the backend because some (small number of) shell-based tools have
trouble as clients? I'd prefer filtering that at the client end, and
allowing capable clients to do whatever they please.
There is a related issue which afaik no one has addressed yet: the
permissions ACLs are stored as a string with a format like
"accountname=permissions" (doing this from memory, so the details may
be wrong) but with quoting allowed for table names and user names one
could embed an equals sign into an account or group name and muck with
permissions. I haven't looked at the code in a long time, but was
thinking about recoding ACLs as a two-field type to enforce an
unambigous interpretation of the two fields. Interested??
- Thomas
--
Thomas Lockhart lockhart@alumni.caltech.edu
South Pasadena, California
Thomas Lockhart <lockhart@alumni.caltech.edu> writes:
>> Meanwhile, database names with single quotes in names don't work very well
>> at all, and because of shell quoting rules this can't be fixed, so I put
>> in error messages to that end.
> That seems to be a bit heavy handed; why bother disallowing things in
> the backend because some (small number of) shell-based tools have
> trouble as clients? I'd prefer filtering that at the client end, and
> allowing capable clients to do whatever they please.
No, you're missing the point: the backend itself uses shell escapes
for some whole-database functions. IIRC, database creation is done with
something likesystem("cp -r base/template1 base/newdb");
So shell metacharacters in database names are Bad News. We need to
put in a filter that will prevent appearances of / | ` etc in DB names.
I assume that's what Peter was doing.
I think we may have some bugs with metacharacters in table names (which
become filenames) as well, but haven't really pushed on it.
> thinking about recoding ACLs as a two-field type to enforce an
> unambigous interpretation of the two fields. Interested??
Seems like a good idea.
regards, tom lane
On 1999-12-14, Thomas Lockhart mentioned: > That seems to be a bit heavy handed; why bother disallowing things in > the backend because some (small number of) shell-based tools have > trouble as clients? I'd prefer filtering that at the client end, and It's really about statements like this: snprintf(buf, sizeof(buf), "rm -rf '%s'", path); There is no way around disallowing single-quotes unless you double quote the argument and be very careful with the escaping. Of course this particular case might as well use unlink(), but there is a recursive copy of the template1 dir which would take a little more work (opendir(), etc.). At that point we could lift that restriction. > permissions. I haven't looked at the code in a long time, but was > thinking about recoding ACLs as a two-field type to enforce an > unambigous interpretation of the two fields. Interested?? I've been puzzled about this for a long time, is there a reason this is stored as an array at all? Why not use tuples likeaclperm char?aclrelation oidaclentity oid /* user or groupsysid */aclisgroup bool /* is it a user or group? */ And then it looks like this: aclperm|aclrel|acluser|aclisgroup -------+------+-------+---------- R |177777| 100|f W |177777| 100|f R |177777| 120|f R |188888| 5|t That's much cleaner. GRANT and REVOKE would be reduced to simple insert/delete equivalents. I'm not sure how the actual authentication code would like that overheadwise, though. A related issue is pg_group, which I'm currently working on. Those arrays are killing me. A simple user/group associating relation would be much nicer. -- Peter Eisentraut Sernanders väg 10:115 peter_e@gmx.net 75262 Uppsala http://yi.org/peter-e/ Sweden
Peter Eisentraut <peter_e@gmx.net> writes:
> It's really about statements like this:
> snprintf(buf, sizeof(buf), "rm -rf '%s'", path);
> There is no way around disallowing single-quotes unless you double quote
> the argument and be very careful with the escaping.
Yes. In fact, I'd argue for filtering the names more heavily than that;
just to take a for-example, Bad Things would ensue if we accepted a
database name of "..".
It is easy to devise cases in which accepting leading "." or embedded "/"
leads to disaster; if you think those are OK, allow me to destroy your
installation for you ;-). I haven't yet thought of a way to cause
trouble with a back-quote in a DB name (given that single quotes are
disallowed) ... but I bet some enterprising hacker can find one.
Beyond the bare minimum security issues, I also think we should take
pity on the poor dbadmin who may have to be looking at these
subdirectories or filenames. Is it really a good idea to allow carriage
returns or other control characters in file/directory names? Is it
even a good idea to allow spaces? I don't think so. If we were not
using these names for Unix file/dir names then we could allow anything
we felt like --- but since we are using them that way, I think that the
safest path is to only allow things that are going to look like ordinary
file names when used in Unix shell commands. Otherwise there's still a
big chance of trouble if the dbadmin gets a little bit careless.
> Of course this particular case might as well use unlink(),
Not unless your system's unlink is much different from mine's...
regards, tom lane
Here's another anomaly I've run across in porting the Ars Digita Community System web development toolkit from Oracle to Postgres: In oracle, if we do: SQL> create table foo(i integer, j integer); Table created. then select like this, we get no rows returned: SQL> select i, count(*) from foo group by i; no rows selected In postgres, the same select on the same empty table yields: test=> select i, count(*) from foo group by i; i|count -+-----| 0 (1 row) test=> Which is correct? It's the count() causing the row to be output, apparently PostgreSQL feels obligated to return at least one value for the aggragate and since there are no groups has to invent one. I assume this is wrong, and that Oracle's right, but haven't dug through Date's book to verify. - Don Baccus, Portland OR <dhogaza@pacifier.com> Nature photos, on-line guides, Pacific Northwest Rare Bird Alert Serviceand other goodies at http://donb.photo.net.
At 04:11 PM 12/14/99 -0800, Don Baccus wrote: >Here's another anomaly I've run across in porting the Ars Digita >Community System web development toolkit from Oracle to Postgres: (always returning a row for a select count(*) ... group by queryeven if there aren't any groups) OK, I've gotten the latest sources with the bright idea of digging around, and in nodeAgg.c the routine ExecAgg.c has been somewhat rewritten, with comments that make it clear that this bug's already been fixed. I should build myself a latest version so I can filter out non-problems before reporting them, sorry... - Don Baccus, Portland OR <dhogaza@pacifier.com> Nature photos, on-line guides, Pacific Northwest Rare Bird Alert Serviceand other goodies at http://donb.photo.net.
Don Baccus <dhogaza@pacifier.com> writes:
> (always returning a row for a select count(*) ... group by query
> even if there aren't any groups)
Yah: if you have aggregates and no GROUP, for empty input you should
get one row out with "default" results (0 for COUNT, null for most other
aggregates). But for GROUP mode, no rows in should yield no rows out,
aggregates or no. It took a fair amount of arguing before everyone was
convinced that that is the correct interpretation of the spec ;-),
which is why it's only been fixed recently.
> OK, I've gotten the latest sources with the bright idea of digging
> around, and in nodeAgg.c the routine ExecAgg.c has been somewhat
> rewritten, with comments that make it clear that this bug's already
> been fixed.
> I should build myself a latest version so I can filter out non-problems
> before reporting them, sorry...
Not a problem. Bug reports on the latest release are fair game.
If it's already been fixed in current sources, whoever fixed it
will surely take pleasure in telling you so...
regards, tom lane
On 1999-12-14, Tom Lane mentioned: > Yes. In fact, I'd argue for filtering the names more heavily than that; > just to take a for-example, Bad Things would ensue if we accepted a > database name of "..". My rm refuses to remove '..' and '.'. > It is easy to devise cases in which accepting leading "." or embedded "/" > leads to disaster; if you think those are OK, allow me to destroy your > installation for you ;-). I haven't yet thought of a way to cause > trouble with a back-quote in a DB name (given that single quotes are > disallowed) ... but I bet some enterprising hacker can find one. The slash problem will disappear (I hope) when I fix that alternate location issue. > Beyond the bare minimum security issues, I also think we should take > pity on the poor dbadmin who may have to be looking at these > subdirectories or filenames. Is it really a good idea to allow carriage > returns or other control characters in file/directory names? Is it > even a good idea to allow spaces? I don't think so. If we were not Spaces why not? I use spaces all the time in filenames. But perhaps we should make a definite list of things we won't allow, such as dots (.), slashes (/), tildes (~), etc., and everything below ASCII 32. But limiting it to a finite list of characters would really be a blow to people using other character sets. > > Of course this particular case might as well use unlink(), > > Not unless your system's unlink is much different from mine's... Is it just me or is your system intentionally designed to be different from anybody elses? :) Last time I checked rm does call unlink. Relying on sh-utils sort of commands has its own set of problems. For example in the 6.5 source, run the postmaster on a terminal, revoke all permissions on a database directory (chmod a-rwx testdb) and try to drop it. My rm will prompt on the postmaster terminal for verification while the whole thing hangs ... -- Peter Eisentraut Sernanders väg 10:115 peter_e@gmx.net 75262 Uppsala http://yi.org/peter-e/ Sweden