Re: [HACKERS] WIP patch (v2) for updatable security barrier views
(2014/01/21 18:18), Dean Rasheed wrote: On 21 January 2014 01:09, KaiGai Kohei kai...@ak.jp.nec.com wrote: (2014/01/13 22:53), Craig Ringer wrote: On 01/09/2014 11:19 PM, Tom Lane wrote: Dean Rasheed dean.a.rash...@gmail.com writes: My first thought was that it should just preprocess any security barrier quals in subquery_planner() in the same way as other quals are preprocessed. But thinking about it further, those quals are destined to become the quals of subqueries in the range table, so we don't actually want to preprocess them at that stage --- that will happen later when the new subquery is planned by recursion back into subquery_planner(). So I think the right answer is to make adjust_appendrel_attrs() handle recursion into sublink subqueries. TBH, this sounds like doubling down on a wrong design choice. I see no good reason that updatable security views should require any fundamental rearrangements of the order of operations in the planner. In that case, would you mind offerign a quick sanity check on the following alternative idea: - Add sourceRelation to Query. This refers to the RTE that supplies tuple projections to feed into ExecModifyTable, with appropriate resjunk ctid and (if requ'd) oid cols present. - When expanding a target view into a subquery, set sourceRelation on the outer view to the index of the RTE of the newly expanded subquery. I have sane opinion. Existing assumption, resultRelation is RTE of the table to be read not only modified, makes the implementation complicated. If we would have a separate sourceRelation, it allows to have flexible form including sub-query with security_barrier on the reader side. Could you tell me the direction you're inclined right now? I wonder whether I should take the latest patch submitted on 09-Jan for a deep code reviewing and testing. Yes, please review the patch from 09-Jan (http://www.postgresql.org/message-id/CAEZATCUiKxOg=voovja2s6g-sixzzxg18totggp8zobq6qn...@mail.gmail.com). The idea behind that patch is to avoid a lot of the complication that leads to and then arises from having a separate sourceRelation in the query. If you go down the route of expanding the subquery in the rewriter and making it the sourceRelation, then you have to make extensive changes to preprocess_targetlist so that it recursively descends into the subquery to pull out variables needed by an update. Also you would probably need additional changes in the rewriter so that later stages didn't trample on the sourceRelation, and correctly updated it in the case of views on top of other views. Also, you would need to make changes to the inheritance planner code, because you'd now have 2 RTEs referring to the target relation (resultRelation and sourceRelation wrapping it in a subquery). Referring to the comment in inheritance_planner(): * Source inheritance is expanded at the bottom of the * plan tree (see allpaths.c), but target inheritance has to be expanded at * the top. except that in the case of the sourceRelation, it is actually effectively the target, which means it shouldn't be expanded, otherwise you'd get plans like the ones I complained about upthread (see the final explain output in http://www.postgresql.org/message-id/caezatcu3vcdkjpnhgfkrmrkz0axkcuh4ce_kqq3z2hzknhi...@mail.gmail.com). Perhaps all of that could be made to work, but I think that it would end up being a far more invasive patch than my 09-Jan patch. My patch avoids both those issues by not doing the subquery expansion until after inheritance expansion, and after targetlist preprocessing. Probably, I could get your point. Even though the sub-query being replaced from a relation with securityQuals is eventually planned according to the usual manner, usual order as a part of regular sub-query planning, however, adjust_appendrel_attrs() is called by inheritance_planner() earlier than sub-query's planning. Maybe, we originally had this problem but not appeared on the surface, because child relations don't have qualifiers on this phase. (It shall be distributed later.) But now, this assumption was broken because of a relation with securityQuals being replaced by a sub-query with SubLink. So, I'm inclined to revise the assumption side, rather than existing assertion checks. Shall we investigate what assumption should be revised if child- relation, being expanded at expand_inherited_tables(), would be a sub-query having Sub-Link? A minor comment is below: ! /* !* Make sure that the query is marked correctly if the added qual !* has sublinks. !*/ ! if (!parsetree-hasSubLinks) ! parsetree-hasSubLinks = checkExprHasSubLink(viewqual); Is this logic really needed? This flag says the top-level query contains SubLinks, however, the above condition checks whether a sub-query to be constructed shall contain SubLinks. Also, securityQuals is not attached to the parse tree right now. Thanks, -- OSS
Re: [HACKERS] dynamic shared memory and locks
(2014/01/22 1:37), Robert Haas wrote: On Mon, Jan 20, 2014 at 11:23 PM, KaiGai Kohei kai...@ak.jp.nec.com wrote: I briefly checked the patch. Most of lines are mechanical replacement from LWLockId to LWLock *, and compiler didn't claim anything with -Wall -Werror option. My concern is around LWLockTranche mechanism. Isn't it too complicated towards the purpose? For example, it may make sense to add char lockname[NAMEDATALEN]; at the tail of LWLock structure if LOCK_DEBUG is enabled. Probably, it also adds an argument of LWLockAssign() to gives the human readable name. Is the additional 64bytes (= NAMEDATALEN) per lock too large for recent hardware? Well, we'd need it when either LOCK_DEBUG was defined or when LWLOCK_STATS was defined or when --enable-dtrace was used, and while the first two are probably rarely enough used that that would be OK, I think the third case is probably fairly common, and I don't think we want to have such a potentially performance-critical difference between builds with and without dtrace. Also... yeah, it's a lot of memory. If we add an additional 64 bytes to the structure, then we're looking at 96 bytes per lwlock instead of 32, after padding out to a 32-byte boundary to avoid crossing cache lines. We need 2 lwlocks per buffer, so that's an additional 128 bytes per 8kB buffer. For shared_buffers = 8GB, that's an extra 128MB for storing lwlocks. I'm not willing to assume nobody cares about that. And while I agree that this is a bit complex, I don't think it's really as bad as all that. We've gotten by for a long time without people being able to put lwlocks in other parts of memory, and most extension authors have gotten by with LWLockAssign() just fine and can continue doing things that way; only users with particularly sophisticated needs should bother to worry about the tranche stuff. Hmm... 1/64 of main memory (if large buffer system) might not be an ignorable memory consumption. One idea I just had is to improve the dsm_toc module so that it can optionally set up a tranche of lwlocks for you, and provide some analogues of RequestAddinLWLocks and LWLockAssign for that case. That would probably make this quite a bit simpler to use, at least for people using it with dynamic shared memory. But I think that's a separate patch. I agree with this idea. It seems to me quite natural to keep properties of objects held on shared memory (LWLock) on shared memory. Also, a LWLock once assigned shall not be never released. So, I think dsm_toc can provide a well suitable storage for them. Thanks, -- OSS Promotion Center / The PG-Strom Project KaiGai Kohei kai...@ak.jp.nec.com -- 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] WIP patch (v2) for updatable security barrier views
(2014/01/13 22:53), Craig Ringer wrote: On 01/09/2014 11:19 PM, Tom Lane wrote: Dean Rasheed dean.a.rash...@gmail.com writes: My first thought was that it should just preprocess any security barrier quals in subquery_planner() in the same way as other quals are preprocessed. But thinking about it further, those quals are destined to become the quals of subqueries in the range table, so we don't actually want to preprocess them at that stage --- that will happen later when the new subquery is planned by recursion back into subquery_planner(). So I think the right answer is to make adjust_appendrel_attrs() handle recursion into sublink subqueries. TBH, this sounds like doubling down on a wrong design choice. I see no good reason that updatable security views should require any fundamental rearrangements of the order of operations in the planner. In that case, would you mind offerign a quick sanity check on the following alternative idea: - Add sourceRelation to Query. This refers to the RTE that supplies tuple projections to feed into ExecModifyTable, with appropriate resjunk ctid and (if requ'd) oid cols present. - When expanding a target view into a subquery, set sourceRelation on the outer view to the index of the RTE of the newly expanded subquery. I have sane opinion. Existing assumption, resultRelation is RTE of the table to be read not only modified, makes the implementation complicated. If we would have a separate sourceRelation, it allows to have flexible form including sub-query with security_barrier on the reader side. Could you tell me the direction you're inclined right now? I wonder whether I should take the latest patch submitted on 09-Jan for a deep code reviewing and testing. Thanks, - In rewriteTargetView, as now, reassign resultRelation to the target view's base rel. This is required so that do any RETURNING and WITH CHECK OPTION fixups required to adjust the RETURNING list to the new result relation, so they act on the final tuple after any BEFORE triggers act. Do not flatten the view subquery and merge the quals (as currently happens); allow it to be expanded as a subquery by the rewriter instead. Don't mess with the view tlist at this point except by removing the whole-row Var added by rewriteTargetListUD. - When doing tlist expansion in preprocess_targetlist, when we process the outer Query (the only one for which query type is not SELECT, and the only one that has a non-zero resultRelation), if resultRelation != sourceRelation recursively follow the chain of sourceRelation s to the bottom one with type RTE_RELATION. Do tlist expansion on that inner-most Query first, using sourceRelation to supply the varno for injected TLEs, including injecting ctid, oid if req'd, etc. During call stack unwind, have each intermediate layer do regular tlist expansion, adding a Var pointing to each tlist entry of the inner subquery. At the outer level of preprocess_targetlist, sort the tlist, now expanded to include all required vars, into attribute order for the resultRelation. (this level is the only one that has resultRelation set). Avoid invoking preprocess_targetlist on the inner Query again when it's processed in turn, or just bail out when we see sourceRelation set since we know it's already been done. (Alternately, it might be possible to run preprocess_targetlist depth-first instead of the current outermost-first, but I haven't looked at that). The optimizer can still flatten non-security-barrier updatable views, following the chain of Vars as it collapses each layer. That's effectively what the current rewriteTargetView code is doing manually at each pass right now. I'm sure there are some holes in this outline, but it's struck me as possibly workable. The key is to set sourceRelation on every inner subquery in the target query chain, not just the outer one, so it can be easily followed from the outer query though the subqueries into the innermost query with RTE_RELATION type. The only alternative I've looked at is looking clumsier the longer I examine it: adding a back-reference in each subquery's Query struct, to the Query containing it and the RTI of the subquery within the outer Query. That way, once rewriting hits the innermost rel with RTE_RELATION type, the rewriter can walk back up the Query tree doing tlist rewriting. I'm not sure if this is workable yet, and it creates ugly pointer-based backrefs *up* the Query chain, making what was previously a tree of Query* into a graph. That could get exciting, though there'd never be any need for mutators to follow the parent query pointer so it wouldn't make tree rewrites harder. -- OSS Promotion Center / The PG-Strom Project KaiGai Kohei kai...@ak.jp.nec.com -- 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] inherit support for foreign tables
(2014/01/14 18:24), Shigeru Hanada wrote: 2013/11/18 Tom Lane t...@sss.pgh.pa.us: Robert Haas robertmh...@gmail.com writes: I think it's been previously proposed that we have some version of a CHECK constraint that effectively acts as an assertion for query optimization purposes, but isn't actually enforced by the system. I can see that being useful in a variety of situations, including this one. Yeah, I think it would be much smarter to provide a different syntax to explicitly represent the notion that we're only assuming the condition is true, and not trying to enforce it. I'd like to revisit this feature. Explicit notation to represent not-enforcing (assertive?) is an interesting idea. I'm not sure which word is appropriate, but for example, let's use the word ASSERTIVE as a new constraint attribute. CREATE TABLE parent ( id int NOT NULL, group int NOT NULL, name text ); CREATE FOREIGN TABLE child_grp1 ( /* no additional column */ ) INHERITS (parent) SERVER server1; ALTER TABLE child_grp1 ADD CONSTRAINT chk_group1 CHECK (group = 1) ASSERTIVE; If ASSERTIVE is specified, it's not guaranteed that the constraint is enforced completely, so it should be treated as a hint for planner. As Robert mentioned, enforcing as much as we can during INSERT/UPDATE is one option about this issue. In addition, an idea which I can't throw away is to assume that all constraints defined on foreign tables as ASSERTIVE. Foreign tables potentially have dangers to have wrong data by updating source data not through foreign tables. This is not specific to an FDW, so IMO constraints defined on foreign tables are basically ASSERTIVE. Of course PG can try to maintain data correct, but always somebody might break it. qu Does it make sense to apply assertive CHECK constraint on the qual of ForeignScan to filter out tuples with violated values at the local side, as if row-level security feature doing. It enables to handle a situation that planner expects only clean tuples are returned but FDW driver is unavailable to anomalies. Probably, this additional check can be turned on/off on the fly, if FDW driver has a way to inform the core system its capability, like FDW_CAN_ENFORCE_CHECK_CONSTRAINT that informs planner to skip local checks. Besides CHECK constraints, currently NOT NULL constraints are virtually ASSERTIVE (not enforcing). Should it also be noted explicitly? Backward compatibility NOT NULL [ASSERTIVE] might be an option. Thanks, -- OSS Promotion Center / The PG-Strom Project KaiGai Kohei kai...@ak.jp.nec.com -- 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] dynamic shared memory and locks
(2014/01/11 3:11), Robert Haas wrote:
On Mon, Jan 6, 2014 at 5:50 PM, Robert Haas robertmh...@gmail.com wrote:
This is only part of the solution, of course: a complete solution will
involve making the hash table key something other than the lock ID.
What I'm thinking we can do is making the lock ID consist of two
unsigned 32-bit integers. One of these will be stored in the lwlock
itself, which if my calculations are correct won't increase the size
of LWLockPadded on any common platforms (a 64-bit integer would).
Let's call this the tranch id. The other will be derived from the
LWLock address. Let's call this the instance ID. We'll keep a
table of tranch IDs, which will be assigned consecutively starting
with 0. We'll keep an array of metadata for tranches, indexed by
tranch ID, and each entry will have three associated pieces of
information: an array base, a stride length, and a printable name.
When we need to identify an lwlock in the log or to dtrace, we'll
fetch the tranch ID from the lwlock itself and use that to index into
the tranch metadata array. We'll then take the address of the lwlock,
subtract the array base address for the tranch, and divide by the
stride length; the result is the instance ID. When reporting the
user, we can report either the tranch ID directly or the associated
name for that tranch; in either case, we'll also report the instance
ID.
So initially we'll probably just have tranch 0: the main LWLock array.
If we move buffer content and I/O locks to the buffer headers, we'll
define tranch 1 and tranch 2 with the same base address: the start of
the buffer descriptor array, and the same stride length, the size of a
buffer descriptor. One will have the associated name buffer content
lock and the other buffer I/O lock. If we want, we can define
split the main LWLock array into several tranches so that we can more
easily identify lock manager locks, predicate lock manager locks, and
buffer mapping locks.
OK, I've implemented this: here's what I believe to be a complete
patch, based on the previous lwlock-pointers.patch but now handling
LOCK_DEBUG and TRACE_LWLOCKS and dtrace and a bunch of other loose
ends. I think this should be adequate for allowing lwlocks to be
stored elsewhere in the main shared memory segment as well as in
dynamic shared memory segments.
Thoughts?
I briefly checked the patch. Most of lines are mechanical replacement
from LWLockId to LWLock *, and compiler didn't claim anything with
-Wall -Werror option.
My concern is around LWLockTranche mechanism. Isn't it too complicated
towards the purpose?
For example, it may make sense to add char lockname[NAMEDATALEN]; at
the tail of LWLock structure if LOCK_DEBUG is enabled. Probably, it
also adds an argument of LWLockAssign() to gives the human readable
name. Is the additional 64bytes (= NAMEDATALEN) per lock too large
for recent hardware?
Below is minor comments.
It seems to me this newer form increased direct reference to
the MainLWLockArray. Is it really graceful?
My recommendation is to define a macro that wraps the reference to
MainLWLockArray.
For example:
#define PartitionLock(i) \
(MainLWLockArray[LOCK_MANAGER_LWLOCK_OFFSET + (i)].lock)
instead of the following manner.
@@ -3400,7 +3406,12 @@ GetLockStatusData(void)
* Must grab LWLocks in partition-number order to avoid LWLock deadlock.
*/
for (i = 0; i NUM_LOCK_PARTITIONS; i++)
- LWLockAcquire(FirstLockMgrLock + i, LW_SHARED);
+ {
+ LWLock *partitionLock;
+
+ partitionLock = MainLWLockArray[LOCK_MANAGER_LWLOCK_OFFSET + i].lock;
+ LWLockAcquire(partitionLock, LW_SHARED);
+ }
This chunk is enclosed by #ifdef EXEC_BACKEND, the legacy LWLockArray
was not removed.
@@ -5633,6 +5630,7 @@ save_backend_variables(BackendParameters *param, Port
*port,
param-SpinlockSemaArray = SpinlockSemaArray;
#endif
param-LWLockArray = LWLockArray;
+ param-MainLWLockArray = MainLWLockArray;
param-ProcStructLock = ProcStructLock;
param-ProcGlobal = ProcGlobal;
param-AuxiliaryProcs = AuxiliaryProcs;
Thanks,
--
OSS Promotion Center / The PG-Strom Project
KaiGai Kohei kai...@ak.jp.nec.com
--
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] inherit support for foreign tables
(2014/01/21 11:44), Shigeru Hanada wrote: Thanks for the comments. 2014/1/21 KaiGai Kohei kai...@ak.jp.nec.com: In addition, an idea which I can't throw away is to assume that all constraints defined on foreign tables as ASSERTIVE. Foreign tables potentially have dangers to have wrong data by updating source data not through foreign tables. This is not specific to an FDW, so IMO constraints defined on foreign tables are basically ASSERTIVE. Of course PG can try to maintain data correct, but always somebody might break it. qu Does it make sense to apply assertive CHECK constraint on the qual of ForeignScan to filter out tuples with violated values at the local side, as if row-level security feature doing. It enables to handle a situation that planner expects only clean tuples are returned but FDW driver is unavailable to anomalies. Probably, this additional check can be turned on/off on the fly, if FDW driver has a way to inform the core system its capability, like FDW_CAN_ENFORCE_CHECK_CONSTRAINT that informs planner to skip local checks. Hmm, IIUC you mean that local users can't (or don't need to) know that data which violates the local constraints exist on remote side. Applying constraints to the data which is modified through FDW would be necessary as well. In that design, FDW is a bidirectional filter which provides these features: 1) Don't push wrong data into remote data source, by applying local constraints to the result of the modifying query executed on local PG. This is not perfect filter, because remote constraints don't mapped automatically or perfectly (imagine constraints which is available on remote but is not supported in PG). 2) Don't retrieve wrong data from remote to local PG, by applying local constraints Yes. (1) can be done with ExecConstraints prior to FDW callback on UPDATE or INSERT, even not a perfect solution because of side-channel on the remote data source. For (2), my proposition tries to drop retrieved violated tuples, however, the result is same. I have a concern about consistency. It has not been supported, but let's think of Aggregate push-down invoked by a query below. SELECT count(*) FROM remote_table; If this query was fully pushed down, the result is the # of records exist on remote side, but the result would be # of valid records when we don't push down the aggregate. This would confuse users. Hmm. In this case, FDW driver needs to be responsible to push-down the additional check quals into remote side, so it does not work transparently towards the ForeignScan. It might be a little bit complicated suggestion for the beginning of the efforts. Besides CHECK constraints, currently NOT NULL constraints are virtually ASSERTIVE (not enforcing). Should it also be noted explicitly? Backward compatibility…. Yep, backward compatibility (especially visible ones to users) should be minimal, ideally zero. NOT NULL [ASSERTIVE] might be an option. Treating [ASSERTIVE | NOT ASSERTIVE] like DEFERRABLE, and allow ingASSERTIVE for only foreign tables? It makes sense, though we need consider exclusiveness . But It needs to default to ASSERTIVE on foreign tables, and NOT ASSERTIVE (means forced) on others. Isn't is too complicated? I think it is not easy to implement assertive checks, except for foreign tables, because all the write stuff to regular tables are managed by PostgreSQL itself. So, it is a good first step to add support ASSERTIVE CHECK on foreign table only, and to enforce FDW drivers nothing special from my personal sense. How about committer's opinion? Thanks, -- OSS Promotion Center / The PG-Strom Project KaiGai Kohei kai...@ak.jp.nec.com -- 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] sepgsql contrib module
(2011/01/27 22:26), Robert Haas wrote: 2011/1/27 KaiGai Koheikai...@ak.jp.nec.com: - Error messages become obtaining %m, when the error was originated from the libselinux interfaces. It will provides DBA a hint why interactions with SELinux does not work well. No space before the : %m, please. Also this word looks like a typo: seuciryt The attached patch eliminated spaces before : %m, and fixed up the typo. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com sepgsql-v9.1-fixup.2.patch Description: application/octect-stream -- 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] sepgsql contrib module
(2011/01/27 0:25), Robert Haas wrote: 2011/1/25 KaiGai Koheikai...@ak.jp.nec.com: (2011/01/26 12:23), KaiGai Kohei wrote: Yikes. On further examination, exec_object_restorecon() is pretty bogus. Surely you need some calls to quote_literal_cstr() in there someplace. Are you concerning about the object name being supplied to selabel_lookup_raw() in exec_object_restorecon()? I also think this quoting you suggested is reasonable. How about the case when the object name only contains alphabet and numerical characters? Oh, quote_literal_cstr() is the wrong function - these are identifiers, not literals. So we should use quote_identifier(). OK, I did with quote_identifier(). The attached patch fixes up several stuffs in sepgsql module. - The object names being supplied to selabel_lookup_raw() to lookup initial labels become qualified by quote_identifier(), if necessary. - On access violation, sepgsql_check_perms() records audit logs. It contains object name being referenced. It became generated using getObjectDescription(). - Also, sepgsql_audit_log() becomes to quote the supplied object name, because it may contains white-space. - Error messages become obtaining %m, when the error was originated from the libselinux interfaces. It will provides DBA a hint why interactions with SELinux does not work well. - Documentation was updated to suggest users to install libselinux v2.0.93 or later, because it used newer features than ones provided in v2.0.80. - Regression Test was updated, because of error message updates. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com sepgsql-v9.1-fixup.1.patch Description: application/octect-stream -- 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] sepgsql contrib module
(2011/01/24 12:49), Robert Haas wrote: On Sun, Jan 23, 2011 at 9:56 PM, Robert Haasrobertmh...@gmail.com wrote: On Sun, Jan 23, 2011 at 8:53 PM, Robert Haasrobertmh...@gmail.com wrote: 2011/1/21 KaiGai Koheikai...@ak.jp.nec.com: The attached patch is a revised version. I've committed this. Cleanup coming... Yikes. On further examination, exec_object_restorecon() is pretty bogus. Surely you need some calls to quote_literal_cstr() in there someplace. Are you concerning about the object name being supplied to selabel_lookup_raw() in exec_object_restorecon()? I also think this quoting you suggested is reasonable. And how about using getObjectDescriptionOids() for the error message, instead of the entirely bogus construction that's there now? It seems to me a good idea. I'll try to revise corresponding code. Also, shouldn't a bunch of these messages end in : %m? When these messages are because of unexpected operating system errors, such as fails in communication with selinux, the %m will give us good suggestions. I'll submit these fixes within a few days, please wait for a while. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] sepgsql contrib module
(2011/01/26 12:23), KaiGai Kohei wrote: Yikes. On further examination, exec_object_restorecon() is pretty bogus. Surely you need some calls to quote_literal_cstr() in there someplace. Are you concerning about the object name being supplied to selabel_lookup_raw() in exec_object_restorecon()? I also think this quoting you suggested is reasonable. How about the case when the object name only contains alphabet and numerical characters? Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] sepgsql contrib module
(2011/01/20 12:10), Robert Haas wrote:
2011/1/5 KaiGai Koheikai...@ak.jp.nec.com:
How about feasibility to merge this 4KL chunks during the rest of
45 days? I think we should decide this general direction at first.
I read through this code tonight and it looks pretty straightforward.
I don't see much reason not to accept this more or less as-is. I'm a
bit suspicious of this line:
{ translation,SEPG_PROCESS__TRANSITION },
I can't help wondering based on the rest of the table whether you
intend to have the same word on that line twice, but you don't. On a
related note, would it make sense to pare down this table to the
entries that are actually used at the moment?
Sorry for giving you a confusion.
It was my typo, so should be fixed as:
{ transition,SEPG_PROCESS_TRANSITION },
This permission shall be checked when a security label of client being
switched from X to Y, typically on execution of trusted-procedure.
Also, I missed to check on sepgsql_needs_fmgr_hook(). We don't want to
allow inlining the function on lack of permission.
I'll fix them soon.
And how about adding a
ProcessUtility_hook to trap evil non-DML statements that some
nefarious user might issues?
It seems to me reasonable as long as the number of controlled command
are limited. For example, LOAD command may be a candidate being
controlled without exceptions.
However, it will be a tough work, if the plug-in tries to parse and
analyze supplied utility commands by itself.
One other problem is that you need to work on your whitespace a bit.
I believe in a few places you have a mixture of tabs and spaces. More
seriously, pgindent is going to completely mangle things like this:
/*
* sepgsql_mode
*
* SEPGSQL_MODE_DISABLED: Disabled on runtime
* SEPGSQL_MODE_DEFAULT : Same as system settings
* SEPGSQL_MODE_PERMISSIVE : Always permissive mode
* SEPGSQL_MODE_INTERNAL: Same as SEPGSQL_MODE_PERMISSIVE,
*except for
no audit prints
*/
You have to write it with a line of dashes on the first and last
lines, if you don't want it reformatted as a paragraph. It might be
worth actually running pgindent over contrib/selinux and then check
over the results.
OK, I'll try to run pgindent to confirm the effect of reformat.
Finally, we need to work on the documentation.
I uploaded my draft here.
http://wiki.postgresql.org/wiki/SEPostgreSQL_Documentation
If reasonable, I'll move them into *.sgml style.
I may want to simplify the step to installation using an installer
script.
But overall, it looks pretty good, IMHO.
Thanks for your reviewing in spite of a large patch.
--
KaiGai Kohei kai...@ak.jp.nec.com
--
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] sepgsql contrib module
(2011/01/20 13:01), Robert Haas wrote: 2011/1/19 KaiGai Koheikai...@ak.jp.nec.com: And how about adding a ProcessUtility_hook to trap evil non-DML statements that some nefarious user might issues? It seems to me reasonable as long as the number of controlled command are limited. For example, LOAD command may be a candidate being controlled without exceptions. However, it will be a tough work, if the plug-in tries to parse and analyze supplied utility commands by itself. I think the key is to either accept or reject the command based on very simple criteria - decide based only on the command type, and ignore its parameters. I can understand this idea, however, it is hard to implement this criteria, because SELinux describes all the rules as a relationship between a client and object using their label, so we cannot know what actions (typically represented in command tag) are allowed or denied without resolving their object names. I uploaded my draft here. http://wiki.postgresql.org/wiki/SEPostgreSQL_Documentation If reasonable, I'll move them into *.sgml style. I have yet to review that, but will try to get to it before too much more time goes by. OK, I try to translate it into *.sgml format. I may want to simplify the step to installation using an installer script. OK, but let's get this nailed down as soon as possible. Tempus fugit. I like to give my higher priority on the ProcessUtility_hook, rather than installation script. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] texteq/byteaeq: avoid detoast [REVIEW]
(2011/01/17 14:51), Itagaki Takahiro wrote: On Mon, Jan 17, 2011 at 04:05, Andy Colsona...@squeakycode.net wrote: This is a review of: https://commitfest.postgresql.org/action/patch_view?id=468 Purpose: Equal and not-equal _may_ be quickly determined if their lengths are different. This _may_ be a huge speed up if we don't have to detoast. We can skip detoast to compare lengths of two text/bytea values with the patch, but we still need detoast to compare the contents of the values. If we always generate same toasted byte sequences from the same raw values, we don't need to detoast at all to compare the contents. Is it possible or not? Are you talking about an idea to apply toast id as an alternative key? I did similar idea to represent security label on user tables for row level security in the v8.4/9.0 based implementation. Because a small number of security labels are shared by massive tuples, it is waste of space if we have all the text data being toasted individually, not only performance loss in toast/detoast. In this case, I represented security label (text) using security-id (oid) which is a primary key pointing out a certain text data in catalog table. It well reduced storage consumption and achieved good performance in comparison operation. One challenge was to reclaim orphan texts. In this case, we needed to lock out a user table referencing the toast values, then we delete all the orphan entries. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
I tried to pick up this patch to review. It seems to me fine, enough simple and works as explained in the implementation level, apart from reasonability of this feature. (Tom was not 100% agree with this feature 1.5month ago.) I'm not certain whether the current regression test should be updated, or not. The pg_regress launches psql with -q option, so completionTag is always ignored. Thanks, (2010/11/29 0:14), Marti Raudsepp wrote: Hi list, Often enough when developing PostgreSQL views and functions, I have pasted the CREATE OR REPLACE commands into the wrong window/shell and ran them there without realizing that I'm creating a function in the wrong database, instead of replacing. Currently psql does not provide any feedback of which action really occured. Only after writing this patch I realized that I could instead raise a NOTICE, like current IF EXISTS/IF NOT EXISTS clauses do. Is that a better way to solve this? This patch returns command tag CREATE X or REPLACE X for LANGAUGE/VIEW/RULE/FUNCTION. This is done by passing completionTag to from ProcessUtility to more functions, and adding a 'bool *didUpdate' argument to some lower-level functions. I'm not sure if passing back the status in a bool* is considered good style, but this way all the functions look consistent. Regards, Marti -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] sepgsql contrib module
I also think wiki page allows us to brush up the documentation rather than exchanging patches effectively. I'll set up a wiki page that contains same contents with *.sgml file to revise documentation stuff to be included into the *.sgml file finally. How about this idea? Sounds good. I set up a wikipage as a source of *.sgml file. http://wiki.postgresql.org/wiki/SEPostgreSQL_Documentation We can fix up cosmetic things later, so should we review the description of the upcoming official documentation? If we use result of the `pg_config --sharedir` here, how about this writing style? Or, do we have any other ideas? I'm not sure - I'll look at your next draft more closely. I added a mention about installation path in the Installation section, as follows: The following instruction assumes your installation is under the /usr/local/pgsql directory, and the database cluster is in /usr/local/pgsql/data. Substitute your paths appropriately. It seems to me natural rather than using `pg_config --sharedir` instead. (we might need to care about installation path of the pg_config in this case.) Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] sepgsql contrib module
2. The docs contains some references to /usr/local/pgsql/share.. Does this really mean whatever sharedir you established when you ran configure, i.e. the output of pg_config --sharedir? I hope so. Yes, it means the sharedir being configured. I found the following description at the installation.sgml. I should put this kind of mention on the documentation. |para | These instructions assume that your existing installation is under the |filename/usr/local/pgsql/ directory, and that the data area is in |filename/usr/local/pgsql/data/. Substitute your paths | appropriately. |/para Why does the user need to know about this at all? Doesn't make install put everything in the right place? It seemed to me a convenient writing-style to inform users an installation path of file. What I like to write is showing users what path stores what files needed in installation process. If we use result of the `pg_config --sharedir` here, how about this writing style? Or, do we have any other ideas? screen $ su # SHAREDIR=`pg_config --sharedir` # semodule -u $SHAREDIR/contrib/sepgsql-regtest.pp # semodule -l : sepgsql-regtest 1.03 : /screen 5. I'm not too sure about this one, but I think it might be good to elaborate on what we mean by respecting the system SE-Linux policy. What kinds of objects do we support checks on? What sorts of checks? What kind of access can we allow/deny? I guess these detailed description makes amount of this chapter disproportionally increase in the future version. My preference is wikipage to provide this kind of detailed information. http://wiki.postgresql.org/wiki/SEPostgreSQL The contents of above wikipage is now obsoleted, because it assumed SELinux support as a built-in feature. But it is a good time to fix up the description. I'd prefer to have it in the actual documentation. I think SE-PostgreSQL is going to need a lot of documentation. A wiki page risks getting out of date or having the wrong information for the version the user has installed. 9.1 may be quite different from 9.2, for example. Indeed, wikipage might not be suitable to document for several different version. OK, I'll try to add description what you suggested above. Most of what you have here right now describes why you might want to use this feature, rather than what the feature actually does. If you want to start by updating the wiki page, that's fine, and may be an easier way for us to collaborate than doing it by exchanging patches. But ultimately I think it needs to go in the docs. The background of this wikipage is that I was persuading people this feature being worthful, so the contents tend to philosophical things rather than actual specifications. I also think wiki page allows us to brush up the documentation rather than exchanging patches effectively. I'll set up a wiki page that contains same contents with *.sgml file to revise documentation stuff to be included into the *.sgml file finally. How about this idea? Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] sepgsql contrib module
(2011/01/06 14:28), Robert Haas wrote: 2011/1/5 KaiGai Koheikai...@ak.jp.nec.com: The attached patch is the modular version of SE-PostgreSQL (take.2). I'm reading through the documentation and so far it looks pretty reasonable. But I have some questions and suggested changes, of course. :-) Thanks for your reviewing in spite of large chunk. 1. Why is sepgsql the right name for this module, as opposed to, say, selinux? We don't call the cube module cubepgsql, or the hstore module hstorepgsql. Maybe there's a reason why this case is different, but I'm not sure. In some previous cases when SELinux model was ported to other systems, its project was named as SE-(other system), such as SE-BSD, SE-X, etc... I named it according to this convention, however, it is indeed uncertain whether 'sepgsql' follows on the convention in pgsql side. I don't think it is a strong reason why the module is named as 'sepgsql' instead of 'selinux'. In advertisement context, we can just call it as SE-PostgreSQL. 2. The docs contains some references to /usr/local/pgsql/share.. Does this really mean whatever sharedir you established when you ran configure, i.e. the output of pg_config --sharedir? I hope so. Yes, it means the sharedir being configured. I found the following description at the installation.sgml. I should put this kind of mention on the documentation. | para | These instructions assume that your existing installation is under the | filename/usr/local/pgsql/ directory, and that the data area is in | filename/usr/local/pgsql/data/. Substitute your paths | appropriately. | /para 3. The language for the sepgsql.permissive GUC suggests that it's PGC_POSTMASTER, but I'd think PGC_SIGHUP ought to be sufficient. Either way, please copy the appropriate language from some existing GUC of the same type instead of inventing a new way to say it. I also have no idea what because it invalidates all the inefficient stuff means. OK, I'll try to find up similar description then fix up both of the code and documentation. 4. Please remove the upcoming features section of the documentation. This material is appropriate for a page on the wiki, but shouldn't be part of the official documentation. Instead, you might want to have a *short* Limitations section. OK, I'll replace an itemization of limitations in this version. 5. I'm not too sure about this one, but I think it might be good to elaborate on what we mean by respecting the system SE-Linux policy. What kinds of objects do we support checks on? What sorts of checks? What kind of access can we allow/deny? I guess these detailed description makes amount of this chapter disproportionally increase in the future version. My preference is wikipage to provide this kind of detailed information. http://wiki.postgresql.org/wiki/SEPostgreSQL The contents of above wikipage is now obsoleted, because it assumed SELinux support as a built-in feature. But it is a good time to fix up the description. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] management of large patches
(2011/01/02 14:32), Robert Haas wrote: We're coming the end of the 9.1 development cycle, and I think that there is a serious danger of insufficient bandwidth to handle the large patches we have outstanding. For my part, I am hoping to find the bandwidth to two, MAYBE three major commits between now and the end of 9.1CF4, but I am not positive that I will be able to find even that much time, and the number of major patches vying for attention is considerably greater than that. Quick estimate: : - SE-Linux integration How about feasibility to commit this 3KL patch in the last 45 days? At least, the idea of security provider enables us to maintain a set of hooks and logic to make access control decision independently. I'm available to provide a set of sources for this module at git.postgresql.org, so we can always obtain a working module from here. The worst scenario for us is nothing were progressed in spite of large man-power to review and discuss. It may be more productive to keep features to be committed on the last CF as small as possible, such as hooks to support a part of DDL permissions or pg_regress enhancement to run regression test. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] sepgsql contrib module
(2010/12/27 17:53), Simon Riggs wrote: On Fri, 2010-12-24 at 11:53 +0900, KaiGai Kohei wrote: The attached patch is the modular version of SE-PostgreSQL. Looks interesting. Couple of thoughts... Docs don't mention row-level security. If we don't have it, I think we should say that clearly. Indeed, it is a good idea the document mentions what features are not implemented in this version clearly, not only row-level security, but DDL permissions and so on. I'd like to revise it soon. I think we need a Guide to Security Labels section in the docs. Very soon, because its hard to know what is being delivered and what is not. Does it describe what is security label and the purpose of them? OK, I'd like to add this section here. Is the pg_seclabel table secure? Looks like the labels will be available to read. If we want to control visibility of each labels, we need row-level granularity here. How do we tell if sepgsql is installed? Check existence of GUC variables of sepgsql.*. What happens if someone alters the configuration so that the sepgsql plugin is no longer installed. Does the hidden data become visible? Yes. If sepgsql plugin is uninstalled, the hidden data become visible. But no matter. Since only a person who is allowed to edit postgresql.conf can uninstall it, we cannot uninstall it in run-time. (An exception is loading a malicious module, but we will be able to hook this operation in the future version.) Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] sepgsql contrib module
(2010/12/30 9:34), Simon Riggs wrote: On Thu, 2010-12-30 at 09:26 +0900, KaiGai Kohei wrote: What happens if someone alters the configuration so that the sepgsql plugin is no longer installed. Does the hidden data become visible? Yes. If sepgsql plugin is uninstalled, the hidden data become visible. But no matter. Since only a person who is allowed to edit postgresql.conf can uninstall it, we cannot uninstall it in run-time. (An exception is loading a malicious module, but we will be able to hook this operation in the future version.) IMHO all security labels should be invisible if the provider is not installed correctly. Probably, it needs row-level granularity to control visibility of each entries of pg_seclabel, because all the provider shares same system catalog. So, I don't think this mechanism is feasible right now. That at least prevents us from accidentally de-installing a module and having top secret data be widely available. If you have multiple providers configured, you need to be careful not to allow a provider that incorrectly implements the plugin API, so that prior plugins are no longer effective. Yep. It is responsibility of DBA who tries to set up security providers. DBA has to install only trustable or well-debugged modules (not limited to security providers) to avoid troubles. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] sepgsql contrib module
(2010/12/24 11:53), KaiGai Kohei wrote: There is one another issue to be discussed. We need a special form of regression test. Because SE-PostgreSQL makes access control decision based on security label of the peer process, we need to switch psql process during regression test. (So, I don't include test cases yet.) We have 'runcon' command to launch a child process with specified security label as long as the security policy allows. If we could launch 'psql' by 'runcon' with specified label, we can describe test-cases on the existing framework on 'make installcheck'. An idea is to add an option to pg_regress to launch psql command with a specified wrapper program (like 'runcon'). In this case, each contrib modules kicks with REGRESS_OPTS setting. One thing to be considered is the security label to be given to the 'runcon' is flexible for each *.sql files. The attached patch adds --launcher=COMMAND option to pg_regress. If a command was specified, pg_regress prepends the specified command string in front of psql command. When we use this option, psql command process will launched via the launcher program. Of course, the launcher has responsibility to execute psql correctly.) This example is a case when I run a regression test on cube module. It tries to launch psql using 'runcon -l s0'. [kai...@saba cube]$ make installcheck REGRESS_OPTS=--launcher='runcon -l s0' --dbname=cube_regress make -C ../../src/test/regress pg_regress make[1]: Entering directory `/home/kaigai/repo/pgsql/src/test/regress' make -C ../../../src/port all make[2]: Entering directory `/home/kaigai/repo/pgsql/src/port' make[2]: Nothing to be done for `all'. make[2]: Leaving directory `/home/kaigai/repo/pgsql/src/port' make[1]: Leaving directory `/home/kaigai/repo/pgsql/src/test/regress' ../../src/test/regress/pg_regress --inputdir=. --psqldir=/usr/local/pgsql/bin --launcher='runcon -l s0' --dbname=cube_regress cube (using postmaster on Unix socket, default port) == dropping database cube_regress == DROP DATABASE == creating database cube_regress == CREATE DATABASE ALTER DATABASE == running regression test queries== test cube ... ok = All 1 tests passed. = During the regression test, I checked security context of the process. [kai...@saba ~]$ env LANG=C pstree -Z systemd(`system_u:system_r:init_t:s0') : |-sshd(`unconfined_u:system_r:sshd_t:s0-s0:c0.c1023') | |-sshd(`unconfined_u:system_r:sshd_t:s0-s0:c0.c1023') | | `-sshd(`unconfined_u:system_r:sshd_t:s0-s0:c0.c1023') | | `-bash(`unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023') | |`-make(`unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023') | | `-pg_regress(`unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023') | | `-psql(`unconfined_u:unconfined_r:unconfined_t:s0') It shows us the launcher program drops privileges of c0.c1023 on end of the security label of processes between pg_regress and psql. How about the idea to implement regression test for SE-PostgreSQL, or possible other stuff which depends on environment variables. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com pg_regress-launcher.patch Description: application/octect-stream -- 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] rest of works for security providers in v9.1
(2010/12/13 21:53), Robert Haas wrote: 2010/12/12 KaiGai Koheikai...@ak.jp.nec.com: I'd like to see opinions what facilities should be developed to the current v9.1 development cycle. It seems to me that the next commit after the label-switcher-function patch ought to be a contrib module that implements a basic form of SE-Linux driven permissions checking. I'm pretty unexcited about continuing to add additional facilities that could be used by a hypothetical module without actually seeing that module, and I think that the label-switcher-function patch is the last piece of core infrastructure that is a hard requirement rather than nice to have. I'd rather have a complete feature with limited capabilities than half a feature with really awesome capabilities. It is a good news for me also, because I didn't imagine SE-PostgreSQL module getting upstreamed, even if contrib module. OK, I'll focus on the works to merge the starter-version of SE-PostgreSQL as a contrib module in the last commit fest. Probably, I need to provide its test cases and minimum documentations in addition to the code itself. Anything else? I suspect that getting fine-grained DDL permissions into PostgreSQL 9.1 is not going to happen. There is a significant amount of complexity there and we are getting short on time. It took us three CommitFests to work through the plan we discussed at PGCon, and this isn't so much simpler that I expect to be able to do it in one. Of course, how you want to spend your time is up to you, but count me as a strong vote for postponing this work to 9.2, when there will be ample time to give it the care and attention it needs. Yep, the label-switcher-function might be a good delimiter. I don't find out any disadvantages to postpone getting DDL permissions. I agree with these enhancements being pushed to v9.2 development. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] rest of works for security providers in v9.1
(2010/12/14 1:03), Robert Haas wrote: On Mon, Dec 13, 2010 at 8:32 AM, KaiGai Koheikai...@kaigai.gr.jp wrote: (2010/12/13 21:53), Robert Haas wrote: 2010/12/12 KaiGai Koheikai...@ak.jp.nec.com: I'd like to see opinions what facilities should be developed to the current v9.1 development cycle. It seems to me that the next commit after the label-switcher-function patch ought to be a contrib module that implements a basic form of SE-Linux driven permissions checking. I'm pretty unexcited about continuing to add additional facilities that could be used by a hypothetical module without actually seeing that module, and I think that the label-switcher-function patch is the last piece of core infrastructure that is a hard requirement rather than nice to have. I'd rather have a complete feature with limited capabilities than half a feature with really awesome capabilities. It is a good news for me also, because I didn't imagine SE-PostgreSQL module getting upstreamed, even if contrib module. OK, I'll focus on the works to merge the starter-version of SE-PostgreSQL as a contrib module in the last commit fest. Probably, I need to provide its test cases and minimum documentations in addition to the code itself. Anything else? Extremely detailed instructions on how to test it. Indeed, it will be necessary. Two more questions: How does the contrib module behave when we try to make all the contrib modules on the platform that doesn't provide libselinux? One idea is to add a few checks about selinux environment in the configure script. I counted number of lines of the sepgsql module that implement only currently supported hooks. It has 3.2KL of code not. How about scale of the patch to review? Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] rest of works for security providers in v9.1
(2010/12/14 9:32), Robert Haas wrote: 2010/12/13 KaiGai Koheikai...@ak.jp.nec.com: It is a good news for me also, because I didn't imagine SE-PostgreSQL module getting upstreamed, even if contrib module. OK, I'll focus on the works to merge the starter-version of SE-PostgreSQL as a contrib module in the last commit fest. Probably, I need to provide its test cases and minimum documentations in addition to the code itself. Anything else? Extremely detailed instructions on how to test it. Indeed, it will be necessary. Two more questions: How does the contrib module behave when we try to make all the contrib modules on the platform that doesn't provide libselinux? One idea is to add a few checks about selinux environment in the configure script. That sounds about right. Presumably, the handling would be similar to what we already do for sslinfo, uuid-ossp, or xml2. OK, I'll follow the manner. I counted number of lines of the sepgsql module that implement only currently supported hooks. It has 3.2KL of code not. Uh, wow. That's rather surprising. I thought that it would be measured in hundreds of lines. Aren't the hooks that we implemented a pretty close match for what SE-Linux needs? What is all that code doing? The hooks are deployed well suitable for SE-Linux needs. Because a certain amount of codes are necessary to communicate between kernel and application using right security labels, the fist meaningful stuff requires this size. See below, [kai...@saba sepgsql]$ wc -l * 337 dml.c 222 hooks.c 132 initdb.sepgsql.in 710 label.c 40 language.c 40 largeobject.c 28 Makefile 70 proc.c 141 relation.c 40 schema.c 740 selinux.c 311 sepgsql.h 465 uavc.c 3276 total The largest selinux.c is the routine to communicate between user-space and kernel-space using libselinux. The second largest label.c is the routine to validate security label and to assign initial security labels. The third largest uavc.c is a facility to cache access control decision recently used. The uavc.c might be stripped out for the first version. The dml.c is as a literal. The hooks.c is entrypoints of hooks. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] rest of works for security providers in v9.1
(2010/12/14 10:41), Robert Haas wrote: 2010/12/13 KaiGai Koheikai...@ak.jp.nec.com: I counted number of lines of the sepgsql module that implement only currently supported hooks. It has 3.2KL of code not. Uh, wow. That's rather surprising. I thought that it would be measured in hundreds of lines. Aren't the hooks that we implemented a pretty close match for what SE-Linux needs? What is all that code doing? The hooks are deployed well suitable for SE-Linux needs. Because a certain amount of codes are necessary to communicate between kernel and application using right security labels, the fist meaningful stuff requires this size. See below, [kai...@saba sepgsql]$ wc -l * 337 dml.c 222 hooks.c 132 initdb.sepgsql.in 710 label.c 40 language.c 40 largeobject.c 28 Makefile 70 proc.c 141 relation.c 40 schema.c 740 selinux.c 311 sepgsql.h 465 uavc.c 3276 total The largest selinux.c is the routine to communicate between user-space and kernel-space using libselinux. The second largest label.c is the routine to validate security label and to assign initial security labels. The third largest uavc.c is a facility to cache access control decision recently used. The uavc.c might be stripped out for the first version. The dml.c is as a literal. The hooks.c is entrypoints of hooks. Hmm. So when you say 3200 lines of code, you're not counting documentation, which you definitely need. Yes, I'll need to add documentation and test cases from now, in addition to the code itself. I'd say post what you have and I'll look it over and give feedback. I am not too keen on stripping out the caching mechanism. I'm not sure that's going to do much to simplify the patch. but I'm pretty sure it will make it a lot less useful. The starter version is not intended to use in production system, so I'm optimistic even if caching mechanism is stripped out at first. I believe raw-query to SE-Linux can be easily replaced by the cached- query in the future. (In fact, I switched it before.) Although the scale is not so large, implementations of post creation hooks for language and largeobject can be stripped out, because we don't have any hooks to apply access controls on these objects, so labels of these objects are nonsense right now. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] rest of works for security providers in v9.1
(2010/12/14 12:10), Robert Haas wrote: 2010/12/13 KaiGai Koheikai...@ak.jp.nec.com: The starter version is not intended to use in production system, Well, what's the point, then? I thought we had enough infrastructure in place at this point to build a simple system that, while it wouldn't meet every use case, would be useful to some people for limited purposes. If that's not the case, I'm disappointed. The point is performance is not first priority right now. I guess its performance does not become a major issue, because lack of some features (such as DDL, row-level) are more glaring than its performance. It is an independent topic whether it is useful for limited purpose, or not. For example, when existing permission checks disallow all the DDL commands from web-applications anyway, it will achieve an expected role. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] rest of works for security providers in v9.1
(2010/12/14 12:53), Robert Haas wrote: 2010/12/13 KaiGai Koheikai...@ak.jp.nec.com: (2010/12/14 12:10), Robert Haas wrote: 2010/12/13 KaiGai Koheikai...@ak.jp.nec.com: The starter version is not intended to use in production system, Well, what's the point, then? I thought we had enough infrastructure in place at this point to build a simple system that, while it wouldn't meet every use case, would be useful to some people for limited purposes. If that's not the case, I'm disappointed. The point is performance is not first priority right now. I guess its performance does not become a major issue, because lack of some features (such as DDL, row-level) are more glaring than its performance. It is an independent topic whether it is useful for limited purpose, or not. For example, when existing permission checks disallow all the DDL commands from web-applications anyway, it will achieve an expected role. But you could also install a control into ProcessUtility_hook, right? Yes, it may be an option to get control DDL statement, although it is not fine-grained. Of course, we have a trade-off to the scale of patch. Saying, for example, you must have we_trust_you_a_lot_t to do any DDL? No. Right now, it does not check anything on DDL commands, so all the clients (independent from its security label) are allowed to run any DDL commands, as long as existing permission allows it. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] rest of works for security providers in v9.1
(2010/12/14 13:31), Robert Haas wrote: 2010/12/13 KaiGai Koheikai...@ak.jp.nec.com: (2010/12/14 12:53), Robert Haas wrote: 2010/12/13 KaiGai Koheikai...@ak.jp.nec.com: (2010/12/14 12:10), Robert Haas wrote: 2010/12/13 KaiGai Koheikai...@ak.jp.nec.com: The starter version is not intended to use in production system, Well, what's the point, then? I thought we had enough infrastructure in place at this point to build a simple system that, while it wouldn't meet every use case, would be useful to some people for limited purposes. If that's not the case, I'm disappointed. The point is performance is not first priority right now. I guess its performance does not become a major issue, because lack of some features (such as DDL, row-level) are more glaring than its performance. It is an independent topic whether it is useful for limited purpose, or not. For example, when existing permission checks disallow all the DDL commands from web-applications anyway, it will achieve an expected role. But you could also install a control into ProcessUtility_hook, right? Yes, it may be an option to get control DDL statement, although it is not fine-grained. Of course, we have a trade-off to the scale of patch. I think it's just as important to have a coherent feature set as to make the patch small. Post something, and then we'll discuss. OK, I'll submit a patch without ProcessUtility_hook at first. Then, let's have a discussion what kind of control is available or reasonable on DDL commands. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] rest of works for security providers in v9.1
I'd like to see opinions what facilities should be developed to the current v9.1 development cycle. We have integrated some of facilities to support a starter- version of SE-PostgreSQL. It allows to hook controls on DML permission checks and assign security labels of client and database obejcts either by-hand or automatically. On the other hand, hooks on DDL permission checks are still future works from now. I believe object_access_hook is applied on various kind of DDL permission checks, but we cannot complete to put these hooks at once, because of patch scale. So, I plan to integrate the following four facilities only in the last commit-fest of v9.1, although it still does not cover comprehensive DDL accesses . * Expand object_access_hook to deliver arguments Some of DDL hooks will need to deliver several arguments in addition to OID of the object being modified. For example, a flag to show whether this deletion is cascaded, or not. So, prototype of the object_access_hook needs to be revised. My idea is to add two arguments: an integer variable for number of arguments and an array variable for the additional information. Then, macros will wrap up invocation of this hook to keep the code simple. * Permission checks on object-prep-creation It was not well concluded in the previous discussion, whether two hooks are needed, or one. I think the idea to divide creation hooks into two phases by its role eventually enables to reduce the burden of code management. Now we have OAT_POST_CREATE hooks just after registration of dependency, basically. It is a simple enough basis, and quite natural places to assign new security labels. However, several cases shall be exceptions of the basis, if we try to check permissions also in the post-creation hooks, in addition to default labeling. For example, heap_create_with_catalog() is called from five places, but only two needs permission checks: DefineRelation() and OpenIntoRel(). A few cases are not obvious whether we need permission checks in this invocation, like a code path from make_new_heap(). It defines a new pg_class entry, but the external module cannot determine from the catalog whether is is invoked on the code path that needs permission checks, or not. So, I want OAT_CREATE hooks being just after existing permission checks for the purpose of access control, not default labeling. * Permission checks on object-deletion The existing code put permission checks of object deletion on command handlers like RemoveRelations(), then it invokes functions in dependency.c to drop the specified and dependent objects (if necessary). I think it is straight-forward to put object-deletion hooks next to the existing permission checks. But it is unavailable to check cascaded objects to be removed here. So, it seems to me findDependentObjects() should be exposed to external modules to inform what objects shall be dropped. Unlike old SE-PostgreSQL implementation, I don't consider it is a good idea to put hook within dependency.c, because we need to inform dependency.c whether this deletion is by-hand, or something internals (such as cleanups of temporary objects). * Permission checks on misc easy implementables Apart from the priority of development, it seems to me that we can hook controls at the following commands quite easy. It is an idea to put hooks that we can implement with little impact around the existing codes. - GRANT/REVOKE - COMMENT ON - SECURITY LABEL Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] Label switcher function
Thanks for your reviewing.
The attached patch is a revised version.
I don't have any objections to your comments.
(2010/12/07 4:38), Robert Haas wrote:
2010/11/25 KaiGai Koheikai...@ak.jp.nec.com:
The attached patch is a revised one.
It provides two hooks; the one informs core PG whether the supplied
function needs to be hooked, or not. the other is an actual hook on
prepare, start, end and abort of function invocations.
typedef bool (*needs_function_call_type)(Oid fn_oid);
typedef void (*function_call_type)(FunctionCallEventType event,
FmgrInfo *flinfo, Datum *private);
The hook prototype was a bit modified since the suggestion from
Robert. Because FmgrInfo structure contain OID of the function,
it might be redundant to deliver OID of the function individually.
Rest of parts are revised according to the comment.
I also fixed up source code comments which might become incorrect.
FCET_PREPARE looks completely unnecessary to me. Any necessary
one-time work can easily be done at FCET_START time, assuming that the
private-data field is initialized to (Datum) 0.
It seems to me a reasonable assumption.
I revised the code, and modified source code comments to ensure
the private shall be initialized to (Datum) 0.
I'm fairly certain that the following is not portable:
+ ObjectAddress object = { .classId = ProcedureRelationId,
+ .objectId = fn_oid,
+ .objectSubId = 0 };
Fixed.
I'd suggest renaming needs_function_call_type and function_call_type
to needs_fmgr_hook_type and fmgr_hook_type.
I also think the suggested names are better than before.
According to the renaming, FunctionCallEventType was also renamed to
FmgrHookEventType. Perhaps, it is a reasonable change.
Thanks,
--
KaiGai Kohei kai...@ak.jp.nec.com
contrib/dummy_seclabel/dummy_seclabel.c | 107 -
src/backend/optimizer/util/clauses.c |8 ++
src/backend/utils/fmgr/fmgr.c | 35 ++---
src/include/fmgr.h| 46 +++
src/test/regress/input/security_label.source | 28 +++
src/test/regress/output/security_label.source | 43 ++-
6 files changed, 255 insertions(+), 12 deletions(-)
diff --git a/contrib/dummy_seclabel/dummy_seclabel.c b/contrib/dummy_seclabel/dummy_seclabel.c
index 8bd50a3..237419a 100644
--- a/contrib/dummy_seclabel/dummy_seclabel.c
+++ b/contrib/dummy_seclabel/dummy_seclabel.c
@@ -12,14 +12,110 @@
*/
#include postgres.h
+#include catalog/pg_proc.h
#include commands/seclabel.h
#include miscadmin.h
PG_MODULE_MAGIC;
+PG_FUNCTION_INFO_V1(dummy_client_label);
+
/* Entrypoint of the module */
void _PG_init(void);
+/* SQL functions */
+Datum dummay_client_label(PG_FUNCTION_ARGS);
+
+static const char *client_label = unclassified;
+
+typedef struct {
+ const char *old_label;
+ const char *new_label;
+ Datum next_private;
+} dummy_stack;
+
+static needs_fmgr_hook_type needs_fmgr_next = NULL;
+static fmgr_hook_type fmgr_next = NULL;
+
+static bool
+dummy_needs_fmgr_hook(Oid fn_oid)
+{
+ char *label;
+ bool result = false;
+ ObjectAddress object;
+
+ if (needs_fmgr_next (*needs_fmgr_next)(fn_oid))
+ return true;
+
+ object.classId = ProcedureRelationId;
+ object.objectId = fn_oid;
+ object.objectSubId = 0;
+ label = GetSecurityLabel(object, dummy);
+ if (label strcmp(label, trusted) == 0)
+ result = true;
+
+ if (label)
+ pfree(label);
+
+ return result;
+}
+
+static void
+dummy_fmgr_hook(FmgrHookEventType event,
+FmgrInfo *flinfo, Datum *private)
+{
+ dummy_stack *stack;
+
+ switch (event)
+ {
+ case FHET_START:
+ /*
+ * In the first call, we allocate a pseudo stack for private
+ * datum of the secondary plugin module.
+ */
+ if (*private == 0)
+ {
+stack = MemoryContextAlloc(flinfo-fn_mcxt,
+ sizeof(dummy_stack));
+stack-old_label = NULL;
+stack-new_label = (!superuser() ? secret : top secret);
+stack-next_private = 0;
+
+if (fmgr_next)
+ (*fmgr_next)(event, flinfo, stack-next_private);
+
+*private = PointerGetDatum(stack);
+ }
+ else
+stack = (dummy_stack *)DatumGetPointer(*private);
+
+ stack-old_label = client_label;
+ client_label = stack-new_label;
+ break;
+
+ case FHET_END:
+ case FHET_ABORT:
+ stack = (dummy_stack *)DatumGetPointer(*private);
+
+ client_label = stack-old_label;
+ stack-old_label = NULL;
+ break;
+
+ default:
+ elog(ERROR, unexpected event type: %d, (int)event);
+ break;
+ }
+}
+
+Datum
+dummy_client_label(PG_FUNCTION_ARGS)
+{
+ if (!client_label)
+ PG_RETURN_NULL();
+
+ PG_RETURN_TEXT_P(cstring_to_text(client_label));
+}
+
static void
dummy_object_relabel(const ObjectAddress *object, const char *seclabel)
{
@@ -28,7 +124,8 @@ dummy_object_relabel(const ObjectAddress *object, const char
Re: [HACKERS] [GENERAL] column-level update privs + lock table
(2010/11/30 21:26), Simon Riggs wrote: On Mon, 2010-11-29 at 21:37 -0500, Josh Kupershmidt wrote: I still see little reason to make LOCK TABLE permissions different for column-level vs. table-level UPDATE privileges Agreed. This is the crux of the debate. Why should this inconsistency be allowed to continue? Are there covert channel issues here, KaiGai? Existing database privilege mechanism (and SELinux, etc...) is not designed to handle covert channel attacks, basically. For example, if a user session with column-level UPDATE privilege tries to update a certain column for each seconds depending on the contents of other table X, other session can probably know the contents of table X using iteration of LOCK command without SELECT permission. It is a typical timing channel attack, but it is not a problem that we should try to tackle, is it? Sorry, I don't have a credible idea to solve this inconsistency right now. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] [GENERAL] column-level update privs + lock table
(2010/11/27 9:11), Robert Haas wrote: 2010/11/25 KaiGai Koheikai...@ak.jp.nec.com: (2010/10/16 4:49), Josh Kupershmidt wrote: [Moving to -hackers] On Fri, Oct 15, 2010 at 3:43 AM, Simon Riggssi...@2ndquadrant.com wrote: On Mon, 2010-10-11 at 09:41 -0400, Josh Kupershmidt wrote: On Thu, Oct 7, 2010 at 7:43 PM, Josh Kupershmidtschmi...@gmail.com wrote: I noticed that granting a user column-level update privileges doesn't allow that user to issue LOCK TABLE with any mode other than Access Share. Anyone think this could be added as a TODO? Seems so to me, but you raise on Hackers. Thanks, Simon. Attached is a simple patch to let column-level UPDATE privileges allow a user to LOCK TABLE in a mode higher than Access Share. Small doc. update and regression test update are included as well. Feedback is welcome. I checked your patch, then I'd like to mark it as ready for committer. The point of this patch is trying to solve an incompatible behavior between SELECT ... FOR SHARE/UPDATE and LOCK command. On ExecCheckRTEPerms(), it allows the required accesses when no columns are explicitly specified in the query and the current user has necessary privilege on one of columns within the target relation. If we stand on the perspective that LOCK command should take same privileges with the case when we use SELECT ... FOR SHARE/UPDATE without specifying explicit columns, like COUNT(*), the existing LOCK command seems to me odd. I think this patch fixes the behavior as we expected. I'm not totally convinced that this is the correct behavior. It seems a bit surprising that UPDATE privilege on a single column is enough to lock out all SELECT activity from the table. It's actually a bit surprising that even full-table UPDATE privileges are enough to do this, but this change would allow people to block access to data they can neither see nor modify. That seems counterintuitive, if not a security hole. In my understanding, UPDATE privilege on a single column also allows lock out concurrent updating even if this query tries to update rows partially. Therefore, the current code considers UPDATE privilege on a single column is enough to lock out the table. Right? My comment was from a standpoint which wants consistent behavior between SELECT ... FOR and LOCK command. If we concerned about this behavior, ExecCheckRTEPerms() might be a place where we also should fix. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] [GENERAL] column-level update privs + lock table
(2010/11/29 10:43), Robert Haas wrote: 2010/11/28 KaiGai Koheikai...@ak.jp.nec.com: I'm not totally convinced that this is the correct behavior. It seems a bit surprising that UPDATE privilege on a single column is enough to lock out all SELECT activity from the table. It's actually a bit surprising that even full-table UPDATE privileges are enough to do this, but this change would allow people to block access to data they can neither see nor modify. That seems counterintuitive, if not a security hole. In my understanding, UPDATE privilege on a single column also allows lock out concurrent updating even if this query tries to update rows partially. Therefore, the current code considers UPDATE privilege on a single column is enough to lock out the table. Right? Against concurrent updates and deletes, yes. Against inserts that don't involve potential unique-index conflicts, and against selects, no. My comment was from a standpoint which wants consistent behavior between SELECT ... FOR and LOCK command. Again, nothing about this makes those consistent. If we concerned about this behavior, ExecCheckRTEPerms() might be a place where we also should fix. I don't understand what you're getting at here. I thought the author concerned about inconsistency between them. (Perhaps, I might misunderstood his motivation?) What was the purpose that this patch tries to solve? In the first message of this topic, he concerned as follows: I noticed that granting a user column-level update privileges doesn't allow that user to issue LOCK TABLE with any mode other than Access Share. Do we need to answer: Yes, it is a specification, so you need to grant table level privileges, instead? Thanks -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] [GENERAL] column-level update privs + lock table
(2010/10/16 4:49), Josh Kupershmidt wrote: [Moving to -hackers] On Fri, Oct 15, 2010 at 3:43 AM, Simon Riggssi...@2ndquadrant.com wrote: On Mon, 2010-10-11 at 09:41 -0400, Josh Kupershmidt wrote: On Thu, Oct 7, 2010 at 7:43 PM, Josh Kupershmidtschmi...@gmail.com wrote: I noticed that granting a user column-level update privileges doesn't allow that user to issue LOCK TABLE with any mode other than Access Share. Anyone think this could be added as a TODO? Seems so to me, but you raise on Hackers. Thanks, Simon. Attached is a simple patch to let column-level UPDATE privileges allow a user to LOCK TABLE in a mode higher than Access Share. Small doc. update and regression test update are included as well. Feedback is welcome. I checked your patch, then I'd like to mark it as ready for committer. The point of this patch is trying to solve an incompatible behavior between SELECT ... FOR SHARE/UPDATE and LOCK command. On ExecCheckRTEPerms(), it allows the required accesses when no columns are explicitly specified in the query and the current user has necessary privilege on one of columns within the target relation. If we stand on the perspective that LOCK command should take same privileges with the case when we use SELECT ... FOR SHARE/UPDATE without specifying explicit columns, like COUNT(*), the existing LOCK command seems to me odd. I think this patch fixes the behavior as we expected. BTW, how about backporting this patch? It seems to me a bug fix, although it contains user visible changes. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] contrib: auth_delay module
(2010/11/26 11:35), Fujii Masao wrote:
On Thu, Nov 25, 2010 at 3:18 PM, KaiGai Koheikai...@ak.jp.nec.com wrote:
The attached patch is revised version.
- Logging part within auth_delay was removed. This module now focuses on
injection of a few seconds delay on authentication failed.
- Documentation parts were added like any other contrib modules.
Something like the following is not required? Though I'm not sure
if there is the case where auth_delay is unload.
void
_PG_fini(void)
{
/* Uninstall hooks. */
ClientAuthentication_hook = original_client_auth_hook;
}
I'm not also sure whether we have situation libraries are unloaded.
Right now, internal_unload_library() is just a placeholder, so
it seems to me _PG_fini() is never invoked.
+ if (status != STATUS_OK)
+ {
+ sleep(auth_delay_seconds);
+ }
We should use pg_usleep rather than sleep?
Indeed, pg_usleep() is mainly used rather than sleep().
+ DefineCustomIntVariable(auth_delay.seconds,
+ Seconds to be delayed on
authentication failed,
+ NULL,
+ auth_delay_seconds,
+ 2,
+ 0, INT_MAX,
+ PGC_POSTMASTER,
+ GUC_UNIT_S,
+ NULL,
+ NULL);
Can we relax the context from PGC_POSTMASTER to PGC_SIGHUP?
It seems to me reasonable change.
I'll revise my patch. How about _PG_fini()?
Thanks,
--
KaiGai Kohei kai...@ak.jp.nec.com
--
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] security hooks on object creation
The attached patch is a revised patch. - The utils/hooks.h was renamed to catalog/objectaccess.h - Numeric in the tail of InvokeObjectAccessHook0() has gone. - Fixed bug in ATExecAddColumn; it gave AttributeRelationId to the hook instead of RelationRelationId. In addition, I found that we didn't put post-creation hook on foreign data wrapper, foreign server and user mapping exceptionally. So, I put this hook around their command handler like any other object classes. Thanks, (2010/11/24 12:07), Robert Haas wrote: 2010/11/23 KaiGai Koheikai...@ak.jp.nec.com: What I'm not quite sure about is where to put the definitions you've added to a new file utils/hooks.h; I don't feel that's a very appropriate location. It's tempting to put them in utils/acl.h just because this is vaguely access-control related and that header is already included in most of the right places, but maybe that's too much of a stretch; or perhaps catalog/catalog.h, although that doesn't feel quite right either. If we are going to add a new header file, I still don't like utils/hooks.h much - it's considerably more generic than can be justified by its contents. I don't think utils/acl.h is long-standing right place, because we intended not to restrict the purpose of this hooks to access controls as you mentioned. I think somewhere under the catalog/ directory is a good idea because it hooks events that user wants (eventually) to modify system catalogs. How about catalog/hooks.h, instead of utils/hooks.h? Well, if we're going to create a new header file for this, I think it should be called something like catalog/objectaccess.h, rather than just hooks.h. But I'd rather reuse something that's already there, all things being equal. -- KaiGai Kohei kai...@ak.jp.nec.com diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index dcc53e1..9a38207 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -40,6 +40,7 @@ #include catalog/index.h #include catalog/indexing.h #include catalog/namespace.h +#include catalog/objectaccess.h #include catalog/pg_attrdef.h #include catalog/pg_constraint.h #include catalog/pg_inherits.h @@ -1188,6 +1189,10 @@ heap_create_with_catalog(const char *relname, } } + /* Post creation of new relation */ + InvokeObjectAccessHook(OAT_POST_CREATE, + RelationRelationId, relid, 0); + /* * Store any supplied constraints and defaults. * diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c index 8b4f8c6..1ed108f 100644 --- a/src/backend/catalog/pg_constraint.c +++ b/src/backend/catalog/pg_constraint.c @@ -18,6 +18,7 @@ #include access/heapam.h #include catalog/dependency.h #include catalog/indexing.h +#include catalog/objectaccess.h #include catalog/pg_constraint.h #include catalog/pg_operator.h #include catalog/pg_type.h @@ -360,6 +361,10 @@ CreateConstraintEntry(const char *constraintName, DEPENDENCY_NORMAL); } + /* Post creation of a new constraint */ + InvokeObjectAccessHook(OAT_POST_CREATE, + ConstraintRelationId, conOid, 0); + return conOid; } diff --git a/src/backend/catalog/pg_conversion.c b/src/backend/catalog/pg_conversion.c index 9578184..853ed0e 100644 --- a/src/backend/catalog/pg_conversion.c +++ b/src/backend/catalog/pg_conversion.c @@ -18,6 +18,7 @@ #include access/sysattr.h #include catalog/dependency.h #include catalog/indexing.h +#include catalog/objectaccess.h #include catalog/pg_conversion.h #include catalog/pg_conversion_fn.h #include catalog/pg_namespace.h @@ -131,6 +132,10 @@ ConversionCreate(const char *conname, Oid connamespace, recordDependencyOnOwner(ConversionRelationId, HeapTupleGetOid(tup), conowner); + /* Post creation of a new conversion */ + InvokeObjectAccessHook(OAT_POST_CREATE, + ConversionRelationId, HeapTupleGetOid(tup), 0); + heap_freetuple(tup); heap_close(rel, RowExclusiveLock); diff --git a/src/backend/catalog/pg_namespace.c b/src/backend/catalog/pg_namespace.c index 71ebd7a..978be51 100644 --- a/src/backend/catalog/pg_namespace.c +++ b/src/backend/catalog/pg_namespace.c @@ -17,6 +17,7 @@ #include access/heapam.h #include catalog/dependency.h #include catalog/indexing.h +#include catalog/objectaccess.h #include catalog/pg_namespace.h #include utils/builtins.h #include utils/rel.h @@ -75,5 +76,9 @@ NamespaceCreate(const char *nspName, Oid ownerId) /* Record dependency on owner */ recordDependencyOnOwner(NamespaceRelationId, nspoid, ownerId); + /* Post creation of new schema */ + InvokeObjectAccessHook(OAT_POST_CREATE, + NamespaceRelationId, nspoid, 0); + return nspoid; } diff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c index 73de672..cd169a6 100644 --- a/src/backend/catalog/pg_operator.c +++ b/src/backend/catalog/pg_operator.c @@ -22,6 +22,7 @@ #include catalog/dependency.h #include catalog/indexing.h #include catalog/namespace.h
Re: [HACKERS] Label switcher function
The attached patch is a revised one.
It provides two hooks; the one informs core PG whether the supplied
function needs to be hooked, or not. the other is an actual hook on
prepare, start, end and abort of function invocations.
typedef bool (*needs_function_call_type)(Oid fn_oid);
typedef void (*function_call_type)(FunctionCallEventType event,
FmgrInfo *flinfo, Datum *private);
The hook prototype was a bit modified since the suggestion from
Robert. Because FmgrInfo structure contain OID of the function,
it might be redundant to deliver OID of the function individually.
Rest of parts are revised according to the comment.
I also fixed up source code comments which might become incorrect.
Thanks,
(2010/11/18 11:30), Robert Haas wrote:
2010/11/17 KaiGai Koheikai...@ak.jp.nec.com:
I revised my patch as I attached.
The hook function is modified and consolidated as follows:
typedef enum FunctionCallEventType
{
FCET_BE_HOOKED,
FCET_PREPARE,
FCET_START,
FCET_END,
FCET_ABORT,
} FunctionCallEventType;
typedef Datum (*function_call_event_type)(Oid functionId,
FunctionCallEventType event,
Datum event_arg);
extern PGDLLIMPORT function_call_event_type function_call_event_hook;
Unlike the subject of this e-mail, now it does not focus on only switching
security labels during execution of a certain functions.
For example, we may use this hook to track certain functions for security
auditing, performance tuning, and others.
In the case of SE-PgSQL, it shall return BoolGetDatum(true), if the target
function is configured as a trusted procedure, then, this invocation will
be hooked by fmgr_security_definer. In the first call, it shall compute
the security context to be assigned during execution on FCET_PREPARE event.
Then, it switches to the computed label on the FCET_START event, and
restore it on the FCET_END or ECET_ABORT event.
This seems like it's a lot simpler than before, which is good. It
looks to me as though there should really be two separate hooks,
though, one for what is now FCET_BE_HOOKED and one for everything
else. For FCET_BE_HOOKED, you want a function that takes an Oid and
returns a bool. For the other event types, the functionId and event
arguments are OK, but I think you should forget about the save_datum
stuff and just always pass fcache-flinfo andfcache-private. The
plugin can get the effect of save_datum by passing around whatever
state it needs to hold on to using fcache-private. So:
bool (*needs_function_call_hook)(Oid fn_oid);
void (*function_call_hook)(Oid fn_oid, FunctionCallEventType event,
FmgrInfo flinfo, Datum *private);
Another general comment is that you've not done a very complete job
updating the comments; there are several of them in fmgr.c that are no
longer accurate. Also, please zap the unnecessary whitespace changes.
Thanks,
--
KaiGai Kohei kai...@ak.jp.nec.com
contrib/dummy_seclabel/dummy_seclabel.c | 102 -
src/backend/optimizer/util/clauses.c |8 ++
src/backend/utils/fmgr/fmgr.c | 44 ---
src/include/fmgr.h| 55 +
src/test/regress/input/security_label.source | 28 +++
src/test/regress/output/security_label.source | 43 ++-
6 files changed, 267 insertions(+), 13 deletions(-)
diff --git a/contrib/dummy_seclabel/dummy_seclabel.c b/contrib/dummy_seclabel/dummy_seclabel.c
index 8bd50a3..7acb512 100644
--- a/contrib/dummy_seclabel/dummy_seclabel.c
+++ b/contrib/dummy_seclabel/dummy_seclabel.c
@@ -12,14 +12,105 @@
*/
#include postgres.h
+#include catalog/pg_proc.h
#include commands/seclabel.h
#include miscadmin.h
PG_MODULE_MAGIC;
+PG_FUNCTION_INFO_V1(dummy_client_label);
+
/* Entrypoint of the module */
void _PG_init(void);
+/* SQL functions */
+Datum dummay_client_label(PG_FUNCTION_ARGS);
+
+static const char *client_label = unclassified;
+
+typedef struct {
+ const char *old_label;
+ const char *new_label;
+ Datum next_private;
+} dummy_stack;
+
+static needs_function_call_type needs_function_call_next = NULL;
+static function_call_type function_call_next = NULL;
+
+static bool
+dummy_needs_function_call(Oid fn_oid)
+{
+ char *label;
+ bool result = false;
+ ObjectAddress object = { .classId = ProcedureRelationId,
+ .objectId = fn_oid,
+ .objectSubId = 0 };
+
+ if (needs_function_call_next
+ (*needs_function_call_next)(fn_oid))
+ return true;
+
+ label = GetSecurityLabel(object, dummy);
+ if (label strcmp(label, trusted) == 0)
+ result = true;
+
+ if (label)
+ pfree(label);
+
+ return result;
+}
+
+static void
+dummy_function_call(FunctionCallEventType event,
+ FmgrInfo *flinfo, Datum *private)
+{
+ dummy_stack *stack;
+
+ switch (event)
+ {
+ case FCET_PREPARE
Re: [HACKERS] contrib: auth_delay module
(2010/11/19 16:57), KaiGai Kohei wrote:
(2010/11/18 2:17), Robert Haas wrote:
On Wed, Nov 17, 2010 at 10:32 AM, Ross J. Reedstromreeds...@rice.edu wrote:
On Tue, Nov 16, 2010 at 09:41:37PM -0500, Robert Haas wrote:
On Tue, Nov 16, 2010 at 8:15 PM, KaiGai Koheikai...@ak.jp.nec.com wrote:
If we don't need a PoC module for each new hooks, I'm not strongly
motivated to push it into contrib tree.
How about your opinion?
I'd say let it go, unless someone else feels strongly about it.
I would use this module (rate limit new connection attempts) as soon as
I could. Putting a cap on potential CPU usage on a production DB by either
a blackhat or mistake by a developer caused by a mistake in
configuration (leaving the port accessible) is definitely useful, even
in the face of max_connections. My production apps already have
their connections and seldom need new ones. They all use CPU though.
If KaiGai updates the code per previous discussion, would you be
willing to take a crack at adding documentation?
P.S. Your email client seems to be setting the Reply-To address to a
ridiculous value.
OK, I'll revise my patch according to the previous discussion.
The attached patch is revised version.
- Logging part within auth_delay was removed. This module now focuses on
injection of a few seconds delay on authentication failed.
- Documentation parts were added like any other contrib modules.
Thanks,
--
KaiGai Kohei kai...@ak.jp.nec.com
contrib/Makefile|1 +
contrib/README |5 +++
contrib/auth_delay/Makefile | 14 +++
contrib/auth_delay/auth_delay.c | 71
doc/src/sgml/auth-delay.sgml| 76 +++
doc/src/sgml/contrib.sgml |1 +
doc/src/sgml/filelist.sgml |1 +
7 files changed, 169 insertions(+), 0 deletions(-)
diff --git a/contrib/Makefile b/contrib/Makefile
index e1f2a84..5747bcc 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -6,6 +6,7 @@ include $(top_builddir)/src/Makefile.global
SUBDIRS = \
adminpack \
+ auth_delay \
auto_explain \
btree_gin \
btree_gist \
diff --git a/contrib/README b/contrib/README
index 6d29cfe..a6abd94 100644
--- a/contrib/README
+++ b/contrib/README
@@ -28,6 +28,11 @@ adminpack -
File and log manipulation routines, used by pgAdmin
by Dave Page dp...@vale-housing.co.uk
+auth_delay
+ Add a few second's delay on authentication failed. It enables to make
+ difficult brute-force attacks on database passwords.
+ by KaiGai Kohei kai...@ak.jp.nec.com
+
auto_explain -
Log EXPLAIN output for long-running queries
by Takahiro Itagaki itagaki.takah...@oss.ntt.co.jp
diff --git a/contrib/auth_delay/Makefile b/contrib/auth_delay/Makefile
new file mode 100644
index 000..09d2d54
--- /dev/null
+++ b/contrib/auth_delay/Makefile
@@ -0,0 +1,14 @@
+# contrib/auth_delay/Makefile
+
+MODULES = auth_delay
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/auth_delay
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/contrib/auth_delay/auth_delay.c b/contrib/auth_delay/auth_delay.c
new file mode 100644
index 000..746ac4b
--- /dev/null
+++ b/contrib/auth_delay/auth_delay.c
@@ -0,0 +1,71 @@
+/* -
+ *
+ * auth_delay.c
+ *
+ * Copyright (C) 2010, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * contrib/auth_delay/auth_delay.c
+ *
+ * -
+ */
+#include postgres.h
+
+#include libpq/auth.h
+#include utils/guc.h
+#include utils/timestamp.h
+
+#include unistd.h
+
+PG_MODULE_MAGIC;
+
+void _PG_init(void);
+
+/* GUC Variables */
+static int auth_delay_seconds;
+
+/* Original Hook */
+static ClientAuthentication_hook_type original_client_auth_hook = NULL;
+
+/*
+ * Check authentication
+ */
+static void
+auth_delay_checks(Port *port, int status)
+{
+ /*
+ * Any other plugins which use the ClientAuthentication_hook.
+ */
+ if (original_client_auth_hook)
+ original_client_auth_hook(port, status);
+
+ /*
+ * Inject a few seconds delay on authentication failed.
+ */
+ if (status != STATUS_OK)
+ {
+ sleep(auth_delay_seconds);
+ }
+}
+
+/*
+ * Module Load Callback
+ */
+void
+_PG_init(void)
+{
+ /* Define custome GUC variables */
+ DefineCustomIntVariable(auth_delay.seconds,
+ Seconds to be delayed on authentication failed,
+ NULL,
+ auth_delay_seconds,
+ 2,
+ 0, INT_MAX,
+ PGC_POSTMASTER,
+ GUC_UNIT_S,
+ NULL,
+ NULL);
+ /* Install Hooks */
+ original_client_auth_hook = ClientAuthentication_hook;
+ ClientAuthentication_hook = auth_delay_checks;
+}
diff --git a/doc/src/sgml/auth-delay.sgml b/doc/src/sgml/auth-delay.sgml
new file mode 100644
index 000..372702d
--- /dev
Re: [HACKERS] security hooks on object creation
Thanks for your reviewing, and sorry for the late reply. I've not been available for a few days. (2010/11/22 12:11), Robert Haas wrote: 2010/11/12 KaiGai Koheikai...@ak.jp.nec.com: (2010/11/12 19:34), KaiGai Kohei wrote: I revised my patch according to the prior suggestions. I'm sorry. I revised my patch, but not attached. Please see this attached one. I'm satisfied with this approach, although I intend to change InvokeObjectAccessHook0 to simply InvokeObjectAccessHook before committing it; OK. We have no other object-access-type which takes any arguments right now. It is quite cosmetic things, so we may be able to add the number of arguments later, such as SysCache. and correct your use of AttributeRelationId to RelationRelationId for consistency with the rest of the code. Oops, it was my bug. I'll fix it. What I'm not quite sure about is where to put the definitions you've added to a new file utils/hooks.h; I don't feel that's a very appropriate location. It's tempting to put them in utils/acl.h just because this is vaguely access-control related and that header is already included in most of the right places, but maybe that's too much of a stretch; or perhaps catalog/catalog.h, although that doesn't feel quite right either. If we are going to add a new header file, I still don't like utils/hooks.h much - it's considerably more generic than can be justified by its contents. I don't think utils/acl.h is long-standing right place, because we intended not to restrict the purpose of this hooks to access controls as you mentioned. I think somewhere under the catalog/ directory is a good idea because it hooks events that user wants (eventually) to modify system catalogs. How about catalog/hooks.h, instead of utils/hooks.h? Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] contrib: auth_delay module
(2010/11/18 2:17), Robert Haas wrote: On Wed, Nov 17, 2010 at 10:32 AM, Ross J. Reedstromreeds...@rice.edu wrote: On Tue, Nov 16, 2010 at 09:41:37PM -0500, Robert Haas wrote: On Tue, Nov 16, 2010 at 8:15 PM, KaiGai Koheikai...@ak.jp.nec.com wrote: If we don't need a PoC module for each new hooks, I'm not strongly motivated to push it into contrib tree. How about your opinion? I'd say let it go, unless someone else feels strongly about it. I would use this module (rate limit new connection attempts) as soon as I could. Putting a cap on potential CPU usage on a production DB by either a blackhat or mistake by a developer caused by a mistake in configuration (leaving the port accessible) is definitely useful, even in the face of max_connections. My production apps already have their connections and seldom need new ones. They all use CPU though. If KaiGai updates the code per previous discussion, would you be willing to take a crack at adding documentation? P.S. Your email client seems to be setting the Reply-To address to a ridiculous value. OK, I'll revise my patch according to the previous discussion. Please wait for about one week. I have a big event in this weekend. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] Label switcher function
(2010/11/18 11:30), Robert Haas wrote:
2010/11/17 KaiGai Koheikai...@ak.jp.nec.com:
I revised my patch as I attached.
The hook function is modified and consolidated as follows:
typedef enum FunctionCallEventType
{
FCET_BE_HOOKED,
FCET_PREPARE,
FCET_START,
FCET_END,
FCET_ABORT,
} FunctionCallEventType;
typedef Datum (*function_call_event_type)(Oid functionId,
FunctionCallEventType event,
Datum event_arg);
extern PGDLLIMPORT function_call_event_type function_call_event_hook;
Unlike the subject of this e-mail, now it does not focus on only switching
security labels during execution of a certain functions.
For example, we may use this hook to track certain functions for security
auditing, performance tuning, and others.
In the case of SE-PgSQL, it shall return BoolGetDatum(true), if the target
function is configured as a trusted procedure, then, this invocation will
be hooked by fmgr_security_definer. In the first call, it shall compute
the security context to be assigned during execution on FCET_PREPARE event.
Then, it switches to the computed label on the FCET_START event, and
restore it on the FCET_END or ECET_ABORT event.
This seems like it's a lot simpler than before, which is good. It
looks to me as though there should really be two separate hooks,
though, one for what is now FCET_BE_HOOKED and one for everything
else. For FCET_BE_HOOKED, you want a function that takes an Oid and
returns a bool. For the other event types, the functionId and event
arguments are OK, but I think you should forget about the save_datum
stuff and just always pass fcache-flinfo andfcache-private. The
plugin can get the effect of save_datum by passing around whatever
state it needs to hold on to using fcache-private. So:
bool (*needs_function_call_hook)(Oid fn_oid);
void (*function_call_hook)(Oid fn_oid, FunctionCallEventType event,
FmgrInfo flinfo, Datum *private);
It seems to me a good idea. The characteristic of FCET_BE_HOOKED event
type was a bit different from other three event types.
Please wait for about a week to revise my patch.
Another general comment is that you've not done a very complete job
updating the comments; there are several of them in fmgr.c that are no
longer accurate. Also, please zap the unnecessary whitespace changes.
Indeed, the comment at middle of the fmgr_info_cxt_security() and just
above definition of the fmgr_security_definer() are not correct.
Did you notice anything else?
Thanks,
--
KaiGai Kohei kai...@ak.jp.nec.com
--
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] contrib: auth_delay module
(2010/11/15 11:50), Robert Haas wrote: On Thu, Nov 4, 2010 at 10:04 AM, Robert Haasrobertmh...@gmail.com wrote: On Thu, Nov 4, 2010 at 6:35 AM, Stephen Frostsfr...@snowman.net wrote: * Jan Urbański (wulc...@wulczer.org) wrote: On 04/11/10 14:09, Robert Haas wrote: Hmm, I wonder how useful this is given that restriction. As KaiGai mentined, it's more to make bruteforcing difficult (read: tmie consuming), right? Which it would still do, since the attacker would be bumping up against max_connections. max_connections would be a DOS point, but that's no different from today. Other things could be put in place to address that (max # of connections from a given IP or range could be implemented using iptables, as an example). 5 second delay w/ max connections at 100 would mean max of 20 attempts per second, no? That's alot fewer than 100*(however many attempts can be done in a second). Doing a stupid while true; psql -d blah; done managed to get 50 successful ident auths+no-db-found errors done in a second on one box here. 5000 20, and I wasn't even trying. OK. I was just asking. I don't object to it if people think it's useful, especially if they are looking at it as I would actually use this on my system rather than I can imagine a hypothetical person using this. I haven't heard anyone say yes, I would actually use this on my system? Any takers? My original motivation is to provide a proof of concept module for the ClientAuthentication_hook, to show this hook is useful to other purposes not only getpeercon() in SE-PgSQL. If we don't need a PoC module for each new hooks, I'm not strongly motivated to push it into contrib tree. How about your opinion? If we're to commit this, then the patch needs to add a new file authdelay.smgl, fill it in with appropriate contents, and update contrib.sgml and filelist.sgml accordingly. Sorry, I missed the manner of contrib modules. It seems to me the dummy_seclabel module also lacks these documentation updates, although its purpose is regression testing. I also note that the patch offers the ability to log superuser logins. Since that seems a bit off-topic for a contrib module called auth_delay, and since we already have a GUC called log_connections, I'm inclined to propose removing that part. I agree with the suggestion. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] Label switcher function
I revised my patch as I attached.
The hook function is modified and consolidated as follows:
typedef enum FunctionCallEventType
{
FCET_BE_HOOKED,
FCET_PREPARE,
FCET_START,
FCET_END,
FCET_ABORT,
} FunctionCallEventType;
typedef Datum (*function_call_event_type)(Oid functionId,
FunctionCallEventType event,
Datum event_arg);
extern PGDLLIMPORT function_call_event_type function_call_event_hook;
Unlike the subject of this e-mail, now it does not focus on only switching
security labels during execution of a certain functions.
For example, we may use this hook to track certain functions for security
auditing, performance tuning, and others.
In the case of SE-PgSQL, it shall return BoolGetDatum(true), if the target
function is configured as a trusted procedure, then, this invocation will
be hooked by fmgr_security_definer. In the first call, it shall compute
the security context to be assigned during execution on FCET_PREPARE event.
Then, it switches to the computed label on the FCET_START event, and
restore it on the FCET_END or ECET_ABORT event.
I also fixed up regression test, dummy_seclabel module and its
documentation as Robert pointed out in another topic.
Thanks,
(2010/11/14 13:16), KaiGai Kohei wrote:
(2010/11/14 11:19), Robert Haas wrote:
2010/11/12 KaiGai Koheikai...@kaigai.gr.jp:
The attached patch allows the security label provider to switch
security label of the client during execution of certain functions.
I named it as label switcher function; also called as trusted-
procedure in SELinux community.
This feature is quite similar idea toward security definer function,
or set-uid program on operating system. It allows label providers
to switch its internal state that holds security label of the
client, then restore it.
If and when a label provider said the function being invoked is
a label-switcher, fmgr_security_definer() traps this invocation
and set some states just before actual invocations.
We added three new hooks for security label provider.
The get_client_label and set_client_label allows the PG core to
save and restore security label of the client; which is mostly
just an internal state of plugin module.
And, the get_switched_label shall return NULL or a valid label
if the supplied function is a label switcher. It also informs
the PG core whether the function is switcher or not.
I don't see why the plugin needs to expose the label stack to core PG.
If the plugin needs a label stack, it can do that all on its own. I
see that we need the hooks to allow the plugin to selectively disable
inlining and to gain control when function execution starts and ends
(or aborts) but I don't think the exact manipulations that the plugin
chooses to do at that point need to be visible to core PG.
Hmm. I designed this patch according to the implementation of existing
security definer function, but it is not a only design.
Does the label stack means that this patch touches xact.c, doesn't it?
Yes, if we have above three hooks around function calls, the core PG
does not need to manage a label stack.
However, I want fmgr_security_definer_cache to have a field to save
private opaque data, because it is not a very-light step to ask SE-Linux
whether the function is trusted-procedure and to allocate a string to
be applied during execution, although switching is a very-light step.
So, I want to compute it at first time of the function calls, like as
security definer function checks syscache at once.
Of course, it is a private opaque data, it will be open for other usage.
For SE-Linux, how do you intend to determine whether or not the
function is a trusted procedure? Will that be a function of the
security label applied to it?
When the function being invoked has a special security label with
a type_transition rule on the current client's label in the
security policy, SE-Linux decides the function is trusted procedure.
In other words, we can know whether or not the function is a trusted
procedure by asking to the security policy. It is a task of the plugin.
Thanks,
--
KaiGai Kohei kai...@ak.jp.nec.com
diff --git a/contrib/dummy_seclabel/dummy_seclabel.c b/contrib/dummy_seclabel/dummy_seclabel.c
index 8bd50a3..557cc0c 100644
--- a/contrib/dummy_seclabel/dummy_seclabel.c
+++ b/contrib/dummy_seclabel/dummy_seclabel.c
@@ -12,14 +12,156 @@
*/
#include postgres.h
+#include catalog/pg_proc.h
#include commands/seclabel.h
#include miscadmin.h
PG_MODULE_MAGIC;
+PG_FUNCTION_INFO_V1(dummy_client_label);
+
+Datum dummay_client_label(PG_FUNCTION_ARGS);
+
/* Entrypoint of the module */
void _PG_init(void);
+static const char *client_label = unclassified;
+
+static function_call_event_type function_call_event_next = NULL;
+
+typedef struct {
+ Datum self;
+ Datum next;
+} private_stack;
+
+static Datum
+dummy_function_call(Oid
Re: [HACKERS] Label switcher function
(2010/11/14 11:19), Robert Haas wrote: 2010/11/12 KaiGai Koheikai...@kaigai.gr.jp: The attached patch allows the security label provider to switch security label of the client during execution of certain functions. I named it as label switcher function; also called as trusted- procedure in SELinux community. This feature is quite similar idea toward security definer function, or set-uid program on operating system. It allows label providers to switch its internal state that holds security label of the client, then restore it. If and when a label provider said the function being invoked is a label-switcher, fmgr_security_definer() traps this invocation and set some states just before actual invocations. We added three new hooks for security label provider. The get_client_label and set_client_label allows the PG core to save and restore security label of the client; which is mostly just an internal state of plugin module. And, the get_switched_label shall return NULL or a valid label if the supplied function is a label switcher. It also informs the PG core whether the function is switcher or not. I don't see why the plugin needs to expose the label stack to core PG. If the plugin needs a label stack, it can do that all on its own. I see that we need the hooks to allow the plugin to selectively disable inlining and to gain control when function execution starts and ends (or aborts) but I don't think the exact manipulations that the plugin chooses to do at that point need to be visible to core PG. Hmm. I designed this patch according to the implementation of existing security definer function, but it is not a only design. Does the label stack means that this patch touches xact.c, doesn't it? Yes, if we have above three hooks around function calls, the core PG does not need to manage a label stack. However, I want fmgr_security_definer_cache to have a field to save private opaque data, because it is not a very-light step to ask SE-Linux whether the function is trusted-procedure and to allocate a string to be applied during execution, although switching is a very-light step. So, I want to compute it at first time of the function calls, like as security definer function checks syscache at once. Of course, it is a private opaque data, it will be open for other usage. For SE-Linux, how do you intend to determine whether or not the function is a trusted procedure? Will that be a function of the security label applied to it? When the function being invoked has a special security label with a type_transition rule on the current client's label in the security policy, SE-Linux decides the function is trusted procedure. In other words, we can know whether or not the function is a trusted procedure by asking to the security policy. It is a task of the plugin. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] security hooks on object creation
I revised my patch according to the prior suggestions.
Invocation of the hooks is encapsulated within macro, not function:
+ #define InvokeObjectAccessHook0(access,classId,objectId,subId)\
+ do {\
+ if (object_access_hook) \
+ (*object_access_hook)((access),(classId),(objectId),(subId)); \
+ } while(0)
The 0 of tail means that it does not takes any arguments except for
object-ids, like syscache code.
It will reduce impact when we want to add arguments of the hooks.
In the previous version, it just support seven object classes that is
allowed to assign security labels. But, Robert pointed out the purpose
of post-creation hook is limited to security labeling. So, I expand its
coverage into all the commentable object classes.
- relations: heap_create_with_catalog()
- constraint: CreateConstraintEntry()
- conversion: ConversionCreate()
- schema: NamespaceCreate()
- operator:OperatorCreate() and OperatorShellMake()
- procedure: ProcedureCreate()
- type:TypeCreate() and TypeShellMake()
- database:createdb()
- cast:CreateCast()
- opfamily:CreateOpFamily()
- opclass: DefineOpClass()
- language:create_proc_lang()
- attribute: ATExecAddColumn()
- tablespace: CreateTableSpace()
- trigger: CreateTrigger()
- ts_parser: DefineTSParser()
- ts_dict: DefineTSDictionary()
- ts_template: DefineTSTemplate()
- ts_config: DefineTSConfiguration()
- role:CreateRole()
- rule:InsertRule()
- largeobject: inv_create()
The post-creation hooks are put on the place just after adding dependency
of the new object, if the object class uses dependency mechanism.
I believe it will be a clear guidance for the future maintenance works.
Thanks,
(2010/11/11 7:41), KaiGai Kohei wrote:
(2010/11/11 3:00), Robert Haas wrote:
On Wed, Nov 10, 2010 at 8:33 AM, KaiGai Koheikai...@kaigai.gr.jp wrote:
(2010/11/10 13:06), Robert Haas wrote:
In this patch, we put InvokeObjectAccessHook0 on the following functions.
- heap_create_with_catalog() for relations/attributes
- ATExecAddColumn() for attributes
- NamespaceCreate() for schemas
- ProcedureCreate() for aggregates/functions
- TypeCreate() and TypeShellMake() for types
- create_proc_lang() for procedural languages
- inv_create() for large objects
I think you ought to try to arrange to avoid the overhead of a
function call in the common case where nobody's using the hook.
That's why I originally suggested making InvokeObjectAccessHook() a
macro around the actual function call.
Hmm. Although I have little preference here, the penalty to call
an empty function (when no plugins are installed) is not visible,
because frequency of DDL commands are not high.
Even so, is it necessary to replace them by macros?
It's a fair point. I'm open to other opinions but my vote is to shove
a macro in there. A pointer test is cheaper than a function call, and
doesn't really complicate things much.
Since I have no strong preference function call here, so, I'll revise my
patch according to your vote.
Thanks,
--
KaiGai Kohei kai...@ak.jp.nec.com
--
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] security hooks on object creation
(2010/11/12 19:34), KaiGai Kohei wrote:
I revised my patch according to the prior suggestions.
I'm sorry. I revised my patch, but not attached.
Please see this attached one.
Thanks,
Invocation of the hooks is encapsulated within macro, not function:
+ #define InvokeObjectAccessHook0(access,classId,objectId,subId)\
+ do {\
+ if (object_access_hook) \
+ (*object_access_hook)((access),(classId),(objectId),(subId)); \
+ } while(0)
The 0 of tail means that it does not takes any arguments except for
object-ids, like syscache code.
It will reduce impact when we want to add arguments of the hooks.
In the previous version, it just support seven object classes that is
allowed to assign security labels. But, Robert pointed out the purpose
of post-creation hook is limited to security labeling. So, I expand its
coverage into all the commentable object classes.
- relations:heap_create_with_catalog()
- constraint: CreateConstraintEntry()
- conversion: ConversionCreate()
- schema: NamespaceCreate()
- operator: OperatorCreate() and OperatorShellMake()
- procedure:ProcedureCreate()
- type: TypeCreate() and TypeShellMake()
- database: createdb()
- cast: CreateCast()
- opfamily: CreateOpFamily()
- opclass: DefineOpClass()
- language: create_proc_lang()
- attribute:ATExecAddColumn()
- tablespace: CreateTableSpace()
- trigger: CreateTrigger()
- ts_parser:DefineTSParser()
- ts_dict: DefineTSDictionary()
- ts_template: DefineTSTemplate()
- ts_config:DefineTSConfiguration()
- role: CreateRole()
- rule: InsertRule()
- largeobject: inv_create()
The post-creation hooks are put on the place just after adding dependency
of the new object, if the object class uses dependency mechanism.
I believe it will be a clear guidance for the future maintenance works.
Thanks,
(2010/11/11 7:41), KaiGai Kohei wrote:
(2010/11/11 3:00), Robert Haas wrote:
On Wed, Nov 10, 2010 at 8:33 AM, KaiGai Koheikai...@kaigai.gr.jp wrote:
(2010/11/10 13:06), Robert Haas wrote:
In this patch, we put InvokeObjectAccessHook0 on the following functions.
- heap_create_with_catalog() for relations/attributes
- ATExecAddColumn() for attributes
- NamespaceCreate() for schemas
- ProcedureCreate() for aggregates/functions
- TypeCreate() and TypeShellMake() for types
- create_proc_lang() for procedural languages
- inv_create() for large objects
I think you ought to try to arrange to avoid the overhead of a
function call in the common case where nobody's using the hook.
That's why I originally suggested making InvokeObjectAccessHook() a
macro around the actual function call.
Hmm. Although I have little preference here, the penalty to call
an empty function (when no plugins are installed) is not visible,
because frequency of DDL commands are not high.
Even so, is it necessary to replace them by macros?
It's a fair point. I'm open to other opinions but my vote is to shove
a macro in there. A pointer test is cheaper than a function call, and
doesn't really complicate things much.
Since I have no strong preference function call here, so, I'll revise my
patch according to your vote.
Thanks,
--
KaiGai Kohei kai...@ak.jp.nec.com
*** a/src/backend/catalog/heap.c
--- b/src/backend/catalog/heap.c
***
*** 63,68
--- 63,69
#include utils/acl.h
#include utils/builtins.h
#include utils/fmgroids.h
+ #include utils/hooks.h
#include utils/inval.h
#include utils/lsyscache.h
#include utils/relcache.h
***
*** 1188,1193 heap_create_with_catalog(const char *relname,
--- 1189,1198
}
}
+ /* Post creation of new relation */
+ InvokeObjectAccessHook0(OAT_POST_CREATE,
+ RelationRelationId, relid, 0);
+
/*
* Store any supplied constraints and defaults.
*
*** a/src/backend/catalog/pg_constraint.c
--- b/src/backend/catalog/pg_constraint.c
***
*** 25,30
--- 25,31
#include utils/array.h
#include utils/builtins.h
#include utils/fmgroids.h
+ #include utils/hooks.h
#include utils/lsyscache.h
#include utils/rel.h
#include utils/syscache.h
***
*** 360,365 CreateConstraintEntry(const char *constraintName,
--- 361,370
DEPENDENCY_NORMAL);
}
+ /* Post creation of a new constraint */
+ InvokeObjectAccessHook0(OAT_POST_CREATE,
+ ConstraintRelationId, conOid, 0);
+
return conOid;
}
*** a/src/backend/catalog/pg_conversion.c
--- b/src/backend/catalog/pg_conversion.c
***
*** 27,32
--- 27,33
#include utils/acl.h
#include utils/builtins.h
#include utils/fmgroids.h
+ #include utils/hooks.h
#include utils/rel.h
#include utils/syscache.h
[HACKERS] Label switcher function
The attached patch allows the security label provider to switch security label of the client during execution of certain functions. I named it as label switcher function; also called as trusted- procedure in SELinux community. This feature is quite similar idea toward security definer function, or set-uid program on operating system. It allows label providers to switch its internal state that holds security label of the client, then restore it. If and when a label provider said the function being invoked is a label-switcher, fmgr_security_definer() traps this invocation and set some states just before actual invocations. We added three new hooks for security label provider. The get_client_label and set_client_label allows the PG core to save and restore security label of the client; which is mostly just an internal state of plugin module. And, the get_switched_label shall return NULL or a valid label if the supplied function is a label switcher. It also informs the PG core whether the function is switcher or not. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp pgsql-switcher-function.1.patch Description: application/octect-stream -- 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] security hooks on object creation
(2010/11/10 13:06), Robert Haas wrote: In this patch, we put InvokeObjectAccessHook0 on the following functions. - heap_create_with_catalog() for relations/attributes - ATExecAddColumn() for attributes - NamespaceCreate() for schemas - ProcedureCreate() for aggregates/functions - TypeCreate() and TypeShellMake() for types - create_proc_lang() for procedural languages - inv_create() for large objects I think you ought to try to arrange to avoid the overhead of a function call in the common case where nobody's using the hook. That's why I originally suggested making InvokeObjectAccessHook() a macro around the actual function call. Hmm. Although I have little preference here, the penalty to call an empty function (when no plugins are installed) is not visible, because frequency of DDL commands are not high. Even so, is it necessary to replace them by macros? I don't want to refer to this as a framework for enhanced security providers. Let's stick with the term object access hook. Calling it an enhanced security provider overspecifies; it could equally well be used for, say, logging. OK. As Itagaki-san also pointed out, we may be able to utilize the hooks in other purposes. Although I designed it in similar manner with security label provider, I'll revise it like as other hooks doing. Is there any compelling reason not to apply this to every object type in the system (e.g. all the ones COMMENT can apply to)? I don't see any reason to restrict it to the set of objects to which it's sensible to apply security labels. Because I thought too many hooks within one patch gives burden to reviewers, so I restricted it on a part of object classes in this version. However,it is not a compelling reason. OK, I'll try to revise the patch soon. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] security hooks on object creation
(2010/11/11 3:00), Robert Haas wrote: On Wed, Nov 10, 2010 at 8:33 AM, KaiGai Koheikai...@kaigai.gr.jp wrote: (2010/11/10 13:06), Robert Haas wrote: In this patch, we put InvokeObjectAccessHook0 on the following functions. - heap_create_with_catalog() for relations/attributes - ATExecAddColumn() for attributes - NamespaceCreate() for schemas - ProcedureCreate() for aggregates/functions - TypeCreate() and TypeShellMake() for types - create_proc_lang() for procedural languages - inv_create() for large objects I think you ought to try to arrange to avoid the overhead of a function call in the common case where nobody's using the hook. That's why I originally suggested making InvokeObjectAccessHook() a macro around the actual function call. Hmm. Although I have little preference here, the penalty to call an empty function (when no plugins are installed) is not visible, because frequency of DDL commands are not high. Even so, is it necessary to replace them by macros? It's a fair point. I'm open to other opinions but my vote is to shove a macro in there. A pointer test is cheaper than a function call, and doesn't really complicate things much. Since I have no strong preference function call here, so, I'll revise my patch according to your vote. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] security hooks on object creation
The attached patch provides plugin modules a hook just after object
creation time. In typical use cases, it enables to assign default
security labels on object creation by the external security providers.
As Robert suggested before, it provides a generic purpose main hook.
It takes an enum of ObjectAccessType which informs plugins what kind
of accesses are required, and identifier of the object to be referenced.
But, in this version, no additional information, such as new name in
ALTER xxx RENAME TO, are not supported.
The ObjectAccessType is defined as follows:
typedef enum ObjectAccessType {
OAT_POST_CREATE,/* Post creation fixups; such as security labeling */
} ObjectAccessType;
We will support more complete kind of access types in the future version,
however, we focus on default labeling rather than DDL permissions right
now, so only OAT_POST_CREATE is defined here.
Perhaps, we will add OAT_ALTER, OAT_DROP, OAT_COMMENT and so on.
In this patch, I put hooks on the place just after creation of database
objects that we can assign security labels. (schema, relation, attribute,
procedure, language, type, large object)
However, I didn't touch or move CommandCounterIncrement() yet, although
we had a long discussion MVCC visibility of new object.
Because I'm not clear whether it is really preferable to inject CCIs
onto random points such as TypeCreate() or ProcedureCreate() under
development of the version killed by myself.
(In other words, it was simply ugly...)
At least, we can see the new entries with SnapshotSelf, although we will
pay performance penalty. If so, it is an idea not to touch anything
related to CCIs.
The purpose of post creation hooks are assignment of default security
labels, not DDL permissions. So, it is not a bad idea not to touch
routines related to CCIs in the earlier version of external security
provider.
In this patch, we put InvokeObjectAccessHook0 on the following functions.
- heap_create_with_catalog() for relations/attributes
- ATExecAddColumn() for attributes
- NamespaceCreate() for schemas
- ProcedureCreate() for aggregates/functions
- TypeCreate() and TypeShellMake() for types
- create_proc_lang() for procedural languages
- inv_create() for large objects
Thanks,
--
KaiGai Kohei kai...@ak.jp.nec.com
*** a/src/backend/catalog/heap.c
--- b/src/backend/catalog/heap.c
***
*** 61,66
--- 61,67
#include storage/freespace.h
#include storage/smgr.h
#include utils/acl.h
+ #include utils/esp.h
#include utils/builtins.h
#include utils/fmgroids.h
#include utils/inval.h
***
*** 1189,1194 heap_create_with_catalog(const char *relname,
--- 1190,1202
}
/*
+ * If installed, ESP can assign initial properties (such as security
+ * labels) of the relation.
+ */
+ InvokeObjectAccessHook0(OAT_POST_CREATE,
+ RelationRelationId, relid, 0);
+
+ /*
* Store any supplied constraints and defaults.
*
* NB: this may do a CommandCounterIncrement and rebuild the relcache
*** a/src/backend/catalog/pg_namespace.c
--- b/src/backend/catalog/pg_namespace.c
***
*** 19,24
--- 19,25
#include catalog/indexing.h
#include catalog/pg_namespace.h
#include utils/builtins.h
+ #include utils/esp.h
#include utils/rel.h
#include utils/syscache.h
***
*** 75,79 NamespaceCreate(const char *nspName, Oid ownerId)
--- 76,87
/* Record dependency on owner */
recordDependencyOnOwner(NamespaceRelationId, nspoid, ownerId);
+ /*
+ * If installed, ESP can assign initial properties (such as security
+ * labels) of the new namespace.
+ */
+ InvokeObjectAccessHook0(OAT_POST_CREATE,
+ NamespaceRelationId, nspoid, 0);
+
return nspoid;
}
*** a/src/backend/catalog/pg_proc.c
--- b/src/backend/catalog/pg_proc.c
***
*** 32,37
--- 32,38
#include tcop/pquery.h
#include tcop/tcopprot.h
#include utils/acl.h
+ #include utils/esp.h
#include utils/builtins.h
#include utils/lsyscache.h
#include utils/syscache.h
***
*** 614,619 ProcedureCreate(const char *procedureName,
--- 615,627
nnewmembers, newmembers);
}
+ /*
+ * If installed, ESP can assign initial properties (such as security
+ * labels) of the new function.
+ */
+ InvokeObjectAccessHook0(OAT_POST_CREATE,
+ ProcedureRelationId, retval, 0);
+
heap_freetuple(tup);
heap_close(rel, RowExclusiveLock);
*** a/src/backend/catalog/pg_type.c
--- b/src/backend/catalog/pg_type.c
***
*** 26,31
--- 26,32
#include miscadmin.h
#include parser/scansup.h
#include utils/acl.h
+ #include utils/esp.h
#include utils/builtins.h
#include utils/fmgroids.h
#include utils/lsyscache.h
***
*** 156,161 TypeShellMake(const char *typeName, Oid typeNamespace, Oid ownerId)
--- 157,169
false);
/*
+ * If installed, ESP can assign initial properties (such as security
Re: [HACKERS] security hooks on object creation
(2010/11/09 20:34), Itagaki Takahiro wrote: 2010/11/9 KaiGai Koheikai...@ak.jp.nec.com: The attached patch provides plugin modules a hook just after object creation time. In typical use cases, it enables to assign default security labels on object creation by the external security providers. It looks like DDL Trigger on other database products. Do we need to consider both security hooks and DDL triggers now? Or, is it enough to design DLL triggers after the hooks are merged? Low-level hooks might be better for security providers because SQL-level triggers could be uninstall by superusers. An interesting viewpoint. Does the DDL trigger allow us to do something on CREATE/ALTER/DROP command? One thing we need to pay attention is that CREATE command is an exception from any other DDL commands, because the database object to be modified does not exist before the actual works. So, I'm saying we need both of prep/post creation hooks in the world of complete features. Meanwhile, I don't think we need security hooks post ALTER/DROP commands. Thus, we will put security hooks next to the existing permission checks, not after the actual works of these commands. Is it reasonable for DDL triggers (if it has something like BEFORE/AFTER)? Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] contrib: auth_delay module
The attached patch is a contrib module to inject a few seconds delay on authentication failed. It is also a proof of the concept using the new ClientAuthentication_hook. This module provides a similar feature to pam_faildelay on operating systems. Injection of a few seconds delay on authentication fails prevents (or makes hard at least) brute-force attacks, because it limits number of candidates that attacker can verify within a unit of time. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp pgsql-v9.1-auth-delay.1.patch Description: application/octect-stream -- 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] contrib: auth_delay module
(2010/11/04 22:05), Itagaki Takahiro wrote: 2010/11/4 KaiGai Koheikai...@kaigai.gr.jp: The attached patch is a contrib module to inject a few seconds delay on authentication failed. It is also a proof of the concept using the new ClientAuthentication_hook. This module provides a similar feature to pam_faildelay on operating systems. Injection of a few seconds delay on authentication fails prevents (or makes hard at least) brute-force attacks, because it limits number of candidates that attacker can verify within a unit of time. +1 for the feature. We have post_auth_delay parameter, but it has different purpose; it's as DEVELOPER_OPTIONS for delay to attach a debugger. BTW, the module could save CPU usage of the server on attacks, but do nothing about connection flood attacks, right? If an attacker attacks the server with multiple connections, the server still consumes max_connections even with the module. Good point. The pam_faildelay being the model of this feature also does nothing for flood of connections attack. However, if it closes the connection immediately, the attacker can try to verify next candidate very soon. Do you have any good idea? Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] security hook on authorization
. On the other hand, eventually we will put a hook on CheckMyDatabase(), if so, it is not a bad idea to kill two birds in a stone (hook) now. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] leaky views, yet again
(2010/10/14 1:52), Tom Lane wrote: Robert Haasrobertmh...@gmail.com writes: On Wed, Oct 13, 2010 at 11:45 AM, Tom Lanet...@sss.pgh.pa.us wrote: That's all true, but you have to consider how much the obstacle actually gets in their way versus how painful it is on your end to create and maintain the obstacle. �I don't think this proposed patch measures up very well on either end of that tradeoff. I think it would behoove us to try to separate concerns about this particular patch from concerns about the viability of the whole approach. Whether or not it's useful to do X is a different question than whether it can be done with few enough lines of code and/or whether this patch actually does it well. I think I left the wrong impression: I'm concerned about the whole approach. I haven't even read this particular patch lately. I think that trying to guarantee that the planner applies independent constraints in a particular order will be expensive, fragile, and prone to recurring security bugs no matter how it's implemented --- unless you do it by lobotomizing query pullup/pushdown, which seems unacceptable from a performance standpoint. Just to give one example of what this patch misses (probably; as I said I haven't read it lately), what about selectivity estimation? One of the things we like to do when we have an otherwise-unknown function is to try it on all the histogram members and see what percentage yield true. That might already represent enough of an information leak to be objectionable ... and yet, if we don't do it, the consequences for performance could be pretty horrid because we'll have to work without any reasonable selectivity estimate at all. There are cases where this technique isn't applied at the moment but probably should be, which is why I characterize the leak-prevention idea as creating future security issues: doing that is a constraint that will have to be accounted for in every future planner change, and we are certain to miss the implications sometimes. Sorry, I might misread what you pointed out. Do you see oprrest/oprjoin being internally invoked as a problem? Well, I also think it is a problem, as long as they can be installed by non-superusers. But, it is a separated problem from the row-level security issues. In my opinion, origin of the matter is incorrect checks on creation of operators. It allows non-superusers to define a new operator with restriction and join estimator as long as he has EXECUTE privilege on the functions. (not EXECUTE privilege of actual user who invokes this function on estimation phase!) Then, the optimizer may internally invoke these functions without any privilege checks on either the function or the table to be estimated. If a malicious one tries to make other users to invoke a trojan-horse, he can define a trappy operator with malicious estimator functions, cannot it? I think it is a situation that we should check superuser privilege which means the specified functions have no malicious intention and equivalent to the privilege to grant 'EXECUTE' to public. Here is similar problem at conversion functions. If malicious one want to install a fake conversion function that records all the stream between server and client, it seems to me not impossible. Well, I'd like to suggest to revise the specifications of permission checks on these commands. If we can ensure these functions are not malicious, no need to care about information leaks via untrusted functions internally used. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] leaky views, yet again
(2010/10/19 21:31), Robert Haas wrote: On Oct 19, 2010, at 4:34 AM, KaiGai Koheikai...@ak.jp.nec.com wrote: (2010/10/14 1:52), Tom Lane wrote: Robert Haasrobertmh...@gmail.com writes: On Wed, Oct 13, 2010 at 11:45 AM, Tom Lanet...@sss.pgh.pa.us wrote: That's all true, but you have to consider how much the obstacle actually gets in their way versus how painful it is on your end to create and maintain the obstacle. �I don't think this proposed patch measures up very well on either end of that tradeoff. I think it would behoove us to try to separate concerns about this particular patch from concerns about the viability of the whole approach. Whether or not it's useful to do X is a different question than whether it can be done with few enough lines of code and/or whether this patch actually does it well. I think I left the wrong impression: I'm concerned about the whole approach. I haven't even read this particular patch lately. I think that trying to guarantee that the planner applies independent constraints in a particular order will be expensive, fragile, and prone to recurring security bugs no matter how it's implemented --- unless you do it by lobotomizing query pullup/pushdown, which seems unacceptable from a performance standpoint. Just to give one example of what this patch misses (probably; as I said I haven't read it lately), what about selectivity estimation? One of the things we like to do when we have an otherwise-unknown function is to try it on all the histogram members and see what percentage yield true. That might already represent enough of an information leak to be objectionable ... and yet, if we don't do it, the consequences for performance could be pretty horrid because we'll have to work without any reasonable selectivity estimate at all. There are cases where this technique isn't applied at the moment but probably should be, which is why I characterize the leak-prevention idea as creating future security issues: doing that is a constraint that will have to be accounted for in every future planner change, and we are certain to miss the implications sometimes. Sorry, I might misread what you pointed out. I think you're still misreading it. Hmm. In my understanding, it seems to me he concerned about possible leaky estimate functions, so he mentioned the horrible performance degrading, if we don't allow to execute these functions. So, I suggested an idea that enforces all estimate functions being installed by superusers; it enables us to assume they are enough safe. Want to try a third time? However, actually, it is still unclear for me... :-( Do you see oprrest/oprjoin being internally invoked as a problem? Well, I also think it is a problem, as long as they can be installed by non-superusers. But, it is a separated problem from the row-level security issues. In my opinion, origin of the matter is incorrect checks on creation of operators. It allows non-superusers to define a new operator with restriction and join estimator as long as he has EXECUTE privilege on the functions. (not EXECUTE privilege of actual user who invokes this function on estimation phase!) Then, the optimizer may internally invoke these functions without any privilege checks on either the function or the table to be estimated. If a malicious one tries to make other users to invoke a trojan-horse, he can define a trappy operator with malicious estimator functions, cannot it? This is a pretty poor excuse for a Trojan horse attack. ...Robert -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] leaky views, yet again
(2010/10/14 1:52), Tom Lane wrote: Robert Haasrobertmh...@gmail.com writes: On Wed, Oct 13, 2010 at 11:45 AM, Tom Lanet...@sss.pgh.pa.us wrote: That's all true, but you have to consider how much the obstacle actually gets in their way versus how painful it is on your end to create and maintain the obstacle. �I don't think this proposed patch measures up very well on either end of that tradeoff. I think it would behoove us to try to separate concerns about this particular patch from concerns about the viability of the whole approach. Whether or not it's useful to do X is a different question than whether it can be done with few enough lines of code and/or whether this patch actually does it well. +1. If the patch I proposed is not enough elegant to commit immediately, we can discuss how we can get the patch fixing the problem commitable. I think I left the wrong impression: I'm concerned about the whole approach. I haven't even read this particular patch lately. I think that trying to guarantee that the planner applies independent constraints in a particular order will be expensive, fragile, and prone to recurring security bugs no matter how it's implemented --- unless you do it by lobotomizing query pullup/pushdown, which seems unacceptable from a performance standpoint. It can be controllable by users. Unless specifying SECURITY VIEW flag, it does not restrain a part of optimizations that allows to execute functions which may leak the supplied arguments unexpectedly earlier than the functions in deeper level. Also, it does not ignore performance point of view. Even if we apply this patch, a part of functions that we believe they don't leak any supplied arguments can be pushed down inside of the join-loop. Eventually, these invocations shall be utilized for index-scanning. The reason why I, Robert and Heikki had a discussion what type of functions should be allowed to push down is to decide a principle to bypass this security barrier. Just to give one example of what this patch misses (probably; as I said I haven't read it lately), what about selectivity estimation? One of the things we like to do when we have an otherwise-unknown function is to try it on all the histogram members and see what percentage yield true. That might already represent enough of an information leak to be objectionable ... and yet, if we don't do it, the consequences for performance could be pretty horrid because we'll have to work without any reasonable selectivity estimate at all. It is a bit unclear for me where is the point of your concerns. If user wants to generate a histogram from result set of a view that filters tuples to be invisible, it should just generate the histogram from the filtered data set. Even if possible malicious functions are appended to WHERE clause, the optimizer does not execute them prior to deeper level functions, as long as is has SECURITY VIEW flag. Of course, here is a performance trade-off, as earlier researcher pointed out, but user can inform on which does he give higher priority. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] security hook on table creation
(2010/10/18 23:14), Robert Haas wrote: If MVCC visibility always would match with existing permission checks, we don't need to pay special attention for these things, do we? It seems to me that you're worrying about the wrong set of problems. The original purpose of what I proposed was to let you set a security context on a new object at creation time, not to provide fine-grained DDL access control. I thought perhaps we could kill two birds with one stone, but if not, let's take three steps back and assume that DDL permissions will be controlled using a coarse-grained check installed in ProcessUtility_hook, so that by the time these hooks get installed they have no job except to apply any necessary label. Ah, yes, the original or primary purpose was automatic assignment of security labels at creation time. Access controls on CREATE statement is a second bird, indeed. I agree with the idea to separate things corresponding to DDL permission checks right now. I'll focus on the creation-time hooks to fix up security label (or something depending on security models) on the next commit fest. At least, ProcessUtility_hook is too coarse-grained to implement full- set of functionalities that we expect, so we will have a discussion about what is the preferable design of access control hooks in the future. But it is not right now. But as to your question, nothing whatever excuses you from needing to worry about MVCC. The entire backend is littered with things that have to worry about MVCC. Whether there's a concern for any particular bit of code depends heavily on what that code does, but I put it in the same place so I needn't worry is only true if you're doing the same thing, which you may not be. Nor is it correct to suppose that doing permissions checking the way you're proposing is going to somehow be free of code maintenance concerns. Indeed, many of the patches you've submitted in this area have been rejected precisely because they would make the code quite difficult to maintain - for example, by passing bits of no obvious relevance to the hooks. Hmm. Indeed, we have many things which need to be carefully implemented, not only access control stuff. Perhaps, it is right. So, author of plugin modules need to pay attention even if minor updates. However, MVCC visibility is one of the issues we need to pay attentions. As you mentioned about, I have a bad memory about difficulty to maintain the code. The existing permission checks are reviewed by larger number of eyeballs than author of enhanced security modules. So, I believe it eventually reduce burden to maintain, and enables to overlook code-paths that bypass the upcoming DDL permission checks. Anyway, let's have a discussion when we put security hooks for DDL checks. But the next patch will focus on assignment of security label at object creation. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] security hook on table creation
(2010/10/15 22:04), Stephen Frost wrote: KaiGai, * KaiGai Kohei (kai...@kaigai.gr.jp) wrote: However, it requires the plugin modules need to know everything; such as what is visible/invisible. It seems to me too closely- coupled interface. I agree with Robert on this one. We're not trying to design a wholly independent security module system for any project to pick up and use here. We're discussing hooks to go into PostgreSQL to support a PostgreSQL security module. In other words, I don't think we need to worry over if the PG-SELinux security module could be re-used for another project or is too PG specific. If it's *not* very PG specific then something is wrong. The issues we're talking about with regard to MVCC, visibility, etc, would all be applicable to any serious database anyway. Sorry for this delayed reply, because I've not been internet connectable for a couple of days. What we are always talking about is a PG specific security module, not universal ones for any other RDBMS. Please imagine a scenario that I'm concerning about, as follows: If and when we will release a minor version up (E.g: 9.1.3 - 9.1.4) which contains hot-fixes around the object creation code and its security hook, it may affect MVCC visibility to the guest of the security hook. In this (bad) case, the security module would lose compatibility across the minor version up. I said it as security module need to know everything. To avoid this, we will need to become paying attention what will be happen on the security hooks whenever we apply these bug fixes. So, I'm saying it will become a burden of management in the future. If MVCC visibility always would match with existing permission checks, we don't need to pay special attention for these things, do we? Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] leaky views, yet again
(2010/10/13 22:43), Tom Lane wrote: Robert Haasrobertmh...@gmail.com writes: With the possible exception of Tom, everyone seems to agree that it would be a good step forward to provide a way of plugging these holes, even if it didn't cover subtler information leaks such as by reading the EXPLAIN output or timing query execution. 1. Does anyone wish to argue (or continue arguing) that plugging these more overt information leaks is not worthwhile? Yeah, I will. Plugging an overt information leak without plugging other channels in the same area isn't a security improvement. It's merely PR, and rather lame PR at that. An attacker is not bound to use only the attack methods you'd like him to. It seems to me an extreme opinion, and different from the standard point of security view. It is a quotation from the classic of security evaluation criteria. Trusted Computer System Evaluation Criteria (TCSEC, DoD) says in the chapter of A GUIDELINE ON COVERT CHANNELS as follows: http://csrc.nist.gov/publications/history/dod85.pdf | From a security perspective, covert channels with low bandwidths represent a | lower threat than those with high bandwidths. However, for many types of | covert channels, techniques used to reduce the bandwidth below a certain rate | (which depends on the specific channel mechanism and the system architecture) | also have the effect of degrading the performance provided to legitimate | system users. Hence, a trade-off between system performance and covert | channel bandwidth must be made The overt channels has a capability to leak massive invisible information, so we need to consider them as a serious threat to be fixed up in higher priority. However, it is doubtful whether the rest of channels provides enough bandwidth as actual threat. It also means degree of the threat is relatively small than the overt channels. Previous security researcher pointed out security is trading-off, not all-or-nothing. If we can plug most part of the threat with reasonable performance degrading, it is worthwhile to fix up. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] security hook on table creation
(2010/10/12 20:59), Robert Haas wrote: 2010/10/11 KaiGai Koheikai...@ak.jp.nec.com: It enables us to put security hooks independent from MVCC visibility of the new database object. If we pay attention for visibility of new database object, it seems to me amount of things to understand and maintain will be increased, although MVCC visibility is fundamentally unrelated stuff from viewpoint of the access control. In the idea of two hooks, the prep-creation hook shall be invoked in same visibility of existing permission checks, and the post-creation hook shall be invoked in same visibility of simple_heap_* operations. I think it enables to reduce amount of things to understand and maintain, because the scope we should pay attention become small, if we can put security hooks independent from MVCC visibility. Perhaps, the problem may be intangible, but I don't think it is fair enough if we have to pay attention about MVCC visibility of plugin modules whenever we try to apply a patch around creation hooks. This may be nothing more than a matter of opinion, but it seems to me that what you're proposing makes this vastly more complicated for no particular benefit. Instead of having one hook that can cover a wide variety of use cases, you're going to need individual hooks for each object type plus the ability to pass data between them. In the broad outline, I also agree with one main security which can cover most of use cases. However, the only difference is that I'm saying we should handle prep-creation case as exception of the main hook. As I introduced before, the idea of two hooks makes obvious where we should put the security hooks; it is next to the existing DAC checking. It is the best guideline, even if we will touch the code around object creation in the future version. If the creation-hook would be put on the place far from existing DAC checks, what provides us a guideline to deploy security hooks? It seems to me the idea of only post-creation hook try to lose this kind of benefit instead of half dozen of exceptions. I think MVCC visibility is just an actualization of the matters. The point is that we can be released from the task to consider where is the right place for security hooks, as long as it will be placed next to the existing DAC checks. It seems to me its simplicity of design is unignorable benefit. And the point of this, apparently, is so that you can avoid using the standard syscache functions that the entire backend uses for retrieving information about objects and instead extract it in some other way; and/or avoid having to deal with the MVCC properties of which the rest of the backend must be aware. It just means it is not impossible... However, it requires the plugin modules need to know everything; such as what is visible/invisible. It seems to me too closely- coupled interface. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] security hook on table creation
(2010/10/12 23:09), Robert Haas wrote: On Tue, Oct 12, 2010 at 9:20 AM, KaiGai Koheikai...@kaigai.gr.jp wrote: As I introduced before, the idea of two hooks makes obvious where we should put the security hooks; it is next to the existing DAC checking. It is the best guideline, even if we will touch the code around object creation in the future version. If the creation-hook would be put on the place far from existing DAC checks, what provides us a guideline to deploy security hooks? It seems to me the idea of only post-creation hook try to lose this kind of benefit instead of half dozen of exceptions. I think MVCC visibility is just an actualization of the matters. The point is that we can be released from the task to consider where is the right place for security hooks, as long as it will be placed next to the existing DAC checks. It seems to me its simplicity of design is unignorable benefit. In either design, you have to decide where to put the post-creation hook. In your design, you ALSO need to decide where to put the pre-creation hook. Deciding where to put the pre-creation hook may be simple, but it is not as simple as not having it at all. If the post-creation hook have two tasks (access control and fix-up security labels) concurrently, things we need to consider and assess is not equal to the case when we only fix-up security labels on the post-creation hooks. MVCC visibility is a typical example. Elsewhere, we need to check up various things (such as completeness of the hook coverage, side-effects of CommandCounterIncrement(), ...) without reliable guidelines. I'm saying we can go through an easy way, as long as we design these hooks according to a simple principle based on the existing features. * pre-creation hooks (for access control) shall be put next to the existing DAC checks. * post-creation hooks (for fix-up security label) shall be put next to the simple_heap_*(). Because OID and labels to be assigned are already given, we don't need to consider such a complex things. Even if we need to decide the place of two hooks, it seems to me still simpler than security hooks from the scratch. A possibly legitimate reason to have a pre-creation hook is to prevent users from consuming more excessive system resources by repeatedly performing operations that SE-Linux will end up denying, but only after they're basically complete. We had similar discussion before when I tried to work on something related to table-inheritance. MergeAttributes() checks ownership of the parent table appeared in the INHERITS() clause, then it immediately raises an error even if one of them was not owned by the current user, because it allows users to prevent accesses unprivileged tables, if we check these ownership at once later. I learned existing privilege-checks are located with their reasons. So, it seems to me a good strategy to follow on existing design. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] host name support in pg_hba.conf
(2010/10/12 3:34), Peter Eisentraut wrote:
On tor, 2010-10-07 at 12:45 +0900, KaiGai Kohei wrote:
* The logic is still unclear for me.
The check_hostname() immediately returns with false, if the resolved
remote hostname is NOT matched with the hostname described in pg_hba.conf.
If the resolved hostname is matched with the hostname described
in pg_hba.conf, we can consider this HbaLine to be a suitable
configuration without any fallbacks. Right?
It so, it should be as follows:
if (strcmp(port-remote_hostname, hostname) == 0)
return true;
In addition, we should go the rest of fallback code on mismatch
cases only, don't we?
The code below that is not a fallback, it is the second part of the
double DNS lookup that has been extensively discussed throughout this
thread. The logic is
get hostname from client's IP address
strcmp hostname to pg_hba.conf
get IP address from hostname
if that IP address == client's IP address; then pg_hba.conf entry OK
Sorry, I missed what you introduced at the head of this thread.
When an entry passes the checks, this routine always checks both of
directions for the supplied entries.
BTW, I have one other question.
Is it really necessary to check reverse dns entries?
If check_hostname() compares remote address of the client and all
the candidates retrieved using getaddrinfo() for the hostname in
pg_hba.conf line (not port-remote_hostname), it seems to me we don't
need to set up reverse dns entries.
A typical ISP often doesn't assign reverse dns entries for cheap class
rental server being shared by multiple users, for instance. It seem to
me this idea allows to apply this new feature more widely.
# Let's try nslookup to www.kaigai.gr.jp, and its retrieved address. :-)
Of course, it is just my idea. If dislike it, please ignore it.
* Why getnameinfo() in the fallback loop?
I checked through my git history; this was actually a leftover from some
debugging code. I'll remove it.
* Slash ('/') after the hostname
At the parse_hba_line(), the parsed token which contains either
hostname or cidr address is sliced into two parts on the first '/'
character, if exist.
Then, even if cidr_slash is not NULL, it shall be ignored when
top-half of the token is hostname, not numeric address.
OK, I'll fix that.
--
KaiGai Kohei kai...@ak.jp.nec.com
--
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] security hook on table creation
It seems to me the conclusion of this discussion is unclear. We commonly try to find out an approach that minimize code complexity to understand and maintain, so the point of issue is clear, but we still don't reach same conclusion, because both of two ideas have merits and demerits each other. * Prep/Post-creation hook merits: simple principle to deploy security hooks. The prep-creation hook shall be after existing DAC checks, and the post-creation hook shall be after modification of system catalogs. demerits: several specific security hooks are necessary, in addition to the main security hook. * Only post-creation hook merits: small number of security hook definitions. demerits: at least, new entries of system catalog must be visible when we invoke the security hooks, so some cases require us to add new CCIs on invocations, but it requires us to verify it is harmless (whenever we will touch the code around security hooks in the future). In my viewpoint, MVCC is one of the most complex things in PG. So, in fact, I also missed the possibility that the gust of security hook cannot reference the entry of new database object, when the idea of post-creation hook was suggested. If we have a strong (and implicit) restriction about places where we should deploy the security hooks on, I don't think it enables to minimize our task to understand and maintain (in the future), although line of change set is a bit smaller now. So, I think the idea of two hooks on creation is better. It tries to put prep-creation hook according to the manner of existing DAC checks, and post-creation hook is next to modification of system catalogs with same visibility of the main entries. It seems to me this simple principle enables to minimize our task to understand and maintain. Thanks, (2010/10/08 9:39), KaiGai Kohei wrote: (2010/10/08 0:21), Robert Haas wrote: On Wed, Oct 6, 2010 at 5:21 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Robert Haas's message of mié oct 06 17:02:22 -0400 2010: 2010/10/5 KaiGai Koheikai...@ak.jp.nec.com: However, we also have a few headache cases. DefineType() creates a new type object and its array type, but it does not call CommandCounterIncrement() by the end of this function, so the new type entries are not visible from the plugin modules, even if we put a security hook at tail of the DefineType(). DefineFunction() also has same matter. It create a new procedure object, but it also does not call CommandCounterIncrement() by the end of this function, except for the case when ProcedureCreate() invokes language validator function. So I guess the first question here is why it's important to be able to see the new entry. I am thinking that you want it so that, for example, you can fetch the namespace OID to perform an SE-Linux type transition. Is that right? I'm not sure that there's any point trying to optimize these to the point of avoiding CommandCounterIncrement. Surely DefineType et al are not performance-sensitive operations. OK, fair enough. Let's just do it unconditionally then. I guess we will need to have such kind of discussion whenever we touch the code around security hooks in the future, if hooks would not be put next to the existing DAC checks. Although we need to define several hooks on object creation in addition to the main hook, separating it into two parts helps us to understand and maintenance. | In the case when we have two hooks, obviously, the prep-creation | hook will be after the DAC checks, and the post-creation hook will | be after the insert/update of system catalogs. I still don't think such a simple principle overs our capability, and also don't think it is more complex than matters around the idea of only post-creation hooks, such as CommandCounterIncrement(). Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] security hook on table creation
(2010/10/12 11:35), Robert Haas wrote: On Mon, Oct 11, 2010 at 9:58 PM, KaiGai Koheikai...@ak.jp.nec.com wrote: It seems to me the conclusion of this discussion is unclear. We commonly try to find out an approach that minimize code complexity to understand and maintain, so the point of issue is clear, but we still don't reach same conclusion, because both of two ideas have merits and demerits each other. * Prep/Post-creation hook merits: simple principle to deploy security hooks. The prep-creation hook shall be after existing DAC checks, and the post-creation hook shall be after modification of system catalogs. demerits: several specific security hooks are necessary, in addition to the main security hook. * Only post-creation hook merits: small number of security hook definitions. demerits: at least, new entries of system catalog must be visible when we invoke the security hooks, so some cases require us to add new CCIs on invocations, but it requires us to verify it is harmless (whenever we will touch the code around security hooks in the future). In my viewpoint, MVCC is one of the most complex things in PG. So, in fact, I also missed the possibility that the gust of security hook cannot reference the entry of new database object, when the idea of post-creation hook was suggested. If we have a strong (and implicit) restriction about places where we should deploy the security hooks on, I don't think it enables to minimize our task to understand and maintain (in the future), although line of change set is a bit smaller now. So, I think the idea of two hooks on creation is better. It tries to put prep-creation hook according to the manner of existing DAC checks, and post-creation hook is next to modification of system catalogs with same visibility of the main entries. It seems to me this simple principle enables to minimize our task to understand and maintain. I don't understand what problem you think having two hooks will solve. It enables us to put security hooks independent from MVCC visibility of the new database object. If we pay attention for visibility of new database object, it seems to me amount of things to understand and maintain will be increased, although MVCC visibility is fundamentally unrelated stuff from viewpoint of the access control. In the idea of two hooks, the prep-creation hook shall be invoked in same visibility of existing permission checks, and the post-creation hook shall be invoked in same visibility of simple_heap_* operations. I think it enables to reduce amount of things to understand and maintain, because the scope we should pay attention become small, if we can put security hooks independent from MVCC visibility. Perhaps, the problem may be intangible, but I don't think it is fair enough if we have to pay attention about MVCC visibility of plugin modules whenever we try to apply a patch around creation hooks. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] security hook on table creation
(2010/10/08 0:21), Robert Haas wrote: On Wed, Oct 6, 2010 at 5:21 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Robert Haas's message of mié oct 06 17:02:22 -0400 2010: 2010/10/5 KaiGai Koheikai...@ak.jp.nec.com: However, we also have a few headache cases. DefineType() creates a new type object and its array type, but it does not call CommandCounterIncrement() by the end of this function, so the new type entries are not visible from the plugin modules, even if we put a security hook at tail of the DefineType(). DefineFunction() also has same matter. It create a new procedure object, but it also does not call CommandCounterIncrement() by the end of this function, except for the case when ProcedureCreate() invokes language validator function. So I guess the first question here is why it's important to be able to see the new entry. I am thinking that you want it so that, for example, you can fetch the namespace OID to perform an SE-Linux type transition. Is that right? I'm not sure that there's any point trying to optimize these to the point of avoiding CommandCounterIncrement. Surely DefineType et al are not performance-sensitive operations. OK, fair enough. Let's just do it unconditionally then. I guess we will need to have such kind of discussion whenever we touch the code around security hooks in the future, if hooks would not be put next to the existing DAC checks. Although we need to define several hooks on object creation in addition to the main hook, separating it into two parts helps us to understand and maintenance. | In the case when we have two hooks, obviously, the prep-creation | hook will be after the DAC checks, and the post-creation hook will | be after the insert/update of system catalogs. I still don't think such a simple principle overs our capability, and also don't think it is more complex than matters around the idea of only post-creation hooks, such as CommandCounterIncrement(). Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] security hook on table creation
(2010/10/07 6:02), Robert Haas wrote:
2010/10/5 KaiGai Koheikai...@ak.jp.nec.com:
I began to revise the security hooks, but I could find a few cases that does
not work with the approach of post-creation security hooks.
If we try to fix up the core PG routine to become suitable for the
post-creation
security hooks, it shall need to put more CommandCounterIncrement() on
various
places, so it made me doubtful whether this approach gives really minimum
impact
to the core PG routine, or not.
We definitely don't want to add CCIs without a good reason.
Some of object classes have CommandCounterIncrement() just after the routine
to create a new object. For example, DefineRelation() calls it just after the
heap_create_with_catalog(), so the new relation entry is visible for plugin
modules using SearchSysCache(), as long as we put the post-creation security
hook aftere the CommandCounterIncrement().
However, we also have a few headache cases.
DefineType() creates a new type object and its array type, but it does not
call CommandCounterIncrement() by the end of this function, so the new type
entries are not visible from the plugin modules, even if we put a security
hook at tail of the DefineType().
DefineFunction() also has same matter. It create a new procedure object,
but it also does not call CommandCounterIncrement() by the end of this
function, except for the case when ProcedureCreate() invokes language
validator function.
So I guess the first question here is why it's important to be able to
see the new entry. I am thinking that you want it so that, for
example, you can fetch the namespace OID to perform an SE-Linux type
transition. Is that right?
We assumed that namespace OID can be fetched from the entry of pg_class,
so we thought the common InvokeObjectAccessHook() dose not need to take
many arguments, because we can pull out corresponding properties of new
object (such as namespace OID) from SysCache using OID of new object.
So, InvokeObjectAccessHook() must deliver OID of the namespace, rather
than OID of the new object, if it is not visible.
Maybe we need a variant of InvokeObjectAccessHook that does a CCI only
if a hook is present. I can't see that we're going to want to pay for
that unconditionally, but it shouldn't surprise anyone that loading an
enhanced security provider comes with some overhead.
The reason why we tried to move the object creation hooks into post
object creation was to reduce number of security hooks and burden of
code maintenance.
However, it seems to me paying attention for object visibility gives
us more burden rather than we have multiple creation hooks.
E.g, it may be possible to design creation of relation as follows:
DefineRelation(...)
{
/* DAC permission checks here */
:
/* MAC permission checks; it returns its private data */
opaque = check_relation_create(...relation parameters...);
:
/* insertion into pg_class catalog */
relationId = heap_create_with_catalog(...);
:
/* assign security labels on the new object */
InvokeObjectAccessHook(OBJECT_TABLE, OAT_POST_CREATE,
relationId, 0, opaque);
}
I'd like to try to avoid that, if we can. That's going to make this
code far more complex to understand and maintain.
Against our feeling, I consider the idea of two hooks help us to
understand and maintain security hooks in the future.
Because the place where we should put the prep-creation hook is
just after the default privilege checks, it is quite obvious.
If we would have only post-creation hook, is it obvious where we
should put the security hook on function creation, for example?
In the case when we have two hooks, obviously, the prep-creation
hook will be after the DAC checks, and the post-creation hook will
be after the insert/update of system catalogs.
It seems to me quite easy to understand and maintain.
Thanks,
--
KaiGai Kohei kai...@ak.jp.nec.com
--
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] security hook on table creation
(2010/10/07 6:21), Alvaro Herrera wrote: Excerpts from Robert Haas's message of mié oct 06 17:02:22 -0400 2010: 2010/10/5 KaiGai Koheikai...@ak.jp.nec.com: However, we also have a few headache cases. DefineType() creates a new type object and its array type, but it does not call CommandCounterIncrement() by the end of this function, so the new type entries are not visible from the plugin modules, even if we put a security hook at tail of the DefineType(). DefineFunction() also has same matter. It create a new procedure object, but it also does not call CommandCounterIncrement() by the end of this function, except for the case when ProcedureCreate() invokes language validator function. So I guess the first question here is why it's important to be able to see the new entry. I am thinking that you want it so that, for example, you can fetch the namespace OID to perform an SE-Linux type transition. Is that right? I'm not sure that there's any point trying to optimize these to the point of avoiding CommandCounterIncrement. Surely DefineType et al are not performance-sensitive operations. Performance optimization is not the point here. If we need to call CommandCounterIncrement() before invocation of security hooks, we also need to analyze its side-effect and to confirm it is harmless. Even if it is harmless, I think it gives us more burden of code maintenance than the idea of two hooks on creation. Maybe we need a variant of InvokeObjectAccessHook that does a CCI only if a hook is present. The problem I see with this idea is that it becomes a lot harder to track down whether it ocurred or not for any given operation. Yes, it is a burden of code maintenance. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] Bug / shortcoming in has_*_privilege
(2010/10/07 2:05), Alvaro Herrera wrote: Another thing that could raise eyebrows is that I chose to remove the missing_ok argument from get_role_oid_or_public, so it's not a perfect mirror of it. None of the current callers need it, but perhaps people would like these functions to be consistent. Tom Lane suggested to add missing_ok argument, although it is not a must- requirement. Actually I think he suggested the opposite. Ahh, I understood his suggestion as literal. Well, I'd like to mark this patch as 'ready for committer'. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] host name support in pg_hba.conf
(2010/10/06 10:21), KaiGai Kohei wrote:
I'll check the patch for more details, please wait for a few days.
I could find out some matters in this patch, independent from the
discussion of localhost. (About pg_hba.conf.sample, I'm sorry for
the missuggestion. Please fix up it according to Tom's comment.)
* The logic is still unclear for me.
The check_hostname() immediately returns with false, if the resolved
remote hostname is NOT matched with the hostname described in pg_hba.conf.
| +static bool
| +check_hostname(hbaPort *port, const char *hostname)
| +{
| + struct addrinfo *gai_result, *gai;
| + int ret;
| + boolfound;
| +
| + /* Lookup remote host name if not already done */
| + if (!port-remote_hostname)
| + {
| + charremote_hostname[NI_MAXHOST];
| +
| + if (pg_getnameinfo_all(port-raddr.addr, port-raddr.salen,
| + remote_hostname, sizeof(remote_hostname),
| + NULL, 0,
| + 0))
| + return false;
| +
| + port-remote_hostname = strdup(remote_hostname);
| + }
| +
| + if (strcmp(port-remote_hostname, hostname) != 0)
| + return false;
| +
| + /* Lookup IP from host name and check against original IP */
However, it seems to me you expected an opposite behavior.
If the resolved hostname is matched with the hostname described
in pg_hba.conf, we can consider this HbaLine to be a suitable
configuration without any fallbacks. Right?
It so, it should be as follows:
if (strcmp(port-remote_hostname, hostname) == 0)
return true;
In addition, we should go the rest of fallback code on mismatch
cases only, don't we?
* Why getnameinfo() in the fallback loop?
At the fallback code when the hostname was matched (I believe this code
is intended to handle the case when hostname was NOT matched.) calls
getnameinfo() for each candidate of remote addresses.
But its result is not referenced by anybody. Is it really necessary?
| + found = false;
| + for (gai = gai_result; gai; gai = gai-ai_next)
| + {
| + charhostinfo[NI_MAXHOST];
| +
| + getnameinfo(gai-ai_addr, gai-ai_addrlen,
| + hostinfo, sizeof(hostinfo),
| + NULL, 0,
| + NI_NUMERICHOST);
| +
| + if (gai-ai_addr-sa_family == port-raddr.addr.ss_family)
| + {
| + if (gai-ai_addr-sa_family == AF_INET)
| + {
| + if (ipv4eq((struct sockaddr_in *) gai-ai_addr,
| + (struct sockaddr_in *) port-raddr.addr))
| + {
| + found = true;
| + break;
| + }
| + }
| + else if (gai-ai_addr-sa_family == AF_INET6)
| + {
| + if (ipv6eq((struct sockaddr_in6 *) gai-ai_addr,
| + (struct sockaddr_in6 *) port-raddr.addr))
| + {
| + found = true;
| + break;
| + }
| + }
| + }
| + }
| +
| + if (gai_result)
| + freeaddrinfo(gai_result);
| +
| + return found;
| +}
* Slash ('/') after the hostname
At the parse_hba_line(), the parsed token which contains either
hostname or cidr address is sliced into two parts on the first '/'
character, if exist.
Then, even if cidr_slash is not NULL, it shall be ignored when
top-half of the token is hostname, not numeric address.
| else
| {
| /* IP and netmask are specified */
| parsedline-ip_cmp_method = ipCmpMask;
|
| /* need a modifiable copy of token */
| token = pstrdup(token);
|
| /* Check if it has a CIDR suffix and if so isolate it */
| cidr_slash = strchr(token, '/');
| if (cidr_slash)
| *cidr_slash = '\0';
:
| ret = pg_getaddrinfo_all(token, NULL, hints, gai_result);
| - if (ret || !gai_result)
| + if (ret == 0 gai_result)
| + memcpy(parsedline-addr, gai_result-ai_addr,
| + gai_result-ai_addrlen);
| + else if (ret == EAI_NONAME)
| + parsedline-hostname = token;
| + else
| {
It allows the following configuration works without any errors.
(In fact, it works for me.)
# IPv4/6 local connections:
host all all kaigai.myhome.cx/today_is_sunny trust
It seems to me, we should raise an error, if both of cidr_slash and
parsedline-hostname are not NULL.
Thanks,
--
KaiGai Kohei kai...@ak.jp.nec.com
--
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] security hook on table creation
(2010/10/01 11:23), Robert Haas wrote:
On Sep 30, 2010, at 9:01 PM, KaiGai Koheikai...@ak.jp.nec.com wrote:
(2010/10/01 3:09), Robert Haas wrote:
2010/9/29 KaiGai Koheikai...@ak.jp.nec.com:
In addition, I want to give these entrypoints its name which
represents an appropriate purpose of the hook, rather than
a uniformed one.
It sounds like you're proposing to create a vast number of hooks
rather than just one. If we have ~20 object types in the system,
that's 40 hooks just for create and drop, and then many more to handle
comment, alter (perhaps in various flavors), etc. I'm pretty
unexcited about that. The main hook function can always dispatch
internally if it so desires, but I don't see any benefit to forcing
people to write the code that way.
What I proposed is to create just one hook and wrapper functions
with appropriate name; that calls the hook with appropriate parameters,
such as SearchSysCache1, 2, 3 and 4.
Seems like you'd end up creating a lot of macros that were only used once.
I began to revise the security hooks, but I could find a few cases that does
not work with the approach of post-creation security hooks.
If we try to fix up the core PG routine to become suitable for the post-creation
security hooks, it shall need to put more CommandCounterIncrement() on various
places, so it made me doubtful whether this approach gives really minimum impact
to the core PG routine, or not.
See the following analysis:
Now we support to assign security label on the seven object classes enumerated
at ExecSecLabelStmt().
Some of object classes have CommandCounterIncrement() just after the routine
to create a new object. For example, DefineRelation() calls it just after the
heap_create_with_catalog(), so the new relation entry is visible for plugin
modules using SearchSysCache(), as long as we put the post-creation security
hook aftere the CommandCounterIncrement().
However, we also have a few headache cases.
DefineType() creates a new type object and its array type, but it does not
call CommandCounterIncrement() by the end of this function, so the new type
entries are not visible from the plugin modules, even if we put a security
hook at tail of the DefineType().
DefineFunction() also has same matter. It create a new procedure object,
but it also does not call CommandCounterIncrement() by the end of this
function, except for the case when ProcedureCreate() invokes language
validator function.
During the discussion, we talked about which is more preferable design
(A) having both of prep/post creation hooks, or (B) having only post
creation hook. My original proposition is similar to the idea (A), but
we could not find out a good reason to justify (A) rather than simple
(B) a week ago.
However, again, it seems to me the idea (A) becomes better, because it
enables to avoid random injection of CommandCounterIncrement() in the
future.
So, I'd like to reconsider the idea with two hooks on creation time.
The issue of prep creation hook was that it needs per object class
definitions because we cannot pull-out information from SysCache
before the new database object being visible.
But it does not mean we eventually need (# of object classes) x (# of
access types) hooks.
E.g, it may be possible to design creation of relation as follows:
DefineRelation(...)
{
/* DAC permission checks here */
:
/* MAC permission checks; it returns its private data */
opaque = check_relation_create(...relation parameters...);
:
/* insertion into pg_class catalog */
relationId = heap_create_with_catalog(...);
:
/* assign security labels on the new object */
InvokeObjectAccessHook(OBJECT_TABLE, OAT_POST_CREATE,
relationId, 0, opaque);
}
In this design, we can share the post-creation hook with any other object
classes, so all we need to provide for each object classes are just prep
creation hooks. It does not seem to me explosion of security hooks.
Could you give me your opinion to handle these problematic code paths?
Thanks,
--
KaiGai Kohei kai...@ak.jp.nec.com
--
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] leaky views, yet again
be better than extending StdRdOptions. Hmm. It might be better than ad-hoc enhancement of StdRdOptions. BTW, which is more preference to store the flag of security view: reloption of the view or a new bool variable in the pg_class? I tried to store this flag within reloptions to minimize the patch size, but it seems to me reloption support for views makes the patch size larger in the result. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] leaky views, yet again
(2010/10/05 23:16), Robert Haas wrote: 2010/10/5 KaiGai Koheikai...@ak.jp.nec.com: The term built-in functions means functions written in INTERNAL language here. But we also have SQL functions by default. Some of them are just a wrapper to internal functions. I'm not sure the checking of INTERNAL language is the best way for the purpose. Did you compare it with other methods? For example, oidFirstNormalObjectId looks workable for me. Hmm. I'm not sure they can be used for index-scans. If these operators are not corresponding to index-scans, I want to keep the logic to check INTERNAL language, because these have obviously no side effects (= not leakable anything). I think the idea that all internal operators are safe has been thoroughly discredited already. How can we distinguish an internal operator and others? Because pg_operator does not have a property that we can use to distinguish them, I tried to check function of the operators... Hmm. It might be better than ad-hoc enhancement of StdRdOptions. BTW, which is more preference to store the flag of security view: reloption of the view or a new bool variable in the pg_class? I tried to store this flag within reloptions to minimize the patch size, but it seems to me reloption support for views makes the patch size larger in the result. I think a boolean in pg_class is the way to go. Is there a padding byte we can steal, to avoid making the fixed-size portion larger? OK, I'll add a boolean in pg_class. Perhaps, 'relissecview'? Thanks -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] leaky views, yet again
(2010/10/05 23:56), Tom Lane wrote: Robert Haasrobertmh...@gmail.com writes: Checking the functions of the operators is the right thing to do, but assuming that internal = safe does not work. For example, pushing down arithmetic operators allows you to probe for any given value in a hidden row by testing whether 1 / (x - constant) throws a division by zero error. Well, if the goal is make it impossible to tell whether such-and-such a value exists, I think this approach can't meet it at all. There are too many side channels. You're focusing here on the error-report side channel, but there's also performance, ie how long did the query take. (BTW, is the intent to somehow lobotomize EXPLAIN so you can't use that to see what happened?) Good point. Major commercial RDBMS with row-level access control (such as Oracle VPD) does not care about any side channels that allows us to infer existence of a certain value. Their features focus on control regular data channels. It allows malicious users to transfer contents of invisible tuples into others unexpectedly. It corresponds to a user defined function which insert the supplied argument into temporary table in my example. So, if we should catch up same level earlier, I think we need to ignore such kind of side channel attacks. If so, the matter become much simple. We need to consider whether contents of the error messages are delivered via main-channel or side-channel. If we consider it is a side-channel, we can trust all the buili-in functions because nothing tries to write out the supplied argument into somewhere. If we consider it is a regular-channel, we need to distinguish safe and unsafe functions based on a certain criteria, maybe, 'safe' flag in pg_proc. In my opinion, I like to categorize error messages as side-channel, because its through-put is much less than regular-channels, so scale of the threat is relatively small. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] leaky views, yet again
(2010/10/06 0:33), KaiGai Kohei wrote: (2010/10/05 23:56), Tom Lane wrote: Robert Haasrobertmh...@gmail.com writes: Checking the functions of the operators is the right thing to do, but assuming that internal = safe does not work. For example, pushing down arithmetic operators allows you to probe for any given value in a hidden row by testing whether 1 / (x - constant) throws a division by zero error. Well, if the goal is make it impossible to tell whether such-and-such a value exists, I think this approach can't meet it at all. There are too many side channels. You're focusing here on the error-report side channel, but there's also performance, ie how long did the query take. (BTW, is the intent to somehow lobotomize EXPLAIN so you can't use that to see what happened?) Good point. Major commercial RDBMS with row-level access control (such as Oracle VPD) does not care about any side channels that allows us to infer existence of a certain value. Their features focus on control regular data channels. It allows Sorry, prevents ^^ malicious users to transfer contents of invisible tuples into others unexpectedly. It corresponds to a user defined function which insert the supplied argument into temporary table in my example. So, if we should catch up same level earlier, I think we need to ignore such kind of side channel attacks. If so, the matter become much simple. We need to consider whether contents of the error messages are delivered via main-channel or side-channel. If we consider it is a side-channel, we can trust all the buili-in functions because nothing tries to write out the supplied argument into somewhere. If we consider it is a regular-channel, we need to distinguish safe and unsafe functions based on a certain criteria, maybe, 'safe' flag in pg_proc. In my opinion, I like to categorize error messages as side-channel, because its through-put is much less than regular-channels, so scale of the threat is relatively small. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] leaky views, yet again
(2010/10/06 4:06), Robert Haas wrote: On Tue, Oct 5, 2010 at 2:48 PM, Tom Lanet...@sss.pgh.pa.us wrote: Heikki Linnakangasheikki.linnakan...@enterprisedb.com writes: On 05.10.2010 21:08, Greg Stark wrote: If the users that have select access on the view don't have DDL access doesn't that make them leak-proof for those users? No. You can use built-in functions for leaking data as well. There's a difference between can be used to extract data wholesale and can be used to probe for the existence of a specific value. We might need to start using more specific terminology than leak. Yeah. There are a lot of cases. The worst is if you can (1a) dump the underlying table wholesale, or maybe (1b) extract it one row at a time or something like that. Not quite as bad is if you can (2) infer the presence or absence of particular values in particular columns, e.g. via division-by-zero. This is still pretty bad though, because you can probably just keep guessing until you eventually can enumerate everything in that column. If it's a text field or a UUID that may be pretty hard, but if the range of interesting values for that column is limited to, say, a million or so, then you can just iterate through them until you find everything. A related problem is where you can (3) infer the frequency of a value based on the plan choice, either by viewing the EXPLAIN output directly or by timing attacks; and then there's (4) the ability to infer something about the overall amount of data in the underlying table. Any others? Of those, I'm inclined to think that it's possible to close off (1) and (2) pretty thoroughly with sufficient care, but the best you'd be able to do for (3) and (4) is refuse to EXPLAIN to a user without sufficient privileges; the timing attacks seem intractable. Thanks for good summarize. I also think the case (1) should be closed off soon, because it allows to expose hidden data-contents without any inference of attacker; and its throughput is unignorably fast, so its degree of threat is relatively higher than other cases. side-note The idea of throughput is not my own idea. It come from the classic of security evaluation criteria: Trusted Computer System Evaluation Criteria (TCSEC, 1985) See the page.80 of: http://csrc.nist.gov/publications/history/dod85.pdf | From a security perspective, covert channels with low bandwidths represent a | lower threat than those with high bandwidths. However, for many types of | covert channels, techniques used to reduce the bandwidth below a certain rate | (which depends on the specific channel mechanism and the system architecture) | also have the effect of degrading the performance provided to legitimate | system users. Hence, a trade-off between system performance and covert | channel bandwidth must be made. /side-node I also think we should care about a part of (2) cases. Could you separate the (2) into two cases. The (2a) allows people to see hidden value using error message. In this case, people can see direct value to be hidden, but thorough-put is not fast. The (2b) allows people to infer existence or absence of a certain value using PK or UNIQUE conflicts. (2a) is the reason why my patch allows to push down only operators with internal functions, because these are not obviously leakable. However, I don't think (2b) is the case we should fix up here, because no commercial RDBMSs with RLS don't handle such kind of side-channel attacks using key conflicts. And, it seems to me the cost will be too expensive to care about the case (3) and (4). So, I think it is worthless to fix up them. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] leaky views, yet again
(2010/10/06 4:15), Heikki Linnakangas wrote: On 05.10.2010 21:48, Tom Lane wrote: Heikki Linnakangasheikki.linnakan...@enterprisedb.com writes: On 05.10.2010 21:08, Greg Stark wrote: If the users that have select access on the view don't have DDL access doesn't that make them leak-proof for those users? No. You can use built-in functions for leaking data as well. There's a difference between can be used to extract data wholesale and can be used to probe for the existence of a specific value. Define wholesale. I'm also not too worried about probing attacks, where you can ask does this value exist?, but there are plenty of built-in fúnctions that can regurgitate the argument verbatim in an error message. Using that, you can build a script to fetch the value for every row in the table, one row at a time. It's orders of magnitude slower than SELECT * FROM table, but orders of magnitude faster than probing for every possible value for every row. We might need to start using more specific terminology than leak. Yeah. There are many different categories of leakage: 1. Wholesale retrieval of all rows. For example, a custom function that emits the argument with a NOTICE, used in a WHERE clause. It contains a custom function with side-effect; such as INSERT the supplied arguments into private tables. 2. Retrieval of a given value in an arbitrary attacker-chosen row. Using a function like text to integer cast that emits the argument in an ERROR falls into this category. You can access any value in the table, but only one at a time because ERROR causes a rollback. 3. Retrieval of some values, not attacker-chosen. For example, statistics might leak the top 100 values. 4. Probing attacks. Let's you check if a row with given value exists. 5. Leakage of statistical information. Lets you know roughly how many rows there are in a table, for example. There's some fuzziness in those too, you might be able to probe for values in an indexed column but not others, for example. Or you might be able to retrieve all values in one column, or all values in another column, but not put them together to form the original rows in the table. IMHO we don't need to protect against 5. Probing attacks can be nasty, but it's next to impossible to protect from them completely. And for many applications, probing attacks are a non-issue. For example, imagine table of usernames and passwords, with a view that lets you see your own password. Probing for other people's passwords would be useless, you might as well try to log in with the guessed password directly. Retrieval of some non-attacker chosen rows, like from statistics, would be nice to avoid where feasible, but I won't lose my sleep over it. I do think we need to protect against 1 and 2. I also agree with this attitude. The case 1 and 2 have differences from others. It allows to expose hidden values, then people may be able to see these values without any inference. So, their through-put is relatively faster than others. It means degree of threat is also higher. If we try to tacker to the matter 1 and 2, suggestions from Itagaki-san are still available, because this patch was designed to prevent people to see hidden data without inferences. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] host name support in pg_hba.conf
(2010/10/06 4:41), Peter Eisentraut wrote: On tis, 2010-10-05 at 12:35 +0900, KaiGai Kohei wrote: It seems to me this patch has been left for a long time, although it tries to provide a useful functionality. In the previous discussion, several issues were pointed out. * Case handling when an IP-address has multiple hostnames. Steve Atkins noticed getaddrinfo() does not have capability to handle this case, because it returns just one hostname from the candidates. So, he suggested to use gethostbyaddr() instead, because it can return all the candidates of hostnames. I also think gethostbyaddr() is more prefereble here rather than getaddrinfo() with the primary hostname as a fallback. I have several concerns about this approach. The first is conceptual: Assume that an IP address resolves as host names foo and bar, perhaps even by accident. You have log_hostnames on, which would be typical if you have an all-names pg_hba.conf. log_hostnames only logs the first host name, say, foo. And then assume that pg_hba.conf is set up to only allow bar in. There you have a debugging and auditing nightmare, because what pg_hba.conf says and what you are logging doesn't match. To address this you would have to change the log_hostnames facility to log *all* host names everywhere the host name is mentioned, which could make this whole thing quite silly. Secondly, long-standing and presumably reputable implementations of a similar feature, namely Apache's mod_authz_host and tcp-wrappers use getnameinfo() in preference of gethostbyaddr(), and don't support multiple host names per IP address. In fact, reading through that source code indicates that gethostbyaddr() has all kinds of bugs and issues, including apparently lack of IPv6 support (on some platforms?). Thirdly, gethostbyname() and gethostbyaddr() are deprecated by POSIX in favor of getaddrinfo() and getnameinfo(), so we shouldn't build new features that depend on them. OK, it seems to me fair enough. I consented with your explanation. I'll check the patch for more details, please wait for a few days. * localhost in the default pg_hba.conf Robert Haas disagreed to switch localhost in the default pg_hba.conf into numeric expression, because /etc/host should determine where the localhost shall be mapped. I agree with the opinion from Robert. We should not have any assumptions of /ets/hosts settings. Note that we already default listen_addresses to 'localhost', so it would only make sense to have pg_hba.conf behave the same by default. To pick up on your argument, we effectively *do* make assumptions about /etc/hosts now, and this change would remove them. Sorry, I misread something. I read the previous discussions again, then I know I misread the reason why Robert disagreed with this replacement. He said we should not assume resolve of localhost being enough fast because of local /etc/hosts, not saying we should not assume localhost is 127.0.0.1 or ::1. Right? Well, in my personal opinion, we should not assume the way to resolve localhost, but we can expect more than 99.9% of hosts resolve localhost using local /etc/hosts. Even if here is a curious setting, it will pay a bit more cost on connection. It is a responsibility of DBA. I agree with replacement 127.0.0.1 and ::1 by localhost. It enables to eliminate an assumption that localhost have either of their addresses. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] host name support in pg_hba.conf
It seems to me this patch has been left for a long time, although it tries to provide a useful functionality. In the previous discussion, several issues were pointed out. * Case handling when an IP-address has multiple hostnames. Steve Atkins noticed getaddrinfo() does not have capability to handle this case, because it returns just one hostname from the candidates. So, he suggested to use gethostbyaddr() instead, because it can return all the candidates of hostnames. I also think gethostbyaddr() is more prefereble here rather than getaddrinfo() with the primary hostname as a fallback. How about the following idea? 1. Add a list of hostname on Port structure to store all the candidates of hostnames for the current remote host. 2. Revise the pg_getnameinfo_all() to call the gethostbyaddr(), then it constructs the list from the returned hostent-h_aliases. 3. Revise Port-remote_host to indicate contents of the list head. 4. Revise the new check_hostname() to compare the supplied hostname from pg_hba.conf and each hostnames in the above list. If pg_getnameinfo_all() would have all the candidates of hostnames, we don't need to lookup dns twice on fallbacks. * localhost in the default pg_hba.conf Robert Haas disagreed to switch localhost in the default pg_hba.conf into numeric expression, because /etc/host should determine where the localhost shall be mapped. I agree with the opinion from Robert. We should not have any assumptions of /ets/hosts settings. * Cost for reverse lookup of DNS Tom Lane concerned about the cost to communicate with DNS for each connections. You answered either caching mechanism in PG or nscd is a solution, and agreed to add an explicit description about names being more expensive than IP addresses in pg_hba.conf. I also think it is fair enough. How about adding a recommendation to install nscd when people uses hostname in pg_hba.conf. Thanks, (2010/08/10 3:47), Peter Eisentraut wrote: Here is a patch for host name support in pg_hba.conf. I have reviewed various past threads about this, and there appeared to have been a 50/50 split of for and against reverse lookup. I went with the reverse lookup, because 0) I like it. 1) It is more secure. 2) It allows extending it to wildcards in the future. 3) Apache (Allow from) does it that way. To clarify how it works: The client's IP address (known from the kernel) is reverse looked up, which results in a host name. That host name is compared with the line in pg_hba.conf. If it matches, a forward lookup is performed on the host name to check if any of the resulting IP addresses match the client's IP address. If yes, the line is considered to match and the authentication method is selected. Anyway, assuming we will go with this, you will also notice that in the patch I changed the default pg_hba.conf to match against localhost instead of numeric addresses. Initially thought of as a temporary change for testing this patch, I think this might actually have some permanent value because it saves you from having to change the IPv4 and IPv6 lines in tandem most of the times, which is a moderately common mistake. We already rely on localhost being (forward) resolvable for the stats collector. Something to think about: Maybe we need a quoting mechanism in case someone names their hosts samenet. -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] Bug / shortcoming in has_*_privilege
(2010/09/07 6:16), Alvaro Herrera wrote: Excerpts from Jim Nasby's message of jue jun 10 17:54:43 -0400 2010: test...@workbook=# select has_table_privilege( 'public', 'test', 'SELECT' ); ERROR: role public does not exist Here's a patch implementing this idea. I checked this patch. It seems to me it replaces whole of get_role_oid() in has_*_privilege functions by the new get_role_oid_or_public(), so this patch allows to accept the pseudo public user in consistent way. The pg_has_role_*() functions are exception. It will raise an error with error message of role public does not exist. Is it an expected bahavior, isn't it? I'm not too sure about the wording in the doc changes. If somebody wants to propose something better, I'm all ears. To facilitate bikeshedding, here's a relevant extract: has_table_privilege checks whether a user can access a table in a particular way. The user can be specified by name; as public, to indicate the PUBLIC pseudo-role; by OID (pg_authid.oid), or, if the argument is omitted, current_user is assumed. (the first appearance of public isliteralpublic/. I had first made itquote but that didn't feel right.) It seems to me fair enough, but I'm not a native in English. Another thing that could raise eyebrows is that I chose to remove the missing_ok argument from get_role_oid_or_public, so it's not a perfect mirror of it. None of the current callers need it, but perhaps people would like these functions to be consistent. Tom Lane suggested to add missing_ok argument, although it is not a must- requirement. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] security hook on table creation
(2010/10/01 3:09), Robert Haas wrote: 2010/9/29 KaiGai Koheikai...@ak.jp.nec.com: In addition, I want to give these entrypoints its name which represents an appropriate purpose of the hook, rather than a uniformed one. It sounds like you're proposing to create a vast number of hooks rather than just one. If we have ~20 object types in the system, that's 40 hooks just for create and drop, and then many more to handle comment, alter (perhaps in various flavors), etc. I'm pretty unexcited about that. The main hook function can always dispatch internally if it so desires, but I don't see any benefit to forcing people to write the code that way. What I proposed is to create just one hook and wrapper functions with appropriate name; that calls the hook with appropriate parameters, such as SearchSysCache1, 2, 3 and 4. However, the reason why I proposed the wrapper functions is mainly from a sense of beauty at the code. So, I choose the term of 'my preference'. Well, at first, I'll try to work on as you suggested. --- BTW, as an aside, the SearchSysCacheX() interface also inspired me. If the hook function can deliver a few Datum values depending on object types and event types, it may allows the main hook to handle most of security checks, even if we need to add various flavors. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] security hook on table creation
Thanks for your reviewing, and sorry for delayed responding due to the LinuxCon Japan for a couple of days. (2010/09/28 12:57), Robert Haas wrote: 2010/9/1 KaiGai Koheikai...@ak.jp.nec.com: This patch allows external security providers to check privileges to create a new relation and to inform the security labels to be assigned on the new one. Review: I took a brief look at this patch tonight and I think it's on the wrong track. There's no reason for the hook function to return the list of security labels and then have the core code turn around and apply them to the object. If the hook function wants to label the object, it can just as easily call SetSecurityLabel() itself. However, it is not actually easy, because we cannot know OID of the new table before invocation of heap_create_with_catalog(). So, we needed to return a list of security labels to caller of the hook, then the core core calls SetSecurityLabel() with newly assigned OID. I don't think it is an option to move the hook after the pollution of system catalogs, although we can pull out any information about the new relation from syscache. It seems to me that there is a general pattern to the hooks that are needed here. For each object type for which we wish to have MAC integration, you need the ability to get control when the object is created and again when the object is dropped. You might want to deny the operation, apply labels to the newly created object, do some logging, or whatever. So it strikes me that you could have a hook function with a signature like this: typedef void (*object_access_hook_type)(ObjectType objtype, Oid oid, int subid, ObjectAccessType op); ...where ObjectAccessType is an enum. Then you could do something like this: #define InvokeObjectAccessHook(objtype, oid, subid, op) \ if (object_access_hook != NULL) \ object_access_hook(objtype, oid, subid, op); Then you can sprinkle calls to that macro in strategically chosen places to trap create, drop, comment, security label, ... whatever the object gets manipulated in a way that something like SE-Linux is apt to care about. So ObjectAccessType can have values like OAT_CREATE, OAT_DROP, OAT_COMMENT, OAT_SECURITY_LABEL, ... Sorry, it seems to me the idea simplifies the issue too much to implement access control features correctly. For example, we need to provide security modules the supplied label on the SECURITY LABEL hook, so it has to take one more argument at least. For example, we will need to provide them OID of the new schema on the ALTER TABLE SET SCHEMA at least too. : So, we need to inform the security modules some more extra information rather than OID of the objects to be referenced. I would like to mark this patch Returned with Feedback, because I think the above suggestions are going to amount to a complete rewrite. It is too early. Please consider again the reason why we needed to return a list of security labels to be assigned on the new relation Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Git downed?
Does the git.postgresql.org down? Harada-san being also unreachable now. -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] Git downed?
(2010/09/29 20:53), Dave Page wrote: 2010/9/29 KaiGai Koheikai...@kaigai.gr.jp: Does the git.postgresql.org down? Harada-san being also unreachable now. Seems to be a network issue. It's fine for some people (like me), and down for others. Thom reports that it's now back for him. Now I'm reachable to the git.postgreql.org. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] security hook on table creation
(2010/09/29 22:00), Robert Haas wrote: On Wed, Sep 29, 2010 at 6:38 AM, KaiGai Koheikai...@kaigai.gr.jp wrote: I don't think it is an option to move the hook after the pollution of system catalogs, although we can pull out any information about the new relation from syscache. Why not? All the existing security checks applies before modifying system catalogs. At least, I cannot find out any constructive reason why we try to apply permission checks on object creation time with different manner towards the existing privilege mechanism... Sorry, it seems to me the idea simplifies the issue too much to implement access control features correctly. For example, we need to provide security modules the supplied label on the SECURITY LABEL hook, so it has to take one more argument at least. For example, we will need to provide them OID of the new schema on the ALTER TABLE SET SCHEMA at least too. : So what? The patch you submitted doesn't provide the OID of the new schema when someone does ALTER TABLE SET SCHEMA, either. I proposed a design which was much more general than what you submitted, and you're now complaining that it's not general enough. It's unrealistic to think you're going to solve every problem with one patch. Sorry, I never said one patch with enough generic hook solves everything. By contraries, I think the proposed prototype of the hook cannot inform the plugins anything except for OID and event type, even if necessary. Some of permission checks needs its specific prototype to inform extra information rather than OIDs; such as new label in SECURITY LABEL hook, new schema in upcoming ALTER TABLE SET SCHEMA, and so on... Of course, we can implement some of permission checks with OID of the target object and event type collectly. E,g. I cannot image any extra information to check permission on COMMENT. I never deny it. Moreover, it's far from obvious that you actually do need the details that you're proposing anyway. Are you really going to write an SE-Linux policy that allows people to change the schema of table A to schema B but not schema C? Or that allows a hypothetical smack plugin to label a given object with one label but not another? And if so, are those absolutely must-have features for the first version or are those things that would be nice to have in version 3 or 4? In your proposition, prototype of the security hook has four arguments: ObjectType, oid, subid and ObjectAccessType, doesn't it? When user tries to change the schema of table from A to B, we can know the current schema of the table using syscache, but we need to inform the plugin that B is the new schema, because we have no way to pull out what schema was provided by the user. As LookupCreationNamespace() checks CREATE permission on the new schema, SELinux also want to check permission on the new schema, not only old one. So, I concerned about the prototype does not inform about new schema that user provided using ALTER TABLE SET SCHEMA statement. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] security hook on table creation
, oid, subid and ObjectAccessType, doesn't it? Yes. When user tries to change the schema of table from A to B, we can know the current schema of the table using syscache, but we need to inform the plugin that B is the new schema, because we have no way to pull out what schema was provided by the user. As LookupCreationNamespace() checks CREATE permission on the new schema, SELinux also want to check permission on the new schema, not only old one. So, I concerned about the prototype does not inform about new schema that user provided using ALTER TABLE SET SCHEMA statement. You're not answering my question. Are you going to write an SE-Linux policy that allows table A to be moved to schema B but not to schema C? And if so, is that an essential feature for the first version or something that can be added later? Ah, Sorry. Yes, I (eventually) want to provide this kind of the policy, but I think its priority is not first, indeed. My understanding from the conversation at BWPUG is that this is not something that Josh Brindle and David Quigley are concerned about. Hooks on object creation are important for type transitions, so that you can automatically assign labels rather than forcing users to apply them by hand; the fact that we can also the entire CREATE operation to get bounced from the same hook is a bonus. Yes, the hooks on object creation time are important, rather than others. But with that exception, they seemed to think that coarse-grained permissions would be fine for a basic implementation: perhaps even just install something in ProcessUtility_hook and bounce DDL across the board, so long as it's controlled by reference to the security policy rather than by DAC. I think we can do better than that in a pretty short period of time if we avoid getting side-tracked, but the key is that we have to avoid getting side-tracked. In this approach, we eventually need to deploy the hooks on object creation as we are currently working on. So, I don't think using ProcessUtility_hook for coarse-grained permissions is a right direction. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] security hook on table creation
(2010/09/30 10:28), Robert Haas wrote:
2010/9/29 KaiGai Koheikai...@ak.jp.nec.com:
But with that exception,
they seemed to think that coarse-grained permissions would be fine for
a basic implementation: perhaps even just install something in
ProcessUtility_hook and bounce DDL across the board, so long as it's
controlled by reference to the security policy rather than by DAC. I
think we can do better than that in a pretty short period of time if
we avoid getting side-tracked, but the key is that we have to avoid
getting side-tracked.
In this approach, we eventually need to deploy the hooks on object creation
as we are currently working on. So, I don't think using ProcessUtility_hook
for coarse-grained permissions is a right direction.
Well, it may be the easiest way to do certain things. For example, if
you wanted to control access to a command such as LOAD (which
presumably you do since otherwise a loadable module could trivially
subvert the security policy), it's unclear to me that there's any need
for a new hook; ProcessUtility_hook might very well be the best way to
tackle that. We need to consider the best way to handle each case.
In some cases, all of the necessary information may not be available
when ProcessUtility_hook is called, but where it is, we shouldn't
reinvent the wheel.
In the ideal world, I want to put a new hook to control LOAD command,
because the given library name is not expanded at ProcessUtility_hook
time (and expand_dynamic_library_name() is a static function), so
we cannot know enough information to apply fine-grained control;
such as per-libraries-control.
So, right-now, all we can do in coarse-grained permissions are to
prohibit LOAD command always when SE-PgSQL is installed.
(For more detail, it is not perfect because we can overwrite the
'local_shared_libraries' setting using connection string.)
However, we understood we need to prioritize our upcoming works,
and I think the security hooks on table creation has the highest
priority than others.
With respect to this patch, I think we are on the same page now, with
possibly some disagreement about how far it makes sense to go with
this that needn't concern us for the present. I'm going to mark this
patch Returned with Feedback, because we need to move on to other
patches that are closer to being committable.
OK. I'll refactor my patch set.
| #define InvokeObjectAccessHook(objtype, oid, subid, op) \
| if (object_access_hook != NULL) \
| object_access_hook(objtype, oid, subid, op);
One my preference is functions, rather than macros, because we
need a *.c file somewhere to put pointer variable of the hook
and it will become a good place to describe source code comments
of the hook.
In addition, I want to give these entrypoints its name which
represents an appropriate purpose of the hook, rather than
a uniformed one.
Example:
/*
* This hook is ...
*/
object_access_hook_type object_access_hook = NULL;
/*
* check_relation_create
*
* This hook is invoked when ...
:
:
* If violated, guest of the hook can raise an error.
*/
void
check_relation_create(Oid oid)
{
if (object_access_hook != NULL)
object_access_hook(OBJECT_TABLE, oid, OAT_CREATE);
}
Thanks,
--
KaiGai Kohei kai...@ak.jp.nec.com
--
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] security label support, revised
(2010/09/27 11:49), Robert Haas wrote: On Sat, Sep 25, 2010 at 7:04 AM, KaiGai Koheikai...@kaigai.gr.jp wrote: * The dummy_esp module and regression test for SECURITY LABEL statement. This module allows only four labels: unclassified, classified, secret and top secret. The later two labels can be set by only superusers. The new regression test uses this dummy_esp module to find out future regression in SECURITY LABEL statement. * A minimum description about external security provider at the tail of Database Roles and Privileges chapter. * Add pg_seclabels system view * Revising pg_dump/pg_dumpall - '--security-label' was replaced by '--no-security-label' - implemented according to the manner in comments. findSecLabels() and collectSecLabels() are added to reduce number of SQL queries, in addition to dumpSecLabel(). Thanks, this looks like mostly good stuff. Here's a new version of the patch with some bug fixes, additional regression tests, and other cleanup. I think this is about ready to commit. Thanks for your reviewing and cleaning-up. I didn't adopt your documentation and I renamed your contrib module from dummy_esp to dummy_seclabel, but the rest I took more or less as you had it. Fair enough. I intended the name of dummy_esp to host any other upcoming regression tests corresponding to security hooks, but right now it just provides dummy labels. For now, I don't want to use the term external security provider because that's not really what this provides - it just provides labels. I initially thought that an external security provider would really turn out to be a concept that was somewhat embedded in the system, but on further reflection I don't think that's the case. I think what we're going to end up with is a collection of hooks that might happen to be useful for security-related things, but not necessarily just those. Right, the security provider plugin which uses the collection of security hooks will provides access control decision, but we don't force anything in the way to make decisions. Someone may provide label based security, but other one may provide non-label based security. All we can say is that guest of the hook on SECURITY LABEL provides security label, but it is unclear whether it also provides any access control, or not. I also think the label provider is a fair enough naming. Anyway, I feel that it's a bit premature to document too much about what this might do someday; the documentation already in the patch is adequate to explain what it does now. I agreed. It is quite internal stuff how security hooks should perform on interactions with plugin modules, so it might be premature. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] pg_comments
Robert, I noticed a problem at the definition of the view. : +UNION ALL +SELECT + d.objoid, d.classoid, d.objsubid, + 'large object'::text AS objtype, + NULL::oid AS objnamespace, + d.objoid::text AS objname, + d.description +FROM + pg_description d + JOIN pg_largeobject_metadata lom ON d.objoid = lom.oid +WHERE + d.classoid = (SELECT oid FROM pg_class WHERE relname = 'pg_largeobject') + AND d.objsubid = 0 +UNION ALL : If and when user create a table named 'pg_largeobject' on anywhere except for the 'pg_catalog' schema, the (SELECT oid FROM pg_class WHERE relname = 'pg_largeobject') may not return 2613. It seems to me the query should be fixed up as follows: : WHERE d.classoid = (SELECT oid FROM pg_class WHERE relname = 'pg_largeobject' AND relnamespace = (SELECT oid FROM pg_namespace WHERE nspname = 'pg_catalog')) : Thanks, (2010/09/20 13:53), Robert Haas wrote: The psql \dd command has a couple of infelicities. 1. It doesn't actually list comments on all of the object types to which they can be applied using the COMMENT command. 2. It also doesn't list comments on access methods, which have comments but are not supported by the COMMENT command. 3. It doesn't even list comments on all of the object types which the psql documentation claims it does. 4. It chooses to print out both the name and object columns in a format which is not 100% compatible with the COMMENT command, so that you can't necessarily use the output of \dd to construct valid input to COMMENT. 5. The SQL query used to generate the output it does produce is 75 lines long, meaning that it's really entertaining if you need, for some reason, to edit that query. In view of the foregoing problems, I'd like to propose adding a new system view, tentatively called pg_comments, which lists all of the comments for everything in the system in such a way that it's reasonably possible to do further filtering out the output in ways that you might care about; and which also gives objects the names and types in a format that matches what the COMMENT command will accept as input. Patch attached. I haven't yet written the documentation for the view or adjusted src/bin/psql/describe.c to do anything useful with it, just so that I won't waste any more time on this if it gets shot down. But for the record, it took me something like three hours to write and test this view, which I think is an excellent argument for why we need it. Supposing no major objections, there are a few things to think about if we wish to have psql use this: A. The obvious thing to do seems to be to retain the existing code for server versions 9.1 and to use pg_comments for= 9.1. I would be inclined not to bother fixing the code for pre-9.1 servers to display comments on everything (a 9.1 psql against a 9.0 or prior server will be no worse than a 9.0 psql against the same server; it just won't be any better). B. The existing code localizes the contents of the object column. This is arguably a misfeature if you are about (4), but if we want to keep the existing behavior I'm not quite sure what the best way to do that is. C. It's not so obvious which comments should be displayed with \dd vs. \ddS. In particular, comments on toast schemas have the same problem recently discussed with \dn, and there is a similar issue with tablespaces. Generally, it's not obvious what to do for objects that don't live in schemas - access methods, for example, are arguably always system objects. But... that's arguable. D. Fixing (4) with respect to object names implies listing argument types for functions and operators, which makes the display output quite wide when using \ddS. I am inclined to say that's just the cost of making the output accurate. There may be other issues I haven't noticed yet, too. Incidentally, if you're wondering what prompted this patch, I was reviewing KaiGai Kohei's patch to add security label support and noticed its complete lack of psql support. I'm actually not really sure that there's any compelling reason to provide psql support, considering that we've gotten to the point where any backslash command is almost bound to be something not terribly mnemonic, and because there are likely to be either no security labels at all or so many that a command that just dumps them ALL out in bulk is all but useless. But we at least need to provide a suitable system view, because the catalog structure used by these catalogs that can handle SQL objects of any type is pretty obnoxious for user querying (though, of course, it's pretty neat as an internal format). -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- KaiGai Kohei kai...@ak.jp.nec.com -- Sent via pgsql-hackers mailing list
Re: [HACKERS] security label support, revised
(2010/09/24 20:56), Robert Haas wrote: 2010/9/23 KaiGai Koheikai...@ak.jp.nec.com: Please see http://archives.postgresql.org/pgsql-hackers/2010-09/msg01080.php OK, I'll emulate this approach at first. Don't worry about this part - I will do this myself. If you can just fix the pg_dump stuff, I think we will be in pretty good shape. Ahh, I already did this part at the today's afternoon: http://bit.ly/9kOsnx And, the pg_dump stuff has been just implemented(, but not tested yet): http://bit.ly/a0eVfL If you prefer to keep the patch small, I'll revert the system_views.sql in the next patch. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] security label support, revised
-label option, we need to dump security labels in a separated section from comments. So, we cannot pack them into COMMENT sections. There are a few other problems. First, there's no psql support of any kind. Now, this is kind of a corner-case feature: so maybe we don't really need it. And as I mentioned on another thread, there aren't a lot of good letters left for backslash-d commands. So I'd be just as happy to add a system view along the lines I previously proposed for comments and call it good. Alternatively, or in addition, we could add a \d command after all. The best way forward is debatable, but we certainly need *something*, because interpreting the pg_seclabel catalog by hand is not for the faint of heart. Do you suggest the new system views should be defined for each supported object classes, such as pg_largeobject_seclabel? It seems to me a bit inflation of number of system views. My preference is psql's \d commands at first. Second, there are no regression tests. It's a bit tricky to think about how to crack that nut because this feature is somewhat unusual in that it can't be used without loading an appropriate loadable module. I'm wondering if we can ship a dummy_seclabel contrib module that can be loaded during the regression test run and then run various tests using that, but I'm not quite sure what the best way to set that up is. SECURITY LABEL is a core feature, so it would be nice to test it in the core regression tests... but maybe that's too complicated to get working, and we should just test it from the contrib module. As you suggested in the following topic, I think it is the best way to use LOAD command at the head of regression test (or just after SECURITY LABEL command being failed due to no modules). I'll add a dummy module into contrib, and regression test for labels. Right now, I don't have any complaint about the patch to the backend you revised, so I'd like to submit the next patch as an incremental one to the seclabel-v4.patch, except for src/bin/pg_dump. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] security label support, revised
(2010/09/24 11:53), Robert Haas wrote: 2010/9/23 KaiGai Koheikai...@ak.jp.nec.com: Most of the contents of the new documentation section on external security providers seemed irrelevant to me, which I guess I can only blame myself for since I was the one who asked for that section to be created, and I didn't specify what it should contain all that well. I took a try at rewriting it to be more on-topic, but it didn't amount to much so I ended up just ripping that part out completely. One headache thing when I tried to describe the new documentation section was what we should describe here, because whole of the chapters in Server Administration are on the standpoint of users, not developers. Under the previous discussion, I suggested to move the most of descriptions about external security providers into chapters in Internals, except for a mention about a fact we have external security provider at the tail of Database Roles and Privileges. How about the idea? Perhaps, the chapters in Internals are appropriate place to introduce specification of security hooks. Perhaps. I know that in the past we have not documented hook functions, and I'm thinking that there may be people (in particular, possibly Tom) who have strong feelings about keeping it that way. Even if that's not the case, once we do start documenting the hooks, we will presumably need to document all of them, and that may be more of a project than I really want to get into right now, especially if I will have to do much of the work myself. I'd be perfectly ecstatic if a committable patch spontaneously materialized, but... If so, maybe, we should keep the scale of documentation to a minimum, then, rest of the detailed specifications of hooks are kept as source code comments. Because authors of ESP obviously reference source code, the comments will provide them enough information. Also, the pg_dump support for security labels should really reuse the existing design for comments, rather than inventing a new and less efficient method, unless there is some really compelling reason why the method used for comments won't work. Please send a reworked patch for just this directory (src/bin/pg_dump). I intended to follow on the existing design for comments. Could you suggest me how should it be fixed up the design? dumpComment calls findComments calls collectComments, which dumps all the comments in one query (not one query per object). Because of the --no-security-label option, we need to dump security labels in a separated section from comments. So, we cannot pack them into COMMENT sections. I'm not proposing that - I just want to avoid sending so many database queries, if that's possible. Ahh, Thanks. It makes me clear. I'll revise dumpSecLabel to call findSecLabels and collectSecLabels on the first call. There are a few other problems. First, there's no psql support of any kind. Now, this is kind of a corner-case feature: so maybe we don't really need it. And as I mentioned on another thread, there aren't a lot of good letters left for backslash-d commands. So I'd be just as happy to add a system view along the lines I previously proposed for comments and call it good. Alternatively, or in addition, we could add a \d command after all. The best way forward is debatable, but we certainly need *something*, because interpreting the pg_seclabel catalog by hand is not for the faint of heart. Do you suggest the new system views should be defined for each supported object classes, such as pg_largeobject_seclabel? It seems to me a bit inflation of number of system views. My preference is psql's \d commands at first. Please see http://archives.postgresql.org/pgsql-hackers/2010-09/msg01080.php OK, I'll emulate this approach at first. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] ALTER TYPE extensions
Sorry, I missed a bug when we create a typed table using composite
type which has been altered.
postgres=# CREATE TYPE comp_1 AS (x int, y int, z int);
CREATE TYPE
postgres=# ALTER TYPE comp_1 DROP ATTRIBUTE y;
ALTER TYPE
postgres=# CREATE TABLE t1 OF comp_1;
ERROR: cache lookup failed for type 0
postgres=# SELECT attname, attnum, attisdropped FROM pg_attribute
WHERE attrelid = 'comp_1'::regclass;
attname| attnum | attisdropped
--++--
x| 1 | f
pg.dropped.2 | 2 | t
z| 3 | f
(3 rows)
Perhaps, we also need to patch at transformOfType() to
skip attributes with attisdropped.
An additional question. It seems me we can remove all the attributes
from the composite type, although CREATE TYPE prohibits to create
a composite type without any attribute.
What does it mean a composite type with no attribute?
Or, do we need a restriction to prevent the last one attribute?
Rest of comments are below.
(2010/09/18 5:44), Peter Eisentraut wrote:
On fre, 2010-09-17 at 18:15 +0900, KaiGai Kohei wrote:
* At the ATPrepAddColumn(), it seems to me someone added a check
to prevent adding a new column to typed table, as you try to
add in this patch.
Good catch. Redundant checks removed.
OK,
* At the ATPrepAlterColumnType(), you enclosed an existing code
block by if (tab-relkind == RELKIND_RELATION) { ... }, but
it is not indented to appropriate level.
Yeah, just to keep the patch small. ;-)
Hmm...
Although I expect the patched routine also should follow the common
coding style in spite of patch size, but it may not be a thing that
I should decide here.
So, I'd like to entrust this decision to committer. OK?
* RENAME ATTRIBUTE ... TO ...
Even if the composite type to be altered is in use, we can alter
the name of attribute. Is it intended?
No. Added a check for it now.
OK,
BTW, is there any requirement from SQL standard about behavior
when we try to add/drop an attribute of composite type in use?
This patch always prohibit it, using find_typed_table_dependencies()
and find_composite_type_dependencies().
However, it seems to me not difficult to alter columns of typed
tables subsequent with this ALTER TYPE, although it might be
not easy to alter definitions of embedded composite type already
in use.
Of course, it may be our future works. If so, it's good.
The prohibition on altering types that are used in typed tables is
actually from the SQL standard. But for now it's just because it's not
implemented; I plan to work on extending that later.
The restriction by find_composite_type_dependencies() was already there
for altering tables, and I just kept it the same for now.
Thanks for your explanation. It made me clear.
Thanks,
--
KaiGai Kohei kai...@ak.jp.nec.com
--
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] ALTER TYPE extensions
(2010/09/22 5:17), Peter Eisentraut wrote: On tis, 2010-09-21 at 17:53 +0900, KaiGai Kohei wrote: Sorry, I missed a bug when we create a typed table using composite type which has been altered. Perhaps, we also need to patch at transformOfType() to skip attributes with attisdropped. Fixed. OK, An additional question. It seems me we can remove all the attributes from the composite type, although CREATE TYPE prohibits to create a composite type without any attribute. What does it mean a composite type with no attribute? Or, do we need a restriction to prevent the last one attribute? We need to allow the creation of zero-attribute types then; same as with CREATE TABLE. I have fixed that now. OK, This version of the patch seems to me OK. I marked it as 'ready for committer'. Thanks, -- KaiGai Kohei kai...@ak.jp.nec.com -- 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] ALTER TYPE extensions
(2010/08/09 5:54), Peter Eisentraut wrote:
For the next review cycle, here is a patch that adds some ALTER TYPE
subcommands for composite types:
ALTER TYPE ... ADD ATTRIBUTE
ALTER TYPE ... DROP ATTRIBUTE
ALTER TYPE ... ALTER ATTRIBUTE ... SET DATA TYPE
ALTER TYPE ... RENAME ATTRIBUTE
These work similarly to the analogous ALTER TABLE / $ACTION COLUMN
commands. The first two above are from the SQL standard.
I checked this patch, then noticed some points:
* At the ATPrepAddColumn(), it seems to me someone added a check
to prevent adding a new column to typed table, as you try to
add in this patch.
Since this patch was submitted about one month ago, it might be
necessary to rebase to the latest master.
* At the ATPrepAlterColumnType(), you enclosed an existing code
block by if (tab-relkind == RELKIND_RELATION) { ... }, but
it is not indented to appropriate level.
|if (tab-relkind == RELKIND_RELATION)
|{
|/*
| * Set up an expression to transform the old data value to the new type.
| * If a USING option was given, transform and use that expression, else
| * just take the old value and try to coerce it. We do this first so that
| * type incompatibility can be detected before we waste effort, and
| * because we need the expression to be parsed against the original table
| * rowtype.
| */
|if (cmd-transform)
|{
|RangeTblEntry *rte;
|
|/* Expression must be able to access vars of old table */
|rte = addRangeTableEntryForRelation(pstate,
|:
Perhaps, it is violated to the common coding style.
* RENAME ATTRIBUTE ... TO ...
Even if the composite type to be altered is in use, we can alter
the name of attribute. Is it intended?
In this case, this renaming does not affects column name of the
typed tables in use.
Is it necessary to prohibit renaming, or also calls renameatt()
for the depending typed tables.
postgres=# CREATE TYPE comp as (a int, b text);
CREATE TYPE
postgres=# CREATE TABLE t OF comp;
CREATE TABLE
postgres=# SELECT * FROM t;
a | b
---+---
(0 rows)
postgres=# ALTER TYPE comp RENAME ATTRIBUTE b TO bbb;
ALTER TYPE
postgres=# CREATE TABLE s OF comp;
CREATE TABLE
postgres=# SELECT * FROM t;
a | b
---+---
(0 rows)
postgres=# SELECT * FROM s;
a | bbb
---+-
(0 rows)
BTW, is there any requirement from SQL standard about behavior
when we try to add/drop an attribute of composite type in use?
This patch always prohibit it, using find_typed_table_dependencies()
and find_composite_type_dependencies().
However, it seems to me not difficult to alter columns of typed
tables subsequent with this ALTER TYPE, although it might be
not easy to alter definitions of embedded composite type already
in use.
Of course, it may be our future works. If so, it's good.
Thanks,
--
KaiGai Kohei kai...@ak.jp.nec.com
--
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] security label support, revised
(2010/09/13 20:46), Robert Haas wrote:
2010/9/13 KaiGai Koheikai...@ak.jp.nec.com:
Robert, although you suggested that only one ESP module shall be
invoked on relabeling an object before, and I agreed this design
at that time, but I noticed that we cannot implement the following
behavior correctly.
SELinux defines these permissions corresponding to table relabeling.
- db_table:{setattr}
It is necessary to change *any* properties of the table.
Security label is one of properties of the table, so, needs to be
checked on relabeling, not only ALTER TABLE and so on.
- db_table:{relabelfrom relabelto}
It is neccesary to change label of the table.
User must have {relabelfrom} permission on the older security label,
and {relabelto} permission on the new security label.
If and when multiple ESP modules are installed, we need to consider
the way to handle SECURITY LABEL statement for other modules.
When user tries to relabel security label of a table managed by
something except for selinux, it is not relevant to {relabelfrom
relabelto} permission in selinux, but we want to check {setattr}
permission on the operation, because it is a property of the table,
although not a security label in selinux.
In the current patch, the core PG (ExecSecurityLabel()) invokes only
one hook that matches with the given FOResp option.
However, I reconsidered this hook should be simply invoked like other
hooks. Then, the ESP module decides whether ignores or handles the
invocation, and also decides to call secondary hook when multiple
modules are loaded. If so, selinux module can check {setattr} and
also calls secondary modules.
In the previous discussion, I missed the possibility of the case
when we want to check relabeling a security label managed by other
ESP. But it might be also necessary to call all the modules which
want to get control on SECURITY LABEL statement, apart from whether
it manages the supplied security label, or not.
Maybe. The whole point of MAC is that the security policy itself is
capable of enforcing all of the necessary protections. It shouldn't
be necessary for it to control what DAC or other MAC providers do,
should it?
Yes, what we should do here is that DAC and any MACs make their own
decision individually, then PG eventually prevents user's request if
one of them denied at least.
At any rate, they'll probably treat it quite a bit
differently than a change of their own label. I think it might be
cleaner to fold that in under some of the DDL permissions checking and
refactoring which we know still needs to be done, rather than cramming
it in here.
Yes, if and when MAC-X and MAC-Y are installed, it is significant event
for MAC-X to change X's label, so MAC-X may need to check special
permissions. But it is a common event for MAC-Y and DAC, so they checks
an appropriate permission to change one of the properties. Hoever, it
does not mean we should not give any chance MAC-Y and DAC to check something.
I'll revise my patch within a couple of days.
Thanks,
Note that presumably COMMENT ON would need similar
treatment, and there may be other things.
--
KaiGai Kohei kai...@kaigai.gr.jp
--
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] security label support, revised
Robert, although you suggested that only one ESP module shall be
invoked on relabeling an object before, and I agreed this design
at that time, but I noticed that we cannot implement the following
behavior correctly.
SELinux defines these permissions corresponding to table relabeling.
- db_table:{setattr}
It is necessary to change *any* properties of the table.
Security label is one of properties of the table, so, needs to be
checked on relabeling, not only ALTER TABLE and so on.
- db_table:{relabelfrom relabelto}
It is neccesary to change label of the table.
User must have {relabelfrom} permission on the older security label,
and {relabelto} permission on the new security label.
If and when multiple ESP modules are installed, we need to consider
the way to handle SECURITY LABEL statement for other modules.
When user tries to relabel security label of a table managed by
something except for selinux, it is not relevant to {relabelfrom
relabelto} permission in selinux, but we want to check {setattr}
permission on the operation, because it is a property of the table,
although not a security label in selinux.
In the current patch, the core PG (ExecSecurityLabel()) invokes only
one hook that matches with the given FOR esp option.
However, I reconsidered this hook should be simply invoked like other
hooks. Then, the ESP module decides whether ignores or handles the
invocation, and also decides to call secondary hook when multiple
modules are loaded. If so, selinux module can check {setattr} and
also calls secondary modules.
In the previous discussion, I missed the possibility of the case
when we want to check relabeling a security label managed by other
ESP. But it might be also necessary to call all the modules which
want to get control on SECURITY LABEL statement, apart from whether
it manages the supplied security label, or not.
Thanks,
(2010/08/31 15:27), KaiGai Kohei wrote:
The attached patch is a revised version of security label support.
summary of changes:
* The logic to translate object-name to object-id was rewritten
with the new get_object_address().
* Right now, it does not support labeling on shared database object
(ie; pg_database), so wrapper functions to XXXLocalSecLabel() were
removed.
* The restriction of same security label within whole of inheritance
tree has gone. And, the 'num_parents' argument was also removed
from the security hook.
* ExecRelationSecLabel() was also removed, although you suggested
to rename it, because it translate the supplied relation name
into relation id and handled child tables, but it get unnecessary.
* The chapter of 'External Security Provider' was added.
It introduces overview of ESP concept and MAC features.
Perhaps, other structures of chapters are more preferable,
but I also think we need a draft at the begining of discussion.
* The '--security-label' option was added to pg_dump/pg_dumpall;
it allows to include security label of objects in the archives.
The '--no-security-label' option was also added to pg_restore;
it allows to skip security labels, even if the archive contains
security labels.
Thanks,
(2010/08/10 5:02), Robert Haas wrote:
2010/7/26 KaiGai Koheikai...@ak.jp.nec.com:
The attached patches are revised ones, as follows.
I think this is pretty good, and I'm generally in favor of committing
it. Some concerns:
1. Since nobody has violently objected to the comment.c refactoring
patch I recently proposed, I'm hopeful that can go in. And if that's
the case, then I'd prefer to see that committed first, and then rework
this to use that code. That would eliminate some code here, and it
would also make it much easier to support labels on other types of
objects.
2. Some of this code refers to local security labels. I'm not sure
what's local about them - aren't they just security labels? On a
related note, I don't like the trivial wrappers you have here, with
DeleteSecurityLabel around DeleteLocalSecLabel, SetSecurityLabel
around SetLocalSecLabel, etc. Just collapse these into a single set
of functions.
3. Is it really appropriate for ExecRelationSecLabel() to have an
Exec prefix? I don't think so.
4. Please get rid of the nkeys++ stuff in DeleteLocalSecLabel() and
just use fixed offsets as we do everywhere else.
5. Why do we think that the relabel hook needs to be passed the number
of expected parents?
6. What are we doing about the assignment of initial security labels?
I had initially thought that perhaps new objects would just start out
unlabeled, and the user would be responsible for labeling them as
needed. But maybe we can do better. Perhaps we should extend the
security provider hook API with a function that gets called when a
labellable object gets created, and let each loaded security provider
return any label it would like attached. Even if we don't do that
now, esp_relabel_hook_entry needs
Re: [HACKERS] leaky views, yet again
(2010/09/02 13:30), KaiGai Kohei wrote: (2010/09/02 12:38), Robert Haas wrote: 2010/9/1 KaiGai Koheikai...@ak.jp.nec.com: (2010/09/02 11:57), Robert Haas wrote: 2010/9/1 KaiGai Koheikai...@ak.jp.nec.com: Right now, it stands on a strict assumption that considers operators implemented with built-in functions are safe; it does not have no possibility to leak supplied arguments anywhere. Please note that this patch does not case about a case when a function inside a view and a function outside a view are distributed into same level and the later function has lower cost value. Without making some attempt to address these two points, I don't see the point of this patch. Also, I believe we decided previously do this deoptimization only in case the user requests it with CREATE SECURITY VIEW. Perhaps, I remember the previous discussion incorrectly. If we have a hint about whether the supplied view is intended to security purpose, or not, it seems to me it is a reliable method to prevent pulling up the subqueries come from security views. Is it too much deoptimization? Well, that'd prevent something like id = 3 from getting pushed down, which seems a bit harsh. I've tried to implement a proof of the concept patch according to the following logic. (For the quick hack, it does not include statement support. It just considers view with name begun from s as security views.) Hmm. If so, we need to remember what FromExpr was come from subqueries of security views, and what were not. Then, we need to prevent leakable clause will be distributed to inside of them. In addition, we also need to care about the order of function calls in same level, because it is not implicitly solved. At first, let's consider top-half of the matter. When views are expanded into subqueries in query-rewriter, Query tree lost an information OID of the view, because RangeTblEntry does not have OID of the relation when it is RTE_SUBQUERY. So, we need to patch here to mark a flag whether the supplied view is security focused, or not. This patch added 'security_view' flag into RangeTblEntry. It shall be set when the query rewriter expands security views. Then, pull_up_simple_subquery() pulls up a supplied subquery into normal join, if possible. In this case, FromExpr is chained into the upper level. Of course, FromExpr does not have a flag to show its origin, so we also need to copy the new flag in RangeTblEntry to FromExpr. This patch also added 'security_view' flag into FromExpr. It shall be set when the pull_up_simple_subquery() pulled up a RangeTblEntry with security_view = true. Then, when distribute_qual_to_rels() is called, the caller also provides a Bitmapset of relation-Ids which are contained under the FromExpr with the flag saying it came from the security views. Even if the supplied clause references a part of the Bitmapset, we need to prevent the clause being pushed down into the relations came from security views, except for ones we can make sure these are safe. Just before distribute_qual_to_rels(), deconstruct_recurse() is invoked. It walks on the supplied join-tree to collect what relations are appeared under the current FromExpr/JoinExpr. The deconstruct_recurse() was modified to take two new arguments of 'bool below_sec_barriers' and 'Relids *sec_barriers'. The first one means the current recursion is under the FromExpr with security_view being true. At that time, it set appeared relations on the sec_barriers, then returns. In the result, the 'sec_barriers' shall become a bitmapset of relations being under the FromExpr which is originated by security views. Then, 'sec_barriers' shall be delivered to distribute_qual_to_rels(). If the supplied qualifier references a part of 'sec_barriers' and contains possibly leakable functions, it appends whole of the sec_barriers to the bitmapset of relations on which the clause is depending. In the result, it shall not be pushed down into the security view. Example) testdb=# CREATE VIEW n_view AS SELECT * FROM t1 JOIN t2 ON t1.a = t2.x; CREATE VIEW testdb=# CREATE VIEW s_view AS SELECT * FROM t1 JOIN t2 ON t1.a = t2.x; CREATE VIEW testdb=# EXPLAIN SELECT * FROM n_view WHERE f_malicious(y); QUERY PLAN --- Hash Join (cost=334.93..365.94 rows=410 width=72) Hash Cond: (t1.a = t2.x) - Seq Scan on t1 (cost=0.00..22.30 rows=1230 width=36) - Hash (cost=329.80..329.80 rows=410 width=36) - Seq Scan on t2 (cost=0.00..329.80 rows=410 width=36) Filter: f_malicious(y) (6 rows) testdb=# EXPLAIN SELECT * FROM s_view WHERE f_malicious(y); QUERY PLAN --- Hash Join (cost=37.68..384.39 rows=410 width=72) Hash Cond: (t1.a = t2.x) Join Filter: f_malicious(t2.y) - Seq Scan on t1 (cost=0.00..22.30 rows=1230 width=36
Re: [HACKERS] leaky views, yet again
(2010/07/21 19:35), KaiGai Kohei wrote: (2010/07/21 19:26), Robert Haas wrote: 2010/7/21 KaiGai Koheikai...@ak.jp.nec.com: On the other hand, if it's enough from a performance point of view to review and mark only a few built-in functions like index operators, maybe it's ok. I also think it is a worthful idea to try as a proof-of-concept. Yeah. So, should we mark this patch as Returned with Feedback, and you can submit a proof-of-concept patch for the next CF? Yes, it's fair enough. The attached patch is a proof-of-concept one according to the suggestion from Heikki before. Right now, it stands on a strict assumption that considers operators implemented with built-in functions are safe; it does not have no possibility to leak supplied arguments anywhere. This patch modifies the logic that the planner decides where the given qualifier clause should be distributed. If the clause does not contain any leakable functions, nothing were changed. In this case, the clause shall be pushed down into inside of the join, if it depends on one-side of the join. Elsewhere, if the clause contains any leakable functions, this patch prevents to push down the clause into join loop, because the clause need to be evaluated after the condition of join. If it would not be prevented, the leakable function may expose the contents to be invisible for users; due to the VIEWs for row-level security purpose. Example postgres=# CREATE OR REPLACE FUNCTION f_policy(int) RETURNS bool LANGUAGE 'plpgsql' AS 'begin return $1 % 2 = 0; end;'; CREATE FUNCTION postgres=# CREATE OR REPLACE FUNCTION f_malicious(text) RETURNS bool LANGUAGE 'plpgsql' COST 0.001 AS 'begin raise notice ''leak: %'', $1; return true; end'; CREATE FUNCTION postgres=# CREATE TABLE t1 (a int primary key, b text); NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index t1_pkey for table t1 CREATE TABLE postgres=# CREATE TABLE t2 (x int primary key, y text); NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index t2_pkey for table t2 CREATE TABLE postgres=# INSERT INTO t1 VALUES (1,'aaa'), (2,'bbb'), (3,'ccc'); INSERT 0 3 postgres=# INSERT INTO t2 VALUES (2,'xxx'), (3,'yyy'), (4,'zzz'); INSERT 0 3 postgres=# CREATE VIEW v AS SELECT * FROM t1 JOIN t2 ON f_policy(a+x); CREATE VIEW * SELECT * FROM v WHERE f_malicious(b); [without this patch] postgres=# EXPLAIN SELECT * FROM v WHERE f_malicious(b); QUERY PLAN -- Nested Loop (cost=0.00..133685.13 rows=168100 width=72) Join Filter: f_policy((t1.a + t2.x)) - Seq Scan on t2 (cost=0.00..22.30 rows=1230 width=36) - Materialize (cost=0.00..24.35 rows=410 width=36) - Seq Scan on t1 (cost=0.00..22.30 rows=410 width=36) Filter: f_malicious(b) (6 rows) == f_malicious() may be raise a notice about invisible tuples. [with this patch] postgres=# EXPLAIN SELECT * FROM v WHERE f_malicious(b); QUERY PLAN --- Nested Loop (cost=0.00..400969.96 rows=168100 width=72) Join Filter: (f_malicious(t1.b) AND f_policy((t1.a + t2.x))) - Seq Scan on t1 (cost=0.00..22.30 rows=1230 width=36) - Materialize (cost=0.00..28.45 rows=1230 width=36) - Seq Scan on t2 (cost=0.00..22.30 rows=1230 width=36) (5 rows) == f_malicious() is moved to outside of the join. It is evaluated earlier than f_policy() in same level due to the function cost, but it is another matter. * SELECT * FROM v WHERE a = 2; [without this patch] postgres=# EXPLAIN SELECT * FROM v WHERE a = 2; QUERY PLAN - Nested Loop (cost=0.00..353.44 rows=410 width=72) Join Filter: f_policy((t1.a + t2.x)) - Index Scan using t1_pkey on t1 (cost=0.00..8.27 rows=1 width=36) Index Cond: (a = 2) - Seq Scan on t2 (cost=0.00..22.30 rows=1230 width=36) (5 rows) [with this patch] postgres=# EXPLAIN SELECT * FROM v WHERE a = 2; QUERY PLAN - Nested Loop (cost=0.00..353.44 rows=410 width=72) Join Filter: f_policy((t1.a + t2.x)) - Index Scan using t1_pkey on t1 (cost=0.00..8.27 rows=1 width=36) Index Cond: (a = 2) - Seq Scan on t2 (cost=0.00..22.30 rows=1230 width=36) (5 rows) == a = 2 is a built-in operator, so we assume it is safe. This clause was pushed down into the join loop, then utilized to index scan. * SELECT * FROM v WHERE a::text = 'a'; [without this patch] postgres=# EXPLAIN SELECT * FROM v WHERE a::text = '2'; QUERY PLAN Nested Loop (cost=0.00
[HACKERS] security hook on table creation
This patch allows external security providers to check privileges
to create a new relation and to inform the security labels to be
assigned on the new one.
This hook is put on DefineRelation() and OpenIntoRel(), but isn't
put on Boot_CreateStmt, create_toast_table() and make_new_heap(),
although they actually create a relation, because I assume these
are the cases when we don't need individual checks and security
labels.
DefineIndex() also creates a new relation (RELKIND_INDEX), but
it seems to me the PG implementation considers indexes are a part
of properties of tables, because it always has same owner-id.
So, it shall be checked at the upcoming ALTER TABLE hook, instead.
The ESP plugins can return a list of security labels of the new
relation, columns and relation-type. If multiple ESPs are installed,
it shall append its security labels on the security labels of the
secondary plugin.
The list shall be a list of SecLabelItem as follows:
typedef struct
{
ObjectAddress object;
const char *tag;
const char *seclabel;
} SecLabelItem;
OID of them are decided in heap_create_with_catalog(), so ESP cannot
know what OID shall be supplied at the point where it makes access
control decision.
So, the core routine fix up the SecLabelItem::object-objectId later,
if InvalidOid is given. I think it is a reasonable idea rather than
putting one more hook to store security labels after the table creation.
Please also note that this patch depends on the security label support
patch that I submitted before.
Thanks,
--
KaiGai Kohei kai...@ak.jp.nec.com
*** a/src/backend/bootstrap/bootparse.y
--- b/src/backend/bootstrap/bootparse.y
***
*** 246,252 Boot_CreateStmt:
(Datum) 0,
false,
true,
! false);
elog(DEBUG4, relation created with oid %u, id);
}
do_end();
--- 246,253
(Datum) 0,
false,
true,
! false,
! NIL);
elog(DEBUG4, relation created with oid %u, id);
}
do_end();
*** a/src/backend/catalog/heap.c
--- b/src/backend/catalog/heap.c
***
*** 49,54
--- 49,55
#include catalog/pg_type.h
#include catalog/pg_type_fn.h
#include catalog/storage.h
+ #include commands/seclabel.h
#include commands/tablecmds.h
#include commands/typecmds.h
#include miscadmin.h
***
*** 94,99 static Oid AddNewRelationType(const char *typeName,
--- 95,101
static void RelationRemoveInheritance(Oid relid);
static void StoreRelCheck(Relation rel, char *ccname, Node *expr,
bool is_local, int inhcount);
+ static void StoreSecLabels(Relation rel, Oid typeId, List *seclabels);
static void StoreConstraints(Relation rel, List *cooked_constraints);
static bool MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr,
bool allow_merge, bool is_local);
***
*** 882,887 AddNewRelationType(const char *typeName,
--- 884,890
* use_user_acl: TRUE if should look for user-defined default permissions;
* if FALSE, relacl is always set NULL
* allow_system_table_mods: TRUE to allow creation in system namespaces
+ * seclabels : list of security labels to be assigned on, if exist
*
* Returns the OID of the new relation
*
***
*** 905,911 heap_create_with_catalog(const char *relname,
Datum reloptions,
bool use_user_acl,
bool allow_system_table_mods,
! bool if_not_exists)
{
Relation pg_class_desc;
Relation new_rel_desc;
--- 908,915
Datum reloptions,
bool use_user_acl,
bool allow_system_table_mods,
! bool if_not_exists,
! List *seclabels)
{
Relation pg_class_desc;
Relation new_rel_desc;
***
*** 1189,1194 heap_create_with_catalog(const char *relname,
--- 1193,1203
}
/*
+ * Store any supplied security labels of the relation
+ */
+ StoreSecLabels(new_rel_desc, new_type_oid, seclabels);
+
+ /*
* Store any supplied constraints and defaults.
*
* NB: this may do a CommandCounterIncrement and rebuild the relcache
***
*** 1808,1813 StoreRelCheck(Relation rel, char *ccname, Node *expr,
--- 1817,1861
}
/*
+ * Store any supplied security labels of the new relation
+ */
+ static void
+ StoreSecLabels(Relation rel, Oid typeId, List *seclabels)
+ {
+ ListCell *l;
+
+ foreach (l, seclabels)
+ {
+ SecLabelItem *sl = lfirst(l);
+ ObjectAddress object;
+
+ Assert(sl-tag != NULL sl-seclabel != NULL);
+
+ memcpy(object, sl-object, sizeof(object));
+
+ /*
+ * It fixes up OID of the new relation and type, because these IDs were
+ * not decided at the point when external security provider made access
+ * control decision and returns security label to be assigned
