Thread: How to get SE-PostgreSQL acceptable
I have re-reviewed the SE-PostgreSQL patch set. I don't want to talk about here whether the security model is appropriate, how foreign keys are to be handled etc. I want to discuss that I really don't like the architecture of this patch. I have said this before in previous review rounds, but let me make it a little clearer here. Steps to get your patch accepted: One feature at a time --------------------- By my count, your patch set implements at least three or four major features: 1a. System-wide consistency in access control 1b. Mandatory access control 2. Row-level security 3. Additional privileges (permission to alter tables, modify large objects, etc.) You may object and say, these morally belong together in a proper/professional/adequate implementation of this feature you have planned. But realistically, they can be separated. And if a feature is controversial, difficult, or complicated, it would be in your interest to deal with one feature at a time. "Deal" means the whole round: discuss design, write patch, review, test, commit, relax. If I had to do this, I would first write a patch for #1: A patch that additionally executes existing privilege checks against an SELinux policy. Existing privilege checks are a well-defined set: they mostly happen through pg_xxx_aclcheck() functions. Hook your checks in there. This patch would already provide useful functionality,but it would be much easier to review and verify, because we know how permission checking works in the existing system, so we can compare them. And it would build confidence among developers and users about the whole idea, about SELinux integration etc. Then you can tackle #3: Place permission checks in more places. This patch would be simple to review and verify, because at this point we already know how SELinux integration works, and making more use of it is not such a big step. At this point we could also discuss whether some of these additional checks are useful enough to also expose via the traditional SQL ACLs, which would further simplify the patch. Row-level security should also be developed as a completely separate feature, without any SELinux tie-in initially. This is not only important to make review and verification simpler, but also because we really need a wider test community for such a tricky feature. And the set of SELinux users is quite limited, and the intersection with PostgreSQL developers is almost empty. This was already previously discussed at length. No in-code frameworks --------------------- Write your code so that it is fully integrated with the existing code. Or write a plugin interface and then write a plugin. But don't invent a "framework" because you are afraid to integrate the new code with the old code. As mentioned above, permission checks are done through pg_xxx_aclcheck() functions, which is enough of a framework. I wouldn't want yet another framework that does more permission checking at other times and places. If the existing interfaces are not adequate foryour purpose, by all means, extend, refactor, or rewrite them. But don't just avoid it because you don't want to interfere with the existing code. So scrap the whole "PGACE" thing. If you need to refactor the aclcheck interfaces, that's another separate patch, which can easily be reviewed and verified, simplifying the following patches even further. These things are not going to get done within two weeks. But if you start producing small, self-contained patches along the above lines, you are much more likely to make progress over the coming development cycle.
Sorry for long description again. Peter Eisentraut wrote: > I have re-reviewed the SE-PostgreSQL patch set. I don't want to talk > about here whether the security model is appropriate, how foreign keys > are to be handled etc. I want to discuss that I really don't like the > architecture of this patch. I have said this before in previous review > rounds, but let me make it a little clearer here. Steps to get your > patch accepted: > > > One feature at a time > --------------------- > > By my count, your patch set implements at least three or four major > features: > > 1a. System-wide consistency in access control > > 1b. Mandatory access control > > 2. Row-level security > > 3. Additional privileges (permission to alter tables, modify large > objects, etc.) > > You may object and say, these morally belong together in a > proper/professional/adequate implementation of this feature you have > planned. But realistically, they can be separated. And if a feature is > controversial, difficult, or complicated, it would be in your interest > to deal with one feature at a time. "Deal" means the whole round: > discuss design, write patch, review, test, commit, relax. I can't afford not to make clear these issues. In this case, (1a) and (1b) are indivisible, because I want to apply SELinux as a security server of (1a), SELinux has to be MAC feature. However, I don't deny a (1b) without (1a) feature like "Oracle Label Security", which is not a facility I want to make. I guess (1b) also contains a feature to manage security label. Please note that I don't really want to (1b) only feature. The system-wide consistency in access control is the soul. We can consider (2) as a separated issue. In fact, I already provide a GUC: sepostgresql_row_level=on/off. It also means whether users accept a set of benefit(row-level granularity) and demerit(cover channel of PK/FK), or not. OK, I don't discuss about covert channel here. The (3) is involved to (1b). As a basic assumption, MAC system need to check *any actions* come from clients, even if they have superuser privileges. We already have SQL-level privileges, like ACL_SELECT and so on. However, some of operations implicitly assume request come from superuser is safe. For example, superuser is allowed to load a discretionary shared library file, but it also means he is trusted. If security design (which is defined by (1a) and (1b) primarily) does not allow unconditionally trusted user, it is quite natural to apply additional privileges, even if vanilla one unconditionally trust superuser. Unfortunatelly, we don't have any access controls on large object, but SELinux does not want to provide an information store without mandatory access controls. So, SE-PostgreSQL applies its access controls on large object. Thus, (3) is also indivisible from (1a). > If I had to do this, I would first write a patch for #1: A patch that > additionally executes existing privilege checks against an SELinux > policy. Existing privilege checks are a well-defined set: they mostly > happen through pg_xxx_aclcheck() functions. Hook your checks in there. I had a concern about pg_xxx_aclcheck(). When we accesses a table via a view, pg_xxx_aclcheck() checkes view's ACLs and table's ACLs are left for unchecked, even if it accesses same object. SELinux always makes its decision based on the attribute of object itself. In other word, the route to access taget object does not make any affect on its decision making. For example, a filesystem object (inode) can have multiple pathname using hard link on operating system. SELinux always makes ite decision based on inode's attribute (called as security context), independent from its pathname. Do you know a previous hot discussion between SELinux and AppArmor in Linux developers? Please note that I don't deny the benefit of view. It has various effective usages so we cannot ignore them. However, it mismatched with the security design, so I needed to put security hooks on different strategic points. > Row-level security should also be developed as a completely separate > feature, without any SELinux tie-in initially. This is not only > important to make review and verification simpler, but also because we > really need a wider test community for such a tricky feature. And the > set of SELinux users is quite limited, and the intersection with > PostgreSQL developers is almost empty. This was already previously > discussed at length. It would be possible. However, note that we should not implement the first step ignoring upcoming facility. Eventually I would implement "SE-PostgreSQL 1st edition" with paying attention for the upcoming row level security. > No in-code frameworks > --------------------- > > Write your code so that it is fully integrated with the existing code. > Or write a plugin interface and then write a plugin. But don't invent a > "framework" because you are afraid to integrate the new code with the > old code. At least, PGACE is not a purpose for me. If fully integrated archtencture is absolutely necessary, I'll scrap PGACE framework. Trusted Solaris folks may concern about it, but I cannot give them higher priority than SE-PostgreSQL getting merged. (Might sound insensitive, I don't have leeway now.) The very earlier SE-PostgreSQL put its code directly, because I didn't pay mention for T-Sol folks at that time. However... > As mentioned above, permission checks are done through pg_xxx_aclcheck() > functions, which is enough of a framework. I wouldn't want yet another > framework that does more permission checking at other times and places. > If the existing interfaces are not adequate for your purpose, by all > means, extend, refactor, or rewrite them. But don't just avoid it > because you don't want to interfere with the existing code. So scrap > the whole "PGACE" thing. However, it is a different issue whether pg_xxx_aclcheck() can work as an alternative of PGACE, or not. SE-PostgreSQL want to make its decision more than pg_xxx_aclcheck(), so, in finally, we need to put a code to check on different strategic points where currently pgaceXXXX() are deployed. > If you need to refactor the aclcheck interfaces, that's another separate > patch, which can easily be reviewed and verified, simplifying the > following patches even further. I believe a basic premise is vanilla PostgreSQL keeps correct behavior based on SQL standard. An enhanced security should apply its access controls orthogonally. So, I cannot believe refactoring pg_xxx_aclcheck() is not acceptable. If vanilla PostgreSQL become to check ACLs on tables, independent from views, do you think it is acceptable? > These things are not going to get done within two weeks. No wonder! However, we have to make clear whether the PGACE architecture is incorrect, or not, at first. I think the name of PGACE is not important, but it is necessary to make SELinux's decision in similar strategic point in finally. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
> However, we have to make clear whether the PGACE architecture > is incorrect, or not, at first. > I think the name of PGACE is not important, but it is necessary > to make SELinux's decision in similar strategic point in finally. I've been thinking about this issue as well. I think a framework of some kind could be acceptable, but only if we're convinced that the framework is really general enough to handle all of the cases that we care about. For example, can we back-port our existing DAC infrastructure to use PGACE? Is it really true that SE-PostgreSQL interacts through the rest of the system ONLY through PGACE? If the answers to both questions are YES, then it's a framework. If the answer to either question is NO, then it's just a bunch of places where you needed to stick code to make SE-PostgreSQL work. I haven't read the code, but from reading the docs, I have a feeling that right now the answer to both questions are NO, which means it doesn't really have a lot of value. One example of this is the pg_security system catalog. The catalog representation you're proposing is probably just fine for associating OIDs to SELinux security labels. But trying to present it as a general thing that some other security implementation could reuse just doesn't seem realistic. Who is to say that the next person who writes an enhanced security feature will want to use text as the representation for their security domains? It could just as easily be two integers, or an array of booleans. This is after all a database product, so the chances that someone would want to do something with structured data seem non-negligible. In the end, you're going to have to be the one who makes the decision on which way to go. In some ways, I think that a plugin architecture would be better for everyone: we worry about the things on our side of the abstraction boundary, and you worry about the things on your side.Potentially you or someone else can release enhancedsecurity plugins without needing any changes to core, and potentially on a different release cycle. On the other hand, a plugin architecture is probably going to be a lot of work. It seems that to install the plugin you would need to make system catalog changes as well as stuff additional system attributes into every table, which are relatively invasive changes. And you'll need to convince everyone that it's really a plug-in architecture and not just a special case for SE-PostgreSQL, so you'll need to prove that there are multiple viable clients, perhaps by backporting our existing DAC. ...Robert
* KaiGai Kohei (kaigai@kaigai.gr.jp) wrote:
> So, I cannot believe refactoring pg_xxx_aclcheck() is not acceptable.
> If vanilla PostgreSQL become to check ACLs on tables, independent
> from views, do you think it is acceptable?
Well, just to be clear, ACLs are checked on tables under views, but
they're checked using the privileges of the view owner rather than
the privileges of the current user. I've run into that empirically
because I've gotten 'permission denied' errors when using a view that
I've clearly got full rights on but was owned by someone else (who
didn't have rights on the table underneath).
That being said, I'd think that if we do need different semantics from
that for SE-PostgreSQL, we could implement it using a GUC or similar to
keep the current behavior as well allow the SE-PostgreSQL behavior.
> However, we have to make clear whether the PGACE architecture
> is incorrect, or not, at first.
It really bothers me that it seems like these kinds of reviews of the
larger patches don't happen until it's time to decide about the next
release. Perhaps these issues were all brought up seperately in prior
threads, or they weren't articulated as requirements or show-stoppers,
and if so then I apologize for not following those more closely.
If the approach Peter outlined is what core wants to see and is willing
to go along with to get SE-PostgreSQL included then let's please decide
that now and agree that unless some serious problem comes up we'll stick
to it and not require the whole thing be rewritten again later.
I'm not sure about KaiGai's feelings on this, but it strikes me that
adding SELinux support for the existing levels of access control in PG
might be straight-forward and small enough to include for 8.4 and would
show some commitment to this approach of "do it for PG, add SELinux
checks for it". Alternatively, maybe a progression-towards-SE-PostgreSQL
wiki/webpage that outlines the plan, current work, what's been
committed, etc, that everyone reviews and agrees to?
As a side-note, I've gotten some extremely positive feedback about
SE-PostgreSQL from folks in my organization who run systems where it
would be used. I'm going to be having a more detailed discussion later
today.
Thanks,
Stephen
> That being said, I'd think that if we do need different semantics from > that for SE-PostgreSQL, we could implement it using a GUC or similar to > keep the current behavior as well allow the SE-PostgreSQL behavior. I don't think a GUC is what you need because you need both behaviors simultaneously, one for MAC and the other for DAC. But I am sure there must be some way to code around the problem. ...Robert
Robert Haas wrote: > I haven't read the code, but from reading the docs, I have a feeling > that right now the answer to both questions are NO, which means it > doesn't really have a lot of value. One example of this is the > pg_security system catalog. The catalog representation you're > proposing is probably just fine for associating OIDs to SELinux > security labels. But trying to present it as a general thing that > some other security implementation could reuse just doesn't seem > realistic. Who is to say that the next person who writes an enhanced > security feature will want to use text as the representation for their > security domains? It could just as easily be two integers, or an > array of booleans. This is after all a database product, so the > chances that someone would want to do something with structured data > seem non-negligible. Text represented security attribute is the most common format for any other security mechanism also. (For example, T-SOL also have its text representation.) In addition, TEXT is the most flexible format. If any other feature want to handle it as an array, it can be stored as a text representation. > In the end, you're going to have to be the one who makes the decision > on which way to go. In some ways, I think that a plugin architecture > would be better for everyone: we worry about the things on our side of > the abstraction boundary, and you worry about the things on your side. > Potentially you or someone else can release enhanced security plugins > without needing any changes to core, and potentially on a different > release cycle. On the other hand, a plugin architecture is probably > going to be a lot of work. It seems that to install the plugin you > would need to make system catalog changes as well as stuff additional > system attributes into every table, which are relatively invasive > changes. And you'll need to convince everyone that it's really a > plug-in architecture and not just a special case for SE-PostgreSQL, so > you'll need to prove that there are multiple viable clients, perhaps > by backporting our existing DAC. Please make clear the meaning of terms. The 'plugin' means a loadable module which provides its own security policy, doesn't it? There is fundamental difference between built-in and plug-in. The most important factor is where the hooks are deployed, and what facility enables to manage security attribute. Even if I implement SE-PostgreSQL as a loadable module, core PostgreSQL has to provide proper hooks in strategic points and facilities to manage security attribute (pg_security system catalog and security_label system column). If you require to implement it without these facilities, I think it is impossible and prefer scraping PGACE and integrate SE- code into core. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
> Text represented security attribute is the most common format > for any other security mechanism also. > (For example, T-SOL also have its text representation.) > In addition, TEXT is the most flexible format. If any other > feature want to handle it as an array, it can be stored as > a text representation. Right, but that sort of misses the point of using a DATABASE. I could store everything as text if I wanted to and skip using a database altogether, but I don't want to. That's why I use a database. > Please make clear the meaning of terms. > The 'plugin' means a loadable module which provides its own security > policy, doesn't it? That is what I meant, yes. > Even if I implement SE-PostgreSQL as a loadable module, core > PostgreSQL has to provide proper hooks in strategic points and > facilities to manage security attribute (pg_security system catalog > and security_label system column). > If you require to implement it without these facilities, I think > it is impossible and prefer scraping PGACE and integrate SE- code > into core. I am not in a position to require anything since I am not a committer, but I would think that you would need to convince people that the facilities which your plugin requires were pretty much the same as the facilities that any other future plugin might require - that the plugin framework was client-agnostic. ...Robert
Stephen Frost wrote: > * KaiGai Kohei (kaigai@kaigai.gr.jp) wrote: >> So, I cannot believe refactoring pg_xxx_aclcheck() is not acceptable. >> If vanilla PostgreSQL become to check ACLs on tables, independent >> from views, do you think it is acceptable? > > Well, just to be clear, ACLs are checked on tables under views, but > they're checked using the privileges of the view owner rather than > the privileges of the current user. I've run into that empirically > because I've gotten 'permission denied' errors when using a view that > I've clearly got full rights on but was owned by someone else (who > didn't have rights on the table underneath). > > That being said, I'd think that if we do need different semantics from > that for SE-PostgreSQL, we could implement it using a GUC or similar to > keep the current behavior as well allow the SE-PostgreSQL behavior. I think it is not reasonable. If there are different philosophies, "one for one" seems to me straight forward approach, for security especially. >> However, we have to make clear whether the PGACE architecture >> is incorrect, or not, at first. > > It really bothers me that it seems like these kinds of reviews of the > larger patches don't happen until it's time to decide about the next > release. Perhaps these issues were all brought up seperately in prior > threads, or they weren't articulated as requirements or show-stoppers, > and if so then I apologize for not following those more closely. > > If the approach Peter outlined is what core wants to see and is willing > to go along with to get SE-PostgreSQL included then let's please decide > that now and agree that unless some serious problem comes up we'll stick > to it and not require the whole thing be rewritten again later. As I noted, PGACE is not my goal. I don't tremble to integrate SELinux related code into the core. > I'm not sure about KaiGai's feelings on this, but it strikes me that > adding SELinux support for the existing levels of access control in PG > might be straight-forward and small enough to include for 8.4 and would > show some commitment to this approach of "do it for PG, add SELinux > checks for it". Alternatively, maybe a progression-towards-SE-PostgreSQL > wiki/webpage that outlines the plan, current work, what's been > committed, etc, that everyone reviews and agrees to? Are you saying enlargement step-by-step, aren't you? At least, it is far preferable to a death punishment. I would like to here Joshua's opinion also. > adding SELinux support for the existing levels of access control in PG is - table/column level access controls - permission checks on database login - permission checks on function invocation- they need a facility to manage security label - I want permission checks on loading a library, though existing PG checks superuser() only. and - removing PGACE, integrate SEPG code into core - permission checks on largeobjects is postponed - row level security is postponed (NOT REJECTED!)- so, writable system column is also postponed If summary is necessary, I'll post it tommorow JST. Because it is not a zero-based implementation, so I believe it can be minimized within acceptable timescale. > As a side-note, I've gotten some extremely positive feedback about > SE-PostgreSQL from folks in my organization who run systems where it > would be used. I'm going to be having a more detailed discussion later > today. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
KaiGai Kohei wrote: >> I'm not sure about KaiGai's feelings on this, but it strikes me that >> adding SELinux support for the existing levels of access control in PG >> might be straight-forward and small enough to include for 8.4 and would >> show some commitment to this approach of "do it for PG, add SELinux >> checks for it". Alternatively, maybe a progression-towards-SE-PostgreSQL >> wiki/webpage that outlines the plan, current work, what's been >> committed, etc, that everyone reviews and agrees to? > > Are you saying enlargement step-by-step, aren't you? > At least, it is far preferable to a death punishment. > > I would like to here Joshua's opinion also. s/here/hear/g Sorry, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
>> Even if I implement SE-PostgreSQL as a loadable module, core >> PostgreSQL has to provide proper hooks in strategic points and >> facilities to manage security attribute (pg_security system catalog >> and security_label system column). >> If you require to implement it without these facilities, I think >> it is impossible and prefer scraping PGACE and integrate SE- code >> into core. > > I am not in a position to require anything since I am not a committer, > but I would think that you would need to convince people that the > facilities which your plugin requires were pretty much the same as the > facilities that any other future plugin might require - that the > plugin framework was client-agnostic. We (as a security folks) know any MAC facility have similar architecture called as reference monitor, so I believe it is quite possible to implement them as same basis. But it is a hard request to take an evidence immediately. IMO, the framework is purely implementation matter, so it is not late when the second one appeared. As I noted to another message, I can accept to integrate limited functional SE-PostgreSQL without any PGACE. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
KaiGai Kohei wrote: > Stephen Frost wrote: >> * KaiGai Kohei (kaigai@kaigai.gr.jp) wrote: >>> So, I cannot believe refactoring pg_xxx_aclcheck() is not acceptable. >>> If vanilla PostgreSQL become to check ACLs on tables, independent >>> from views, do you think it is acceptable? >> Well, just to be clear, ACLs are checked on tables under views, but >> they're checked using the privileges of the view owner rather than >> the privileges of the current user. I've run into that empirically >> because I've gotten 'permission denied' errors when using a view that >> I've clearly got full rights on but was owned by someone else (who >> didn't have rights on the table underneath). >> >> That being said, I'd think that if we do need different semantics from >> that for SE-PostgreSQL, we could implement it using a GUC or similar to >> keep the current behavior as well allow the SE-PostgreSQL behavior. > > I think it is not reasonable. > If there are different philosophies, "one for one" seems to me > straight forward approach, for security especially. > When we talk about mandatory access control we are generally very clear that for it to be mandatory and for the policy to be analyzable you must identify objects unambiguously. For SE-Postgres this is an absolute necessity. At the same time, if view-based ACL's are there, and people want them and use them then that doesn't affect us (the people that want to use sepostgres). They must be orthogonal and sepostgres must have a way of unambiguously labeling objects. >>> However, we have to make clear whether the PGACE architecture >>> is incorrect, or not, at first. >> It really bothers me that it seems like these kinds of reviews of the >> larger patches don't happen until it's time to decide about the next >> release. Perhaps these issues were all brought up seperately in prior >> threads, or they weren't articulated as requirements or show-stoppers, >> and if so then I apologize for not following those more closely. >> As a new-comer to this discussion (and indeed this list/community) I find it baffling that a patchset that has been around this long is just now having basic architectural questions asked about it. >> If the approach Peter outlined is what core wants to see and is willing >> to go along with to get SE-PostgreSQL included then let's please decide >> that now and agree that unless some serious problem comes up we'll stick >> to it and not require the whole thing be rewritten again later. > > As I noted, PGACE is not my goal. > I don't tremble to integrate SELinux related code into the core. > Flask (from which SELinux is derived) has the basic architecture of separating enforcement from policy. This generally means that a security server (or backend) knows what the policy is and how to calculate access vectors and it passes those answers to the enforcement points. Flask itself is a framework. In Linux and Xorg a more general framework was built that interfaces to flask, because the flask security server wasn't seen as something that could implement any kind of security policy (whether that is correct or not is a point of contention, as the selinux security server already implements a kind of RBAC, type enforcement and multilevel security). PCACE is, architecturally, similar to LSM in the linux kernel and XACE in in Xorg but is not an absolute necessity for SELinux integration. >> I'm not sure about KaiGai's feelings on this, but it strikes me that >> adding SELinux support for the existing levels of access control in PG >> might be straight-forward and small enough to include for 8.4 and would >> show some commitment to this approach of "do it for PG, add SELinux >> checks for it". Alternatively, maybe a progression-towards-SE-PostgreSQL >> wiki/webpage that outlines the plan, current work, what's been >> committed, etc, that everyone reviews and agrees to? > If you look at the patches you'll see that the bulk of them are adding the SELinux infrastructure. An access vector cache, the label translation functions, the libselinux interface and so on. Over 2/3's of the patch is in src/backend/security. I'm not sure how much it would cut to remove row level access controls, but I do have some points here. To me, row level access controls are the most important part, this is the feature that lets us put secret and top secret data in the same table and use the clients selinux context to decide what they can see, transparently and without modification to the application. Without it you have to use seperate postgres instances, modify the application to select from different sets of tables depending on their label and so on. That said, SELinux didn't start out as fine grained as it is today. A fair number of object classes were present back in '99 when it started but more have been added over time, the policy infrastructure allows that to happen, in fact the policy allows object class/permissions to be added, removed and modified if the access controls were inadequate or unnecessary. > Are you saying enlargement step-by-step, aren't you? > At least, it is far preferable to a death punishment. > > I would like to here Joshua's opinion also. > namespace collision... >> adding SELinux support for the existing levels of access control in PG > > is > > - table/column level access controls > - permission checks on database login > - permission checks on function invocation > - they need a facility to manage security label > - I want permission checks on loading a library, > though existing PG checks superuser() only. > > and > - removing PGACE, integrate SEPG code into core > - permission checks on largeobjects is postponed > - row level security is postponed (NOT REJECTED!) > - so, writable system column is also postponed > > If summary is necessary, I'll post it tommorow JST. > > Because it is not a zero-based implementation, so I believe it can > be minimized within acceptable timescale. > >> As a side-note, I've gotten some extremely positive feedback about >> SE-PostgreSQL from folks in my organization who run systems where it >> would be used. I'm going to be having a more detailed discussion later >> today. > It's good to hear that, I hope you'll ask them how important row level access controls are.
Joshua Brindle <method@manicmethod.com> writes:
> I'm not sure how much it would cut to remove row level access
> controls, but I do have some points here. To me, row level access
> controls are the most important part, this is the feature that lets us
> put secret and top secret data in the same table and use the clients
> selinux context to decide what they can see,
For me, the row-level access controls are really the sticking point.
There is absolutely nothing you can say that will convince me that they
don't break SQL in fundamental ways, and I also don't believe that it's
going to be possible to implement them without a constant stream of bugs
of omission and commission. (Those two points are not unrelated.)
Now that you've admitted that you aren't trying to accomplish the
equivalent sort of data hiding within the filesystem, the argument that
you have to have them seems fairly weak, certainly not strong enough to
justify the costs. We have already touched on some ways that you can
accomplish similar goals with just table- and column-level access
permissions, which are well understood and don't violate anyone's
expectations. There's probably a need to twiddle our permissions rules
for inheritance/partitioning cases, but that was already on the table
anyway for other reasons.
I could be persuaded to get behind a patch that does Peter's step #1
(ie, use SELinux permissions as an additional filter for existing SQL
permission checks). I don't believe I will ever think that row-level
checks are a good idea; as long as those are in the patch I will vote
against applying it.
regards, tom lane
Tom Lane wrote: > Joshua Brindle <method@manicmethod.com> writes: >> I'm not sure how much it would cut to remove row level access >> controls, but I do have some points here. To me, row level access >> controls are the most important part, this is the feature that lets us >> put secret and top secret data in the same table and use the clients >> selinux context to decide what they can see, > > For me, the row-level access controls are really the sticking point. > There is absolutely nothing you can say that will convince me that they > don't break SQL in fundamental ways, and I also don't believe that it's > going to be possible to implement them without a constant stream of bugs > of omission and commission. (Those two points are not unrelated.) > This isn't going to be something that the vast majority of your users use. The people that need them understand the ramifications of row filtering and the absence of inaccessible rows won't be surprising. > Now that you've admitted that you aren't trying to accomplish the > equivalent sort of data hiding within the filesystem, the argument that We apply access controls on reading and writing files, this is equivalent (in my mind) to applying access controls on tuples, as they are the structured object holding data (just like files). > you have to have them seems fairly weak, certainly not strong enough to > justify the costs. We have already touched on some ways that you can The costs are nil for people who don't want this feature. > accomplish similar goals with just table- and column-level access > permissions, which are well understood and don't violate anyone's I've already pointed out how table and column level access control don't provide enough. Holding data of multiple classifications in a single table is something of great concern. Unmodified applications need to be able to query out of a table and get the data they are cleared to see. Column level access control doesn't help because those same applications, of course, will need access to the data in those columns. Splitting data into different tables isn't an option in many cases because applications aren't always configurable wrt to the database or tables they use. Further, clients of multiple clearances should be able to use the same application, and we can not trust the application to make the decision as to which database or table is correct. And even further, maintaining multiple sets of databases or tables that hold the same kinds of information (and therefore have the same schema) is a maintenance, backup and restoration issue. > expectations. There's probably a need to twiddle our permissions rules > for inheritance/partitioning cases, but that was already on the table > anyway for other reasons. > partitions don't help because, once again, the application would be making the determination about which partition to query. For mandatory access control that we need in the kind of environments to work the application doesn't get to make security relevant decisions, the database holds the data and needs to say who can access it. Further, partitioning isn't fine grained. I can't say user X can read secret rows and read/write top secret rows and get that data out in a transparent way (the applications would have to be aware of the partitions). Relabeling of data also looks like a challenge with partitions (if I correctly understand how they work). > I could be persuaded to get behind a patch that does Peter's step #1 > (ie, use SELinux permissions as an additional filter for existing SQL > permission checks). I don't believe I will ever think that row-level > checks are a good idea; as long as those are in the patch I will vote > against applying it. > We've already used postgresql in sensitive environments and had to make compromises because of the lack of fine grained access control. We've been telling customers for years that we hope postgresql will have said access controls in the future and that it will help them solve many of the problems they have. We've been enthusiastic about the work KaiGai has been doing with respect to the environments we operate in and it would be a shame for all that to be discarded.
Joshua Brindle <method@manicmethod.com> writes: > partitions don't help because, once again, the application would be making the > determination about which partition to query. Not necessarily since the database can be programmed to automatically put the records into the right partition. Right now it's a pain but we're definitely headed in that direction. > Further, partitioning isn't fine grained. I can't say user X can read secret > rows and read/write top secret rows and get that data out in a transparent way > (the applications would have to be aware of the partitions). Relabeling of data > also looks like a challenge with partitions (if I correctly understand how they > work). I think the "transparent" is the source of the problem. The application should issue a query for the data it wants. It shouldn't "transparently" get magic extra clauses attached to the query. That's where the SQL semantics are being violated. Row-level security isn't inherently a problem. It's just that the security is affecting the data returned that's causing a problem. I don't think partitioning is really the same thing as row-level security. But I wonder if some of the same infrastructure could be used for both -- once we have it. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Get trained by Bruce Momjian - ask me about EnterpriseDB'sPostgreSQL training!
Gregory Stark <stark@enterprisedb.com> writes:
> I don't think partitioning is really the same thing as row-level
> security.
Of course not, but it seems to me that it can be used to accomplish most
of the same practical use-cases. The main gripe about doing it via
partitioning is that the user's nose gets rubbed in the fact that there
can't be an enormous number of different security classifications in the
same table (since he has to explicitly make a partition for each one).
But the proposed implementation of row-level security would poop out
pretty darn quick for such a case, too, and frankly I'm not seeing an
application that would demand it.
regards, tom lane
On Wed, Jan 28, 2009 at 01:49:21PM -0500, Joshua Brindle wrote: > use. The people that need them understand the ramifications of row > filtering and the absence of inaccessible rows won't be surprising. I wish there was even a little bit of evidence that users of a security system may be relied upon to understand its implications and effects. In my experience, however, they often don't. >> you have to have them seems fairly weak, certainly not strong enough to >> justify the costs. We have already touched on some ways that you can > > The costs are nil for people who don't want this feature. That's also false, because developers who don't care about the feature have to continue to maintain it as part of the system. If maintenance were free, I suspect nobody would be objecting to the feature. But this feature will in fact constrain future development and will impose maintenance requirements on the programmers of the system. Those maintenance requirements in turn amount to a cost that every user has to pay, because time spent addressing issues that result from this feature (or accommodating it in new development) is time that is not spent on other problems users might face. If I imagined I were project manager of the PostgreSQL project (a preposterous supposition, let me be clear), then I'd be very worried that this feature, which is apparently poorly understood by at least one big contributor to the project, would amount to a significant drag on future development work. In that case, I'd have to ask why having this feature as part of the main line of PostgreSQL is a good trade-off. Happily, I'm not someone who has to make that determination, so I can't say whether it _is_ a good trade-off. But I think that's what the resistance to the feature is all about, so you'll need to make the case that the trade-off is a good one. A -- Andrew Sullivan ajs@crankycanuck.ca
Andrew Sullivan <ajs@crankycanuck.ca> writes:
> On Wed, Jan 28, 2009 at 01:49:21PM -0500, Joshua Brindle wrote:
>> The costs are nil for people who don't want this feature.
> That's also false, because developers who don't care about the feature
> have to continue to maintain it as part of the system. If maintenance
> were free, I suspect nobody would be objecting to the feature. But
> this feature will in fact constrain future development and will impose
> maintenance requirements on the programmers of the system.
Right. The major implementation problem I have with row-level security
is that it will require sticking its hands into every part of the
backend; at least if you want it to be actually *secure* with no holes,
and if not I guess I'm failing to grasp the point. Every future patch
will have to be vetted to ensure that it's not accidentally breaking
that security. This stems directly from the fact that you're trying to
impose behavior that's fundamentally at odds with SQL, and therefore
there isn't any well-defined choke point at which you could apply the
checks and be done with it. The system simply isn't modularized that
way. (Of course we could throw it all away and start over...)
BTW, in regard to the upthread question about how much of the patch
could be discarded if we removed row-level security: having now taken
another look through it, I'd put the fraction at well north of 90%.
(That's exclusive of the security policy file, which I don't understand
at all and so can't tell how much might be specific to row security.)
What's worse, the current patch footprint is conservative because the
placement of hooks is simply wrong. You can't usefully apply checks in
simple_heap_insert, for example, since it has no idea who called it or
why. It's got to be done at a higher level and therefore in a lot more
places. And I don't see any attempt at all to restrict system-driven
fetches, yet surely there has to be some control over that (otherwise
why are we worrying about system-driven updates?)
regards, tom lane
Sorry for top-posting and avoiding to paste online doc URL. Joshua, you sound like you're missing an important point. Sorry for misunderstanding if that's not true. Partitioning is supported in a way that a query does not need to know about it, be it a SELECT, INSERT, UPDATE or DELETE. And 8.4 onwards, some more. What Tom is talking about is adding support for discarding partitions based on GRANTed rights rather than WHERE clauses. No application rewrite. Columns obfuscating remains to be cared of: what about allowing views as partitions? HTH, regards, -- dim Le 28 janv. 09 à 19:49, Joshua Brindle <method@manicmethod.com> a écrit : > Tom Lane wrote: >> Joshua Brindle <method@manicmethod.com> writes: >>> I'm not sure how much it would cut to remove row level access >>> controls, but I do have some points here. To me, row level access >>> controls are the most important part, this is the feature that >>> lets us >>> put secret and top secret data in the same table and use the clients >>> selinux context to decide what they can see, >> For me, the row-level access controls are really the sticking point. >> There is absolutely nothing you can say that will convince me that >> they >> don't break SQL in fundamental ways, and I also don't believe that >> it's >> going to be possible to implement them without a constant stream of >> bugs >> of omission and commission. (Those two points are not unrelated.) >> > > This isn't going to be something that the vast majority of your > users use. The people that need them understand the ramifications of > row filtering and the absence of inaccessible rows won't be > surprising. > > >> Now that you've admitted that you aren't trying to accomplish the >> equivalent sort of data hiding within the filesystem, the argument >> that > > > We apply access controls on reading and writing files, this is > equivalent (in my mind) to applying access controls on tuples, as > they are the structured object holding data (just like files). > >> you have to have them seems fairly weak, certainly not strong >> enough to >> justify the costs. We have already touched on some ways that you can > > The costs are nil for people who don't want this feature. > >> accomplish similar goals with just table- and column-level access >> permissions, which are well understood and don't violate anyone's > > I've already pointed out how table and column level access control > don't provide enough. Holding data of multiple classifications in a > single table is something of great concern. Unmodified applications > need to be able to query out of a table and get the data they are > cleared to see. Column level access control doesn't help because > those same applications, of course, will need access to the data in > those columns. > > Splitting data into different tables isn't an option in many cases > because applications aren't always configurable wrt to the database > or tables they use. Further, clients of multiple clearances should > be able to use the same application, and we can not trust the > application to make the decision as to which database or table is > correct. > > And even further, maintaining multiple sets of databases or tables > that hold the same kinds of information (and therefore have the same > schema) is a maintenance, backup and restoration issue. > >> expectations. There's probably a need to twiddle our permissions >> rules >> for inheritance/partitioning cases, but that was already on the table >> anyway for other reasons. > > partitions don't help because, once again, the application would be > making the determination about which partition to query. For > mandatory access control that we need in the kind of environments to > work the application doesn't get to make security relevant > decisions, the database holds the data and needs to say who can > access it. > > Further, partitioning isn't fine grained. I can't say user X can > read secret rows and read/write top secret rows and get that data > out in a transparent way (the applications would have to be aware of > the partitions). Relabeling of data also looks like a challenge with > partitions (if I correctly understand how they work). > >> I could be persuaded to get behind a patch that does Peter's step #1 >> (ie, use SELinux permissions as an additional filter for existing SQL >> permission checks). I don't believe I will ever think that row-level >> checks are a good idea; as long as those are in the patch I will vote >> against applying it. > > We've already used postgresql in sensitive environments and had to > make compromises because of the lack of fine grained access control. > We've been telling customers for years that we hope postgresql will > have said access controls in the future and that it will help them > solve many of the problems they have. > > We've been enthusiastic about the work KaiGai has been doing with > respect to the environments we operate in and it would be a shame > for all that to be discarded. > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jan 28, 2009 at 3:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Andrew Sullivan <ajs@crankycanuck.ca> writes: >> On Wed, Jan 28, 2009 at 01:49:21PM -0500, Joshua Brindle wrote: >>> The costs are nil for people who don't want this feature. > >> That's also false, because developers who don't care about the feature >> have to continue to maintain it as part of the system. If maintenance >> were free, I suspect nobody would be objecting to the feature. But >> this feature will in fact constrain future development and will impose >> maintenance requirements on the programmers of the system. > > Right. The major implementation problem I have with row-level security > is that it will require sticking its hands into every part of the > backend; at least if you want it to be actually *secure* with no holes, > and if not I guess I'm failing to grasp the point. Every future patch > will have to be vetted to ensure that it's not accidentally breaking > that security. This stems directly from the fact that you're trying to > impose behavior that's fundamentally at odds with SQL, and therefore > there isn't any well-defined choke point at which you could apply the > checks and be done with it. The system simply isn't modularized that > way. (Of course we could throw it all away and start over...) > > BTW, in regard to the upthread question about how much of the patch > could be discarded if we removed row-level security: having now taken > another look through it, I'd put the fraction at well north of 90%. > (That's exclusive of the security policy file, which I don't understand > at all and so can't tell how much might be specific to row security.) > What's worse, the current patch footprint is conservative because the > placement of hooks is simply wrong. You can't usefully apply checks in > simple_heap_insert, for example, since it has no idea who called it or > why. It's got to be done at a higher level and therefore in a lot more > places. And I don't see any attempt at all to restrict system-driven > fetches, yet surely there has to be some control over that (otherwise > why are we worrying about system-driven updates?) I'm not clear that I understand why it would be necessary for row-level security to touch every part of the code. If the current implementation requires that, then maybe that's a defect in the implementation rather than an inherent problem with row-level security. It seems to me that the crucial decision is "Where are we trying to erect the security wall?". If we're trying to put it between the client and the postgres backend, then maybe the right thing to do is apply some sort of processing step to each query that is submitted, essentially rewriting "SELECT * FROM a, b" as "SELECT * FROM a, b WHERE you_can_see(a.securityid) AND you_can_see(b.securityid)". Maybe you (Tom) still won't like this because it breaks SQL semantics, but it seems like it would at least centralize the code. Unless I'm totally off base here? ...Robert
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> For me, the row-level access controls are really the sticking point.
> There is absolutely nothing you can say that will convince me that they
> don't break SQL in fundamental ways, and I also don't believe that it's
> going to be possible to implement them without a constant stream of bugs
> of omission and commission. (Those two points are not unrelated.)
And, just to go full circle, row-level access controls are exactly what
the other enterprise RDBMSs have and is what is used in these security
circles today. One of the major issues, as I understand it, is to be
able to use stock applications with multiple security levels where the
application doesn't know (or care about) the security level. Doing that
through views and partitions and triggers and whatnot for each and every
application that is run on these systems will be a big hurdle to those
users, if it ends up being workable at all.
Thanks,
Stephen
Joshua Brindle wrote: > Tom Lane wrote: >> Joshua Brindle <method@manicmethod.com> writes: >>> I'm not sure how much it would cut to remove row level access >>> controls, but I do have some points here. To me, row level access >>> controls are the most important part, this is the feature that lets us >>> put secret and top secret data in the same table and use the clients >>> selinux context to decide what they can see, Help me understand this. It seems to me exactly as easy/hard to make sure that the secret and top-secret rows are put into their appropriate partitions, as it is to make sure that the secret and top-secret rows are tagged with the right row-level-access-info. If the idea is that the top-secret-row-inserter magically forces the row-level tag "top-secret" it seems just as easy if the top-secret row-inserter only has write permission to the "top-secret" partition and not the others. If the idea is that the less-secret-reader can't read rows tagged "top-secret" it seems just as easy if the less-secret-reader has no read access on the top-secret partition. At first glance, partition level seems quite a bit easier to manage in all the cases I can think of immediately. > partitions don't help because, once again, the application would be > making the determination about which partition to query. For mandatory I think that's not true. I would hope that the application queries the master table, and the SEPostgres ACLs prevent any data coming from the inappropriate partitions. > access control that we need in the kind of environments to work the > application doesn't get to make security relevant decisions, the > database holds the data and needs to say who can access it. > > Further, partitioning isn't fine grained. I can't say user X can read > secret rows and read/write top secret rows and get that data out in a Why not? It seems one can define the user with read access on the partition with secret rows and read/write on top-secret rows. Queries done on the master partition should get the data out in a transparent way. > transparent way (the applications would have to be aware of the > partitions). Relabeling of data also looks like a challenge with > partitions (if I correctly understand how they work). ISTM we need to have a discussion of how partitioned tables work - and what the postgres roadmap is. If they can't yet, I think they should. >> I could be persuaded to get behind a patch that does Peter's step #1 >> (ie, use SELinux permissions as an additional filter for existing SQL >> permission checks). I don't believe I will ever think that row-level >> checks are a good idea; as long as those are in the patch I will vote >> against applying it. >> > > We've already used postgresql in sensitive environments and had to make > compromises because of the lack of fine grained access control. We've > been telling customers for years that we hope postgresql will have said > access controls in the future and that it will help them solve many of > the problems they have. > > We've been enthusiastic about the work KaiGai has been doing with > respect to the environments we operate in and it would be a shame for > all that to be discarded. AFACT there's nowhere near a consensus that it should be discarded (or accepted); but rather that if the project can be split into two phases parts of it could go in sooner. There seems to be much less debate about the column/table/partition level MAC parts. Once those get in, I think the next valuable discussion would be whether the fine-grained access control is best achieved through improving the partition system or by adding the row-level acls as proposed.
Stephen Frost wrote: > And, just to go full circle, row-level access controls are exactly what > the other enterprise RDBMSs have and is what is used in these security > circles today. One of the major issues, as I understand it, is to be > able to use stock applications with multiple security levels where the > application doesn't know (or care about) the security level. Doing that > through views and partitions and triggers and whatnot for each and every > application that is run on these systems will be a big hurdle to those > users, if it ends up being workable at all. That seems to me to be a shortcoming of the partition system and a good TODO for the future partitioning improvements. Why shouldn't be just as easy to make sure a row ends up in the right partition as opposed to making sure it's tagged with right row-level ACLs.
Ron Mayer wrote: > Stephen Frost wrote: >> And, just to go full circle, row-level access controls are exactly what >> the other enterprise RDBMSs have and is what is used in these security >> circles today. One of the major issues, as I understand it, is to be >> able to use stock applications with multiple security levels where the >> application doesn't know (or care about) the security level. Doing that >> through views and partitions and triggers and whatnot for each and every >> application that is run on these systems will be a big hurdle to those >> users, if it ends up being workable at all. > > That seems to me to be a shortcoming of the partition system and a good > TODO for the future partitioning improvements. > > Why shouldn't be just as easy to make sure a row ends up in the > right partition as opposed to making sure it's tagged with right > row-level ACLs. > In reality it isn't, unless postgres won't mind thousands of partitions being made. In the military/gov implementations BLP lets you have hierarchical levels and non-hierarchical categories. Since I linked to an article about it upthread I assumed everyone participating was familiar but perhaps not. Typically there are 3 levels, unclass, secret, top secret. In addition to those 3 levels there may be a few, hundreds or even thousands of categories. Those categories apply to each of the levels so if you are using 1000 categories you have 3*1000 possible BLP labels. On top of that SELinux has users, roles and types, which are all also multiplied. There is a reason we don't do things like have 3 filesystems, one for unclass, one for secret and one for top secret. Partitions/views may be a good way to implement row-level access control when there are a few different options but they won't necessarily be known ahead of time. We don't want to encode policy logic in to the database itself, that is why we have a separate security server that handles the policy. The system should scale to any number of contexts that are available on the system. Relabeling with partitions (that is, moving things from one partition to another) seems to be handled with triggers, which is inconvenient to the application developers but also devastating to the security system (unless you can map triggers back to who did the operation that caused them), and managing partitions on dozens or hundreds of tables and moving data between them in those tables seems at the very least challenging. I also have questions concerning partitions. Perhaps someone wants to use partitions for a functional purpose (eg., to split log entries by month/year). What would be the affect of concurrently using partitions to split security contexts out? Nonetheless, this conversation seems moot now that Tom has walled off and won't even discuss row-level access controls. Cheers.
Tom Lane <tgl@sss.pgh.pa.us> writes: > I don't believe I will ever think that row-level checks are a good idea; as > long as those are in the patch I will vote against applying it. I think we're conflating two behaviours here. The data hiding behaviour clearly changes the semantics of queries in ways that make a lot of deductions about the data incorrect. That's a pretty severe problem which would cause massive consequences down the road. The imposing of security restrictions based on the data in the row isn't really in the same league. I'm not sure I see any semantic problems with it at all. It's still true that it would be quite invasive to Postgres to implement. The current security restrictions are all checked and determined statically based on the query. Once a query is planned we can execute it without checking any security constraints at run-time. I don't think raising partitioning makes a useful contribution to this discussion. Firstly, it's not like partitioning doesn't change SQL semantics in its own way anyways. Even aside from that, partitioning is largely about the location that data is stored. Forcing data to be stored in different physical places just because our security model is based on the place the data is stored is kind of silly. Unless perhaps we implement partitioning which supports having many partitions share the same underlying, er, physical partition. But then I don't see how that's any different from row-level permissions. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's RemoteDBA services!
Joshua Brindle wrote: > Nonetheless, this conversation seems moot now that Tom has walled off > and won't even discuss row-level access controls. I think that's a bit of an overstatement. He says he's against them[1] and he says that they are the sticking point on this patch[2], and that they break SQL[2] and that he believes that implementations of row level acls he can imagine would be buggy[2]. Elsewhere other people on the core team are suggesting that others want to see SQL-level row permissions[3]. My reading of the discussion is that row-level access controls aren't vetoed permanently, but rather that (a) it's still clear what SQL semantics they'll break, (b) the implementations discussed so far seem at high risk of bugs to some people, and (c) some people haven't been sold on the need for them. None of those necessarily state that the feature will never get into postgres; but it makes it sound like a really high bar to jump over for a release that was originally scheduled to be done a while ago. [1] http://archives.postgresql.org/pgsql-hackers/2009-01/msg02389.php [2] http://archives.postgresql.org/pgsql-hackers/2009-01/msg02339.php [3] http://archives.postgresql.org/pgsql-hackers/2009-01/msg02391.php
Robert Haas <robertmhaas@gmail.com> writes:
> I'm not clear that I understand why it would be necessary for
> row-level security to touch every part of the code.
OK, I admit it's not "every" part, just every place that fetches,
inserts, updates, or deletes tuples. There are quite a few ;-)
As an example, the present patch imagines that it will have adequate
control over inserts by putting hooks into simple_heap_insert and the
(rather small number of) places that call heap_insert directly. But
there are dozens of calls of simple_heap_insert and no way for the
function itself to know why it is being called or on whose behalf.
The patch's hook function tries to work around the fact that it hasn't
got that information by means of heuristics. Aside from the question of
whether there are bugs in those heuristics today (I'm certain there
are), every time we accept a patch that adds another call of
simple_heap_insert, we're going to have to revisit the hook to see
if it needs to be twiddled.
Another problem is that since the hook only knows the parameters to
simple_heap_insert plus global state (such as current_user), it can't
cope very well with security-related context changes. We have already
heard that situations involving views are simply broken in the patch as
it stands --- row-level permissions are checked against current_user
and not the view owner, and there's no good way to fix that.
> It seems to me that the crucial decision is "Where are we
> trying to erect the security wall?". If we're trying to put it
> between the client and the postgres backend, then maybe the right
> thing to do is apply some sort of processing step to each query that
> is submitted, essentially rewriting "SELECT * FROM a, b" as "SELECT *
> FROM a, b WHERE you_can_see(a.securityid) AND
> you_can_see(b.securityid)". Maybe you (Tom) still won't like this
> because it breaks SQL semantics, but it seems like it would at least
> centralize the code.
Well, it wouldn't break SQL semantics from the point of view of the
planner, so that's one demerit taken off the books. However, I seem
to recall that exactly that approach was taken in a older version of
the patch (many moons ago) and we found fatal problems in it too.
The "where's the wall" point is a good one. I think part of the issue
is that the patch is to some extent trying to erect a security wall
that's between the data and large parts of the backend C code. Which is
an interesting idea and maybe could have been made to work if it'd been
designed in from the beginning, but to retrofit it today seems pretty
impractical.
regards, tom lane
Tom Lane wrote: > As an example, the present patch imagines that it will have adequate > control over inserts by putting hooks into simple_heap_insert and the > (rather small number of) places that call heap_insert directly. But > there are dozens of calls of simple_heap_insert and no way for the > function itself to know why it is being called or on whose behalf. > The patch's hook function tries to work around the fact that it hasn't > got that information by means of heuristics. Aside from the question of > whether there are bugs in those heuristics today (I'm certain there > are), every time we accept a patch that adds another call of > simple_heap_insert, we're going to have to revisit the hook to see > if it needs to be twiddled. > > Another problem is that since the hook only knows the parameters to > simple_heap_insert plus global state (such as current_user), it can't > cope very well with security-related context changes. We have already > heard that situations involving views are simply broken in the patch as > it stands --- row-level permissions are checked against current_user > and not the view owner, and there's no good way to fix that. > Leaving aside any other issues, it seems to me that the chance of remedying these defects reasonably in a couple of weeks is just about nil. That leaves the following questions: can the row-level part of the patch be separated out, and if so how easily, and is what would be left worth doing? cheers andrew
Joshua Brindle <method@manicmethod.com> writes:
> In reality it isn't, unless postgres won't mind thousands of
> partitions being made. In the military/gov implementations BLP lets
> you have hierarchical levels and non-hierarchical categories. Since I
> linked to an article about it upthread I assumed everyone
> participating was familiar but perhaps not. Typically there are 3
> levels, unclass, secret, top secret. In addition to those 3 levels
> there may be a few, hundreds or even thousands of categories. Those
> categories apply to each of the levels so if you are using 1000
> categories you have 3*1000 possible BLP labels. On top of that SELinux
> has users, roles and types, which are all also multiplied.
Hmm. If that's the expected application environment then the patch as
proposed has fatal performance problems anyway, for lack of a way to
get rid of no-longer-referenced pg_security rows. We had been led to
understand that there wouldn't be all that many distinct labels in use,
but this seems to imply that there are going to be $bignum of them.
That changes pg_security leakage from a can-live-with-for-first-cut
issue to a must-fix-to-be-credible issue.
(Just for context, thousands of partitions isn't practical with our
current implementation, but might be in the future.)
> Nonetheless, this conversation seems moot now that Tom has walled off
> and won't even discuss row-level access controls.
You realize, I trust, that I don't have the only vote around here.
But I am convinced that the row-level-security aspect of this proposal
is far less than fully baked, and isn't going to become so in the time
frame available for 8.4.
regards, tom lane
Good morning, I started to follow the discussion. (Time difference is unconfortable for me!) >> adding SELinux support for the existing levels of access control in PG > > is > > - table/column level access controls > - permission checks on database login > - permission checks on function invocation > - they need a facility to manage security label > - I want permission checks on loading a library, > though existing PG checks superuser() only. > > and > - removing PGACE, integrate SEPG code into core > - permission checks on largeobjects is postponed > - row level security is postponed (NOT REJECTED!) > - so, writable system column is also postponed If I postponed a part of functionalities as Stephen suggested, how many lines can be reduced? It is a quick estimation. Currently, the main patch has: 110 files changed, 9813 insertions(+), 16 deletions(-), 924 modifications(!) * src/backend/commands/copy.c | 293 +++! Most of them are to support writable system column, so about -300lines are expected. * src/backend/executor/execMain.c | 209 +++ Most of them are to support writable system column, so about -200 linesare expected * src/backend/security/pgaceCommon.c | 729 ++++++++++++ It will get scraped, but management of security attributehas to SELinux specific code, so -250 lines are expected * src/backend/security/pgaceHooks.c | 1547 ++++++++++++++++++++++++++ It will be gone, so -1550 lines are expected * src/backend/security/rowacl/rowacl.c | 721 ++++++++++++ It will be postponed, -700 lines are expected * src/backend/security/sepgsql/hooks.c | 1019 +++++++++++++++++ A part of permission checks (aka row,blob) is postponed,so -300 lines are expected. * src/include/security/pgace.h | 181 +++ * src/include/security/rowacl.h | 41 It will be gone, so -200 lines are expected At the total, -3,200 lines are expected. In addition, any other small-sized stuffs can be postponed. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
Tom Lane wrote: > Joshua Brindle <method@manicmethod.com> writes: >> In reality it isn't, unless postgres won't mind thousands of >> partitions being made. In the military/gov implementations BLP lets >> you have hierarchical levels and non-hierarchical categories. Since I >> linked to an article about it upthread I assumed everyone >> participating was familiar but perhaps not. Typically there are 3 >> levels, unclass, secret, top secret. In addition to those 3 levels >> there may be a few, hundreds or even thousands of categories. Those >> categories apply to each of the levels so if you are using 1000 >> categories you have 3*1000 possible BLP labels. On top of that SELinux >> has users, roles and types, which are all also multiplied. > > Hmm. If that's the expected application environment then the patch as It's one application environment, maybe not a common one but one I've seen. > proposed has fatal performance problems anyway, for lack of a way to > get rid of no-longer-referenced pg_security rows. We had been led to > understand that there wouldn't be all that many distinct labels in use, > but this seems to imply that there are going to be $bignum of them. > That changes pg_security leakage from a can-live-with-for-first-cut > issue to a must-fix-to-be-credible issue. I see. SELinux handles this by dynamically filling a sidtab (context to security identifier table) at runtime. This can be done because the contexts are stored as strings in the xattr's. I understand KaiGai wanted to save space in the tables so he stored sids, which would require a persistent sid mapping. I'm not sure this is a deal breaker though, I still know people that would rather have it now that may have performance issues than wait for something that can prune the sid mappings. > > (Just for context, thousands of partitions isn't practical with our > current implementation, but might be in the future.) > There are still other unresolved issues with partitions that I pointed out upthread. >> Nonetheless, this conversation seems moot now that Tom has walled off >> and won't even discuss row-level access controls. > > You realize, I trust, that I don't have the only vote around here. > But I am convinced that the row-level-security aspect of this proposal > is far less than fully baked, and isn't going to become so in the time > frame available for 8.4. > I know, but I also know how opensource communities work :) I haven't seen any enthusiastic support from other commiters which means, at least for the time being, it's being tabled. I mean no disrespect, I also agree with your assessment about the maintenance burden. As an opensource developer myself I would be nervous about intrusive patches that I don't understand, and what that means for me when I write new patches. This brings up my next point. The purpose of pgace is to mitigate this issue. The core team only needs to know when/where/why data can be selected, updated, deleted, etc and maintain the pgace hooks. As long as your side of the boundary stays correct the security model shouldn't break. Granted if you add new things (a new loadable module system, new kinds of large objects, etc) those would need to have the hooks added from the get go. This is similar to how the Linux kernel works, and the Xorg's XACE works. We do watch for updates and sometimes people mess things up but it isn't hard to spot. Especially with a small core team like postgres I'd think you guys could successfully spot places access controls need to be added.
Andrew Dunstan wrote: > Tom Lane wrote: >> As an example, the present patch imagines that it will have adequate >> control over inserts by putting hooks into simple_heap_insert and the >> (rather small number of) places that call heap_insert directly. But >> there are dozens of calls of simple_heap_insert and no way for the >> function itself to know why it is being called or on whose behalf. >> The patch's hook function tries to work around the fact that it hasn't >> got that information by means of heuristics. Aside from the question of >> whether there are bugs in those heuristics today (I'm certain there >> are), every time we accept a patch that adds another call of >> simple_heap_insert, we're going to have to revisit the hook to see >> if it needs to be twiddled. >> >> Another problem is that since the hook only knows the parameters to >> simple_heap_insert plus global state (such as current_user), it can't >> cope very well with security-related context changes. We have already >> heard that situations involving views are simply broken in the patch as >> it stands --- row-level permissions are checked against current_user >> and not the view owner, and there's no good way to fix that. >> > > Leaving aside any other issues, it seems to me that the chance of > remedying these defects reasonably in a couple of weeks is just about nil. > > That leaves the following questions: can the row-level part of the patch > be separated out, and if so how easily, and is what would be left worth > doing? At least, I believe the row-level access control make PostgreSQL more attractive in security aspect like a few major commercial dbms, and its tradeoff (PK/FK covert channel, optimization issue) should be a decision by end-users. However, I can accept an opinion the row-level facilities can be separated and postponed, then, step-by-step enlargement approach, as Peter & Stephen suggested. Even if we don't have row-level at v8.4, column-level MAC has its (may be limited) worth, for example, a column to store credit-card number never visible from web application context. I found a proverbial phrase in my dictionaly: Between two stools you fall to the ground. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
Obviously, we cannot make clear state of the row-level access controls by the date of v8.4 freeze. I agree the row-level access controls can be separated and postponed to v8.5 development cycle. So, I'll cut off a few part of previous patches for v8.4. Stephen Frost gave me a guideline about what should be remained or postponed *now*. It is SE-PG feature covers granularity existing PG facility enables to provide. (Typically, table/column level checks without row level) * The following features are still included for v8.4 (stage #01) - Centralized SELinux policy and getpeercon(3). - Table/Columnlevel access controls. Keep the current behavior. It checks client's privileges and raises error, if violated. - Functions, Databases The places to make decision are controversial. Is pg_xxx_aclcheck() possible to add SELinx check? I'll check it later. - Permission checks on shared library file The vanilla PG trusts superuser, but MAC don't trust him. So, we have tocheck the library's attribute. It should not be postponed because a malicious library once loaded can do anything internally. * The following features are postponed for v8.5 - Row-level access controls - Binary-large-objects access controls - Writablesystem column Because Row-level one is postponed, we don't need to export/import security_label of tuples to/fromusers now. * The following features are scraped. - PGACE security framework As I estimated in the previous message, scale of the main patch will be 6000-7000 lines. I'll pay my highest efforts to show the stripped patches within 5 days, due to Feb 04 JST. Thanks, | ---- Road Map (My plan) ---- | | * The stage#1 patches. V ---- PostgreSQL v8.4 ---- | | * Platform independent row-level access controls | * Writable system column (security_label, security_acl). | Maybe, we can also discuss writable "oid" in same time. V ---- First CommitFest ---- | | * SE-PostgreSQL row-level access controls. V ---- Second CommitFest ---- | | * Rest of features like binary large objects. V ---- Third CommitFest ---- | : V ---- PostgreSQL v8.5 ---- Ron Mayer wrote: > Joshua Brindle wrote: >> Nonetheless, this conversation seems moot now that Tom has walled off >> and won't even discuss row-level access controls. > > I think that's a bit of an overstatement. > > He says he's against them[1] and he says that they are the sticking > point on this patch[2], and that they break SQL[2] and that he believes > that implementations of row level acls he can imagine would be buggy[2]. > > Elsewhere other people on the core team are suggesting that > others want to see SQL-level row permissions[3]. > > > My reading of the discussion is that row-level access controls aren't > vetoed permanently, but rather that (a) it's still clear what SQL > semantics they'll break, (b) the implementations discussed so far > seem at high risk of bugs to some people, and (c) some people haven't > been sold on the need for them. None of those necessarily state > that the feature will never get into postgres; but it makes it sound > like a really high bar to jump over for a release that was originally > scheduled to be done a while ago. > > [1] http://archives.postgresql.org/pgsql-hackers/2009-01/msg02389.php > [2] http://archives.postgresql.org/pgsql-hackers/2009-01/msg02339.php > [3] http://archives.postgresql.org/pgsql-hackers/2009-01/msg02391.php -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
On Wed, Jan 28, 2009 at 6:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > As an example, the present patch imagines that it will have adequate > control over inserts by putting hooks into simple_heap_insert and the > (rather small number of) places that call heap_insert directly. But > there are dozens of calls of simple_heap_insert and no way for the > function itself to know why it is being called or on whose behalf. > The patch's hook function tries to work around the fact that it hasn't > got that information by means of heuristics. Aside from the question of > whether there are bugs in those heuristics today (I'm certain there > are), every time we accept a patch that adds another call of > simple_heap_insert, we're going to have to revisit the hook to see > if it needs to be twiddled. I agree that that's no good. > Another problem is that since the hook only knows the parameters to > simple_heap_insert plus global state (such as current_user), it can't > cope very well with security-related context changes. We have already > heard that situations involving views are simply broken in the patch as > it stands --- row-level permissions are checked against current_user > and not the view owner, and there's no good way to fix that. I thought that was intentional, and I sort of think that it's the right decision. >> It seems to me that the crucial decision is "Where are we >> trying to erect the security wall?". If we're trying to put it >> between the client and the postgres backend, then maybe the right >> thing to do is apply some sort of processing step to each query that >> is submitted, essentially rewriting "SELECT * FROM a, b" as "SELECT * >> FROM a, b WHERE you_can_see(a.securityid) AND >> you_can_see(b.securityid)". Maybe you (Tom) still won't like this >> because it breaks SQL semantics, but it seems like it would at least >> centralize the code. > > Well, it wouldn't break SQL semantics from the point of view of the > planner, so that's one demerit taken off the books. However, I seem > to recall that exactly that approach was taken in a older version of > the patch (many moons ago) and we found fatal problems in it too. Can you (or someone) provide a pointer to the archives? I can't immediately see any reason why that problem wouldn't be fixable. > The "where's the wall" point is a good one. I think part of the issue > is that the patch is to some extent trying to erect a security wall > that's between the data and large parts of the backend C code. Which is > an interesting idea and maybe could have been made to work if it'd been > designed in from the beginning, but to retrofit it today seems pretty > impractical. Agreed. With all respect to Kaigai-san, I am suspicious that some of this may be unintentional. It may be that cleaning this up and putting the wall in one well-defined spot will allay a number of your concerns. ...Robert
On Wed, Jan 28, 2009 at 6:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Hmm. If that's the expected application environment then the patch as > proposed has fatal performance problems anyway, for lack of a way to > get rid of no-longer-referenced pg_security rows. We had been led to > understand that there wouldn't be all that many distinct labels in use, > but this seems to imply that there are going to be $bignum of them. > That changes pg_security leakage from a can-live-with-for-first-cut > issue to a must-fix-to-be-credible issue. It's worth noting that this is yet another thing that is mostly a problem in the context of row-level security. It seems to me that if security labels are only applied to tables and columns, then it will be possible to scan the whole database relatively quickly and find all the labels that are still in use, probably without breaking a sweat. On the other hand, when you have row-level security, it gets a lot harder. I'm wondering if this problem could be solved with a sort of mark-and-sweep garbage collection: add a boolean column `used' to pg_security (which I really think out to be renamed to pg_selinux_context or something, and make a new table if we someday support Trusted Solaris or whatever). Whenever you do an OID lookup and find a row that says "false", do a non-transactional update and change it to say "true". Then you can write something which goes through and sets all the rows to false and then visits every row of every table in the database and forces OID lookups on the security ID of each. When you get done, any rows that still say false are unreferenced and can be killed. Also... even if there are thousands of contexts, it only matters to the extent that there is a lot of churn, and I'm not sure whether that's something that is expected. Josh Brindle, any thoughts? > (Just for context, thousands of partitions isn't practical with our > current implementation, but might be in the future.) > >> Nonetheless, this conversation seems moot now that Tom has walled off >> and won't even discuss row-level access controls. > > You realize, I trust, that I don't have the only vote around here. > But I am convinced that the row-level-security aspect of this proposal > is far less than fully baked, and isn't going to become so in the time > frame available for 8.4. You have a pretty big vote, though. But, I see signs though that you might relent at some future point when and if the proposal is well-baked. In any case, whether or not we do row-level security at some point in the future, you've certainly convinced me that the issues have not been thought through in enough detail to consider including any of this in 8.4. ...Robert
> I found a proverbial phrase in my dictionaly: > Between two stools you fall to the ground. LOL. I like that one - and it's very apt. ...Robert
KaiGai,
* KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
> Obviously, we cannot make clear state of the row-level access
> controls by the date of v8.4 freeze.
Indeed, I have to agree that's where things are headed, which is
certainly unfortunate but I think it's good that we were able to provide
some additional feedback on what the PG community is looking for.
> I agree the row-level access controls can be separated and
> postponed to v8.5 development cycle.
Having row-level security would be very nice in 8.5.
> So, I'll cut off a few part of previous patches for v8.4.
> Stephen Frost gave me a guideline about what should be remained
> or postponed *now*. It is SE-PG feature covers granularity
> existing PG facility enables to provide.
> (Typically, table/column level checks without row level)
This feels like a pretty reasonable compromise to me, and was based on
Peter's suggestion of an overall plan, but I'm not a committer and I
havn't seen any response to my request that they comment on that plan.
I'm sure your enthusiasm is appreciated though, and regardless of its
inclusion in 8.4, one of Peter's points was to develop the patches as
smaller changes that build on each other anyway.
Thanks,
Stephen
Robert,
* Robert Haas (robertmhaas@gmail.com) wrote:
> pg_security (which I really think out to be renamed to
> pg_selinux_context or something, and make a new table if we someday
> support Trusted Solaris or whatever).
Err, this doesn't really make sense if we're doing row-level security,
that's not something which is tied to SELinux or Trusted Solaris. Of
course, it's likely we'll need such a pg_selinux_context table or
something too.. Or maybe pg_security can be pg_rls instead. Just
wanted to avoid confusion over this point.. Assuming Peter's approach
is the path that is generally agreed upon by core..
Thanks,
Stephen
Robert Haas wrote: > On Wed, Jan 28, 2009 at 6:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Then you can write something which goes through and sets all the rows > to false and then visits every row of every table in the database and > forces OID lookups on the security ID of each. When you get done, any > rows that still say false are unreferenced and can be killed. > > Also... even if there are thousands of contexts, it only matters to > the extent that there is a lot of churn, and I'm not sure whether > that's something that is expected. Josh Brindle, any thoughts? > I wouldn't expect a whole lot of churn. Maybe when a project is archived you'd grab everything with a particular security context, save it off and remove it from the table. Constant relabels or removals based on context don't seem too likely though.
Robert Haas wrote: > On Wed, Jan 28, 2009 at 6:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> As an example, the present patch imagines that it will have adequate >> control over inserts by putting hooks into simple_heap_insert and the >> (rather small number of) places that call heap_insert directly. But >> there are dozens of calls of simple_heap_insert and no way for the >> function itself to know why it is being called or on whose behalf. >> The patch's hook function tries to work around the fact that it hasn't >> got that information by means of heuristics. Aside from the question of >> whether there are bugs in those heuristics today (I'm certain there >> are), every time we accept a patch that adds another call of >> simple_heap_insert, we're going to have to revisit the hook to see >> if it needs to be twiddled. > > I agree that that's no good. My concern is that superuser is allowed to modify system catalog by hand, like: UPDATE pg_proc SET probin = '/tmp/malicious_library.so' WHERE oid = ...; It is logically same as ALTER FUNCTION. Even if I remove a hook from simple_heap_xxxx(), it is necessary to check queries from clients. >> Another problem is that since the hook only knows the parameters to >> simple_heap_insert plus global state (such as current_user), it can't >> cope very well with security-related context changes. We have already >> heard that situations involving views are simply broken in the patch as >> it stands --- row-level permissions are checked against current_user >> and not the view owner, and there's no good way to fix that. > > I thought that was intentional, and I sort of think that it's the > right decision. From the viewpoint of SELinux, it is not a matter because core PostgreSQL does not change client's security context. But, it is indeed controversial for Row-level ACLs. (We have a time about this issue.) >>> It seems to me that the crucial decision is "Where are we >>> trying to erect the security wall?". If we're trying to put it >>> between the client and the postgres backend, then maybe the right >>> thing to do is apply some sort of processing step to each query that >>> is submitted, essentially rewriting "SELECT * FROM a, b" as "SELECT * >>> FROM a, b WHERE you_can_see(a.securityid) AND >>> you_can_see(b.securityid)". Maybe you (Tom) still won't like this >>> because it breaks SQL semantics, but it seems like it would at least >>> centralize the code. >> Well, it wouldn't break SQL semantics from the point of view of the >> planner, so that's one demerit taken off the books. However, I seem >> to recall that exactly that approach was taken in a older version of >> the patch (many moons ago) and we found fatal problems in it too. > > Can you (or someone) provide a pointer to the archives? I can't > immediately see any reason why that problem wouldn't be fixable. IIRC, 0racle or M$ has a patent to rewrite WHERE clause for security purpose, so Tom suggested it should be implemented using a hook deployed within executor. At least, it also enables code more simple. >> The "where's the wall" point is a good one. I think part of the issue >> is that the patch is to some extent trying to erect a security wall >> that's between the data and large parts of the backend C code. Which is >> an interesting idea and maybe could have been made to work if it'd been >> designed in from the beginning, but to retrofit it today seems pretty >> impractical. > > Agreed. With all respect to Kaigai-san, I am suspicious that some of > this may be unintentional. It may be that cleaning this up and > putting the wall in one well-defined spot will allay a number of your > concerns. A wall between the data and _backend C code_ is not my intention. Its purpose is a wall between the data and clients including superuser via SQL queries. It is a basic assumption we cannot acquire any accesses from system internal entities, as SELinux do nothing for kernel loadable modules. However, please note that making a decision at more hot point is more good design, because it enables to reduce potential bypasses. At the begining, I choosed simple_heap_xxxx() as a most hot point. However, it can be replacable, if we can find any other place without omission and consistent. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
* KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
>> I agree that that's no good.
As do I.
> My concern is that superuser is allowed to modify system catalog
> by hand, like:
>
> UPDATE pg_proc SET probin = '/tmp/malicious_library.so'
> WHERE oid = ...;
That UPDATE still goes through permissions checking, and that checking
even includes an explicit check when system catalogs are involved.
Appropriate hooks in that permission checking could prevent this from
ever being allowed.
> It is logically same as ALTER FUNCTION.
Sure, but I think it's straight-forward to make a case for "don't update
the system catalogs when you're running SE-PostgreSQL, use the
appropriate ALTER commands", and then remove the ability to do so when
SE-PostgreSQL is enabled.
>> Can you (or someone) provide a pointer to the archives? I can't
>> immediately see any reason why that problem wouldn't be fixable.
>
> IIRC, 0racle or M$ has a patent to rewrite WHERE clause for security
> purpose, so Tom suggested it should be implemented using a hook
> deployed within executor.
> At least, it also enables code more simple.
It'd probably be Oracle.. I'm not a big fan of that approach anyway
though, although I don't have any particular reason beyond 'it feels
kludgy' at the moment.
Thanks,
Stephen
Robert Haas wrote: > On Wed, Jan 28, 2009 at 6:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Hmm. If that's the expected application environment then the patch as >> proposed has fatal performance problems anyway, for lack of a way to >> get rid of no-longer-referenced pg_security rows. We had been led to >> understand that there wouldn't be all that many distinct labels in use, >> but this seems to imply that there are going to be $bignum of them. >> That changes pg_security leakage from a can-live-with-for-first-cut >> issue to a must-fix-to-be-credible issue. > > It's worth noting that this is yet another thing that is mostly a > problem in the context of row-level security. It seems to me that if > security labels are only applied to tables and columns, then it will > be possible to scan the whole database relatively quickly and find all > the labels that are still in use, probably without breaking a sweat. > On the other hand, when you have row-level security, it gets a lot > harder. Yes, I think we shoule not ignore upcoming feature, even if it is not on time of the next release. > I'm wondering if this problem could be solved with a sort of > mark-and-sweep garbage collection: add a boolean column `used' to > pg_security (which I really think out to be renamed to > pg_selinux_context or something, and make a new table if we someday > support Trusted Solaris or whatever). Whenever you do an OID lookup > and find a row that says "false", do a non-transactional update and > change it to say "true". It seems to me reference-counter is more preferable than boolean, at least. But it makes performance pain on writer access when it is expanded to row-level security. > Then you can write something which goes through and sets all the rows > to false and then visits every row of every table in the database and > forces OID lookups on the security ID of each. When you get done, any > rows that still say false are unreferenced and can be killed. > > Also... even if there are thousands of contexts, it only matters to > the extent that there is a lot of churn, and I'm not sure whether > that's something that is expected. Josh Brindle, any thoughts? Unless the security policy writer got crazy, it would be unrealistic. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
On Wed, Jan 28, 2009 at 9:27 PM, Stephen Frost <sfrost@snowman.net> wrote: > Robert, > > * Robert Haas (robertmhaas@gmail.com) wrote: >> pg_security (which I really think out to be renamed to >> pg_selinux_context or something, and make a new table if we someday >> support Trusted Solaris or whatever). > > Err, this doesn't really make sense if we're doing row-level security, > that's not something which is tied to SELinux or Trusted Solaris. Of > course, it's likely we'll need such a pg_selinux_context table or > something too.. Or maybe pg_security can be pg_rls instead. Just > wanted to avoid confusion over this point.. Assuming Peter's approach > is the path that is generally agreed upon by core.. I don't think there's anything about pg_security that is specific to row-level security. ...Robert
> My concern is that superuser is allowed to modify system catalog > by hand, like: > > UPDATE pg_proc SET probin = '/tmp/malicious_library.so' > WHERE oid = ...; > > It is logically same as ALTER FUNCTION. > > Even if I remove a hook from simple_heap_xxxx(), it is necessary > to check queries from clients. That's a valid concern, I think all we're saying here is that you need to find a better place to block that, maybe by assigning pg_proc an security label that prevents modification by the superuser. > IIRC, 0racle or M$ has a patent to rewrite WHERE clause for security > purpose, so Tom suggested it should be implemented using a hook > deployed within executor. > At least, it also enables code more simple. Patents suck. I guess we need to understand this issue better before we can make any decisions. > A wall between the data and _backend C code_ is not my intention. > > Its purpose is a wall between the data and clients including superuser > via SQL queries. It is a basic assumption we cannot acquire any accesses > from system internal entities, as SELinux do nothing for kernel loadable > modules. > > However, please note that making a decision at more hot point is more > good design, because it enables to reduce potential bypasses. > At the begining, I choosed simple_heap_xxxx() as a most hot point. > However, it can be replacable, if we can find any other place without > omission and consistent. That's why I was thinking about the planner... ...Robert
Robert Haas wrote: > On Wed, Jan 28, 2009 at 9:27 PM, Stephen Frost <sfrost@snowman.net> wrote: >> Robert, >> >> * Robert Haas (robertmhaas@gmail.com) wrote: >>> pg_security (which I really think out to be renamed to >>> pg_selinux_context or something, and make a new table if we someday >>> support Trusted Solaris or whatever). >> Err, this doesn't really make sense if we're doing row-level security, >> that's not something which is tied to SELinux or Trusted Solaris. Of >> course, it's likely we'll need such a pg_selinux_context table or >> something too.. Or maybe pg_security can be pg_rls instead. Just >> wanted to avoid confusion over this point.. Assuming Peter's approach >> is the path that is generally agreed upon by core.. > > I don't think there's anything about pg_security that is specific to > row-level security. Yes, SELinux requires any objects (not only tuples) to be labeled. The pg_security is also necessary for tables/columns/... Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
On Wed, Jan 28, 2009 at 10:15 PM, KaiGai Kohei <kaigai@ak.jp.nec.com> wrote: > It seems to me reference-counter is more preferable than boolean, > at least. But it makes performance pain on writer access when it > is expanded to row-level security. A reference counter will never work. You could easily end up serializing all access to the database around the row-level lock on one popular security context. That is a performance problem two or three orders of magnitude worse than anything that has been suggested so far in connection with this feature, and probably six orders of magnitude worse than the problem you're trying to solve (pg_security, or whatever we want to call it, getting too big). For the record, I think Tom's concern in this area is largely off-base, especially in light of the fact that Joshua Brindle and Kaigai both agree that churn is not likely to be large. I think we need some kind of plausible way to clean out the table, but I don't think it needs to be fully automated or super-efficient, just something that someone could do if they felt the need. ...Robert
Robert Haas wrote: > On Wed, Jan 28, 2009 at 10:15 PM, KaiGai Kohei <kaigai@ak.jp.nec.com> wrote: >> It seems to me reference-counter is more preferable than boolean, >> at least. But it makes performance pain on writer access when it >> is expanded to row-level security. > > A reference counter will never work. You could easily end up > serializing all access to the database around the row-level lock on > one popular security context. That is a performance problem two or > three orders of magnitude worse than anything that has been suggested > so far in connection with this feature, and probably six orders of > magnitude worse than the problem you're trying to solve (pg_security, > or whatever we want to call it, getting too big). Yes, I don't think it is a good approach also. Just I though it was relatively preferable than boolean. > For the record, I think Tom's concern in this area is largely > off-base, especially in light of the fact that Joshua Brindle and > Kaigai both agree that churn is not likely to be large. I think we > need some kind of plausible way to clean out the table, but I don't > think it needs to be fully automated or super-efficient, just > something that someone could do if they felt the need. If pg_security is actually overflowed or makes compress storages by unused ones, I recommend to stop the system and run a utility to reclaim them. But I don't think it actually happen. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
Robert Haas wrote: >> My concern is that superuser is allowed to modify system catalog >> by hand, like: >> >> UPDATE pg_proc SET probin = '/tmp/malicious_library.so' >> WHERE oid = ...; >> >> It is logically same as ALTER FUNCTION. >> >> Even if I remove a hook from simple_heap_xxxx(), it is necessary >> to check queries from clients. > > That's a valid concern, I think all we're saying here is that you need > to find a better place to block that, maybe by assigning pg_proc an > security label that prevents modification by the superuser. On SE-PostgreSQL, we have two kind of superuser: 1. A superuser with privileged domain for ALTER FUNCTION. 2. A superuserwith unprivileged domain for ALTER FUNCTION. SE-PostgreSQL also allows (1) to modify pg_proc by hand, becuase security policy allows it. (But, OS feature can block someone untrusted (like web app) to translate into privileged domain.) Stephen's suggestion (deny to update all the system catalog) seems to me a bit rough. I don't make sure there is no application which depends on superuser is writable to system catalog. In addition, this limitation is not based on security policy. So, I think we have to deploy a hook on ExecUpdate() at least, simple_heap_update() aside. If we cannot obtain enough information from context, we can apply possible maximum permissions here. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
Tom Lane wrote: > Gregory Stark <stark@enterprisedb.com> writes: > > I don't think partitioning is really the same thing as row-level > > security. > > Of course not, but it seems to me that it can be used to accomplish most > of the same practical use-cases. The main gripe about doing it via > partitioning is that the user's nose gets rubbed in the fact that there > can't be an enormous number of different security classifications in the > same table (since he has to explicitly make a partition for each one). > But the proposed implementation of row-level security would poop out > pretty darn quick for such a case, too, and frankly I'm not seeing an > application that would demand it. OK, putting on my crazy idea hat, if we split the primary and foreign keys by partition, it would give us polyinstantiation: http://en.wikipedia.org/wiki/Polyinstantiation because our unique indexes do not apply across partitions. Polyinstantiation is a desirable security feature and one that would be tough to implement without partitions. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Robert Haas wrote: > On Wed, Jan 28, 2009 at 6:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Hmm. If that's the expected application environment then the patch as > > proposed has fatal performance problems anyway, for lack of a way to > > get rid of no-longer-referenced pg_security rows. We had been led to > > understand that there wouldn't be all that many distinct labels in use, > > but this seems to imply that there are going to be $bignum of them. > > That changes pg_security leakage from a can-live-with-for-first-cut > > issue to a must-fix-to-be-credible issue. > > It's worth noting that this is yet another thing that is mostly a > problem in the context of row-level security. It seems to me that if > security labels are only applied to tables and columns, then it will > be possible to scan the whole database relatively quickly and find all > the labels that are still in use, probably without breaking a sweat. > On the other hand, when you have row-level security, it gets a lot > harder. If we are not labeling every row, why not just use a TEXT column without using an OID to reference pg_security; there aren't that places, pg_class, pg_attribute, etc, i.e. they are not on every data row. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > Tom Lane wrote: >> Gregory Stark <stark@enterprisedb.com> writes: >>> I don't think partitioning is really the same thing as row-level >>> security. >> Of course not, but it seems to me that it can be used to accomplish most >> of the same practical use-cases. The main gripe about doing it via >> partitioning is that the user's nose gets rubbed in the fact that there >> can't be an enormous number of different security classifications in the >> same table (since he has to explicitly make a partition for each one). >> But the proposed implementation of row-level security would poop out >> pretty darn quick for such a case, too, and frankly I'm not seeing an >> application that would demand it. > > OK, putting on my crazy idea hat, if we split the primary and foreign > keys by partition, it would give us polyinstantiation: > > http://en.wikipedia.org/wiki/Polyinstantiation > > because our unique indexes do not apply across partitions. > Polyinstantiation is a desirable security feature and one that would be > tough to implement without partitions. The issue is not such a simple one. SELinux allows to change its security context, if user has appropriate privileges (relabelfrom and relabelto). When a tuple (label=Secret, PK=123) is inserted then it is relabeled to (label=TopSecret, PK=123), it has to return an error due to the PK confliction, even if he does not have permission to refer it. I guess you already noticed that the previous security research report about polyinstantiation implicitly assumes a fixed form security policy, like "higher can read lower", "higher cannot write lower" and so on. But, SELinux's security policy does not assume anything. It always returns its decision based on flexible security policy. So, polyinstantiation is very difficult technology. :-( (And will be overspec for enterprise class purpose.) Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
FYI, I have created a wiki page that summerizes the SE-PostgreSQL information we have gathered so far: http://wiki.postgresql.org/wiki/SEPostgreSQL-patch -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > Robert Haas wrote: >> On Wed, Jan 28, 2009 at 6:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Hmm. If that's the expected application environment then the patch as >>> proposed has fatal performance problems anyway, for lack of a way to >>> get rid of no-longer-referenced pg_security rows. We had been led to >>> understand that there wouldn't be all that many distinct labels in use, >>> but this seems to imply that there are going to be $bignum of them. >>> That changes pg_security leakage from a can-live-with-for-first-cut >>> issue to a must-fix-to-be-credible issue. >> It's worth noting that this is yet another thing that is mostly a >> problem in the context of row-level security. It seems to me that if >> security labels are only applied to tables and columns, then it will >> be possible to scan the whole database relatively quickly and find all >> the labels that are still in use, probably without breaking a sweat. >> On the other hand, when you have row-level security, it gets a lot >> harder. > > If we are not labeling every row, why not just use a TEXT column without > using an OID to reference pg_security; there aren't that places, > pg_class, pg_attribute, etc, i.e. they are not on every data row. We should not assume every row are not labeled forever, at least. It will lose expandability soon. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
On Wed, Jan 28, 2009 at 9:49 PM, KaiGai Kohei <kaigai@ak.jp.nec.com> wrote:
Yes, it was Oracle. There are a couple newer revisions, but they're all based primarily on Patent #6487552, Database Fine-grained Access Control, Filed Oct 5, 1998/Issued Nov 26, 2002. The patent covers defining a security context, retrieving-defined policies from that context, and applying those policies by directly calling a security-context-related stored procedure in the WHERE clause as well as dynamically adding security-related predicates to the WHERE-clause.IIRC, 0racle or M$ has a patent to rewrite WHERE clause for security
purpose, so Tom suggested it should be implemented using a hook
deployed within executor.
--
Jonah H. Harris, Senior DBA
myYearbook.com
Robert Haas <robertmhaas@gmail.com> writes: > I'm wondering if this problem could be solved with a sort of > mark-and-sweep garbage collection: >... > Then you can write something which goes through and sets all the rows > to false and then visits every row of every table in the database and > forces OID lookups on the security ID of each. When you get done, any > rows that still say false are unreferenced and can be killed. This sounds awfully similar to the bitmap index vacuum problem. I wonder if security labels could be implemented as some kind of funky special index. Just thinking out loud. I don't have a well-formed idea based on this. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's 24x7 Postgres support!
Bruce Momjian wrote: > Tom Lane wrote: >> Gregory Stark <stark@enterprisedb.com> writes: >>> I don't think partitioning is really the same thing as row-level >>> security. >> Of course not, but it seems to me that it can be used to accomplish most >> of the same practical use-cases. The main gripe about doing it via >> partitioning is that the user's nose gets rubbed in the fact that there >> can't be an enormous number of different security classifications in the >> same table (since he has to explicitly make a partition for each one). >> But the proposed implementation of row-level security would poop out >> pretty darn quick for such a case, too, and frankly I'm not seeing an >> application that would demand it. > > OK, putting on my crazy idea hat, if we split the primary and foreign > keys by partition, it would give us polyinstantiation: > > http://en.wikipedia.org/wiki/Polyinstantiation > > because our unique indexes do not apply across partitions. > Polyinstantiation is a desirable security feature and one that would be > tough to implement without partitions. > Polyinstantiation in this manner won't do it I don't think (if I'm understanding you correctly). As KaiGai already said, SELinux policy is flexible so we'll have more than just BLP policy to worry about. Also a top secret user will need to see all rows when he selects, and they should still have unique keys. He won't be able to write to secret or unclass rows but he'll be able to see them. Further, a secret user may be able to see top secret rows, but not some of the columns, going back to my coordinates example. He'd have to use a trusted stored procedure that can do the necessary operations to fuzz the data. A flexible policy is really the crux of this. We aren't trying to reproduce what other DBMS's have done, we are trying to integrate with a completely flexible policy from the ground up.
> > I don't think partitioning is really the same thing as row-level security. > > Of course not, but it seems to me that it can be used to accomplish most > of the same practical use-cases. The main gripe about doing it via > partitioning is that the user's nose gets rubbed in the fact that there > can't be an enormous number of different security classifications in the > same table (since he has to explicitly make a partition for each one). Imho a useful partitioning feature that is worth extra syntax additions will have to include the ability to automatically create partitions on demand (and maybe remove empty ones during vacuum). (I have refrained from discussing partitioning until now, because I thought this is not the time. But the certainty with which manual creation is implied here makes me nervous.) I short it (imho) requires a partitioning clause (much like a group by clause in sql) and optionally an expression to produce a partition name (+ maybe for the nostalgic a tablespace name mapping expression). If partitioning for row level sec includes a sec column as proposed, I think the two could be combined as a means for performance optimization. But I am not sure partitioning alone can efficiently replace the sec column approach. (especially in the admittedly unlikely >100 sec label scenario). (When a constraint says the partition only contains visible security labels, the sec check can be done at the partition level (including CE for denied labels)) Andreas
Joshua Brindle wrote: > Bruce Momjian wrote: > > Tom Lane wrote: > >> Gregory Stark <stark@enterprisedb.com> writes: > >>> I don't think partitioning is really the same thing as row-level > >>> security. > >> Of course not, but it seems to me that it can be used to accomplish most > >> of the same practical use-cases. The main gripe about doing it via > >> partitioning is that the user's nose gets rubbed in the fact that there > >> can't be an enormous number of different security classifications in the > >> same table (since he has to explicitly make a partition for each one). > >> But the proposed implementation of row-level security would poop out > >> pretty darn quick for such a case, too, and frankly I'm not seeing an > >> application that would demand it. > > > > OK, putting on my crazy idea hat, if we split the primary and foreign > > keys by partition, it would give us polyinstantiation: > > > > http://en.wikipedia.org/wiki/Polyinstantiation > > > > because our unique indexes do not apply across partitions. > > Polyinstantiation is a desirable security feature and one that would be > > tough to implement without partitions. > > > > Polyinstantiation in this manner won't do it I don't think (if I'm understanding > you correctly). As KaiGai already said, SELinux policy is flexible so we'll have > more than just BLP policy to worry about. > > Also a top secret user will need to see all rows when he selects, and they > should still have unique keys. He won't be able to write to secret or unclass > rows but he'll be able to see them. Yea, it would take some work but it is an idea. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
> Yea, it would take some work but it is an idea. It's *an* idea,yes. But it introduces as many (or more) problems than it solves. --Josh
Josh Berkus wrote: > > > Yea, it would take some work but it is an idea. > > It's *an* idea,yes. But it introduces as many (or more) problems than > it solves. Ah, but my problems might be easier solved than the row-level permission problems. ;-) -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > Josh Berkus wrote: >>> Yea, it would take some work but it is an idea. >> It's *an* idea,yes. But it introduces as many (or more) problems than >> it solves. > > Ah, but my problems might be easier solved than the row-level permission > problems. ;-) > Or might not. Multi-partition indexes? Multi-partition uniqueness? Automated moving of rows between partitions? --Josh
Josh Berkus wrote: > Bruce Momjian wrote: > > Josh Berkus wrote: > >>> Yea, it would take some work but it is an idea. > >> It's *an* idea,yes. But it introduces as many (or more) problems than > >> it solves. > > > > Ah, but my problems might be easier solved than the row-level permission > > problems. ;-) > > > > Or might not. Multi-partition indexes? Multi-partition uniqueness? > Automated moving of rows between partitions? Are you trying to make some kind of point? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > Josh Berkus wrote: >> Bruce Momjian wrote: >>> Josh Berkus wrote: >>>>> Yea, it would take some work but it is an idea. >>>> It's *an* idea,yes. But it introduces as many (or more) problems than >>>> it solves. >>> Ah, but my problems might be easier solved than the row-level permission >>> problems. ;-) >>> >> Or might not. Multi-partition indexes? Multi-partition uniqueness? >> Automated moving of rows between partitions? > > Are you trying to make some kind of point? > IMVHO Josh was describing a nice-to-have TODO list for a partitions feature in general. :-) Maybe he was saying that when they partitioning feature is designed that they try to think of polyinstantiation as they design it :-)
Bruce, > Are you trying to make some kind of point? > Yeah, that we're certainly not doing any of this for 8.4. If we're going for radical new approaches for row-level, why not also look at the VIEWS approach? If we worked out the same problems we need to fix for Bernd's patch, using automated manatory views to enforce row-level access is also plausible. --Josh
Josh Berkus wrote: > Bruce, > > > Are you trying to make some kind of point? > > > > Yeah, that we're certainly not doing any of this for 8.4. > > If we're going for radical new approaches for row-level, why not also > look at the VIEWS approach? If we worked out the same problems we need > to fix for Bernd's patch, using automated manatory views to enforce > row-level access is also plausible. Sure, we can explore that too. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Joshua, Kohei-san, So, for 8.4: *if* we included in 8.4 a version of SEPostgres with all features *except* row-level security, would it still be useful to the SELinux community? I think we're just not going to work out the headache-inducing issues around row-level security in time for 8.4, and it seems to me that integrated system-level security labels at the table-and-column level are still very useful, even without row-level security. --Josh
On Fri, Jan 30, 2009 at 5:37 PM, Josh Berkus <josh@agliodbs.com> wrote: > Bruce, >> Are you trying to make some kind of point? >> > > Yeah, that we're certainly not doing any of this for 8.4. > > If we're going for radical new approaches for row-level, why not also look > at the VIEWS approach? If we worked out the same problems we need to fix > for Bernd's patch, using automated manatory views to enforce row-level > access is also plausible. I'm rather enchanted with the idea of using table partitioning to implement row-level security, but the obstacles seem rather formidable. Right now, a partitioned relation behaves nothing like a regular relation, and to use it for this purpose you'd need to make it transparent. IOW, you'd need to be able to define indices that spanned multiple partitions (including enforcement of unique constraints), you'd need to be able to make foreign keys that could point to a row in arbitrary subset of the partitions, you'd need automatic creation and deletion of partitions, you'd need better planner support for partitions, and you'd need to somehow deal with the issue of pg_class bloat. Plus, to make it truly transparent, you'd need multiple layers of partitioning, in case someone wanted to do row-level security and range partitioning simultaneously. Now, the plus side is that if we could do all of that, we'd have the infrastructure to support some truly awesome partitioning stuff, and not just row-level security. But it seems awfully hard. ...Robert
Josh Berkus wrote: > Joshua, Kohei-san, > > So, for 8.4: *if* we included in 8.4 a version of SEPostgres with all > features *except* row-level security, would it still be useful to the > SELinux community? > > I think we're just not going to work out the headache-inducing issues > around row-level security in time for 8.4, and it seems to me that > integrated system-level security labels at the table-and-column level > are still very useful, even without row-level security. > > Hasn't a plan for this already been posted? See http://archives.postgresql.org/pgsql-hackers/2009-01/msg02407.php cheers andrew
* Andrew Dunstan (andrew@dunslane.net) wrote: > Josh Berkus wrote: >> So, for 8.4: *if* we included in 8.4 a version of SEPostgres with all >> features *except* row-level security, would it still be useful to the >> SELinux community? >> >> I think we're just not going to work out the headache-inducing issues >> around row-level security in time for 8.4, and it seems to me that >> integrated system-level security labels at the table-and-column level >> are still very useful, even without row-level security. I tend to agree that they will be very useful. I'm not sure there will be much adoption without row-level in the security community though, to be honest. I'd like to see it as part of an overall plan to eventually do row-level support. Given the size of this overall work and feature set, I think it's appropriate to do it in a staged manner regardless. > Hasn't a plan for this already been posted? See > http://archives.postgresql.org/pgsql-hackers/2009-01/msg02407.php Sure, that's a plan, but Josh's question is certainly appropriate. Thanks, Stephen
Josh Berkus wrote: > Joshua, Kohei-san, > > So, for 8.4: *if* we included in 8.4 a version of SEPostgres with all > features *except* row-level security, would it still be useful to the > SELinux community? Yes, obviously. I think the granularity of access controls is an aspect of security. > I think we're just not going to work out the headache-inducing issues > around row-level security in time for 8.4, and it seems to me that > integrated system-level security labels at the table-and-column level > are still very useful, even without row-level security. For example, table-and-column level access control can provide such a worth which enables to store customer's credit-card-number within unaccessable column from all the web application (children of Apache) but accessable from settlement system (child of crond). It enables to prevent SQL injection to steal very sensitive info. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Andrew Dunstan wrote: > > > Josh Berkus wrote: >> Joshua, Kohei-san, >> >> So, for 8.4: *if* we included in 8.4 a version of SEPostgres with all >> features *except* row-level security, would it still be useful to the >> SELinux community? >> >> I think we're just not going to work out the headache-inducing issues >> around row-level security in time for 8.4, and it seems to me that >> integrated system-level security labels at the table-and-column level >> are still very useful, even without row-level security. > > Hasn't a plan for this already been posted? See > http://archives.postgresql.org/pgsql-hackers/2009-01/msg02407.php FYI: * previous full-functional SE-PostgreSQL/Row-ACLs [kaigai@fedora10 security]$ wc -l *.c */*.c 729 pgaceCommon.c 1547 pgaceHooks.c 721 rowacl/rowacl.c 1200 sepgsql/avc.c 623 sepgsql/core.c 1019 sepgsql/hooks.c 785 sepgsql/permissions.c 1097 sepgsql/proxy.c 7721 total * A lite SE-PostgreSQL without row-level security, large object support, writable system column [kaigai@fedora10 sepgsql]$ wc -l *.c 904 checker.c 1181 avc.c 360 core.c 55 dummy.c 683 hooks.c 478 label.c 553 perms.c 4214 total Today, I'll debug the modified code... -- KaiGai Kohei <kaigai@kaigai.gr.jp>
KaiGai Kohei wrote: > > Hasn't a plan for this already been posted? See > > http://archives.postgresql.org/pgsql-hackers/2009-01/msg02407.php > > FYI: > > * previous full-functional SE-PostgreSQL/Row-ACLs > > [kaigai@fedora10 security]$ wc -l *.c */*.c > 729 pgaceCommon.c > 1547 pgaceHooks.c > 721 rowacl/rowacl.c > 1200 sepgsql/avc.c > 623 sepgsql/core.c > 1019 sepgsql/hooks.c > 785 sepgsql/permissions.c > 1097 sepgsql/proxy.c > 7721 total > > * A lite SE-PostgreSQL without row-level security, > large object support, writable system column > > [kaigai@fedora10 sepgsql]$ wc -l *.c > 904 checker.c > 1181 avc.c > 360 core.c > 55 dummy.c > 683 hooks.c > 478 label.c > 553 perms.c > 4214 total > > Today, I'll debug the modified code... Wow, that was fast. Where are you storing the security information for tables and columns? Did you add a special column to pg_class, etc? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > KaiGai Kohei wrote: >>> Hasn't a plan for this already been posted? See >>> http://archives.postgresql.org/pgsql-hackers/2009-01/msg02407.php >> FYI: >> >> * previous full-functional SE-PostgreSQL/Row-ACLs >> >> [kaigai@fedora10 security]$ wc -l *.c */*.c >> 729 pgaceCommon.c >> 1547 pgaceHooks.c >> 721 rowacl/rowacl.c >> 1200 sepgsql/avc.c >> 623 sepgsql/core.c >> 1019 sepgsql/hooks.c >> 785 sepgsql/permissions.c >> 1097 sepgsql/proxy.c >> 7721 total >> >> * A lite SE-PostgreSQL without row-level security, >> large object support, writable system column >> >> [kaigai@fedora10 sepgsql]$ wc -l *.c >> 904 checker.c >> 1181 avc.c >> 360 core.c >> 55 dummy.c >> 683 hooks.c >> 478 label.c >> 553 perms.c >> 4214 total >> >> Today, I'll debug the modified code... > > Wow, that was fast. Where are you storing the security information for > tables and columns? Did you add a special column to pg_class, etc? Security information is stored within padding field of HeapTupleHeader as we did. It can be fetched via sepgsql_(table|column|...)_getcon() functions, and can be set via SECURITY_LABEL = 'xxx'. -- KaiGai Kohei <kaigai@kaigai.gr.jp>
KaiGai Kohei wrote: > >> Today, I'll debug the modified code... > > > > Wow, that was fast. Where are you storing the security information for > > tables and columns? Did you add a special column to pg_class, etc? > > Security information is stored within padding field of HeapTupleHeader > as we did. It can be fetched via sepgsql_(table|column|...)_getcon() > functions, and can be set via SECURITY_LABEL = 'xxx'. Well, we are not using row-level security values so why not store it in its own column regular or as part of the existing ACL structure. I think it will be very odd for system tables to have this special column but not user rows. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > KaiGai Kohei wrote: >>>> Today, I'll debug the modified code... >>> Wow, that was fast. Where are you storing the security information for >>> tables and columns? Did you add a special column to pg_class, etc? >> Security information is stored within padding field of HeapTupleHeader >> as we did. It can be fetched via sepgsql_(table|column|...)_getcon() >> functions, and can be set via SECURITY_LABEL = 'xxx'. > > Well, we are not using row-level security values so why not store it in > its own column regular or as part of the existing ACL structure. I > think it will be very odd for system tables to have this special column > but not user rows. Sorry, my description might easily make confusion. I read it again myself, indeed, it makes confusion. :( SECURITY_LABEL = 'xxx' means following sytle: CREATE TABLE t ( a int, b text SECURITY_LABEL = '...' ) SECURITY_LABEL = '...'; I don't provide both of "security_label" and "security_acl" system columns for system/user tables. I didn't write it explicitly, it might make you confusing. User cannot see what security label is assigned to them due to lack of system column, so new sepgsql_xxx_getcon() functions are provided an interface to see security label. In this patch, I don't touch new system columns. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
KaiGai,
* KaiGai Kohei (kaigai@kaigai.gr.jp) wrote:
> I don't provide both of "security_label" and "security_acl"
> system columns for system/user tables.
> I didn't write it explicitly, it might make you confusing.
>
> User cannot see what security label is assigned to them
> due to lack of system column, so new sepgsql_xxx_getcon()
> functions are provided an interface to see security label.
>
> In this patch, I don't touch new system columns.
I think Bruce's question was where you stored the security_acl and
security_label columns. Based on your response (and a bit of purusal
through the code.google site), it looks like you still have security_acl
and security_label defined as internal columns and being included
for at least system tables (or is it everywhere?). I think what people
are looking for, instead, is either additional columns to just the
existing system tables that need them (eg: pg_class, pg_attribute) or
included in the existing ACL structure (the aclitem structure). Another
option might be a seperate set of tables.
This would further reduce the patch pretty significantly, I suspect,
though you will need to rework some things. In terms of minimally
invasive, I would guess modifying the existing ACL structure for the ACL
info, and a seperate table to track the labels for different
objects/sub-objects (similar to pg_depend) would be your best approach.
That would require no changes to existing system tables, but a few
changes in places where the ACL is handled, and then the hooks in the
right places to do the permission checking.
Just my 2c.
Thanks,
Stephen
Stephen Frost wrote: > KaiGai, > > * KaiGai Kohei (kaigai@kaigai.gr.jp) wrote: >> I don't provide both of "security_label" and "security_acl" >> system columns for system/user tables. >> I didn't write it explicitly, it might make you confusing. >> >> User cannot see what security label is assigned to them >> due to lack of system column, so new sepgsql_xxx_getcon() >> functions are provided an interface to see security label. >> >> In this patch, I don't touch new system columns. > > I think Bruce's question was where you stored the security_acl and > security_label columns. Based on your response (and a bit of purusal > through the code.google site), it looks like you still have security_acl > and security_label defined as internal columns and being included > for at least system tables (or is it everywhere?). In the current working tree, it (security id) is stored within padding field of HeapTupleHeader, so internal facility can read it via HeapTupleGetSecLabel(), but I already removed "security_acl" and "security_label" definition. Its basic structure is unchanged, the text form of security label is stored within pg_security system catalog. > I think what people > are looking for, instead, is either additional columns to just the > existing system tables that need them (eg: pg_class, pg_attribute) or > included in the existing ACL structure (the aclitem structure). Another > option might be a seperate set of tables. It should not be held in text form within each tuples. Please remind why I rework for the feature again. Its facilities are a bit large to get included at a time, so its development should be done step-by-step approach and separatable ones (row-level, largeobjects) are postponed. This change is extreamly large, almost same as implementing from zero. I think it is far from step-by-step. > This would further reduce the patch pretty significantly, I suspect, > though you will need to rework some things. No, it need to rework *any* things. :( Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
KaiGai,
* KaiGai Kohei (kaigai@kaigai.gr.jp) wrote:
> Stephen Frost wrote:
> > I think Bruce's question was where you stored the security_acl and
> > security_label columns. Based on your response (and a bit of purusal
> > through the code.google site), it looks like you still have security_acl
> > and security_label defined as internal columns and being included
> > for at least system tables (or is it everywhere?).
>
> In the current working tree, it (security id) is stored within
> padding field of HeapTupleHeader, so internal facility can read
> it via HeapTupleGetSecLabel(), but I already removed "security_acl"
> and "security_label" definition.
> Its basic structure is unchanged, the text form of security label
> is stored within pg_security system catalog.
I'm pretty sure that structure is part of what people were unhappy about
though, and what makes it a much more invasive change that violates
certain levels in the system by requiring information at much lower
levels than it had before.
> > I think what people
> > are looking for, instead, is either additional columns to just the
> > existing system tables that need them (eg: pg_class, pg_attribute) or
> > included in the existing ACL structure (the aclitem structure). Another
> > option might be a seperate set of tables.
>
> It should not be held in text form within each tuples.
> Please remind why I rework for the feature again.
> Its facilities are a bit large to get included at a time,
> so its development should be done step-by-step approach and
> separatable ones (row-level, largeobjects) are postponed.
> This change is extreamly large, almost same as implementing
> from zero. I think it is far from step-by-step.
Yes, the development should be step-by-step, with minimally invasive
changes each time. The first step is just hooking into SELinux for
access levels that PostgreSQL already supports (database, schema,
table, column). Requiring that the HeapTupleHeader be changed and that
additional internal system columns be added, along with SELinux checks
at levels that aren't appropriate, is alot more than this first patch
should really need. Those kinds of changes would be with the patch to
add non-SELinux row-level security. Following that, SELinux hooks would
be added to the RLS checks.
I think you need to look at it from the point of view of the committers
and consider the whole patch, not just the quantity of code under
security/. While that is important, the changes to the code *outside*
of security/ is what the committers are going to be most concerned about
because that gets into the overall structure and design and are areas
which are much more likely to affect users who don't use this feature,
performance, etc.
Or at least, that's what I'd expect. Getting some commentary from core
would be nice..
Thanks,
Stephen
On Sat, Jan 31, 2009 at 8:32 AM, Stephen Frost <sfrost@snowman.net> wrote: > * KaiGai Kohei (kaigai@kaigai.gr.jp) wrote: >> Stephen Frost wrote: >> > I think Bruce's question was where you stored the security_acl and >> > security_label columns. Based on your response (and a bit of purusal >> > through the code.google site), it looks like you still have security_acl >> > and security_label defined as internal columns and being included >> > for at least system tables (or is it everywhere?). >> >> In the current working tree, it (security id) is stored within >> padding field of HeapTupleHeader, so internal facility can read >> it via HeapTupleGetSecLabel(), but I already removed "security_acl" >> and "security_label" definition. >> Its basic structure is unchanged, the text form of security label >> is stored within pg_security system catalog. > > I'm pretty sure that structure is part of what people were unhappy about > though, and what makes it a much more invasive change that violates > certain levels in the system by requiring information at much lower > levels than it had before. IANAC, but that's my impression too. The simplified patch shouldn't assume that row-level security in its current form is going to end up getting put back in. AFAICS, there's no reason why the security ID for tables can't be a regular attribute in pg_class, or why the security attribute for columns can't be a regular attribute in pg_attribute. ...Robert
>>> I think what people >>> are looking for, instead, is either additional columns to just the >>> existing system tables that need them (eg: pg_class, pg_attribute) or >>> included in the existing ACL structure (the aclitem structure). Another >>> option might be a seperate set of tables. >> It should not be held in text form within each tuples. >> Please remind why I rework for the feature again. >> Its facilities are a bit large to get included at a time, >> so its development should be done step-by-step approach and >> separatable ones (row-level, largeobjects) are postponed. >> This change is extreamly large, almost same as implementing >> from zero. I think it is far from step-by-step. > > Yes, the development should be step-by-step, with minimally invasive > changes each time. The first step is just hooking into SELinux for > access levels that PostgreSQL already supports (database, schema, > table, column). Requiring that the HeapTupleHeader be changed and that > additional internal system columns be added, along with SELinux checks > at levels that aren't appropriate, is alot more than this first patch > should really need. Those kinds of changes would be with the patch to > add non-SELinux row-level security. Following that, SELinux hooks would > be added to the RLS checks. A concetn is current implementation assumes objects to be checked (tables, columns, ..., now) have its security identifier. Whole of the access control facility makes its decision based on security identifier. When the text form is used? It is used only when SE-PostgreSQL communicate with in-kernel SELinux because it is protocol. The result of decision making is cached in userspace to reduce kernel invocation, and all the entries are tagged by security identifier. So, any other facility does not need to handle text form. Removing security id also means replacement of whole of the current SE-PostgreSQL facilities! BTW, I had tried an approach which have security label by text, but its performance pain (due to string comparison) is unacceptable. (IIRC, about 50% of transaction per sec by pg_bench.) Indeed, we don't apply MAC on row-level in v8.4, so it does not make serious matter on table/column level, but is surely has to be scrapped once and implementaed from zero again. > I think you need to look at it from the point of view of the committers > and consider the whole patch, not just the quantity of code under > security/. While that is important, the changes to the code *outside* > of security/ is what the committers are going to be most concerned about > because that gets into the overall structure and design and are areas > which are much more likely to affect users who don't use this feature, > performance, etc. IIRC, we recently discussed that we should not afraid to apply core code if it tries to get merged, so, I scraped all the "PGACE" facilities. Keeping independency under "security/*" is a concept of PGACE. It enabled to implement a new security feature with minimum impact to the core code, but, we concluded it is not necessary a few days ago, then it is just before to finish most of works... Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Robert Haas wrote: > On Sat, Jan 31, 2009 at 8:32 AM, Stephen Frost <sfrost@snowman.net> wrote: >> * KaiGai Kohei (kaigai@kaigai.gr.jp) wrote: >>> Stephen Frost wrote: >>>> I think Bruce's question was where you stored the security_acl and >>>> security_label columns. Based on your response (and a bit of purusal >>>> through the code.google site), it looks like you still have security_acl >>>> and security_label defined as internal columns and being included >>>> for at least system tables (or is it everywhere?). >>> In the current working tree, it (security id) is stored within >>> padding field of HeapTupleHeader, so internal facility can read >>> it via HeapTupleGetSecLabel(), but I already removed "security_acl" >>> and "security_label" definition. >>> Its basic structure is unchanged, the text form of security label >>> is stored within pg_security system catalog. >> I'm pretty sure that structure is part of what people were unhappy about >> though, and what makes it a much more invasive change that violates >> certain levels in the system by requiring information at much lower >> levels than it had before. > > IANAC, but that's my impression too. The simplified patch shouldn't > assume that row-level security in its current form is going to end up > getting put back in. AFAICS, there's no reason why the security ID > for tables can't be a regular attribute in pg_class, or why the > security attribute for columns can't be a regular attribute in > pg_attribute. If it is "identifier", it can be compoundable. I dislike it is held as "text". It fundamentaly breaks SE-PostgreSQL's architecture, and requires to scrap near future. -- KaiGai Kohei <kaigai@kaigai.gr.jp>
>> IANAC, but that's my impression too. The simplified patch shouldn't >> assume that row-level security in its current form is going to end up >> getting put back in. AFAICS, there's no reason why the security ID >> for tables can't be a regular attribute in pg_class, or why the >> security attribute for columns can't be a regular attribute in >> pg_attribute. > > If it is "identifier", it can be compoundable. > > I dislike it is held as "text". It fundamentaly breaks SE-PostgreSQL's > architecture, and requires to scrap near future. I think the column in pg_attribute and pg_class can and should be an OID. The issue is whether it's a regular OID column or a new system column. Stephen and I are saying it should be a regular column. pg_security can stick around to map OIDs to text labels. ...Robert
Robert Haas wrote: >>> IANAC, but that's my impression too. The simplified patch shouldn't >>> assume that row-level security in its current form is going to end up >>> getting put back in. AFAICS, there's no reason why the security ID >>> for tables can't be a regular attribute in pg_class, or why the >>> security attribute for columns can't be a regular attribute in >>> pg_attribute. >> If it is "identifier", it can be compoundable. >> >> I dislike it is held as "text". It fundamentaly breaks SE-PostgreSQL's >> architecture, and requires to scrap near future. > > I think the column in pg_attribute and pg_class can and should be an > OID. The issue is whether it's a regular OID column or a new system > column. Stephen and I are saying it should be a regular column. > pg_security can stick around to map OIDs to text labels. OK, I accept to omit a facility to save security id on padding field of HeapTupleHeader *in this step*, if is has no other matter unexpected. One melancholic thing is adding a member into pg_proc. It defines more than 2000 of entries which I have to modify correctly. :( Is there any script to help it? Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
* KaiGai Kohei (kaigai@kaigai.gr.jp) wrote: > One melancholic thing is adding a member into pg_proc. > It defines more than 2000 of entries which I have to modify correctly. :( > Is there any script to help it? Not that I'm aware of.. You might be able to create an appropriate regex though, esp. if you're adding it at the end.. Stephen
KaiGai Kohei wrote: > > One melancholic thing is adding a member into pg_proc. > It defines more than 2000 of entries which I have to modify correctly. :( > Is there any script to help it? > > Last time I added a column to a large catalog, I used a perl script to help me, IIRC, but I haven't kept it. Could be a useful tool if somebody could generalize such a thing. cheers andrew
Andrew Dunstan wrote: > > > KaiGai Kohei wrote: >> >> One melancholic thing is adding a member into pg_proc. >> It defines more than 2000 of entries which I have to modify correctly. :( >> Is there any script to help it? >> >> > > Last time I added a column to a large catalog, I used a perl script to > help me, IIRC, but I haven't kept it. Could be a useful tool if somebody > could generalize such a thing. My preference is sed & awk. :) OK, I'll also do it by myself. -- KaiGai Kohei <kaigai@kaigai.gr.jp>
KaiGai Kohei wrote:
> Andrew Dunstan wrote:
>>
>>
>> KaiGai Kohei wrote:
>>>
>>> One melancholic thing is adding a member into pg_proc.
>>> It defines more than 2000 of entries which I have to modify
>>> correctly. :(
>>> Is there any script to help it?
>>>
>>>
>>
>> Last time I added a column to a large catalog, I used a perl script to
>> help me, IIRC, but I haven't kept it. Could be a useful tool if
>> somebody could generalize such a thing.
>
> My preference is sed & awk. :)
>
> OK, I'll also do it by myself.
FYI:
* revision 1468 $ diffstat sepostgresql-sepgsql-8.4devel-3-r1467.patch : 110 files changed, 9813 insertions(+), 16
deletions(-),924 modifications(!)
* current revision $ diffstat sepostgresql-sepgsql-8.4devel-3-r1492.patch : src/include/catalog/pg_attribute.h
| 507 !!! src/include/catalog/pg_class.h | 28 src/include/catalog/pg_database.h |
10src/include/catalog/pg_proc.h | 4234 !!!!!!!!!!!!!!!!!!!!!!!!!! : 68 files changed, 5302
insertions(+),2 deletions(-), 4910 modifications(!)
Don't say patch too large. :D
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
* KaiGai Kohei (kaigai@kaigai.gr.jp) wrote: > Don't say patch too large. :D Those are include files that are changing.. People do understand that adding a column to a catalog can introduce alot of changed lines. Stephen
Josh Berkus wrote: > Joshua, Kohei-san, > > So, for 8.4: *if* we included in 8.4 a version of SEPostgres with all > features *except* row-level security, would it still be useful to the > SELinux community? > > I think we're just not going to work out the headache-inducing issues > around row-level security in time for 8.4, and it seems to me that > integrated system-level security labels at the table-and-column level > are still very useful, even without row-level security. > Sorry for the delay in answering, I'm currently on vacation (I haven't been able to catch up on this thread yet either, I'll try to a little later). The answer is yes, at least to get people started using it and make sure there are no practical issues with the security model sans row access control. But as I said earlier row based access control is going to be the most compelling part so hopefully the issues everyone is having can get worked out and the community will agree on the path forward, sooner rather than later.
On 1/30/09 5:43 PM, "Josh Berkus" <josh@agliodbs.com> wrote: > Joshua, Kohei-san, > > So, for 8.4: *if* we included in 8.4 a version of SEPostgres with all > features *except* row-level security, would it still be useful to the > SELinux community? > Yes, it's definitely still useful. While many of the use cases we've wanted this for require row-level access control, there have been several that did not. It is definitely still useful, especially if it is a path toward row-level access control in a later release. Chad
Robert Haas wrote: > >> IANAC, but that's my impression too. The simplified patch shouldn't > >> assume that row-level security in its current form is going to end up > >> getting put back in. AFAICS, there's no reason why the security ID > >> for tables can't be a regular attribute in pg_class, or why the > >> security attribute for columns can't be a regular attribute in > >> pg_attribute. > > > > If it is "identifier", it can be compoundable. > > > > I dislike it is held as "text". It fundamentaly breaks SE-PostgreSQL's > > architecture, and requires to scrap near future. > > I think the column in pg_attribute and pg_class can and should be an > OID. The issue is whether it's a regular OID column or a new system > column. Stephen and I are saying it should be a regular column. > pg_security can stick around to map OIDs to text labels. Why an OID? We store acl items now without a lookup table; I think there will be at most the same number of SE-Linux entries. Also, by using text we avoid the problem of cleaning out unreferenced pg_security rows, improve performance (no lookups), and simplify the code. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > Robert Haas wrote: >>>> IANAC, but that's my impression too. The simplified patch shouldn't >>>> assume that row-level security in its current form is going to end up >>>> getting put back in. AFAICS, there's no reason why the security ID >>>> for tables can't be a regular attribute in pg_class, or why the >>>> security attribute for columns can't be a regular attribute in >>>> pg_attribute. >>> If it is "identifier", it can be compoundable. >>> >>> I dislike it is held as "text". It fundamentaly breaks SE-PostgreSQL's >>> architecture, and requires to scrap near future. >> I think the column in pg_attribute and pg_class can and should be an >> OID. The issue is whether it's a regular OID column or a new system >> column. Stephen and I are saying it should be a regular column. >> pg_security can stick around to map OIDs to text labels. > > Why an OID? We store acl items now without a lookup table; I think > there will be at most the same number of SE-Linux entries. Also, by > using text we avoid the problem of cleaning out unreferenced pg_security > rows, improve performance (no lookups), and simplify the code. The reason why I concern about text formed security context is it has variable length, so it requires to deform/form a HeapTuple again when SE-PostgreSQL assigns a default security context. If a user inserts a new tuple into pg_xxxx without explicit security context, it has to be labeled based on security context. We cannot estimate what string will be given prior to ExecInsert(), it needs to put a security label on the given HeapTuple. If is is a fixed length variable (like "oid"), it is not necessary to deform/form them. So, I prefer the security identifier. In addition, it also has performance gain. The current architecture does not need to look up pg_security in most cases. SE-PostgreSQL caches results of access controls in userspace to reduce the number of kernel invocation. (In generally, context switch is a heavy one.) All cached entries are tagged by its security identifier, so we can lookup the entry without string comparing. The text form is used only when it could not find the entry on the cache. In this case, SE-PostgreSQL translate security identifier into text form and ask for in-kernel SELinux. It requires a text form due to the protocol. At least, we cannot apply this scheme on the next phase (row-level) due to the storage consumption and others. So, I don't think it is a preferable way to design the first step without ignoring upcoming expandability. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
KaiGai Kohei wrote: > > Why an OID? We store acl items now without a lookup table; I think > > there will be at most the same number of SE-Linux entries. Also, by > > using text we avoid the problem of cleaning out unreferenced pg_security > > rows, improve performance (no lookups), and simplify the code. > > The reason why I concern about text formed security context is > it has variable length, so it requires to deform/form a HeapTuple > again when SE-PostgreSQL assigns a default security context. > If a user inserts a new tuple into pg_xxxx without explicit security > context, it has to be labeled based on security context. We cannot > estimate what string will be given prior to ExecInsert(), it needs > to put a security label on the given HeapTuple. > If is is a fixed length variable (like "oid"), it is not necessary > to deform/form them. So, I prefer the security identifier. > > In addition, it also has performance gain. > The current architecture does not need to look up pg_security in most > cases. SE-PostgreSQL caches results of access controls in userspace > to reduce the number of kernel invocation. > (In generally, context switch is a heavy one.) > All cached entries are tagged by its security identifier, so we can > lookup the entry without string comparing. The text form is used > only when it could not find the entry on the cache. In this case, > SE-PostgreSQL translate security identifier into text form and > ask for in-kernel SELinux. It requires a text form due to the > protocol. That is an interesting optimization I had not thought of. > At least, we cannot apply this scheme on the next phase (row-level) > due to the storage consumption and others. So, I don't think it is > a preferable way to design the first step without ignoring upcoming > expandability. The big problem is that the security value on system tables controls the _object_ represented by the row, while on user tables the security value represents access to the row. That is just an odd design, and why a regular system table security value makes sense, independent of the row-level security feature. FYI, it is possible we might implement row-level security a different way in 8.5. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
>> Why an OID? We store acl items now without a lookup table; I think >> there will be at most the same number of SE-Linux entries. Also, by >> using text we avoid the problem of cleaning out unreferenced pg_security >> rows, improve performance (no lookups), and simplify the code. > > In addition, it also has performance gain. > The current architecture does not need to look up pg_security in most > cases. SE-PostgreSQL caches results of access controls in userspace I think this is a very compelling point. ...Robert
Bruce Momjian wrote:
>> At least, we cannot apply this scheme on the next phase (row-level)
>> due to the storage consumption and others. So, I don't think it is
>> a preferable way to design the first step without ignoring upcoming
>> expandability.
>
> The big problem is that the security value on system tables controls the
> _object_ represented by the row, while on user tables the security value
> represents access to the row. That is just an odd design, and why a
> regular system table security value makes sense, independent of the
> row-level security feature.
I don't think there is a fundamental differences between "ALTER FUNCTION"
and "UPDATE pg_proc SET ...", for example. It is necessary to apply
same privileges in this case.
(In this case, db_procedure:{setattr} is checked on the object.)
The security label of system catalogs (like pg_class, pg_proc, ...) are
also used when the objects are used as target of user's request, like
a target of SELECT statement, a target of function invocation.
Please note that different permissions are checked in this case.
(db_table:{select} and db_procedure:{execute})
Sorry, it is a bit unclear what is a problem you pointed out.
I guessed you concerned about a tuple (within system catalogs) is handled
as an object when user tries to modify the system catalogs by hand.
However, I cannot understand why it is an odd design.
If we keep free to update system catalogs, it makes a bypassable
route to create/alter/drop objects.
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
Bruce Momjian wrote: > KaiGai Kohei wrote: >>> Why an OID? We store acl items now without a lookup table; I think >>> there will be at most the same number of SE-Linux entries. Also, by >>> using text we avoid the problem of cleaning out unreferenced pg_security >>> rows, improve performance (no lookups), and simplify the code. >> The reason why I concern about text formed security context is >> it has variable length, so it requires to deform/form a HeapTuple >> again when SE-PostgreSQL assigns a default security context. >> If a user inserts a new tuple into pg_xxxx without explicit security >> context, it has to be labeled based on security context. We cannot >> estimate what string will be given prior to ExecInsert(), it needs >> to put a security label on the given HeapTuple. >> If is is a fixed length variable (like "oid"), it is not necessary >> to deform/form them. So, I prefer the security identifier. >> >> In addition, it also has performance gain. >> The current architecture does not need to look up pg_security in most >> cases. SE-PostgreSQL caches results of access controls in userspace >> to reduce the number of kernel invocation. >> (In generally, context switch is a heavy one.) >> All cached entries are tagged by its security identifier, so we can >> lookup the entry without string comparing. The text form is used >> only when it could not find the entry on the cache. In this case, >> SE-PostgreSQL translate security identifier into text form and >> ask for in-kernel SELinux. It requires a text form due to the >> protocol. > > That is an interesting optimization I had not thought of. > Just as an FYI, SELinux does this in general. There is an access vector cache in the kernel that caches the access computations, and there is also a userspace implementation in libselinux that most apps use. KaiGai reimplemented the AVC because he wanted it to work in a shm and be shared by multiple postgres processes. There is also a sidtab which is just a hashtable that maps string contexts to sids. The sidtab is filled at runtime and not persistent across boots, which means the contexts are generally stored as text on the persistent medium (like the xattr's on the filesystem). It doesn't matter from a security perspective whether the contexts are stored as strings or sids, its just an optimization you guys need to work out. >> At least, we cannot apply this scheme on the next phase (row-level) >> due to the storage consumption and others. So, I don't think it is >> a preferable way to design the first step without ignoring upcoming >> expandability. > > The big problem is that the security value on system tables controls the > _object_ represented by the row, while on user tables the security value > represents access to the row. That is just an odd design, and why a > regular system table security value makes sense, independent of the > row-level security feature. > I may not be understanding this but I don't see why. In SELinux everything is an object, and all objects have contexts. No access is specified on the object or in the context, that is all done in the policy currently loaded in the security server. system tables and user tables shouldn't be treated differently implementation wise, they should just have a context and defer the decision making to the policy. In practice the system tables (and rows within the tables) would have a context that restricts access tightly, but this is up to the policy, not the implementation. > FYI, it is possible we might implement row-level security a different > way in 8.5.
Joshua Brindle wrote: > > The big problem is that the security value on system tables controls the > > _object_ represented by the row, while on user tables the security value > > represents access to the row. That is just an odd design, and why a > > regular system table security value makes sense, independent of the > > row-level security feature. > > > > I may not be understanding this but I don't see why. In SELinux everything is an > object, and all objects have contexts. No access is specified on the object or > in the context, that is all done in the policy currently loaded in the security > server. system tables and user tables shouldn't be treated differently > implementation wise, they should just have a context and defer the decision > making to the policy. > > In practice the system tables (and rows within the tables) would have a context > that restricts access tightly, but this is up to the policy, not the implementation. > > > FYI, it is possible we might implement row-level security a different > > way in 8.5. Seeing a pg_attribute row and seeing the column referenced by the row are not the same thing. Also, we are discussing system catalog values, (table, column, function), etc, so I don't see an performance issue. I haven't heard of anyone complaining about our ACL parsing overhead recently. A cache could still be used, but on the text string, not the oid. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote:
> Joshua Brindle wrote:
>>> The big problem is that the security value on system tables controls the
>>> _object_ represented by the row, while on user tables the security value
>>> represents access to the row. That is just an odd design, and why a
>>> regular system table security value makes sense, independent of the
>>> row-level security feature.
>>>
>> I may not be understanding this but I don't see why. In SELinux everything is an
>> object, and all objects have contexts. No access is specified on the object or
>> in the context, that is all done in the policy currently loaded in the security
>> server. system tables and user tables shouldn't be treated differently
>> implementation wise, they should just have a context and defer the decision
>> making to the policy.
>>
>> In practice the system tables (and rows within the tables) would have a context
>> that restricts access tightly, but this is up to the policy, not the implementation.
>>
>>> FYI, it is possible we might implement row-level security a different
>>> way in 8.5.
>
> Seeing a pg_attribute row and seeing the column referenced by the row
> are not the same thing.
Yes, it is quite different. It seems to me we are now confusing.
Are you saying: a) SELECT attname FROM pg_attribute where attrelid='t'::regclass and attname='a'; and b) SELECT a FROM
t;
are different, aren't you?
Yes, it is not same thing.
For the query a), SE-PostgreSQL should check db_column:{getattr} permission
on the selected tuples, when row-level security is available.
In this case, it also checks db_column:{select} permission on the
"attname" column and db_table:{select} on the "pg_attribute" table.
For the query b), SE-PostgreSQL checks db_column:{select} permission on
the column "a", and it also checks db_table:{select} on the table "t".
And db_tuple:{select} permission when row-level security is available.
Please note that it checks db_column:* class permission on tuples within
pg_attribute system catalog, although db_tuple:* class ones are applied
on user defined tables.
When it checks permission of column, for example, it requires a label
assigned to the target object. In this case, an object is a row within
pg_attribute system catalog. It needs to be labeled as a column.
Thus, we have to add a field to hold its security label within pg_attribute
system catalog.
My concern is INSERT/UPDATE/DELETE these system catalogs by hand.
When user tries to insert a tuple without explicit security context,
it is necessary to be labeled as default one.
But, if it has variable length form, we have to deform the given
HeapTuple once then form HeapTuple again with text variable.
If it has fixed length oid, we can put it directly, as "oid" doing
at heap_insert() or heap_update().
> Also, we are discussing system catalog values, (table, column,
> function), etc, so I don't see an performance issue. I haven't heard of
> anyone complaining about our ACL parsing overhead recently. A cache
> could still be used, but on the text string, not the oid.
Yes, performance is not the first issue here.
The variable length type makes hard to assign a newly inserted tuple
(into pg_class, etc...) a default security context.
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
If we add a field on pg_xxxx to store security label in text form,
it is necessary to attach a default one at the following points.
* pg_class - InsertPgClassTuple() at heap.c
* pg_attribute - InsertPgAttributeTuple() at heap.c
* pg_proc - ProcedureCreate() at pg_proc.c
* pg_database - createdb() at dbcommands.c
* for whole of them - InsertOneTuple() at bootstrap.c - ExecInsert() at execMain.c
The reason why I prefer security identifier ("oid") is that we can put
a hook to assign a default security context inside the simple_heap_insert().
But, if above functions are the all to insert a new tuple into these
issued relations, it may be a reasonable approach for those four system
catalogs. Please point out if I overlooks somewhere.
At the previous message, I noted I'll submit revised patches on thie
Wednesday, but it become impossible due to the change. (T-T)
Please wait for a while.
KaiGai Kohei wrote:
> Bruce Momjian wrote:
>> Joshua Brindle wrote:
>>>> The big problem is that the security value on system tables controls
>>>> the
>>>> _object_ represented by the row, while on user tables the security
>>>> value
>>>> represents access to the row. That is just an odd design, and why a
>>>> regular system table security value makes sense, independent of the
>>>> row-level security feature.
>>>>
>>> I may not be understanding this but I don't see why. In SELinux
>>> everything is an object, and all objects have contexts. No access is
>>> specified on the object or in the context, that is all done in the
>>> policy currently loaded in the security server. system tables and
>>> user tables shouldn't be treated differently implementation wise,
>>> they should just have a context and defer the decision making to the
>>> policy.
>>>
>>> In practice the system tables (and rows within the tables) would have
>>> a context that restricts access tightly, but this is up to the
>>> policy, not the implementation.
>>>
>>>> FYI, it is possible we might implement row-level security a different
>>>> way in 8.5.
>>
>> Seeing a pg_attribute row and seeing the column referenced by the row
>> are not the same thing.
>
> Yes, it is quite different. It seems to me we are now confusing.
>
> Are you saying:
> a) SELECT attname FROM pg_attribute where attrelid='t'::regclass and
> attname='a';
> and
> b) SELECT a FROM t;
> are different, aren't you?
>
> Yes, it is not same thing.
>
> For the query a), SE-PostgreSQL should check db_column:{getattr} permission
> on the selected tuples, when row-level security is available.
> In this case, it also checks db_column:{select} permission on the
> "attname" column and db_table:{select} on the "pg_attribute" table.
>
> For the query b), SE-PostgreSQL checks db_column:{select} permission on
> the column "a", and it also checks db_table:{select} on the table "t".
> And db_tuple:{select} permission when row-level security is available.
>
> Please note that it checks db_column:* class permission on tuples within
> pg_attribute system catalog, although db_tuple:* class ones are applied
> on user defined tables.
>
> When it checks permission of column, for example, it requires a label
> assigned to the target object. In this case, an object is a row within
> pg_attribute system catalog. It needs to be labeled as a column.
> Thus, we have to add a field to hold its security label within pg_attribute
> system catalog.
>
> My concern is INSERT/UPDATE/DELETE these system catalogs by hand.
> When user tries to insert a tuple without explicit security context,
> it is necessary to be labeled as default one.
> But, if it has variable length form, we have to deform the given
> HeapTuple once then form HeapTuple again with text variable.
> If it has fixed length oid, we can put it directly, as "oid" doing
> at heap_insert() or heap_update().
>
>> Also, we are discussing system catalog values, (table, column,
>> function), etc, so I don't see an performance issue. I haven't heard of
>> anyone complaining about our ACL parsing overhead recently. A cache
>> could still be used, but on the text string, not the oid.
>
> Yes, performance is not the first issue here.
> The variable length type makes hard to assign a newly inserted tuple
> (into pg_class, etc...) a default security context.
>
> Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
The following patches are updated ones: [1/5] http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r1522.patch [2/5] http://sepgsql.googlecode.com/files/sepostgresql-utils-8.4devel-3-r1522.patch [3/5] http://sepgsql.googlecode.com/files/sepostgresql-policy-8.4devel-3-r1522.patch [4/5] http://sepgsql.googlecode.com/files/sepostgresql-docs-8.4devel-3-r1522.patch [5/5] http://sepgsql.googlecode.com/files/sepostgresql-tests-8.4devel-3-r1522.patch - List of updates: * The facilities of PGACE are removed. * The facilities of row-level access controls are separated. *The facilities of security attribute management are separated. - The pg_security system catalog, the idea of securityidentifier and the "security_label" system column are included. - AVC become to accept text form security context. - pg_class, pg_attribute, pg_database and pg_proc got a new field to store text form security context. * Afew of security hooks are integrated into pg_xxx_aclcheck() - sepgsqlCheckProcedureExecute() from pg_proc_aclmask() - sepgsqlCheckDatabaseAccess() from pg_database_aclmask() * Access controls on large objects are separated. * The baselinesecurity policy module is omitted, so the 3rd patch provides only developer's policy. * Descriptions about PGACEand row-level access controls are separated. * Testcases are reworked. * Anyway, most of patches are reworked! - Scale of patches It may seem you the updated version is not smaller than previous version, but more than half of affectedlines are come from changes in system catalog. * The previous full-functional version (r1467) $ diffstat sepostgresql-sepgsql-8.4devel-3-r1467.patch : 110 fileschanged, 9813 insertions(+), 16 deletions(-), 924 modifications(!) * Current version (r1522) $ diffstat sepostgresql-sepgsql-8.4devel-3-r1522.patch : src/include/catalog/pg_attribute.h | 500 !!! src/include/catalog/pg_class.h | 12 src/include/catalog/pg_database.h | 6 src/include/catalog/pg_proc.h | 4207 !!!!!!!!!!!!!!!!!!!!!!!!!! : 65 files changed, 4737 insertions(+), 11 deletions(-), 4908 modifications(!) Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
KaiGai Kohei escribió: > One melancholic thing is adding a member into pg_proc. > It defines more than 2000 of entries which I have to modify correctly. :( > Is there any script to help it? Try a search for "coccinelle", "sdiff", or was it "spatch"? It got featured on http://LWN.net/ not many weeks ago. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes: > KaiGai Kohei escribi�: >> One melancholic thing is adding a member into pg_proc. >> It defines more than 2000 of entries which I have to modify correctly. :( >> Is there any script to help it? > Try a search for "coccinelle", "sdiff", or was it "spatch"? It got > featured on http://LWN.net/ not many weeks ago. FWIW, every single time I've had to add a column to pg_proc (and I've done it several times now), an Emacs macro got the job done with a few minutes' thought. I'm sure sed would work as well. The contents of those DATA lines are really pretty stylized. regards, tom lane
Tom Lane escribió: > Alvaro Herrera <alvherre@commandprompt.com> writes: > > KaiGai Kohei escribi�: > >> One melancholic thing is adding a member into pg_proc. > >> It defines more than 2000 of entries which I have to modify correctly. :( > >> Is there any script to help it? > > > Try a search for "coccinelle", "sdiff", or was it "spatch"? It got > > featured on http://LWN.net/ not many weeks ago. Here it is: http://lwn.net/Articles/315686/ > FWIW, every single time I've had to add a column to pg_proc (and I've > done it several times now), an Emacs macro got the job done with a few > minutes' thought. I'm sure sed would work as well. The contents of > those DATA lines are really pretty stylized. I've had to do it only once, but yes, I did it with a simple s// command in Vim.
The series of SE-PostgreSQL patches are updated: [1/5] http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r1530.patch [2/5] http://sepgsql.googlecode.com/files/sepostgresql-utils-8.4devel-3-r1530.patch [3/5] http://sepgsql.googlecode.com/files/sepostgresql-policy-8.4devel-3-r1530.patch [4/5] http://sepgsql.googlecode.com/files/sepostgresql-docs-8.4devel-3-r1530.patch [5/5] http://sepgsql.googlecode.com/files/sepostgresql-tests-8.4devel-3-r1530.patch - List of updates: * These are rebased to the latest CVS HEAD because of conflictions. - The src/include/catalog/pg_proc.h got a conflictiondue to the newly added SQL functions. - The src/bin/pg_dump/pg_dump.c got a confliction due to the stuff to dump "toast_reloptions". * bugfix: An incorrect procedure entry for sepgsql_server_getcon(). * cleanup: A strange error message in testcases. Rest of parts are unchanged. Please comment anything. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
KaiGai Kohei wrote: > The series of SE-PostgreSQL patches are updated: > [1/5] http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r1530.patch > [2/5] http://sepgsql.googlecode.com/files/sepostgresql-utils-8.4devel-3-r1530.patch > [3/5] http://sepgsql.googlecode.com/files/sepostgresql-policy-8.4devel-3-r1530.patch > [4/5] http://sepgsql.googlecode.com/files/sepostgresql-docs-8.4devel-3-r1530.patch > [5/5] http://sepgsql.googlecode.com/files/sepostgresql-tests-8.4devel-3-r1530.patch BTW, what is the current status of revewing the patches? Is it necessary to wait for a few days more? If you have anything unclear, please feel free to ask me anything. Thanks, > - List of updates: > * These are rebased to the latest CVS HEAD because of conflictions. > - The src/include/catalog/pg_proc.h got a confliction due to the > newly added SQL functions. > - The src/bin/pg_dump/pg_dump.c got a confliction due to the stuff > to dump "toast_reloptions". > * bugfix: An incorrect procedure entry for sepgsql_server_getcon(). > * cleanup: A strange error message in testcases. > > Rest of parts are unchanged. > > Please comment anything. > > Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
KaiGai Kohei wrote: > KaiGai Kohei wrote: >> The series of SE-PostgreSQL patches are updated: >> [1/5] >> http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r1530.patch >> >> [2/5] >> http://sepgsql.googlecode.com/files/sepostgresql-utils-8.4devel-3-r1530.patch >> >> [3/5] >> http://sepgsql.googlecode.com/files/sepostgresql-policy-8.4devel-3-r1530.patch >> >> [4/5] >> http://sepgsql.googlecode.com/files/sepostgresql-docs-8.4devel-3-r1530.patch >> >> [5/5] >> http://sepgsql.googlecode.com/files/sepostgresql-tests-8.4devel-3-r1530.patch >> > > BTW, what is the current status of revewing the patches? > Is it necessary to wait for a few days more? > > If you have anything unclear, please feel free to ask me anything. > Yes, what was the decision about 8.4? Is this going to make it in? > Thanks, > >> - List of updates: >> * These are rebased to the latest CVS HEAD because of conflictions. >> - The src/include/catalog/pg_proc.h got a confliction due to the >> newly added SQL functions. >> - The src/bin/pg_dump/pg_dump.c got a confliction due to the stuff >> to dump "toast_reloptions". >> * bugfix: An incorrect procedure entry for sepgsql_server_getcon(). >> * cleanup: A strange error message in testcases. >> >> Rest of parts are unchanged. >> >> Please comment anything. >> >> Thanks, >
On Fri, Feb 13, 2009 at 9:07 AM, Joshua Brindle <method@manicmethod.com> wrote: > KaiGai Kohei wrote: >> >> KaiGai Kohei wrote: >>> >>> The series of SE-PostgreSQL patches are updated: >>> [1/5] >>> http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r1530.patch >>> [2/5] >>> http://sepgsql.googlecode.com/files/sepostgresql-utils-8.4devel-3-r1530.patch >>> [3/5] >>> http://sepgsql.googlecode.com/files/sepostgresql-policy-8.4devel-3-r1530.patch >>> [4/5] >>> http://sepgsql.googlecode.com/files/sepostgresql-docs-8.4devel-3-r1530.patch >>> [5/5] >>> http://sepgsql.googlecode.com/files/sepostgresql-tests-8.4devel-3-r1530.patch >> >> BTW, what is the current status of revewing the patches? >> Is it necessary to wait for a few days more? >> >> If you have anything unclear, please feel free to ask me anything. >> > > Yes, what was the decision about 8.4? Is this going to make it in? > can you try the functional parts of it? ie: compile with the patch with --enable-selinux and test if the patch does wath you expect? i will try it but i have to install a VM to install selinux on it... then i will try some cases... can you give me an example of a typical scenario to make those tests? i will do some code review too but i'm not a hacker so you will need to wait for them to review it anyway... -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157
Jaime Casanova wrote: > On Fri, Feb 13, 2009 at 9:07 AM, Joshua Brindle <method@manicmethod.com> wrote: >> KaiGai Kohei wrote: >>> KaiGai Kohei wrote: >>>> The series of SE-PostgreSQL patches are updated: >>>> [1/5] >>>> http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r1530.patch >>>> [2/5] >>>> http://sepgsql.googlecode.com/files/sepostgresql-utils-8.4devel-3-r1530.patch >>>> [3/5] >>>> http://sepgsql.googlecode.com/files/sepostgresql-policy-8.4devel-3-r1530.patch >>>> [4/5] >>>> http://sepgsql.googlecode.com/files/sepostgresql-docs-8.4devel-3-r1530.patch >>>> [5/5] >>>> http://sepgsql.googlecode.com/files/sepostgresql-tests-8.4devel-3-r1530.patch >>> BTW, what is the current status of revewing the patches? >>> Is it necessary to wait for a few days more? >>> >>> If you have anything unclear, please feel free to ask me anything. >>> >> Yes, what was the decision about 8.4? Is this going to make it in? >> > > can you try the functional parts of it? ie: compile with the patch > with --enable-selinux and test if the patch does wath you expect? > > i will try it but i have to install a VM to install selinux on it... > then i will try some cases... can you give me an example of a typical > scenario to make those tests? If you can help to test the patches, I recommend you to install Fedora 10 on your VM images, because it includes SELinux in the default and its default security policy (selinux-policy-targeted) also supports SE-PostgreSQL. Then, could you try the following steps? 1) installation $ ./configure --enable-selinux $ make $ make -C src/backend/security/sepgsql/policy(NOTE: We provide a policymodule for development purpose) $ su # make install # /usr/sbin/semodule -i src/backend/security/sepgsql/policy/sepostgresql-devel.pp(NOTE:It installs the development policy) # /sbin/restorecon -R/usr/local/pgsql(NOTE: It assigns correct security context for installed binaries) $ export PGDATA=/path/to/database $chcon -t postgresql_db_t -R $PGDATA(NOTE: It assigns correct security context for database files) $ initdb --enable-selinux(NOTE:--enable-selinux turns on SE-PostgreSQL feature) $ pg_ctl start 2) check installation 2-1) Please confirm SE-PostgreSQL works $ psql postgres psql (8.4devel) Type "help" for help. postgres=# SHOW sepostgresql; sepostgresql -------------- on (1 row) 2-2) Please confirm client's privileges $ id -Z unconfined_u:unconfined_r:unconfined_t $ psql postgres psql (8.4devel) Type "help" for help. postgres=# SELECT sepgsql_getcon(); sepgsql_getcon ---------------------------------------- unconfined_u:unconfined_r:unconfined_t (1 row) NOTE: It has to be matched with privileges on OS. 2-3) Please confirm server's privileges postgres=# SELECT sepgsql_server_getcon(); sepgsql_server_getcon ------------------------------------ unconfined_u:system_r:postgresql_t (1 row) NOTE: It is necessary restricted domain (like PHP scripts) to connect PostgreSQL server process. 2-4) Please confirm to connect from restricted domain $ runcon -t sepgsql_test_t -- psql postgres psql (8.4devel) Type "help" for help. postgres=# SELECT sepgsql_getcon(); sepgsql_getcon ------------------------------------------ unconfined_u:unconfined_r:sepgsql_test_t (1 row) NOTE: The "sepgsql_test_t" has restricted privileges same as PHP scripts invoked from Apache web server. NOTE:If SELinux denied to connect, please try the following command (in root): # setsebool -P allow_user_postgresql_connect1 3) Example of a typical scenario 3-1) Setup of column level access controls postgres=# CREATE TABLE customer ( cid int primary key, cname text, credit varchar(32) SECURITY_LABEL = 'system_u:object_r:sepgsql_secret_table_t:s0' ); NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "customer_pkey"for table "customer" CREATE TABLE postgres=# INSERT INTO customer VALUES (1, 'kaigai', '1111-2222-3333-4444'), (2, 'yamada', '5555-6666-7777-8888'), (3, 'kimura', '9999-0000-1234-5678'); INSERT 0 3 postgres=# SELECT * FROM customer; cid | cname | credit -----+--------+--------------------- 1 | kaigai | 1111-2222-3333-4444 2 | yamada | 5555-6666-7777-8888 3 | kimura | 9999-0000-1234-5678 (3 rows) postgres=# CREATE OR REPLACE FUNCTION show_credit (int) RETURNS text LANGUAGE 'sql' SECURITY_LABEL = 'system_u:object_r:sepgsql_trusted_proc_exec_t:s0' AS 'SELECT regexp_replace(credit, ''-[0-9]+'', ''-xxxx'', ''g'') FROMcustomer WHERE cid = $1'; CREATE FUNCTION 3-2) Example of column level access controls $ runcon -t sepgsql_test_t -- psql postgres psql (8.4devel) Type "help"for help. postgres=# SELECT * FROM customer; ERROR: SELinux: denied { select } scontext=unconfined_u:unconfined_r:sepgsql_test_ttcontext=system_u:object_r:sepgsql_secret_table_t tclass=db_column name=customer.credit(NOTE:SE-PostgreSQL prevent restricted domain to select a column labeled as 'sepgsql_secret_table_t') postgres=# SELECT cid, cname FROM customer; cid | cname -----+-------- 1 | kaigai 2| yamada 3 | kimura (3 rows) postgres=# SELECT cid, cname, show_credit(cid) FROM customer; cid | cname | show_credit -----+--------+--------------------- 1 | kaigai | 1111-xxxx-xxxx-xxxx 2 | yamada | 5555-xxxx-xxxx-xxxx 3 | kimura| 9999-xxxx-xxxx-xxxx (3 rows)(NOTE: The show_credit() is labeled as 'sepgsql_trusted_proc_exec_t', it enables to switch client privilege during the function running.)(NOTE: Please note that sepgsql_test_t has same privileges withPHP script invoked from web servers, so it means PHP script cannot show "customer.credit" directly.) Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On Fri, Feb 13, 2009 at 8:32 PM, KaiGai Kohei <kaigai@kaigai.gr.jp> wrote: >> > If you can help to test the patches, I recommend you to install Fedora 10 > on your VM images, because it includes SELinux in the default and its > default security policy (selinux-policy-targeted) also supports > SE-PostgreSQL. > i only have ubuntu at hand, and i can install selinux from repositories... do you see any problem with that? i'm creating the VM now... will test on weekend... -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157
Jaime Casanova wrote: > On Fri, Feb 13, 2009 at 8:32 PM, KaiGai Kohei <kaigai@kaigai.gr.jp> wrote: >> If you can help to test the patches, I recommend you to install Fedora 10 >> on your VM images, because it includes SELinux in the default and its >> default security policy (selinux-policy-targeted) also supports >> SE-PostgreSQL. >> > > i only have ubuntu at hand, and i can install selinux from > repositories... do you see any problem with that? > i'm creating the VM now... will test on weekend... Now I checked the repository of Ubuntu, however, I can find several matters to build/run SE-PostgreSQL. - The default security policy is quite old. Is does not contain SE-PostgreSQL support which is merged at the refpolicy-20080702. - The libselinux is a bit old. It does not contains several symbols required by SE-PostgreSQL, so it is not possible tocompile. If you set up a VM from scratch, Fedora enables to install via remote repository using minimum bootable image. http://download.fedora.redhat.com/pub/fedora/linux/releases/10/Fedora/i386/iso/ I don't think it is not a essential for you to resolve troubles related to SELinux on Ubuntu. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
KaiGai Kohei wrote: > If you set up a VM from scratch, Fedora enables to install > via remote repository using minimum bootable image. > > http://download.fedora.redhat.com/pub/fedora/linux/releases/10/Fedora/i386/iso/ > > > I don't think it is not a essential for you to resolve troubles > related to SELinux on Ubuntu. Oops, s/I don't think/I think/g Sorry, I often misuse negative sentences. :( -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On Sat, Feb 14, 2009 at 12:46 AM, KaiGai Kohei <kaigai@kaigai.gr.jp> wrote: > > Now I checked the repository of Ubuntu, however, I can find > several matters to build/run SE-PostgreSQL. > - The default security policy is quite old. > Is does not contain SE-PostgreSQL support which is merged > at the refpolicy-20080702. > - The libselinux is a bit old. It does not contains several > symbols required by SE-PostgreSQL, so it is not possible > to compile. > so we need to state pre-requisites in docs, maybe make configure to check the version of libselinux, haven't looked is the patch already does that... > If you set up a VM from scratch, Fedora enables to install > via remote repository using minimum bootable image. > http://download.fedora.redhat.com/pub/fedora/linux/releases/10/Fedora/i386/iso/ > ok, i'm going to download the image -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157
On 2/14/09, Jaime Casanova <jcasanov@systemguards.com.ec> wrote: > On Sat, Feb 14, 2009 at 12:46 AM, KaiGai Kohei <kaigai@kaigai.gr.jp> wrote: >> > >> If you set up a VM from scratch, Fedora enables to install >> via remote repository using minimum bootable image. >> >> http://download.fedora.redhat.com/pub/fedora/linux/releases/10/Fedora/i386/iso/ >> > > ok, i'm going to download the image > 5 days later... i'm having troubles with my home internet conection (that's a usual problem when living in a third-world country ;) i think the problem was solved so i will try to start the download again... or will have to make some other tricks for getting it... but i will make these tests starting saturday (we are in holydays until tuesday and don't think i will go to beach) -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157
The series of SE-PostgreSQL patches for v8.4 are updated: [1/5] http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r1590.patch [2/5] http://sepgsql.googlecode.com/files/sepostgresql-utils-8.4devel-3-r1590.patch [3/5] http://sepgsql.googlecode.com/files/sepostgresql-policy-8.4devel-3-r1590.patch [4/5] http://sepgsql.googlecode.com/files/sepostgresql-docs-8.4devel-3-r1590.patch [5/5] http://sepgsql.googlecode.com/files/sepostgresql-tests-8.4devel-3-r1590.patch - List of updates: * These are rebased to the latest CVS HEAD due to conflictions. * bugfix: incorrect columns were checkedon trigger invocations as FK checks. Rest of parts are unchanged. I guess the reviewer (Bruce?) is in mid-flow of reviewing the patches. If it is not comprehensive yet, any comments will help to improve the code. Could you tell me what is the current status? Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
The series of SE-PostgreSQL patches for v8.4 were updated: [1/5] http://sepgsql.googlecode.com/files/sepgsql-core-8.4devel-r1608.patch [2/5] http://sepgsql.googlecode.com/files/sepgsql-utils-8.4devel-r1608.patch [3/5] http://sepgsql.googlecode.com/files/sepgsql-policy-8.4devel-r1608.patch [4/5] http://sepgsql.googlecode.com/files/sepgsql-docs-8.4devel-r1608.patch [5/5] http://sepgsql.googlecode.com/files/sepgsql-tests-8.4devel-r1608.patch - List of updates: * bugfix: sepgsqlCheckProcedureEntrypoint() was invoked twice when security invoker functions are invoked. Rest of parts are unchanged. Don't mind contracted filename. Please comment anything. It will help to improve our code. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
The series of SE-PostgreSQL patches for v8.4 were updated: [1/5] http://sepgsql.googlecode.com/files/sepgsql-core-8.4devel-r1627.patch [2/5] http://sepgsql.googlecode.com/files/sepgsql-utils-8.4devel-r1627.patch [3/5] http://sepgsql.googlecode.com/files/sepgsql-policy-8.4devel-r1627.patch [4/5] http://sepgsql.googlecode.com/files/sepgsql-docs-8.4devel-r1627.patch [5/5] http://sepgsql.googlecode.com/files/sepgsql-tests-8.4devel-r1627.patch - List of updates: * It is rebased to the latest CVS HEAD. - Adding a new funcction into pg_proc.h made a patch confliction. - Changing usage messages at initdb made a patch confliction. * sepgsqlCheckDatabaseInstallModule() is removedfrom commands/foreigncmds.c because CreateFdwStmt->library has gone in the recent changes. Rest of parts are unchanged, so don't consider this updates needs to review whole of patches again, please. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
On Wed, Feb 25, 2009 at 11:04 PM, KaiGai Kohei <kaigai@ak.jp.nec.com> wrote: > The series of SE-PostgreSQL patches for v8.4 were updated: > > [1/5] http://sepgsql.googlecode.com/files/sepgsql-core-8.4devel-r1627.patch > [2/5] http://sepgsql.googlecode.com/files/sepgsql-utils-8.4devel-r1627.patch > [3/5] > http://sepgsql.googlecode.com/files/sepgsql-policy-8.4devel-r1627.patch > [4/5] http://sepgsql.googlecode.com/files/sepgsql-docs-8.4devel-r1627.patch > [5/5] http://sepgsql.googlecode.com/files/sepgsql-tests-8.4devel-r1627.patch > > - List of updates: > * It is rebased to the latest CVS HEAD. actually i see fails when trying to apply sepgsql-core-8.4devel-r1627.patch to head (in pg_proc.h)... """ Hunk #4 FAILED at 113. 1 out of 4 hunks FAILED -- saving rejects to file src/include/catalog/pg_proc.h.rej """ -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157
Jaime Casanova wrote: > On Wed, Feb 25, 2009 at 11:04 PM, KaiGai Kohei <kaigai@ak.jp.nec.com> wrote: >> The series of SE-PostgreSQL patches for v8.4 were updated: >> >> [1/5] http://sepgsql.googlecode.com/files/sepgsql-core-8.4devel-r1627.patch >> [2/5] http://sepgsql.googlecode.com/files/sepgsql-utils-8.4devel-r1627.patch >> [3/5] >> http://sepgsql.googlecode.com/files/sepgsql-policy-8.4devel-r1627.patch >> [4/5] http://sepgsql.googlecode.com/files/sepgsql-docs-8.4devel-r1627.patch >> [5/5] http://sepgsql.googlecode.com/files/sepgsql-tests-8.4devel-r1627.patch >> >> - List of updates: >> * It is rebased to the latest CVS HEAD. > > actually i see fails when trying to apply > sepgsql-core-8.4devel-r1627.patch to head (in pg_proc.h)... > """ > Hunk #4 FAILED at 113. > 1 out of 4 hunks FAILED -- saving rejects to file > src/include/catalog/pg_proc.h.rej > """ I'll check it soon, please wait for a while. Thanks for your interesting! -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
KaiGai Kohei wrote: > Jaime Casanova wrote: >>> - List of updates: >>> * It is rebased to the latest CVS HEAD. >> >> actually i see fails when trying to apply >> sepgsql-core-8.4devel-r1627.patch to head (in pg_proc.h)... >> """ >> Hunk #4 FAILED at 113. >> 1 out of 4 hunks FAILED -- saving rejects to file >> src/include/catalog/pg_proc.h.rej >> """ > > I'll check it soon, please wait for a while. I could not reproduce this patch confliction you reported. Could you confirm whether the base tree is updated to the latest one, or not? The previous revision (r1608) conflicts to this commit, so it is necessary the base tree to be updated in this two days. http://git.postgresql.org/?p=postgresql.git;a=commitdiff;h=a3c594d20e6e713c2bed8e0ba416ab34cdec8ecf#patch24 [kaigai@masu tmp]$ cvs -z3 -d :pserver:anoncvs@anoncvs.postgresql.org:/projects/cvsroot \ export-r HEAD -d pgsql.cvs pgsql [kaigai@masu tmp]$ wget http://sepgsql.googlecode.com/files/sepgsql-core-8.4devel-r1627.patch [kaigai@masu tmp]$ cd pgsql.cvs/ [kaigai@masu pgsql.cvs]$ cat ../sepgsql-core-8.4devel-r1627.patch | patch -p1 patching file configure : patching file src/include/catalog/pg_database.h patching file src/include/catalog/pg_proc.h <---- (*) patching file src/include/catalog/pg_proc_fn.h : patching file src/include/utils/syscache.h [kaigai@masu pgsql.cvs]$ Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
On Thu, Feb 26, 2009 at 1:46 AM, KaiGai Kohei <kaigai@ak.jp.nec.com> wrote: > KaiGai Kohei wrote: >> >> Jaime Casanova wrote: >>>> >>>> - List of updates: >>>> * It is rebased to the latest CVS HEAD. >>> >>> actually i see fails when trying to apply >>> sepgsql-core-8.4devel-r1627.patch to head (in pg_proc.h)... >>> """ >>> Hunk #4 FAILED at 113. >>> 1 out of 4 hunks FAILED -- saving rejects to file >>> src/include/catalog/pg_proc.h.rej >>> """ >> >> I'll check it soon, please wait for a while. > > I could not reproduce this patch confliction you reported. > Could you confirm whether the base tree is updated to the latest one, or > not? > > The previous revision (r1608) conflicts to this commit, so it is necessary > the base tree to be updated in this two days. > ah! please forget it... it's to late here... i updated my local repo but doesn't execute the cvs update -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157
KaiGai Kohei wrote: > The series of SE-PostgreSQL patches for v8.4 were updated: > [1/5] http://sepgsql.googlecode.com/files/sepgsql-core-8.4devel-r1608.patch > [2/5] http://sepgsql.googlecode.com/files/sepgsql-utils-8.4devel-r1608.patch > [3/5] http://sepgsql.googlecode.com/files/sepgsql-policy-8.4devel-r1608.patch > [4/5] http://sepgsql.googlecode.com/files/sepgsql-docs-8.4devel-r1608.patch > [5/5] http://sepgsql.googlecode.com/files/sepgsql-tests-8.4devel-r1608.patch > > - List of updates: > * bugfix: sepgsqlCheckProcedureEntrypoint() was invoked twice when > security invoker functions are invoked. > > Rest of parts are unchanged. Don't mind contracted filename. > Please comment anything. It will help to improve our code. I did an analysis of the "core" file: http://sepgsql.googlecode.com/files/sepgsql-core-8.4devel-r1608.patch changed lines 3226new files 4075syscatalog 9977----total 17278 The good news is that 3226 is the affect on the non-system-catalog main core code, and is a context diff size, not total changed lines. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > KaiGai Kohei wrote: >> The series of SE-PostgreSQL patches for v8.4 were updated: >> [1/5] http://sepgsql.googlecode.com/files/sepgsql-core-8.4devel-r1608.patch >> [2/5] http://sepgsql.googlecode.com/files/sepgsql-utils-8.4devel-r1608.patch >> [3/5] http://sepgsql.googlecode.com/files/sepgsql-policy-8.4devel-r1608.patch >> [4/5] http://sepgsql.googlecode.com/files/sepgsql-docs-8.4devel-r1608.patch >> [5/5] http://sepgsql.googlecode.com/files/sepgsql-tests-8.4devel-r1608.patch >> >> - List of updates: >> * bugfix: sepgsqlCheckProcedureEntrypoint() was invoked twice when >> security invoker functions are invoked. >> >> Rest of parts are unchanged. Don't mind contracted filename. >> Please comment anything. It will help to improve our code. > > I did an analysis of the "core" file: > > http://sepgsql.googlecode.com/files/sepgsql-core-8.4devel-r1608.patch > > changed lines 3226 > new files 4075 > syscatalog 9977 > ---- > total 17278 > > The good news is that 3226 is the affect on the non-system-catalog main > core code, and is a context diff size, not total changed lines. Hum...? What utility did you use to compute the lines? It seems to me the changed lines except for system catalogs are larger than actual one. The diffstat says: 65 files changed, 4769 insertions(+), 11 deletions(-), 4945 modifications(!) The (4244 + 500) of 4945 modifications come from pg_proc.h and pg_attribute.h due to a new field to store security label of procedures and columns. The new files adds 4014 in total, so rest of (755 + 11 + 201 = 967) lines are estimated changes in the main core code. Anyway, I believe the burden of reviewer became smaller than the prior full-set version. Thanks, ------------------------------------------------------------- [kaigai@masu ~]$ diffstat ~/sepgsql-core-8.4devel-r1608.patch configure | 113 configure.in | 13 src/Makefile.global.in | 1 src/backend/Makefile | 7 src/backend/access/heap/heapam.c | 12 src/backend/bootstrap/bootparse.y | 4 src/backend/bootstrap/bootstrap.c | 3 src/backend/catalog/aclchk.c | 11 src/backend/catalog/heap.c | 94 src/backend/catalog/index.c | 8 src/backend/catalog/pg_aggregate.c | 3 src/backend/catalog/pg_proc.c | 9 src/backend/catalog/toasting.c | 3 src/backend/commands/cluster.c | 4 src/backend/commands/copy.c | 9 src/backend/commands/dbcommands.c | 33 src/backend/commands/foreigncmds.c | 7 src/backend/commands/functioncmds.c | 77 src/backend/commands/lockcmds.c | 4 src/backend/commands/proclang.c | 6 src/backend/commands/tablecmds.c | 99 src/backend/commands/trigger.c | 6 src/backend/executor/execMain.c | 22 src/backend/nodes/copyfuncs.c | 25 src/backend/nodes/equalfuncs.c | 21 src/backend/nodes/outfuncs.c | 28 src/backend/nodes/readfuncs.c | 41 src/backend/optimizer/plan/planner.c | 1 src/backend/parser/gram.y | 63 src/backend/postmaster/postmaster.c | 43 src/backend/rewrite/rewriteHandler.c | 6 src/backend/security/Makefile | 11 src/backend/security/sepgsql/Makefile | 16 src/backend/security/sepgsql/avc.c | 1157 +++++++ src/backend/security/sepgsql/checker.c | 902 +++++ src/backend/security/sepgsql/core.c | 235 + src/backend/security/sepgsql/dummy.c | 37 src/backend/security/sepgsql/hooks.c | 576 +++ src/backend/security/sepgsql/label.c | 360 ++ src/backend/security/sepgsql/perms.c | 463 ++ src/backend/storage/ipc/ipci.c | 2 src/backend/tcop/utility.c | 5 src/backend/utils/cache/catcache.c | 32 src/backend/utils/cache/syscache.c | 15 src/backend/utils/fmgr/dfmgr.c | 10 src/backend/utils/fmgr/fmgr.c | 8 src/backend/utils/init/postinit.c | 11 src/backend/utils/misc/guc.c | 18 src/backend/utils/misc/postgresql.conf.sample | 3 src/include/catalog/heap.h | 9 src/include/catalog/pg_attribute.h | 500 !!! src/include/catalog/pg_class.h | 12 src/include/catalog/pg_database.h | 6 src/include/catalog/pg_proc.h | 4244 !!!!!!!!!!!!!!!!!!!!!!!!!! src/include/catalog/pg_proc_fn.h | 3 src/include/fmgr.h | 10 src/include/nodes/nodes.h | 3 src/include/nodes/parsenodes.h | 30 src/include/nodes/plannodes.h | 2 src/include/pg_config.h.in | 3 src/include/security/sepgsql.h | 257 + src/include/storage/lwlock.h | 1 src/include/utils/catcache.h | 1 src/include/utils/errcodes.h | 5 src/include/utils/syscache.h | 2 65 files changed,4769 insertions(+), 11 deletions(-), 4945 modifications(!) -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On Sat, Feb 14, 2009 at 12:46 AM, KaiGai Kohei <kaigai@kaigai.gr.jp> wrote: > Jaime Casanova wrote: >> >> i only have ubuntu at hand, and i can install selinux from >> repositories... do you see any problem with that? >> i'm creating the VM now... will test on weekend... > > Now I checked the repository of Ubuntu, however, I can find > several matters to build/run SE-PostgreSQL. > - The default security policy is quite old. > Is does not contain SE-PostgreSQL support which is merged > at the refpolicy-20080702. > - The libselinux is a bit old. It does not contains several > symbols required by SE-PostgreSQL, so it is not possible > to compile. > > If you set up a VM from scratch, Fedora enables to install > via remote repository using minimum bootable image. > http://download.fedora.redhat.com/pub/fedora/linux/releases/10/Fedora/i386/iso/ > at last i managed to install fedora 10 and it seems like it have selinux installed (both "locate selinux" and "locate libselinux" gives results). executing: yum install libselinux gives "Package libselinux-2.0.73-1.fc10.i386 already installed and latest version" nevertheless, when i run: ./configure --prefix=/usr/local/pgsql/8.4.se --enable-cassert --enable-debug --enable-depend --enable-selinux i get this message: checking for getpeercon in -lselinux... no configure: error: "--enable-selinux requires libselinux." attached config.log -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157
Attachment
Jaime Casanova <jcasanov@systemguards.com.ec> writes:
> executing: yum install libselinux gives "Package
> libselinux-2.0.73-1.fc10.i386 already installed and latest version"
Yeah, but have you got libselinux-devel? A general rule on Red Hat
based systems is that compiling a program that depends on library
package foo will require foo-devel to be installed too.
regards, tom lane
On Fri, Feb 27, 2009 at 5:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Jaime Casanova <jcasanov@systemguards.com.ec> writes: >> executing: yum install libselinux gives "Package >> libselinux-2.0.73-1.fc10.i386 already installed and latest version" > > Yeah, but have you got libselinux-devel? A general rule on Red Hat > based systems is that compiling a program that depends on library > package foo will require foo-devel to be installed too. > ah! ok, i tried libselinux-dev and nothing was found (i'm more familiar with the debian naming convention ;) -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157
The series of SE-PostgreSQL patches for v8.4 were updated: [1/5] http://sepgsql.googlecode.com/files/sepgsql-core-8.4devel-r1668.patch [2/5] http://sepgsql.googlecode.com/files/sepgsql-utils-8.4devel-r1668.patch [3/5] http://sepgsql.googlecode.com/files/sepgsql-policy-8.4devel-r1668.patch [4/5] http://sepgsql.googlecode.com/files/sepgsql-docs-8.4devel-r1668.patch [5/5] http://sepgsql.googlecode.com/files/sepgsql-tests-8.4devel-r1668.patch - List of updates: * It is rebased to the latest CVS HEAD. * sepgsqlCheckProcedureInstall() is moved to sepgsql/hooks.c from sepgsql/perms.c, like as other sepgsqlCheckXXXX() is delopyed on. * sepgsqlCheckDatabaseAccess() is moved to pg_database_aclcheck() from pg_database_aclmask(), because pg_aclmask() can be invoked on ExecGrant_xxxx, but SE-PostgreSQLshould not intervene existing DAC policy. * sepgsqlCheckProcedureExecute() is moved to pg_proc_aclcheck() in same reason. These changes are obvious and minor, and rest of implementations keep unchanged, so don't consider this updates needs to review whole of patches again, please. I would like to know the current status of reviewing the patches. It is welcome, if it is not still 100% completed and partial ones. And, please tell me, if I missed anything. Anyway, it is obvious we don't have enough time! Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
Ok, I've taken a quick look at this too. My first impression is that this is actually not a very big patch. Much much smaller than I was afraid of. It seems that dropping the row-level security and the other change you've already done have helped a great deal. My first question is, why does the patch need the walker implementation to gather all the accessed tables and columns? Can't you hook into the usual pg_xxx_aclcheck() functions? In fact, Peter asked that same question here: http://archives.postgresql.org/pgsql-hackers/2009-01/msg02295.php (among other things). Many things have changed since, but I don't think that question has been adequately answered. Different handling of permissions on views was mentioned, but I think that could be handled with just a few extra checks in the rewriter or executor. The hooks in simple_heap_insert also seem a bit weird. Perhaps an artifact of the row-level security stuff that's no longer there. ISTM that setting the defaults should be done in the same places where the defaults for acl columns are filled, e.g in ProcedureCreate. PS. s/proselabal/proselabel -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki, Thanks for your comments. Heikki Linnakangas wrote: > Ok, I've taken a quick look at this too. My first impression is that > this is actually not a very big patch. Much much smaller than I was > afraid of. It seems that dropping the row-level security and the other > change you've already done have helped a great deal. > > My first question is, why does the patch need the walker implementation > to gather all the accessed tables and columns? Can't you hook into the > usual pg_xxx_aclcheck() functions? In fact, Peter asked that same > question here: > http://archives.postgresql.org/pgsql-hackers/2009-01/msg02295.php (among > other things). Many things have changed since, but I don't think that > question has been adequately answered. Different handling of permissions > on views was mentioned, but I think that could be handled with just a > few extra checks in the rewriter or executor. Yes, one major reason is to handle views. SE-PostgreSQL need to check permissions on after it is extracted. The other one is it has two kind of reader permissions ("select" and "use"). The "select" permission is applied when user tries to read tables/columns and its contents are returned to the user. The "use" permission is applied when user tries to read table/columns, but its contents are consumed internally (not returned to user directly). For example: SELECT a, b FROM t WHERE b > 10 and c = 'aaa'; In this case, db_column:{select} permission is applied on "t.a". db_column:{select use} permission is applied on "t.b". db_column:{use} permission is applied on "t.c". db_table:{select use} permission is applied on "t" However, I don't absolutely oppose to integrate them into a single reader "select" permission, because it was originally a single permission, then "use" is added. The purpose of "use" permission is to set up a writable table, but not readable. When we use UPDATE or DELETE statement, it need to add WHERE clause to make clear its target, but it always requires reader permission. So, I separated it into two cases. Thus, it is not a reason as strong as one for views. I'll check some of corner cases, such as inherited tables, COPY statement, trigger invocations and others, to consider whether your suggestion is possible, or not. Please wait for a while to fix my attitude. > The hooks in simple_heap_insert also seem a bit weird. Perhaps an > artifact of the row-level security stuff that's no longer there. ISTM > that setting the defaults should be done in the same places where the > defaults for acl columns are filled, e.g in ProcedureCreate. Its purpose is not set a default security label in the current version. (It is set in ProcedureCreate and others.) Its purpose is to check user's privileges on tables, columns, procedures and databases, and raises an error if violated. Please note that user's privileges are not limited to create/alter/drop them. One sensitive permission is "db_procedure:{install}". It is checked when user defined functions are set up as a function internally invoked. For example, "pg_conversion.conproc" is internally invoked to translate a text, but it does not check pg_proc_aclcheck() in runtime. We consider all user defined functions should be checked either of: - "db_procedure:{execute}" permission for the clientin runtime or - "db_procedure:{install}" permission for the DBA on installation time Needless to say, "{install}" is more sensitive permission because it means anyones to invoke it implicitly. So, the default policy only allows it on functions defined by DBA, but the "execute" permission is allowed normal users to invoke functions defined by himself. sepgsqlCheckProcedureInstall() checks this permission called from the hooks of simple_heap_insert()/_update().From the viewpoint of security, it is good design to put hooks on more frequently used point, than checking it many points. It is same reason why SELinux checks system-calls from applications. It is the only path to access resources managed by operating system, so necessary and sufficient. > PS. s/proselabal/proselabel Oops, Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
KaiGai Kohei wrote:
> The other one is it has two kind of reader permissions ("select" and
> "use").
> The "select" permission is applied when user tries to read tables/columns
> and its contents are returned to the user.
> The "use" permission is applied when user tries to read table/columns,
> but its contents are consumed internally (not returned to user directly).
>
> For example:
> SELECT a, b FROM t WHERE b > 10 and c = 'aaa';
>
> In this case,
> db_column:{select} permission is applied on "t.a".
> db_column:{select use} permission is applied on "t.b".
> db_column:{use} permission is applied on "t.c".
> db_table:{select use} permission is applied on "t"
>
> However, I don't absolutely oppose to integrate them into a single
> reader "select" permission, because it was originally a single
> permission, then "use" is added.
If you have "use" permisson on c, you can easily use it to find out the
exact value. Just do queries like "SELECT 'foo' FROM t WHERE b > 10 AND
c = 'aaa' AND c BETWEEN 1 AND 1000" repeatedly with different ranges to
zoom into the exact value. So I think separating those two permissions
is a mistake,
> Please note that user's privileges are not limited to create/alter/drop
> them. One sensitive permission is "db_procedure:{install}".
> It is checked when user defined functions are set up as a function
> internally invoked.
>
> For example, "pg_conversion.conproc" is internally invoked to translate
> a text, but it does not check pg_proc_aclcheck() in runtime.
> We consider all user defined functions should be checked either of:
> - "db_procedure:{execute}" permission for the client in runtime
> or
> - "db_procedure:{install}" permission for the DBA on installation time
>
> Needless to say, "{install}" is more sensitive permission because it
> means anyones to invoke it implicitly. So, the default policy only
> allows it on functions defined by DBA, but the "execute" permission
> is allowed normal users to invoke functions defined by himself.
Hmm. We normally rely on the fact that a conversion function needs to be
a C-function, and because only superusers can create C-functions we have
assumed that they're safe to call. Which was actually not true until
recently, when we added checks into all the conversion functions to
check that the source and target encoding of the strings passed as
arguments match the ones specified in the CREATE CONVERSION command.
There has been talks of making CREATE CONVERSION superuser-only, so we
could easily just do that. Can you give some other examples of where the
"install" permission is used?
But if I've understood correctly, one goal is to restrict the actions of
superusers as well. Is there something to disallow superusers from
creating C-functions? If yes, isn't that enough protection from things
like the conversion functions?
-- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas wrote:
> KaiGai Kohei wrote:
>> The other one is it has two kind of reader permissions ("select" and
>> "use").
>> The "select" permission is applied when user tries to read tables/columns
>> and its contents are returned to the user.
>> The "use" permission is applied when user tries to read table/columns,
>> but its contents are consumed internally (not returned to user directly).
>>
>> For example:
>> SELECT a, b FROM t WHERE b > 10 and c = 'aaa';
>>
>> In this case,
>> db_column:{select} permission is applied on "t.a".
>> db_column:{select use} permission is applied on "t.b".
>> db_column:{use} permission is applied on "t.c".
>> db_table:{select use} permission is applied on "t"
>>
>> However, I don't absolutely oppose to integrate them into a single
>> reader "select" permission, because it was originally a single
>> permission, then "use" is added.
>
> If you have "use" permisson on c, you can easily use it to find out the
> exact value. Just do queries like "SELECT 'foo' FROM t WHERE b > 10 AND
> c = 'aaa' AND c BETWEEN 1 AND 1000" repeatedly with different ranges to
> zoom into the exact value. So I think separating those two permissions
> is a mistake,
Hmm.
At least, I can agree to integrate these two permissions into a single
"select". It is originally upper compatible to "use".
>> Please note that user's privileges are not limited to create/alter/drop
>> them. One sensitive permission is "db_procedure:{install}".
>> It is checked when user defined functions are set up as a function
>> internally invoked.
>>
>> For example, "pg_conversion.conproc" is internally invoked to translate
>> a text, but it does not check pg_proc_aclcheck() in runtime.
>> We consider all user defined functions should be checked either of:
>> - "db_procedure:{execute}" permission for the client in runtime
>> or
>> - "db_procedure:{install}" permission for the DBA on installation time
>>
>> Needless to say, "{install}" is more sensitive permission because it
>> means anyones to invoke it implicitly. So, the default policy only
>> allows it on functions defined by DBA, but the "execute" permission
>> is allowed normal users to invoke functions defined by himself.
>
> Hmm. We normally rely on the fact that a conversion function needs to be
> a C-function, and because only superusers can create C-functions we have
> assumed that they're safe to call. Which was actually not true until
> recently, when we added checks into all the conversion functions to
> check that the source and target encoding of the strings passed as
> arguments match the ones specified in the CREATE CONVERSION command.
>
> There has been talks of making CREATE CONVERSION superuser-only, so we
> could easily just do that. Can you give some other examples of where the
> "install" permission is used?
Because SE-PostgreSQL works orthogonally to the existing access controls,
so we need to consider two cases. a) A superuser connected from unprivileged domain (like: user_t, httpd_t) b) A
superuserconnected from privileged domain (like: sysadm_t, unconfined_t)
The superuser is a concept of PostgreSQL, and the domain is a concept
of SELinux. It seems to me you assumes the b) case.
The a) case should be focused on here.
In the a) case, SE-PostgreSQL does not prevent to create C-function
(as far as shared library has an appropriate label: "lib_t"), and newly
created functions are labeled as "unpriv-user defined functions" which
depends on the domain.
It allows unpriv-domains to invoke functions defined by himself,
but does not allow to "install" as a conversion and others, because
it can be implicitly used by other domains.
NOTE: unprivileged domain cannot create files labeled as "lib_t" on the operating system, so all they can do is to
loadlibraries already set up.
Our assumption is a client connected from user_t or httpd_t is suspicious,
even if they logged in as a superuser, so SE-PostgreSQL applies additional
checks.
> But if I've understood correctly, one goal is to restrict the actions of
> superusers as well. Is there something to disallow superusers from
> creating C-functions? If yes, isn't that enough protection from things
> like the conversion functions?
Please note that SE-PostgreSQL focuses on the domain a client connected
from, not attributes of database user.
(Needless to say, vanilla PostgreSQL concurrently prevents C-functions by normal database users.)
The conversion is an example of "install" permissions.
The list of functions to be checked are here.
http://code.google.com/p/sepgsql/source/browse/trunk/sepgsql/src/backend/security/sepgsql/hooks.c#446
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
KaiGai Kohei wrote: > Heikki, Thanks for your comments. > > Heikki Linnakangas wrote: >> Ok, I've taken a quick look at this too. My first impression is that >> this is actually not a very big patch. Much much smaller than I was >> afraid of. It seems that dropping the row-level security and the other >> change you've already done have helped a great deal. >> >> My first question is, why does the patch need the walker >> implementation to gather all the accessed tables and columns? Can't >> you hook into the usual pg_xxx_aclcheck() functions? In fact, Peter >> asked that same question here: >> http://archives.postgresql.org/pgsql-hackers/2009-01/msg02295.php >> (among other things). Many things have changed since, but I don't >> think that question has been adequately answered. Different handling >> of permissions on views was mentioned, but I think that could be >> handled with just a few extra checks in the rewriter or executor. > > Yes, one major reason is to handle views. SE-PostgreSQL need to check > permissions on after it is extracted. : > I'll check some of corner cases, such as inherited tables, COPY > statement, trigger invocations and others, to consider whether > your suggestion is possible, or not. > Please wait for a while to fix my attitude. Heikki, I now feel tempted by an idea to utilize the facilities of table/column-level privileges. One matter was "use" permission, but I can agree to integrate it into "select" permission as the original design did. The other is view. When we use a view in the query, it is extracted as a subquery and its query tree is fetched from pg_rewrite.ev_action which is already parsed. It means we need to ensure the parsed representation is not manipulated. The simplest solution is to prevent updating the pg_rewrite.ev_action by hand when SE-PostgreSQL is enabled. I think smaller hard-wired rules are better, but it is a very corner-case and its benefit cannot be ignorable. - It enables to reduce the "walker" code from sepgsql/checker.c. (I guess it makesreduce a few hundreds lines.) - It helps to maintain code to pick up what tables/columns are accessed. If nobody disagree it, I'll integrate "use" permission into "select" and remove the "walker" code from sepgsql/checker.c due to the next Monday. It affects on sepgsql/checker.c, but I expect little changes on others. I'm happy, if you don't stop reviewing patches except for checker.c. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
KaiGai Kohei wrote: > One matter was "use" permission, but I can agree to integrate > it into "select" permission as the original design did. Ok, great. > The other is view. When we use a view in the query, it is extracted > as a subquery and its query tree is fetched from pg_rewrite.ev_action > which is already parsed. It means we need to ensure the parsed > representation is not manipulated. The simplest solution is to prevent > updating the pg_rewrite.ev_action by hand when SE-PostgreSQL is enabled. Agreed. If SE-PostgreSQL is enabled, you need to forbid manual updates to a lot of catalog tables. This is just another case of the same. > I think smaller hard-wired rules are better, but it is a very corner-case > and its benefit cannot be ignorable. > - It enables to reduce the "walker" code from sepgsql/checker.c. > (I guess it makes reduce a few hundreds lines.) > - It helps to maintain code to pick up what tables/columns are > accessed. > > If nobody disagree it, I'll integrate "use" permission into "select" and > remove the "walker" code from sepgsql/checker.c due to the next Monday. > It affects on sepgsql/checker.c, but I expect little changes on others. > I'm happy, if you don't stop reviewing patches except for checker.c. Sounds good, though I'm not 100% sure I understood what you're going to replace the walker with. Seeing the patch will surely enlighten that :-). -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
As I promised last week, SE-PostgreSQL patches are revised here: [1/5] http://sepgsql.googlecode.com/files/sepgsql-core-8.4devel-r1704.patch [2/5] http://sepgsql.googlecode.com/files/sepgsql-utils-8.4devel-r1704.patch [3/5] http://sepgsql.googlecode.com/files/sepgsql-policy-8.4devel-r1704.patch [4/5] http://sepgsql.googlecode.com/files/sepgsql-docs-8.4devel-r1704.patch [5/5] http://sepgsql.googlecode.com/files/sepgsql-tests-8.4devel-r1704.patch * List of updates:- It is rebased to the latest CVS HEAD. - The two reader permission ("select" and "use") are integrated into a single one ("select") as the original design didtwo year's ago. (It also enables to pick up read columns from rte->selectedCols.) - The 'walker' code in sepgsql/checker.c is removed. In the previous revision, SE-PostgreSQL walked on the given query tree to pick up all the appeared tables/columns. The reason why we needed separated walker phase is SE-PostgreSQL wantedto apply two kind of "reader" permissions, but these are integrated into one. (In addition, column-level privilegesare not available when I started to develop SE-PostgreSQL. :-)) In the currect version, SE-PostgreSQL knowswhat tables/columns are appeared in the given query, from relid, selectedCols and modifiedCols in RangeTblEntry. Then,it makes access controls decision communicating with in-kernel SELinux. After the existing DAC are checked, SE-PostgreSQLalso checks client's privileges on the appeared tables/columns as DAC doing. Required privilges are followsthese rules: * If ACL_SELECT is set on rte->requiredPerms, client need to have db_table:{select} and db_column:{select}for the tables/columns. * If ACL_INSERT is set on rte->requiredPerms, client need to have db_table:{insert}and db_column:{insert} for the tables/columns. * If ACL_UPDATE is set on rte->requiredPerms, client needto have db_table:{update} and db_column:{update} for the tables/columns. * If ACL_DELETE is set on rte->requiredPerms,client need to have db_table:{delete} for the tables. This design change gives us a great benefitin code maintenance. A trade-off is hardwired rules to modify "pg_rewrite" system catalog. The correctness accesscontrols depends on this catalog is protected from unexpected manipulation by hand, because it stores a parsed querytree of views. - T_SelinuxItem is removed from include/node/nodes.h, and the codes related to the node type is eliminated from copyfuncs.cant others. It was used to store all appeared tables/columns in the walker phase, but now it is unnecessary. - Several functions are moved to appropriate files: - The codes related to permission bits are consolidated to 'sepgsql/perms.c'as its filename means. (It was placed at 'avc.c' due to historical reason.) - A few hooks (such as sepgsqlHeapTupleInsert)are moved to 'checker.c', and rest of simple hooks are kept in 'hooks.c'. - The scale of patches become a bit slim more. <rev.1668> <rev.1704> security/Makefile | 11 -> | 11 security/sepgsql/Makefile | 16 -> | 16 security/sepgsql/avc.c | 1157 +++++++ -> | 837 +++++ security/sepgsql/checker.c | 902 +++++ -> | 538 +++security/sepgsql/core.c | 235 + -> | 247 + security/sepgsql/dummy.c | 37 -> | 43 security/sepgsql/hooks.c | 748 ++++ -> | 621 +++ security/sepgsql/label.c | 360 ++ -> | 360 ++ security/sepgsql/perms.c | 295 + -> | 400 ++ [kaigai@saba ~]$ diffstat sepgsql-core-8.4devel-r1668.patch : 64 files changed, 4770 insertions(+), 11 deletions(-),4947 modifications(!) [kaigai@saba ~]$ diffstat sepgsql-core-8.4devel-r1704.patch : 60 files changed, 4048 insertions(+), 11 deletions(-),4944 modifications(!) About 700 lines can be reduced in total! I believe this revision can reduce the burden of reviewers. Please any comments! * An issue:I found a new issue in this revision. - ACL_SELECT_FOR_UPDATE and ACL_UPDATE have identical value. In this version, SE-PostgreSQL knows what permission should be checked via RangeTblEntry::requiredPerms, and it appliesits access control policy with translating them into SELinux's permissions. But we have a trouble in the following query. ---------------- [kaigai@saba ~]$ psql postgres psql (8.4devel) Type "help"for help. postgres=# CREATE TABLE t1 (a int, b text) postgres-# SECURITY_LABEL = 'system_u:object_r:sepgsql_ro_table_t:s0'; CREATETABLE -- NOTE: "sepgsql_ro_table_t" means read-only table from unpriv clients. postgres=# INSERT INTO t1 VALUES (1,'aaa'), (2, 'bbb'); INSERT 0 2 postgres=# \q [kaigai@saba ~]$ runcon -t sepgsql_test_t -- psql postgres psql (8.4devel)Type "help" for help. -- NOTE: "sepgsql_test_t" means unpriv client. postgres=# SELECT * FROM t1; a | b ---+----- 1 | aaa 2 | bbb (2 rows) postgres=# SELECT * FROM t1 FOR SHARE OF t1; ERROR: SELinux: denied { update } scontext=unconfined_u:unconfined_r:sepgsql_test_t:s0-s0:c0.c63tcontext=system_u:object_r:sepgsql_ro_table_t:s0 tclass=db_tablename=t1 ---------------- It raises an error with the client does not have db_table:{update} permission on the table labeled as "sepgsql_ro_table_t",although this query does not modify the table: "t1". Our desirable behavior is SE-PostgreSQL checks db_table:{select lock} and db_column:{select} permissions for the tables/columns.(*) db_table:{lock} means permission to lock a table explicitly. The reason why the ACL_UPDATE is set on RangeTblEntry::requiredPerms is ACL_SELECT_FOR_UPDATE is defined as same value withACL_UPDATE, so we cannot distinguish which permission is required, or both of them. I want these permissions have different values without any impacts for user interfaces. One possible idea is ACL_UPDATE_CHR('w') to be extracted into (ACL_UPDATE | ACL_SELECT_FOR_UPDATE) bits, not a single bit for each character,and it is granted by the token of "update" in GRANT/REVOKE statement. Basically, I don't want go back to the "walker" approach. The ACL_SELECT_FOR_UPDATE without identical value is much desirablefor me. What is your opinion? Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
KaiGai Kohei wrote: > As I promised last week, SE-PostgreSQL patches are revised here: There's checks for reading/writing files with COPY, in sepgsqlCheckFileRead sepgsqlCheckFileWrite). Doesn't the OS do similar checks when the process tries to invoke the read()/write()? Is that not enough? -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
KaiGai Kohei wrote: > As I promised last week, SE-PostgreSQL patches are revised here: I think I now understand what sepgsqlCheckProcedureInstall is trying to achieve. It's trying to stop attacks where you trick another user to run your malicious code. We had a serious vulnerability of that kind a while ago (http://archives.postgresql.org//pgsql-hackers/2008-01/msg00268.php) when ANALYZE and VACUUM FULL ran expression and partial index predicates with (typically) superuser privileges. It seems that sepgsqlCheckProcedureInstall is trying to provide a more thorough solution to the trojan horse problem than what we did back then. It stops you from installing an untrusted function as a type input/output function, operator implementing function etc. Now that could be useful on its own, quite apart from the rest of the SE-PostgreSQL patch, in which case I'd like to see that implemented as a separate patch, so that you can use the facility even if you're not using SE-PostgreSQL. Some details of that: > + void > + sepgsqlCheckProcedureInstall(Relation rel, HeapTuple newtup, HeapTuple oldtup) > + { > + /* > + * db_procedure:{install} check prevent a malicious functions > + * to be installed, as a part of system catalogs. > + * It is necessary to prevent other person implicitly to invoke > + * malicious functions. > + */ > + switch (RelationGetRelid(rel)) > + { > + case AggregateRelationId: > + /* > + * db_procedure:{execute} is checked on invocations of: > + * pg_aggregate.aggfnoid > + * pg_aggregate.aggtransfn > + * pg_aggregate.aggfinalfn > + */ > + break; > + > + case AccessMethodRelationId: > + CHECK_PROC_INSTALL_PERM(pg_am, aminsert, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_am, ambeginscan, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_am, amgettuple, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_am, amgetbitmap, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_am, amrescan, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_am, amendscan, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_am, ammarkpos, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_am, amrestrpos, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_am, ambuild, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_am, ambulkdelete, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_am, amvacuumcleanup, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_am, amcostestimate, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_am, amoptions, newtup, oldtup); > + break; ISTM that you should just forbid any changes to pg_am in the default policy. That's very low level stuff. If you can modify that, you can wreck a lot of havoc, quite possibly turning it into a vulnerability even if you can't directly install a malicious function there. > + case AccessMethodProcedureRelationId: > + CHECK_PROC_INSTALL_PERM(pg_amproc, amproc, newtup, oldtup); > + break; > + > + case CastRelationId: > + CHECK_PROC_INSTALL_PERM(pg_cast, castfunc, newtup, oldtup); > + break; We check execute permission on the cast function at runtime. > + case ConversionRelationId: > + CHECK_PROC_INSTALL_PERM(pg_conversion, conproc, newtup, oldtup); > + break; This ought to be unnecessary now. Only C-functions can be installed as conversion procs, and a C function can do anything, so there's little point in checking this anymore. > + case ForeignDataWrapperRelationId: > + CHECK_PROC_INSTALL_PERM(pg_foreign_data_wrapper, fdwvalidator, newtup, oldtup); > + break; Hmm, calls to fdwvalidator are not at all performance critical, so maybe we should just check execute permission when it's called. > + case LanguageRelationId: > + CHECK_PROC_INSTALL_PERM(pg_language, lanplcallfoid, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_language, lanvalidator, newtup, oldtup); > + break; I think these need to be C-functions. > + case OperatorRelationId: > + CHECK_PROC_INSTALL_PERM(pg_operator, oprcode, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_operator, oprrest, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_operator, oprjoin, newtup, oldtup); > + break; oprcode is checked for execute permission when the operator is used. For oprrest and oprjoin, we could add checks into the planner where they're called. (pg_operator.oprcom and pg_operator.oprnegate are missing?) > + case TSParserRelationId: > + CHECK_PROC_INSTALL_PERM(pg_ts_parser, prsstart, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_ts_parser, prstoken, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_ts_parser, prsend, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_ts_parser, prsheadline, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_ts_parser, prslextype, newtup, oldtup); > + break; > + > + case TSTemplateRelationId: > + CHECK_PROC_INSTALL_PERM(pg_ts_template, tmplinit, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_ts_template, tmpllexize, newtup, oldtup); > + break; Not sure about these. Maybe we should add checks to where these are called. > + case TypeRelationId: > + CHECK_PROC_INSTALL_PERM(pg_type, typinput, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_type, typoutput, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_type, typreceive, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_type, typsend, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_type, typmodin, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_type, typmodout, newtup, oldtup); > + CHECK_PROC_INSTALL_PERM(pg_type, typanalyze, newtup, oldtup); > + break; Hmm, input/output functions have to be in C, so I'm not concerned about those. send/receive and typmodin/typmodout are a bit problematic. ANALYZE calls typanalyze as the table owner, so I think that's safe. All of these require the victim to willingly (although indirectly) call a non-security definer function created by the attacker, with varying degrees of difficultness in tricking someone to do that. Can't you just create a policy that forbids creating non-security definer functions in the first place? It's much more coarse-grained, but would probably be enough in practice. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas wrote: > KaiGai Kohei wrote: >> As I promised last week, SE-PostgreSQL patches are revised here: > > There's checks for reading/writing files with COPY, in > sepgsqlCheckFileRead sepgsqlCheckFileWrite). Doesn't the OS do similar > checks when the process tries to invoke the read()/write()? Is that not > enough? Please note that who invokes read()/write() system calls. In this case, PostgreSQL server process invokes these system calls instead of the client process. So, operating system need to allow the PostgreSQL server process to invoke these system calls on the target files of COPY TO/FROM. In addition, SE-PostgreSQL also checks read/write permission of client process for these files. Why it is possible is client's privileges are represented in same form of operating system. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
KaiGai Kohei wrote: > As I promised last week, SE-PostgreSQL patches are revised here: The patch adds permission checks to SET/SHOW. If that's useful functionality, it would be nice to see that as a separate patch, not requiring SE-Linux. I think it's leaking because current_setting(), set_config() and pg_show_all_settings() functions don't perform the same checks. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> KaiGai Kohei wrote:
>> As I promised last week, SE-PostgreSQL patches are revised here:
> The patch adds permission checks to SET/SHOW. If that's useful
> functionality, it would be nice to see that as a separate patch, not
> requiring SE-Linux.
My goodness. This patch seems to be going FAR beyond what I thought
its charter was.
regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote: > Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes: > > KaiGai Kohei wrote: > >> As I promised last week, SE-PostgreSQL patches are revised here: > > > The patch adds permission checks to SET/SHOW. If that's useful > > functionality, it would be nice to see that as a separate patch, not > > requiring SE-Linux. > > My goodness. This patch seems to be going FAR beyond what I thought > its charter was. I agree. I thought the idea was that the first round of SE-PostgreSQL additions would be to add SE hooks for permissions that PG already implements. Other permissions would then be implemented in a PG-way first, and SE hooks then added to those later. Stephen
Heikki Linnakangas wrote: > KaiGai Kohei wrote: >> As I promised last week, SE-PostgreSQL patches are revised here: > > I think I now understand what sepgsqlCheckProcedureInstall is trying to > achieve. It's trying to stop attacks where you trick another user to run > your malicious code. We had a serious vulnerability of that kind a while > ago (http://archives.postgresql.org//pgsql-hackers/2008-01/msg00268.php) > when ANALYZE and VACUUM FULL ran expression and partial index predicates > with (typically) superuser privileges. > > It seems that sepgsqlCheckProcedureInstall is trying to provide a more > thorough solution to the trojan horse problem than what we did back > then. It stops you from installing an untrusted function as a type > input/output function, operator implementing function etc. Now that > could be useful on its own, quite apart from the rest of the > SE-PostgreSQL patch, in which case I'd like to see that implemented as a > separate patch, so that you can use the facility even if you're not > using SE-PostgreSQL. Yes, the purpose of sepgsqlCheckProcedureInstall() is to prevent users to invoke functions installed by other malicious/untrusted one, typically known as trojan-horse. Basically, I can agree the vanilla PostgreSQL also provide similar stuff to prevent to install "untrusted" functions as a part of system internal codes. If we have such a facility as a basis, we can implement sepgsqlCheckProcedureInstall() hook more simple and easier to maintenance. [snip] >> + case ConversionRelationId: >> + CHECK_PROC_INSTALL_PERM(pg_conversion, conproc, newtup, >> oldtup); >> + break; > > This ought to be unnecessary now. Only C-functions can be installed as > conversion procs, and a C function can do anything, so there's little > point in checking this anymore. We should not assume only C-functions can be installed on pg_conversion (and other internal stuff), because a superuser can update system catalog by hand. Example) postgres=# CREATE OR REPLACE FUNCTION testfn() postgres-# RETURNS int LANGUAGE sql AS 'SELECT 1'; CREATEFUNCTION postgres=# UPDATE pg_conversion SET conproc = 'testfn'::regproc where oid=11276; UPDATE 1 postgres=# setclient_encoding = 'sjis'; server closed the connection unexpectedly This probably means the server terminatedabnormally before or while processing the request. The connection to the server was lost. Attemptingreset: WARNING: terminating connection because of crash of another server process DETAIL: The postmaster hascommanded this server process to roll back the current transaction and exit, because another server process exited abnormallyand possibly corrupted shared memory. HINT: In a moment you should be able to reconnect to the database and repeatyour command. Failed. !> SE-PostgreSQL intends to acquire them and apply access control policy in this case also. Aside note: sepgsqlCheckDatabaseInstallModule() check permissions on a newly installed C-library file, to prevent to load a file deployed by untrusted client. >> + case ForeignDataWrapperRelationId: >> + CHECK_PROC_INSTALL_PERM(pg_foreign_data_wrapper, >> fdwvalidator, newtup, oldtup); >> + break; > > Hmm, calls to fdwvalidator are not at all performance critical, so maybe > we should just check execute permission when it's called. If pg_proc_aclcheck() on its invocation, it is not necessary to check on the installation time. [snip] >> + case OperatorRelationId: >> + CHECK_PROC_INSTALL_PERM(pg_operator, oprcode, newtup, oldtup); >> + CHECK_PROC_INSTALL_PERM(pg_operator, oprrest, newtup, oldtup); >> + CHECK_PROC_INSTALL_PERM(pg_operator, oprjoin, newtup, oldtup); >> + break; > > oprcode is checked for execute permission when the operator is used. For > oprrest and oprjoin, we could add checks into the planner where they're > called. (pg_operator.oprcom and pg_operator.oprnegate are missing?) If runtime checks are added, it is more desirable. >> + case TSParserRelationId: >> + CHECK_PROC_INSTALL_PERM(pg_ts_parser, prsstart, newtup, >> oldtup); >> + CHECK_PROC_INSTALL_PERM(pg_ts_parser, prstoken, newtup, >> oldtup); >> + CHECK_PROC_INSTALL_PERM(pg_ts_parser, prsend, newtup, oldtup); >> + CHECK_PROC_INSTALL_PERM(pg_ts_parser, prsheadline, newtup, >> oldtup); >> + CHECK_PROC_INSTALL_PERM(pg_ts_parser, prslextype, newtup, >> oldtup); >> + break; >> + + case TSTemplateRelationId: >> + CHECK_PROC_INSTALL_PERM(pg_ts_template, tmplinit, newtup, >> oldtup); >> + CHECK_PROC_INSTALL_PERM(pg_ts_template, tmpllexize, newtup, >> oldtup); >> + break; > > Not sure about these. Maybe we should add checks to where these are called. I'll check the behavior of them tomorrow. >> + case TypeRelationId: >> + CHECK_PROC_INSTALL_PERM(pg_type, typinput, newtup, oldtup); >> + CHECK_PROC_INSTALL_PERM(pg_type, typoutput, newtup, oldtup); >> + CHECK_PROC_INSTALL_PERM(pg_type, typreceive, newtup, oldtup); >> + CHECK_PROC_INSTALL_PERM(pg_type, typsend, newtup, oldtup); >> + CHECK_PROC_INSTALL_PERM(pg_type, typmodin, newtup, oldtup); >> + CHECK_PROC_INSTALL_PERM(pg_type, typmodout, newtup, oldtup); >> + CHECK_PROC_INSTALL_PERM(pg_type, typanalyze, newtup, oldtup); >> + break; > > Hmm, input/output functions have to be in C, so I'm not concerned about > those. send/receive and typmodin/typmodout are a bit problematic. > ANALYZE calls typanalyze as the table owner, so I think that's safe. > > > All of these require the victim to willingly (although indirectly) call > a non-security definer function created by the attacker, with varying > degrees of difficultness in tricking someone to do that. Can't you just > create a policy that forbids creating non-security definer functions in > the first place? It's much more coarse-grained, but would probably be > enough in practice. I think it is possible, and I welcome the vanilla PostgreSQL also have such a facility. It also make easier to maintain SE-PostgreSQL code. :-) The issue it what policy should be applied on the vanilla side. The following rules may be able to be a draft. - Any installed functions should not security definer. - Any installed functionsshould be owned by superuser. (to prevent replacement by normal users.) What is your opinion? I'll try to consider it more... Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
KaiGai Kohei <kaigai@kaigai.gr.jp> writes:
> Yes, the purpose of sepgsqlCheckProcedureInstall() is to prevent users
> to invoke functions installed by other malicious/untrusted one, typically
> known as trojan-horse.
> ...
> We should not assume only C-functions can be installed on pg_conversion
> (and other internal stuff), because a superuser can update system catalog
> by hand.
> ...
> SE-PostgreSQL intends to acquire them and apply access control policy
> in this case also.
I don't think that anyone except KaiGai-san has bought into the concept
that sepostgres should get to override superuser capabilities, much less
that it should be trying to control semantics at this kind of level of
detail.
I've been convinced for awhile that the sepostgres project is going
off the rails, and these last couple of exchanges just confirm the fear.
This is absolutely *not* the kind of thing that we should be designing
four months after feature freeze.
regards, tom lane
On Mon, Mar 9, 2009 at 1:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > KaiGai Kohei <kaigai@kaigai.gr.jp> writes: >> Yes, the purpose of sepgsqlCheckProcedureInstall() is to prevent users >> to invoke functions installed by other malicious/untrusted one, typically >> known as trojan-horse. >> ... >> We should not assume only C-functions can be installed on pg_conversion >> (and other internal stuff), because a superuser can update system catalog >> by hand. >> ... >> SE-PostgreSQL intends to acquire them and apply access control policy >> in this case also. > > I don't think that anyone except KaiGai-san has bought into the concept > that sepostgres should get to override superuser capabilities, much less > that it should be trying to control semantics at this kind of level of > detail. I'd find that VERY surprising. One of the major features of MAC systems is that the system policy trumps decisions by individual users, so root or the database superuser is confined by that policy just like everyone else. They may or may not have the ability to change the policy, but that's a separate issue. > I've been convinced for awhile that the sepostgres project is going > off the rails, and these last couple of exchanges just confirm the fear. I'm not sure what you mean by "going off the rails". I think we are still beating our way through what Peter Eisentraut said in one of his first reviews of this patch: SE-PostgreSQL shouldn't implement MAC that isn't a mirror of existing DAC capabilities. If more capabilities are needed, the DAC side of things should be designed and implemented first. Interestingly, Heikki's latest review comments are coming back to exactly this point. So I think we have unanimity that everything that doesn't meet this criterion should be ripped out for now. But I don't see anyone arguing that those capabilities are intrinsically worthless, except possibly you, just that we won't be ready to support them in SE-PostgreSQL until we support them in some more general sense. > This is absolutely *not* the kind of thing that we should be designing > four months after feature freeze. On this point I am in agreement. We need very much to bring this "November" CommitFest to an end. Unfortunately, the pace of reviewing slowed dramatically after Thanksgiving and has since dropped to a crawl. However, since the decision to bump Hot Standby was made, things have picked up again, mostly due to a bunch of reviewing by Heikki. The thing we need to do now is make that reviewing reach some conclusion about exactly what needs to be fixed and what of it will be fixed by the author vs. by the committer. It would be easier to make the decision to bump SE-PostgreSQL if it were the only thing holding up beta, but we're not there yet. ...Robert
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Mar 9, 2009 at 1:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I've been convinced for awhile that the sepostgres project is going
>> off the rails, and these last couple of exchanges just confirm the fear.
> I'm not sure what you mean by "going off the rails". I think we are
> still beating our way through what Peter Eisentraut said in one of his
> first reviews of this patch: SE-PostgreSQL shouldn't implement MAC
> that isn't a mirror of existing DAC capabilities. If more
> capabilities are needed, the DAC side of things should be designed and
> implemented first. Interestingly, Heikki's latest review comments are
> coming back to exactly this point. So I think we have unanimity that
> everything that doesn't meet this criterion should be ripped out for
> now. But I don't see anyone arguing that those capabilities are
> intrinsically worthless, except possibly you, just that we won't be
> ready to support them in SE-PostgreSQL until we support them in some
> more general sense.
I'm not saying that I think the capability is intrinsically worthless.
What I *am* saying is that I have zero confidence in the current
development process, ie one guy producing patches without any previous
design discussion. What's missing is
1. Community buy-in on the objectives and user-visible semantics.
2. High-level review of the proposed implementation method.
3. Review of the coding details.
We seem to be starting at #3. Now it's not really KaiGai-san's fault;
the fundamental problem IMHO is that no one else is taking very much
interest in the patch. But that in itself speaks volumes about whether
we actually want this patch or should accept it.
regards, tom lane
On Mon, Mar 9, 2009 at 4:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Mon, Mar 9, 2009 at 1:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> I've been convinced for awhile that the sepostgres project is going >>> off the rails, and these last couple of exchanges just confirm the fear. > >> I'm not sure what you mean by "going off the rails". I think we are >> still beating our way through what Peter Eisentraut said in one of his >> first reviews of this patch: SE-PostgreSQL shouldn't implement MAC >> that isn't a mirror of existing DAC capabilities. If more >> capabilities are needed, the DAC side of things should be designed and >> implemented first. Interestingly, Heikki's latest review comments are >> coming back to exactly this point. So I think we have unanimity that >> everything that doesn't meet this criterion should be ripped out for >> now. But I don't see anyone arguing that those capabilities are >> intrinsically worthless, except possibly you, just that we won't be >> ready to support them in SE-PostgreSQL until we support them in some >> more general sense. > > I'm not saying that I think the capability is intrinsically worthless. > What I *am* saying is that I have zero confidence in the current > development process, ie one guy producing patches without any previous > design discussion. What's missing is > > 1. Community buy-in on the objectives and user-visible semantics. > 2. High-level review of the proposed implementation method. > 3. Review of the coding details. > > We seem to be starting at #3. OK, I agree. > Now it's not really KaiGai-san's fault; > the fundamental problem IMHO is that no one else is taking very much > interest in the patch. But that in itself speaks volumes about whether > we actually want this patch or should accept it. Are you sure that this isn't just because the original patch was so enormous? If you're referring to reviewing, it's certainly easier to find someone willing to review a 100-line patch than it is to find someone willing to review a 10,000-line patch. But in terms of potential user feedback, there have been a number of people writing in about how much they would like to use this feature, and some security folks have written in with positive comments, too. It also seems to me that with Heikki's feedback this is rapidly shrinking down to a project of managable size and scope. I don't think it's there yet, and maybe it won't get there soon enough to include in 8.4, but it certainly seems to be moving in the right direction. ...Robert
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Mar 9, 2009 at 4:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Now it's not really KaiGai-san's fault;
>> the fundamental problem IMHO is that no one else is taking very much
>> interest in the patch. �But that in itself speaks volumes about whether
>> we actually want this patch or should accept it.
> Are you sure that this isn't just because the original patch was so
> enormous? If you're referring to reviewing, it's certainly easier to
> find someone willing to review a 100-line patch than it is to find
> someone willing to review a 10,000-line patch.
Well, the huge size of the original patch didn't help any, for sure.
But the nature of this type of problem --- particularly given the
not-designed-for-it architecture we have --- is that there are going to
be a lot of subtle issues and very probably a lot of places to touch.
It gets even worse if you want to put performance constraints on the
result. I will not have any confidence in SEPostgres until both the
design and the code details have been reviewed by a fair number of
experienced PG hackers; and what I see happening is that there simply
aren't enough of them who care.
If it were a small localized patch I might not particularly care ...
but what I'm afraid of is that we'll have a monstrous patch that does
severe damage to readability and modifiability of the backend, and
has a bunch of bugs to boot (every one of which will qualify as a
security issue when it's discovered). And on top of that, I'm still
not sold that there is enough of a user base for it to justify the
effort we'll have to put into it. If there were, we'd be seeing more
interest in reviewing it.
regards, tom lane
On Mon, 2009-03-09 at 16:39 -0400, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > On Mon, Mar 9, 2009 at 4:04 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> Now it's not really KaiGai-san's fault; > >> the fundamental problem IMHO is that no one else is taking very much > >> interest in the patch. But that in itself speaks volumes about whether > >> we actually want this patch or should accept it. > > > Are you sure that this isn't just because the original patch was so > > enormous? If you're referring to reviewing, it's certainly easier to > > find someone willing to review a 100-line patch than it is to find > > someone willing to review a 10,000-line patch. > > Well, the huge size of the original patch didn't help any, for sure. > But the nature of this type of problem --- particularly given the > not-designed-for-it architecture we have --- is that there are going to > be a lot of subtle issues and very probably a lot of places to touch. > It gets even worse if you want to put performance constraints on the > result. I will not have any confidence in SEPostgres until both the > design and the code details have been reviewed by a fair number of > experienced PG hackers; and what I see happening is that there simply > aren't enough of them who care. > > If it were a small localized patch I might not particularly care ... > but what I'm afraid of is that we'll have a monstrous patch that does > severe damage to readability and modifiability of the backend, and > has a bunch of bugs to boot (every one of which will qualify as a > security issue when it's discovered). And on top of that, I'm still > not sold that there is enough of a user base for it to justify the > effort we'll have to put into it. If there were, we'd be seeing more > interest in reviewing it. Can't it be kept separately maintained release for a version or two, so that we will have both PostgreSQL and SE-PostgreSQL and thus have an easy way to compare both correctness and performance ? Anyone remember how did Linux implement/introduce SE Linux ? -- Hannu Krosing http://www.2ndQuadrant.com PostgreSQL Scalability and Availability Services, Consulting and Training
Hannu Krosing <hannu@2ndQuadrant.com> writes:
> Can't it be kept separately maintained release for a version or two, so
> that we will have both PostgreSQL and SE-PostgreSQL and thus have an
> easy way to compare both correctness and performance ?
Actually, KaiGai-san has been distributing a patched SEPostgres on
Fedora for awhile already (and it's been rather a pain in the neck
I fear to keep it in sync with the regular distribution). One thing
I would love to know is how many people are actually using that, but
AFAIK there's no good way to find out.
regards, tom lane
On Mon, 2009-03-09 at 19:45 -0400, Tom Lane wrote: > Hannu Krosing <hannu@2ndQuadrant.com> writes: > > Can't it be kept separately maintained release for a version or two, so > > that we will have both PostgreSQL and SE-PostgreSQL and thus have an > > easy way to compare both correctness and performance ? > > Actually, KaiGai-san has been distributing a patched SEPostgres on > Fedora for awhile already (and it's been rather a pain in the neck > I fear to keep it in sync with the regular distribution). One thing > I would love to know is how many people are actually using that, but > AFAIK there's no good way to find out. To point out the obvious, we are in a quandary here. Nobody argues the feature would be interesting and although I don't have use for it I could see where some people would. I also see where it would open doors for us. Is there any possibility of having it be enabled at compile time? The default would be know but those distributions that would like to make use of it could? I am actually surprised we are not seeing traction on this from SuSE and Redhat. My understanding is that they are both SE Linux supporters. Joshua D. Drake > > regards, tom lane > -- PostgreSQL - XMPP: jdrake@jabber.postgresql.org Consulting, Development, Support, Training 503-667-4564 - http://www.commandprompt.com/ The PostgreSQL Company, serving since 1997
"Joshua D. Drake" <jd@commandprompt.com> writes:
> Is there any possibility of having it be enabled at compile time?
That's been assumed right along (unless you think it's okay for Postgres
to stop working on every non-SELinux platform). The problem here is
mostly about whether we have enough confidence in the code to put our
project name on it.
regards, tom lane
On Mon, 2009-03-09 at 20:05 -0400, Tom Lane wrote: > "Joshua D. Drake" <jd@commandprompt.com> writes: > > Is there any possibility of having it be enabled at compile time? > > That's been assumed right along (unless you think it's okay for Postgres > to stop working on every non-SELinux platform). Good point. > The problem here is > mostly about whether we have enough confidence in the code to put our > project name on it. This patch has been bandied about for what, two years? There is a known fork of our project that runs with it. It has a live googlecode site: http://code.google.com/p/sepgsql/ Which has lots of documentation. I also appears to be active within the SE community: http://www.nsa.gov/applications/search/index.cfm?q=se-postgresql It is also part of the Secure OS project out of Japan (as far as I can tell). I know we are a little uncomfortable here but KaiGai-San (forgive me if I type that wrong) has proven to be a contributor in his own right, jumping over every hurdle we have presented him. He is obviously sticking around for a while. If we accept this code, we lose a fork of our project (good) and we pull those people into our project (better) and hopefully they will help us mature the project over time (best). Sincerely, Joshua D. Drake > > regards, tom lane > -- PostgreSQL - XMPP: jdrake@jabber.postgresql.org Consulting, Development, Support, Training 503-667-4564 - http://www.commandprompt.com/ The PostgreSQL Company, serving since 1997
Joshua D. Drake wrote: > http://www.nsa.gov/applications/search/index.cfm?q=se-postgresql > > It is also part of the Secure OS project out of Japan (as far as I can > tell). > > I know we are a little uncomfortable here but KaiGai-San (forgive me if > I type that wrong) has proven to be a contributor in his own right, > jumping over every hurdle we have presented him. He is obviously > sticking around for a while. KaiGai-San also submitted a patch for an unrelated bug today. :-) -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Hannu Krosing wrote: > Can't it be kept separately maintained release for a version or two, so > that we will have both PostgreSQL and SE-PostgreSQL and thus have an > easy way to compare both correctness and performance ? > > Anyone remember how did Linux implement/introduce SE Linux ? SELinux has been distributed as a part of mainlined Linux 2.6.x series for the recent five years. It can be enabled/disabled on both of compile time and system bootup time, by user's preference. SELinux is implemented as a security module on the LSM framework which provides a set of neutral hooks for any other stuffs. SE-PostgreSQL also had a similar stuff named as PGACE, but I agreed an opinion that we (pgsql-hackers) don't want in-code framework two months ago, so the PGACE has gone now. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
On Mon, 2009-03-09 at 20:31 -0400, Bruce Momjian wrote: > Joshua D. Drake wrote: > > http://www.nsa.gov/applications/search/index.cfm?q=se-postgresql > > > > It is also part of the Secure OS project out of Japan (as far as I can > > tell). > > > > I know we are a little uncomfortable here but KaiGai-San (forgive me if > > I type that wrong) has proven to be a contributor in his own right, > > jumping over every hurdle we have presented him. He is obviously > > sticking around for a while. > > KaiGai-San also submitted a patch for an unrelated bug today. :-) I also found some completely external references to it: http://lwn.net/Articles/242087/ http://itknowledgeexchange.techtarget.com/enterprise-linux/se-postgres-tightens-sql-security/ Sincerely, Joshua D. Drake -- PostgreSQL - XMPP: jdrake@jabber.postgresql.org Consulting, Development, Support, Training 503-667-4564 - http://www.commandprompt.com/ The PostgreSQL Company, serving since 1997
"Joshua D. Drake" <jd@commandprompt.com> writes:
> I know we are a little uncomfortable here but KaiGai-San (forgive me if
> I type that wrong) has proven to be a contributor in his own right,
Not to put too fine a point on it, but: no, he hasn't. Show me one
significant patch he's contributed before/beside this one. The only
thing I see in the CVS logs is that he helped Stephen Frost with column
privileges; I don't recall who did how much, but in any case that patch
still needed nontrivial fixes when it got to me.
Frankly, what we have here is a large patch, with insanely difficult
correctness requirements, written by a Postgres newbie. If it doesn't
scare you, you haven't been paying attention. We have a long track
record of problems with patches written by people who thought they were
ready to do major backend hacking without having bitten off some smaller
chunks first.
Perhaps it would help you calibrate the problem if I stated that
I wouldn't trust a patch for this purpose written by myself, let
alone somebody who hasn't been hacking the backend for ten years.
(Where "this purpose" means the type of control KaiGai-san seems
to hope to enforce, as opposed to just plugging some additional
constraints into the existing ACL-check routines.)
regards, tom lane
Joshua D. Drake wrote: > On Mon, 2009-03-09 at 19:45 -0400, Tom Lane wrote: >> Hannu Krosing <hannu@2ndQuadrant.com> writes: >>> Can't it be kept separately maintained release for a version or two, so >>> that we will have both PostgreSQL and SE-PostgreSQL and thus have an >>> easy way to compare both correctness and performance ? >> Actually, KaiGai-san has been distributing a patched SEPostgres on >> Fedora for awhile already (and it's been rather a pain in the neck >> I fear to keep it in sync with the regular distribution). One thing >> I would love to know is how many people are actually using that, but >> AFAIK there's no good way to find out. > > To point out the obvious, we are in a quandary here. Nobody argues the > feature would be interesting and although I don't have use for it I > could see where some people would. I also see where it would open doors > for us. > > Is there any possibility of having it be enabled at compile time? The > default would be know but those distributions that would like to make > use of it could? It was the design a half year ago, but Bruce suggested me a certain feature should not be enabled/disabled by compile time options, except for library/platform dependency. In addition, he also suggested a feature should be turned on/off by configuration option, because of it enables to distribute a single binary for more wider users. SE-PostgreSQL need the libselinux to communicate the in-kernel SELinux. So, --enable-selinux is necessary on compile time, it is fair enough. If we omit it, all the sepgsqlXXXX() invocations are replaced by empty macros. If we compile it with --enable-selinux, it has two working modes controled by a guc option: sepostgresql (bool). If it is disabled, all the sepgsqlXXXX() invocations returns at the head of themself without doing anything. I believe this behavior follows the previous suggestion. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
> Perhaps it would help you calibrate the problem if I stated that > I wouldn't trust a patch for this purpose written by myself, let > alone somebody who hasn't been hacking the backend for ten years. > (Where "this purpose" means the type of control KaiGai-san seems > to hope to enforce, as opposed to just plugging some additional > constraints into the existing ACL-check routines.) It seems to me this comment is a bit emotional... :( If we need ten more years of experimence before proposing a new security feature, all the new developers (outcome from other community) need to wait for the v8.14 (not 8.1.4) development cycle opened at 2019(?). I would like folks to review what the patch tries to do, not who submitted the patch. (And, I also would like experts to help/suggest this development.) Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
On Mon, Mar 9, 2009 at 1:52 AM, KaiGai Kohei <kaigai@ak.jp.nec.com> wrote: > As I promised last week, SE-PostgreSQL patches are revised here: > > [1/5] http://sepgsql.googlecode.com/files/sepgsql-core-8.4devel-r1704.patch > [2/5] http://sepgsql.googlecode.com/files/sepgsql-utils-8.4devel-r1704.patch > [3/5] http://sepgsql.googlecode.com/files/sepgsql-policy-8.4devel-r1704.patch > [4/5] http://sepgsql.googlecode.com/files/sepgsql-docs-8.4devel-r1704.patch > [5/5] http://sepgsql.googlecode.com/files/sepgsql-tests-8.4devel-r1704.patch > has anyone noted that the links are malformed? in my browser they include the [x/5 part of the next line anyway, i have given up trying to test the functional parts of the patch (my knowledge of selinux is almost zero and is a lot of info just to understand the basics... i'm still on that but don't think will get anything for 8.4... if someone can provide some simple info on that will be great) but now i'm trying the performance impacts of it... what seems interesting is that on some queries are some little gain with the patch applied... that seems interesting 'cause i thought it will be the opposite... i want to try to isolate where is the difference... can someone explain me how can i trace that? (sorry for my ignorance but if i don't ask that ignorance will stay) -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157
Jaime Casanova wrote: > On Mon, Mar 9, 2009 at 1:52 AM, KaiGai Kohei <kaigai@ak.jp.nec.com> wrote: >> As I promised last week, SE-PostgreSQL patches are revised here: >> >> [1/5] http://sepgsql.googlecode.com/files/sepgsql-core-8.4devel-r1704.patch >> [2/5] http://sepgsql.googlecode.com/files/sepgsql-utils-8.4devel-r1704.patch >> [3/5] http://sepgsql.googlecode.com/files/sepgsql-policy-8.4devel-r1704.patch >> [4/5] http://sepgsql.googlecode.com/files/sepgsql-docs-8.4devel-r1704.patch >> [5/5] http://sepgsql.googlecode.com/files/sepgsql-tests-8.4devel-r1704.patch >> > > has anyone noted that the links are malformed? in my browser they > include the [x/5 part of the next line Above URLs might be a bit long. I'll omit the "[x/5]" part on the next submission. > i want to try to isolate where is the difference... can someone > explain me how can i trace that? (sorry for my ignorance but if i > don't ask that ignorance will stay) The "sepgsql_enable_auditallow" system boolean will help you to understand what permissions are checked on the given query. ------------------------- % make -C src/backend/security/sepgsql/policy # su # semodule -i src/backend/security/sepgsql/policy/sepostgresql-devel.pp (installation of development purpose policy) # setsebool sepgsql_enable_auditallow 1 % psql postgres NOTICE: SELinux: granted { access } scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c63 tcontext=unconfined_u:object_r:sepgsql_db_t:s0tclass=db_database name=postgres psql (8.4devel) Type "help" for help. postgres=# SELECT * FROM t1; NOTICE: SELinux: granted { select } scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c63 tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_table name=t1 NOTICE: SELinux: granted { select } scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c63 tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_column name=t1.a NOTICE: SELinux: granted { select } scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c63 tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_column name=t1.b NOTICE: SELinux: granted { select } scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c63 tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_column name=t1.c a | b | c ---+---+--- (0 rows) postgres=# INSERT INTO t1 (a,c) VALUES (1,2); NOTICE: SELinux: granted { insert } scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c63 tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_table name=t1 NOTICE: SELinux: granted { insert } scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c63 tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_column name=t1.a NOTICE: SELinux: granted { insert } scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c63 tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_column name=t1.c INSERT 0 1 postgres=# ------------------------- The meanings of each fields: - The "scontext" is the client's privileges - The "tcontext" is the security context of tables,columns and so on. - The "tclass" shows the kind of target object. - The "name" is the name of object. I recommend you to turn off it in normal case due to noisy and disk consumption with logs. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
Tom, > Frankly, what we have here is a large patch, with insanely difficult > correctness requirements, written by a Postgres newbie. If it doesn't > scare you, you haven't been paying attention. We have a long track > record of problems with patches written by people who thought they were > ready to do major backend hacking without having bitten off some smaller > chunks first. If that was the case, why didn't you say it 4 months ago? It seems rather unfair to Kaigai and everyone else who worked on it to be getting cold feet about the entire concept after several months of debugging. --Josh Berkus
Josh Berkus <josh@agliodbs.com> writes: >> Frankly, what we have here is a large patch, with insanely difficult >> correctness requirements, written by a Postgres newbie. If it doesn't >> scare you, you haven't been paying attention. We have a long track >> record of problems with patches written by people who thought they were >> ready to do major backend hacking without having bitten off some smaller >> chunks first. > If that was the case, why didn't you say it 4 months ago? It seems > rather unfair to Kaigai and everyone else who worked on it to be getting > cold feet about the entire concept after several months of debugging. Josh, I've had cold feet about this patch from day one, and have not been very shy about expressing it, for instance here http://archives.postgresql.org/pgsql-hackers/2008-05/msg00122.php here http://archives.postgresql.org//pgsql-hackers/2008-09/msg01662.php and here http://archives.postgresql.org//pgsql-hackers/2009-01/msg01928.php (those are just samples from long threads in each case). Despite all that arm-waving, no one besides KaiGai-san has really stepped up to work on it. That leaves me not only with worries about the patch itself, but with an extremely low estimate of the community's interest in it. And it is too big, complicated, and risky to go in if there's not strong community support for it. regards, tom lane
Tom Lane wrote: > Despite all that arm-waving, no one besides KaiGai-san has really > stepped up to work on it. That leaves me not only with worries about > the patch itself, but with an extremely low estimate of the community's > interest in it. And it is too big, complicated, and risky to go in > if there's not strong community support for it. The only reason why I separated a few major facilities (writable system columns, row-level controls, largeobject permission and so on) and reduces the scale of patches as someone required is to help SE-PostgreSQL getting merged into the v8.4. In addition, as I said before, I can provide my efforts to maintenance SE-PostgreSQL feature to the future version, once it getting merged, no need to say. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
Heikki Linnakangas wrote:
>> + void
>> + sepgsqlCheckProcedureInstall(Relation rel, HeapTuple newtup,
>> HeapTuple oldtup)
>> + { [snip]
>> + + case AccessMethodRelationId:
>> + CHECK_PROC_INSTALL_PERM(pg_am, aminsert, newtup, oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_am, ambeginscan, newtup, oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_am, amgettuple, newtup, oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_am, amgetbitmap, newtup, oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_am, amrescan, newtup, oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_am, amendscan, newtup, oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_am, ammarkpos, newtup, oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_am, amrestrpos, newtup, oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_am, ambuild, newtup, oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_am, ambulkdelete, newtup, oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_am, amvacuumcleanup, newtup,
>> oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_am, amcostestimate, newtup, oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_am, amoptions, newtup, oldtup);
>> + break;
>
> ISTM that you should just forbid any changes to pg_am in the default
> policy. That's very low level stuff. If you can modify that, you can
> wreck a lot of havoc, quite possibly turning it into a vulnerability
> even if you can't directly install a malicious function there.
Heikki,
My opinion is still we should check "db_procedure:{install}" privilege
for functions expected to be implemented by C-language.
In the basis of security, it requires security facilities should
improve confidentiality, integrity and availability in the assumption
and environment required by the facility.
For example, existing database ACL improves confidentiality and
integrity with applying DAC policy, and improves availability to
prevent to load C-library by users except for superuser.
(Here, the assumption is that database superuser is trusted.)
If we write a oid of SQL-function onto "pg_am.aminsert", it will
not work correctly independent from existence of maliciou code,
but it also enables to crash the backend immediately.
It can be a damage to the availability of the backend, so I still
think we need to check this permission for functions expected to
be implemented by C-language.
NOTE: when we create a new C-function or replace its definition, sepgsqlCheckDatabaseInstallModule() checks whether
thegiven loadable library file has appropriate security context, or not. In the default security policy, only files
labeledas "lib_t" are allowed to load.
>> + case AccessMethodProcedureRelationId:
>> + CHECK_PROC_INSTALL_PERM(pg_amproc, amproc, newtup, oldtup);
>> + break;
>> + + case CastRelationId:
>> + CHECK_PROC_INSTALL_PERM(pg_cast, castfunc, newtup, oldtup);
>> + break;
>
> We check execute permission on the cast function at runtime.
We have a corner case.
The ri_HashCompareOp() in ri_triggers.c can invokes castfunc without
runtime checks, if I can understand the implementation correctly.
Indeed, most cases invokes pg_proc_aclcheck() and SE-PostgreSQL also
checks "db_procedure:{execute}" permission in runtime.
This design requires either of "install" or "execute" should be checked
at least, so double checks are not matter.
[snip]
>> + case ForeignDataWrapperRelationId:
>> + CHECK_PROC_INSTALL_PERM(pg_foreign_data_wrapper,
>> fdwvalidator, newtup, oldtup);
>> + break;
>
> Hmm, calls to fdwvalidator are not at all performance critical, so maybe
> we should just check execute permission when it's called.
If pg_proc_aclcheck() is added on the invocation of fdwvalidator,
I'll remove "install" checks on it from here.
[snip]
>> + case OperatorRelationId:
>> + CHECK_PROC_INSTALL_PERM(pg_operator, oprcode, newtup, oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_operator, oprrest, newtup, oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_operator, oprjoin, newtup, oldtup);
>> + break;
>
> oprcode is checked for execute permission when the operator is used. For
> oprrest and oprjoin, we could add checks into the planner where they're
> called. (pg_operator.oprcom and pg_operator.oprnegate are missing?)
For example, ExecInitAgg() set up opcode function as follows: fmgr_info(get_opcode(eq_opr), &(peraggstate->equalfn));
and it can be invoked later without checks.
I think it is a case to be checked. Indeed, pg_operator.oprcom and
pg_operator.oprnegate were missed. Thanks for your pointed out.
>> + case TSParserRelationId:
>> + CHECK_PROC_INSTALL_PERM(pg_ts_parser, prsstart, newtup,
>> oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_ts_parser, prstoken, newtup,
>> oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_ts_parser, prsend, newtup, oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_ts_parser, prsheadline, newtup,
>> oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_ts_parser, prslextype, newtup,
>> oldtup);
>> + break;
>> + + case TSTemplateRelationId:
>> + CHECK_PROC_INSTALL_PERM(pg_ts_template, tmplinit, newtup,
>> oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_ts_template, tmpllexize, newtup,
>> oldtup);
>> + break;
>
> Not sure about these. Maybe we should add checks to where these are called.
DefineTSParser() and DefineTSTemplate() checks the invoker has superuser
privileges, so these operations are considered as trusted.
However, I'm not clear whether adding checks affects compatibility, or not.
Thanks,
>> + case TypeRelationId:
>> + CHECK_PROC_INSTALL_PERM(pg_type, typinput, newtup, oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_type, typoutput, newtup, oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_type, typreceive, newtup, oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_type, typsend, newtup, oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_type, typmodin, newtup, oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_type, typmodout, newtup, oldtup);
>> + CHECK_PROC_INSTALL_PERM(pg_type, typanalyze, newtup, oldtup);
>> + break;
>
> Hmm, input/output functions have to be in C, so I'm not concerned about
> those. send/receive and typmodin/typmodout are a bit problematic.
> ANALYZE calls typanalyze as the table owner, so I think that's safe.
>
> All of these require the victim to willingly (although indirectly) call
> a non-security definer function created by the attacker, with varying
> degrees of difficultness in tricking someone to do that. Can't you just
> create a policy that forbids creating non-security definer functions in
> the first place? It's much more coarse-grained, but would probably be
> enough in practice.
>
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
On Tue, 2009-03-10 at 09:56 +0900, KaiGai Kohei wrote: > Joshua D. Drake wrote: ... > > Is there any possibility of having it be enabled at compile time? The > > default would be know but those distributions that would like to make > > use of it could? > > It was the design a half year ago, but Bruce suggested me a certain > feature should not be enabled/disabled by compile time options, > except for library/platform dependency. > > In addition, he also suggested > a feature should be turned on/off by configuration option, because of > it enables to distribute a single binary for more wider users. > > SE-PostgreSQL need the libselinux to communicate the in-kernel SELinux. > So, --enable-selinux is necessary on compile time, it is fair enough. > If we omit it, all the sepgsqlXXXX() invocations are replaced by empty > macros. seems ok. Another option to disable it would be something similar to how we currently handle DTrace ? > If we compile it with --enable-selinux, it has two working modes > controled by a guc option: sepostgresql (bool). > If it is disabled, all the sepgsqlXXXX() invocations returns at > the head of themself without doing anything. > > I believe this behavior follows the previous suggestion. Have you been able to measure any speed difference between --enable-selinux on and off ? -- Hannu Krosing http://www.2ndQuadrant.com PostgreSQL Scalability and Availability Services, Consulting and Training
Hannu Krosing wrote:
> On Tue, 2009-03-10 at 09:56 +0900, KaiGai Kohei wrote:
>> Joshua D. Drake wrote:
> ...
>>> Is there any possibility of having it be enabled at compile time? The
>>> default would be know but those distributions that would like to make
>>> use of it could?
>> It was the design a half year ago, but Bruce suggested me a certain
>> feature should not be enabled/disabled by compile time options,
>> except for library/platform dependency.
>>
>> In addition, he also suggested
>> a feature should be turned on/off by configuration option, because of
>> it enables to distribute a single binary for more wider users.
>>
>> SE-PostgreSQL need the libselinux to communicate the in-kernel SELinux.
>> So, --enable-selinux is necessary on compile time, it is fair enough.
>> If we omit it, all the sepgsqlXXXX() invocations are replaced by empty
>> macros.
>
> seems ok.
>
> Another option to disable it would be something similar to how we
> currently handle DTrace ?
DTrace uses Makefile to hack it.
I don't think it is a good example for me.
[src/backend/utils/Makefile] probes.h: probes.d ifeq ($(enable_dtrace), yes) $(DTRACE) -C -h -s $< -o $@.tmp
sed -e 's/POSTGRESQL_/TRACE_POSTGRESQL_/g' $@.tmp >$@ rm $@.tmp else sed -f
$(srcdir)/Gen_dummy_probes.sed$< >$@ endif
Another example:
* POSIX fadvise It puts #ifdef ... #endif block inside the functions, so it means caller invokes an empty functions
whenPOSIX fadvise is disabled.
int FilePrefetch(File file, off_t offset, int amount) { #if defined(USE_POSIX_FADVISE) && defined(POSIX_FADV_WILLNEED)
int returnCode;
Assert(FileIsValid(file)); [snip] return returnCode; #else Assert(FileIsValid(file)); return 0; #endif }
* LDAP It put #ifdef .. #endif block both of implementations and caller side, but the number of blocks are quite
small.
Basically, I think many of #ifdef ... #endif blocks are noisy, so
the current manner (using empty macro) can keep the code clean.
But, I'll follows the manner if we have anything in this situation.
>> If we compile it with --enable-selinux, it has two working modes
>> controled by a guc option: sepostgresql (bool).
>> If it is disabled, all the sepgsqlXXXX() invocations returns at
>> the head of themself without doing anything.
>>
>> I believe this behavior follows the previous suggestion.
>
> Have you been able to measure any speed difference between
> --enable-selinux on and off ?
I don't have measurement on the current revision, so please wait for
a while to get it measured.
Previous measurement includes effects in row-level access controls: http://kaigai.sblo.jp/article/20303941.html (01 Oct
2008)
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
Stephen Frost wrote: > * Tom Lane (tgl@sss.pgh.pa.us) wrote: >> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes: >>> KaiGai Kohei wrote: >>>> As I promised last week, SE-PostgreSQL patches are revised here: >>> The patch adds permission checks to SET/SHOW. If that's useful >>> functionality, it would be nice to see that as a separate patch, not >>> requiring SE-Linux. >> My goodness. This patch seems to be going FAR beyond what I thought >> its charter was. > > I agree. I thought the idea was that the first round of SE-PostgreSQL > additions would be to add SE hooks for permissions that PG already > implements. Other permissions would then be implemented in a PG-way > first, and SE hooks then added to those later. This seems to be a recurring theme with this patch. We stripped row-level permissions, now we have SET/SHOW and the "function installation" permissions. And the read/write file permissions. To make progress, we need to consider each new feature like that separately, as separate patches. KaiGei: Let's drop SET/SHOW permissions from the patch, I presume that's not critical for the overall goal. Dropping the "function installation" permissions would simplify the patch a lot. It would make the patch as whole a lot easier to swallow. Let's ask the same question as with the row-level permissions: If we drop the function installation stuff, would the rest of the patch still be useful? We can revisit that part in the future. I have the same concern as Tom about trying to curtail what superusers can do. We have not been concerned about what a superuser can and can't do before, so there could be small loopholes all over the codebase that we haven't thought about. Just as an example, you added checks to COPY to prevent reads from/writes to files. That's restricted to superusers. However, you left pg_read_file() in src/backend/utils/adt/genfile.c wide open. If we drop the goal of trying to restrict what a superuser can do, is the patch still useful? One idea is to add a single "is superuser" permission to sepgsql. That can be checked in a single place: superuser_arg(). If SE-Linux says that you don't have superuser permissions, then superuser() will return false even if the current user is marked as a superuser in pg_role. It would give the same level of protection as sprinkling those fine-grained checks all over the code, just in a more coarse-grain fashion. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas wrote: > This seems to be a recurring theme with this patch. We stripped > row-level permissions, now we have SET/SHOW and the "function > installation" permissions. And the read/write file permissions. To make > progress, we need to consider each new feature like that separately, as > separate patches. > > KaiGei: Let's drop SET/SHOW permissions from the patch, I presume that's > not critical for the overall goal. OK, its significance is relatively low. > Dropping the "function installation" permissions would simplify the > patch a lot. It would make the patch as whole a lot easier to swallow. > Let's ask the same question as with the row-level permissions: If we > drop the function installation stuff, would the rest of the patch still > be useful? We can revisit that part in the future. As far as we have the idea of "superuser" permission on SELinux also, we can do it later. > I have the same concern as Tom about trying to curtail what superusers > can do. We have not been concerned about what a superuser can and can't > do before, so there could be small loopholes all over the codebase that > we haven't thought about. Just as an example, you added checks to COPY > to prevent reads from/writes to files. That's restricted to superusers. > However, you left pg_read_file() in src/backend/utils/adt/genfile.c wide > open. Oops, it was my overlooks. > If we drop the goal of trying to restrict what a superuser can do, is > the patch still useful? I want to keep permission checks on files specified by users, because the "superuser" permission affects very wide scope, and all or nothing policy in other word. However, the combination of clients and files is not so simple, and I think it is necessary to apply permission checks individually. I can agree to postpone the checks for procedure installation, C-libraries installation/loading. > One idea is to add a single "is superuser" permission to sepgsql. That > can be checked in a single place: superuser_arg(). If SE-Linux says that > you don't have superuser permissions, then superuser() will return false > even if the current user is marked as a superuser in pg_role. It would > give the same level of protection as sprinkling those fine-grained > checks all over the code, just in a more coarse-grain fashion. At least, I think it is not a strange design. In-kernel SELinux also has similar permission (capability class) to restrict root privileges, in additon to the invididual checks. (NOTE: In Linux, root privilges is a set of capability.) Maybe, I can submit the revised patch within 1.5 days. Please wait for a while. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
KaiGai Kohei <kaigai@kaigai.gr.jp> writes: > Heikki Linnakangas wrote: >> If we drop the goal of trying to restrict what a superuser can do, is the >> patch still useful? > > I want to keep permission checks on files specified by users, because > the "superuser" permission affects very wide scope, and all or nothing > policy in other word. > However, the combination of clients and files is not so simple, and > I think it is necessary to apply permission checks individually. I would think the big advantage of something like SELinux is precisely in cases like this. So for example a client that has a capability that allows him to read a file can pass that capability to the server and be able to use COPY to read it directly on the server. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's PostGIS support!
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> If we drop the goal of trying to restrict what a superuser can do, is
> the patch still useful?
> One idea is to add a single "is superuser" permission to sepgsql.
The agreement back in January was that what we'd consider for 8.4 is
a patch that adds SELinux-driven enforcement of permissions checks
that already exist in Postgres. Allowing the above seems to me to
fit within that charter, but this other stuff definitely doesn't.
regards, tom lane
On Tue, Mar 10, 2009 at 08:02:05PM +0900, KaiGai Kohei wrote: > Please wait for a while. With all due respect to your hard work, waiting for this patch, even one more hour, is exactly what we shouldn't do for 8.4. Sad as it is, even if this patch were causing no controversy in its design, it would still be too late on grounds of its size and invasiveness. Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On Mon, 2009-03-09 at 20:55 -0400, Tom Lane wrote: > "Joshua D. Drake" <jd@commandprompt.com> writes: > > I know we are a little uncomfortable here but KaiGai-San (forgive me if > > I type that wrong) has proven to be a contributor in his own right, > > Perhaps it would help you calibrate the problem if I stated that > I wouldn't trust a patch for this purpose written by myself, let > alone somebody who hasn't been hacking the backend for ten years. > (Where "this purpose" means the type of control KaiGai-san seems > to hope to enforce, as opposed to just plugging some additional > constraints into the existing ACL-check routines.) I think you misunderstand me. I have watched this thread very closely because it has specific strategic interest. For the record: * This patch does scare me* With great risk comes great reward So my question is, if the default is that sepostgres is disabled and can only be enabled via a compile time option, are your concerns just as weighty? What about marking the feature "experimental". ./configure --help --enable-se Enables SE version of PostgreSQL for linux platforms (experimental) Yes it would be a break from what we do but it wouldn't hurt us to be just a "little" bit less stodgy as long as it is done in a responsible manner. Sincerely, Joshua D. Drake > > regards, tom lane > -- PostgreSQL - XMPP: jdrake@jabber.postgresql.org Consulting, Development, Support, Training 503-667-4564 - http://www.commandprompt.com/ The PostgreSQL Company, serving since 1997
"Joshua D. Drake" <jd@commandprompt.com> writes:
> I think you misunderstand me. I have watched this thread very closely
> because it has specific strategic interest. For the record:
> * This patch does scare me
> * With great risk comes great reward
... or great failure. My key concern is that we are setting ourselves
up for failure by accepting a patch that hasn't attracted sufficient
community interest. This patch needs way more eyeballs on it than it
has gotten; which is not only bad in terms of the level of trust we
should have in the patch right now, but it is a very negative signal
about how much maintenance manpower it can expect in the future.
Now the entire effort on KaiGai-san's part has been founded on the
assumption that "if you build it, they will come"; and that is exactly
the same argument I hear you making for continued investment in the
project. But the track record so far, in terms of attracting hackers
to work on the patch, says otherwise.
regards, tom lane
On Tue, 2009-03-10 at 13:08 -0400, Tom Lane wrote: > "Joshua D. Drake" <jd@commandprompt.com> writes: > > I think you misunderstand me. I have watched this thread very closely > > because it has specific strategic interest. For the record: > > > * This patch does scare me > > * With great risk comes great reward > > ... or great failure. Sure, which all humans and projects must do at some point. It is how one learns after all. Sometimes the only thing you can do is fail. On the other hand if we succeed it will be a great reward. > My key concern is that we are setting ourselves > up for failure by accepting a patch that hasn't attracted sufficient > community interest. This patch needs way more eyeballs on it than it > has gotten; which is not only bad in terms of the level of trust we > should have in the patch right now, but it is a very negative signal > about how much maintenance manpower it can expect in the future. > > Now the entire effort on KaiGai-san's part has been founded on the > assumption that "if you build it, they will come"; and that is exactly > the same argument I hear you making for continued investment in the > project. Yes but I am also offering an opportunity for others to show up. Which denying the patch does not do. If we provide SE support (even with marking it experimental), I would wager that some Linux distributions would begin to test it themselves which would allow us in turn to benefit by taking it out of experimental. Since RH, SuSE etc... are not going to Patch postgresql outside of some general compatibility issues. But all of this is moot. I see this as coming down to a simple result. * We don't enable it by default.* We mark it as experimental (or beta or whatever) Is there a serious regression in this line of thinking? It isn't unheard of in other projects. It allows the user to make a determination if they want to test/use the feature. It also continues the positive process of removing the fork which is pulling community away from us (at least to some degree) because those who are using SEpostgres are doing so out of his tree and not ours. Sincerely, Joshua D. Drake > > regards, tom lane > -- PostgreSQL - XMPP: jdrake@jabber.postgresql.org Consulting, Development, Support, Training 503-667-4564 - http://www.commandprompt.com/ The PostgreSQL Company, serving since 1997
Joshua D. Drake escribió: > Yes but I am also offering an opportunity for others to show up. Which > denying the patch does not do. If we provide SE support (even with > marking it experimental), I would wager that some Linux distributions > would begin to test it themselves which would allow us in turn to > benefit by taking it out of experimental. Since RH, SuSE etc... are not > going to Patch postgresql outside of some general compatibility issues. It was said upthread that SEPostgres is already packaged for Fedora. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Tue, 2009-03-10 at 14:47 -0300, Alvaro Herrera wrote: > Joshua D. Drake escribió: > > > Yes but I am also offering an opportunity for others to show up. Which > > denying the patch does not do. If we provide SE support (even with > > marking it experimental), I would wager that some Linux distributions > > would begin to test it themselves which would allow us in turn to > > benefit by taking it out of experimental. Since RH, SuSE etc... are not > > going to Patch postgresql outside of some general compatibility issues. > > It was said upthread that SEPostgres is already packaged for Fedora. Yes for but not by, AFAIK it is not actually included with Fedora. Essentially it is packaged like the PGSQLRPMS are packaged, available but outside of the project itself. Joshua D. Drake > -- PostgreSQL - XMPP: jdrake@jabber.postgresql.org Consulting, Development, Support, Training 503-667-4564 - http://www.commandprompt.com/ The PostgreSQL Company, serving since 1997
"Joshua D. Drake" <jd@commandprompt.com> writes:
> On Tue, 2009-03-10 at 14:47 -0300, Alvaro Herrera wrote:
>> It was said upthread that SEPostgres is already packaged for Fedora.
> Yes for but not by, AFAIK it is not actually included with Fedora.
"Included with Fedora" is an extremely loose concept. You can get it
via "yum install" from the standard Fedora download servers. I don't
believe it's counted as part of the "PostgreSQL" package group, nor
included in the core distro CD set, but the CD-set approach to
distribution seems to be dying anyway. There's too much stuff out
there.
However, if we did accept the patch, then the question would immediately
become whether Devrim and I and other packagers for SELinux-capable
distros ought to enable the feature in our builds. It does not work
to deny responsibility for something by making it a configure option.
You're just putting the hard decision onto packagers, who have no more
knowledge than you do about what their users want, and (probably)
considerably less understanding of the benefits/risks of some new
configure option they happen to notice.
regards, tom lane
Tom Lane wrote: > "Joshua D. Drake" <jd@commandprompt.com> writes: >> I know we are a little uncomfortable here but KaiGai-San (forgive me if >> I type that wrong) has proven to be a contributor in his own right, > > Not to put too fine a point on it, but: no, he hasn't. Show me one > significant patch he's contributed before/beside this one. The only I thought Joshua was talking about his contribtions to F/OSS in general. He's credited on the NSA site for SELinux kernel scalability and locking issues: http://www.nsa.gov/research/selinux/contrib.shtml "Kaigai Kohei of NEC replaced the original Access Vector Cache (AVC)locking scheme with a RCU-based approach, which solved the major SELinux kernel scalability problem, and fixed other locking issues in the SELinux kernel code. He later optimized the SELinux ebitmap implementation to improve performanceon AVC misses. He also developed SE PostgreSQL, and is one of the developers for the SE busybox project." At first glance it seems it'd be valuable to have him as an active member of this community. > Frankly, what we have here is a large patch, with insanely difficult > correctness requirements, written by a Postgres newbie. I'm kinda hoping the discussion could turn to "what parts (no matter how small) seem both useful safe enough for 8.4" - even if the main use of the small parts ar just as hooks to make it easier for SEPostgres to live as a parallel side project. As far as I can tell, the community feels interested in the feature set; but relatively unable to contribute since none of the people have that much of a security background. It seems the best way to fix that would be to get more people with a security background more involved. Not push them away.
On Tue, 2009-03-10 at 10:49 -0700, Joshua D. Drake wrote: > > It was said upthread that SEPostgres is already packaged for Fedora. > > Yes for but not by, AFAIK it is not actually included with Fedora. It is, with the names "sepostgresql*". > Essentially it is packaged like the PGSQLRPMS are packaged, available > but outside of the project itself. It is included in Fedora standard repositories: https://bugzilla.redhat.com/show_bug.cgi?id=249522 (It also includes my objections.) Regards, -- Devrim GÜNDÜZ, RHCE devrim~gunduz.org, devrim~PostgreSQL.org, devrim.gunduz~linux.org.tr http://www.gunduz.org
On Tue, 2009-03-10 at 14:14 -0400, Tom Lane wrote: > "Joshua D. Drake" <jd@commandprompt.com> writes: > > On Tue, 2009-03-10 at 14:47 -0300, Alvaro Herrera wrote: > >> It was said upthread that SEPostgres is already packaged for Fedora. > > You're just putting the hard decision onto packagers, who have no more > knowledge than you do about what their users want, and (probably) > considerably less understanding of the benefits/risks of some new > configure option they happen to notice. Which is exactly why we have two types of RPMS, --integer-datetimes and not. We would do the same thing with SE-Postgres. Joshua D. Drake -- PostgreSQL - XMPP: jdrake@jabber.postgresql.org Consulting, Development, Support, Training 503-667-4564 - http://www.commandprompt.com/ The PostgreSQL Company, serving since 1997
"Joshua D. Drake" <jd@commandprompt.com> writes:
> On Tue, 2009-03-10 at 14:14 -0400, Tom Lane wrote:
>> You're just putting the hard decision onto packagers, who have no more
>> knowledge than you do about what their users want, and (probably)
>> considerably less understanding of the benefits/risks of some new
>> configure option they happen to notice.
> Which is exactly why we have two types of RPMS, --integer-datetimes and
> not.
Maybe Devrim is doing that, but nobody else is. Debian went for
--integer-datetimes years ago, Red Hat stuck with floats. Nobody
is going to go to the trouble of maintaining two sets of RPMs, even
assuming they notice there's a choice.
regards, tom lane
On Tue, 2009-03-10 at 14:59 -0400, Tom Lane wrote: > "Joshua D. Drake" <jd@commandprompt.com> writes: > > On Tue, 2009-03-10 at 14:14 -0400, Tom Lane wrote: > >> You're just putting the hard decision onto packagers, who have no more > >> knowledge than you do about what their users want, and (probably) > >> considerably less understanding of the benefits/risks of some new > >> configure option they happen to notice. At this point I don't know that any of this is going anywhere. I have presented what I think is a reasonable compromise to accept the feature. A compile-time option which was as designed all along with a flag called experimental. Which will be vastly easier to get people to test over time versus having to run a fork. I am for including this patch. I believe it is worth the risk. Sincerely, Joshua D. Drake -- PostgreSQL - XMPP: jdrake@jabber.postgresql.org Consulting, Development, Support, Training 503-667-4564 - http://www.commandprompt.com/ The PostgreSQL Company, serving since 1997
On Tue, 2009-03-10 at 14:59 -0400, Tom Lane wrote: > > > Which is exactly why we have two types of RPMS, --integer-datetimes > and > > not. > > Maybe Devrim is doing that, but nobody else is. It is only available *if* yum repo conf file is specially configured and if the distro is Fedora 10 and RHEL 5. Those packages are not distributed in regular channels. I already used this switch in 8.4 packages to follow upstream. -- Devrim GÜNDÜZ, RHCE devrim~gunduz.org, devrim~PostgreSQL.org, devrim.gunduz~linux.org.tr http://www.gunduz.org
Ron Mayer <rm_pg@cheapcomplexdevices.com> writes: > As far as I can tell, the community feels interested in the > feature set; but relatively unable to contribute since none > of the people have that much of a security background. It > seems the best way to fix that would be to get more people > with a security background more involved. It's experience with the Postgres code base that I'm worried about. I don't question KaiGai-san's security background; I do doubt that he knows where all the skeletons are buried in the PG backend. A couple of very recent examples of that: his patch to fix a problem with inheritance of column privileges was approximately the right thing, but inefficiently duplicated the functionality of nearby code: http://archives.postgresql.org/pgsql-hackers/2009-03/msg00196.php and it didn't take Heikki long at all to note an oversight in the part of the latest sepostgres patch that attempted to confine superusers' file read/write abilities: http://archives.postgresql.org/pgsql-hackers/2009-03/msg00446.php More generally, there's been no discussion or community buy-in on design questions such as whether the patch should even try to confine superusers on such a fine-grained basis. (I agree with Heikki's thought that this may be a lost cause given our historical design assumption that superusers can do anything.) So I remain strongly of the opinion that what this patch lacks is review from longtime PG hackers. It's not the security community that is missing from the equation. regards, tom lane
On Tue, 2009-03-10 at 11:44 -0700, Joshua D. Drake wrote: > We would do the same thing with SE-Postgres. No, no. I already experienced this with --integer-datetimes sets, and I don't ever want to maintain another set. It is horrible. -- Devrim GÜNDÜZ, RHCE devrim~gunduz.org, devrim~PostgreSQL.org, devrim.gunduz~linux.org.tr http://www.gunduz.org
Hannu Krosing wrote: >> If we compile it with --enable-selinux, it has two working modes >> controled by a guc option: sepostgresql (bool). >> If it is disabled, all the sepgsqlXXXX() invocations returns at >> the head of themself without doing anything. >> >> I believe this behavior follows the previous suggestion. > > Have you been able to measure any speed difference between > --enable-selinux on and off ? At the last night, I measured TPS by pgbench in three cases:1) A binary compiled without --enable-selinux2) A binary compiledwith --enable-selinux, runtime disabled3) A binary compiled with --enable-selinux, runtime enabled Then, I cannot observe statically meaningful differences here. * EnvironmentCPU: Core2Duo E6400 (2.13GHz)Mem: 2048MBkernel: 2.6.28-3.fc11.i686 * Parameters- shared_buffers = 512MB- rest of parameters are in the default * Benchmarch% pgbench -i -s 10 postgres% pgbench -c 2 -t 100000 postgres ---> 6 times * Results(1) compiled without --enable-selinux1st: 478.5655692nd: 478.2233913rd: 442.3656364th: 468.9884995th: 482.1738366th:448.208615 -----------------AVG: 466.420924 (STD: 17.0404) (2) compiled with --enable-selinux, runtime disabled1st: 469.0057772nd: 485.6020913rd: 449.0961234th: 460.6573685th: 476.7919236th:444.027405 -----------------AVG: 464.196781 (STD: 16.0456) (3) compiled with --enable-selinux, runtime enabled1st: 462.7022422nd: 473.3120133rd: 442.2143474th: 468.4656145th: 473.4986826th:468.973759 -----------------AVG: 464.861109 (STD: 11.7768) -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
Heikki, it is the list of updated patches: http://sepgsql.googlecode.com/files/sepgsql-core-8.4devel-r1710.patch http://sepgsql.googlecode.com/files/sepgsql-utils-8.4devel-r1710.patch http://sepgsql.googlecode.com/files/sepgsql-policy-8.4devel-r1710.patch http://sepgsql.googlecode.com/files/sepgsql-docs-8.4devel-r1710.patch http://sepgsql.googlecode.com/files/sepgsql-tests-8.4devel-r1710.patch - List of updates: * Permission checks on SET/SHOW were removed. * Add a new permission: db_database:{superuser} sepgsqlCheckDatabaseSuperuser()is invoked from superuser_arg() to check whether the clietn can perform as a superuser inthis database, or not. * Permission checks on procedure installation is separated. * Permission checks on install/loadC-libraries are separated. * Read file checks on pg_read_file() is added. - Scale of patches: * r1710 (the latest revision) 60 files changed, 3686 insertions(+), 10 deletions(-), 4952 modifications(!)* r1704 (previous revision) 60 files changed, 4048 insertions(+), 11 deletions(-), 4944 modifications(!) ... about 300 lines were downsized. - Remaining issue: * ACL_SELECT_FOR_UPDATE has same value with ACL_UPDATE, so SE-PostgreSQL checks db_table:{update} permissionon SELECT ... FOR SHARE OF, instead of db_table:{lock} permission. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
Tom Lane wrote: > Ron Mayer <rm_pg@cheapcomplexdevices.com> writes: >> As far as I can tell, the community feels interested in the >> feature set; but relatively unable to contribute since none >> of the people have that much of a security background. It >> seems the best way to fix that would be to get more people >> with a security background more involved. > > It's experience with the Postgres code base that I'm worried about. > I don't question KaiGai-san's security background; I do doubt that > he knows where all the skeletons are buried in the PG backend. > A couple of very recent examples of that: his patch to fix a problem > with inheritance of column privileges was approximately the right thing, > but inefficiently duplicated the functionality of nearby code: > http://archives.postgresql.org/pgsql-hackers/2009-03/msg00196.php > and it didn't take Heikki long at all to note an oversight in the part > of the latest sepostgres patch that attempted to confine superusers' > file read/write abilities: > http://archives.postgresql.org/pgsql-hackers/2009-03/msg00446.php Indeed, I have less than three years experience of development in PostgreSQL backend. However, I don't believe it is a productive discussion to point out such kind of failures. At least, I think it is worthwhile to report bugs/submit patches much more than keeping silent with being afraid of failures. If submitted patches are not still enough elegant, we can fix and improve them via discussions. > More generally, there's been no discussion or community buy-in on > design questions such as whether the patch should even try to confine > superusers on such a fine-grained basis. (I agree with Heikki's > thought that this may be a lost cause given our historical design > assumption that superusers can do anything.) > > So I remain strongly of the opinion that what this patch lacks is > review from longtime PG hackers. It's not the security community > that is missing from the equation. Two months ago, I agreed to postpone some of features especially hot in discussion, to reduce the scale of patches and burden of reviewers on the v8.4 development phase. In addition, I also reduced more than 1,000 lines as Heikki suggested. Its purpose is to focus the points to be discussed. I would like to have a productive discssion. -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
KaiGai Kohei wrote:
> * ACL_SELECT_FOR_UPDATE has same value with ACL_UPDATE, so SE-PostgreSQL
> checks db_table:{update} permission on SELECT ... FOR SHARE OF,
> instead of db_table:{lock} permission.
This again falls into the category of trying to have more fine-grained
permissions than vanilla PostgreSQL has. Just give up on the lock
permission, and let it check update permission instead. Yes, it can be
annoying that you need update-permission to do SELECT FOR SHARE, but
that's an existing problem and not in scope for this patch.
-- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas wrote:
> KaiGai Kohei wrote:
>> * ACL_SELECT_FOR_UPDATE has same value with ACL_UPDATE, so SE-PostgreSQL
>> checks db_table:{update} permission on SELECT ... FOR SHARE OF,
>> instead of db_table:{lock} permission.
>
> This again falls into the category of trying to have more fine-grained
> permissions than vanilla PostgreSQL has. Just give up on the lock
> permission, and let it check update permission instead. Yes, it can be
> annoying that you need update-permission to do SELECT FOR SHARE, but
> that's an existing problem and not in scope for this patch.
Can I consider the term of "problem" means it can be resolved
in the future (v8.5, if possible) version?
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
KaiGai Kohei wrote:
> Heikki Linnakangas wrote:
>> KaiGai Kohei wrote:
>>> * ACL_SELECT_FOR_UPDATE has same value with ACL_UPDATE, so
>>> SE-PostgreSQL
>>> checks db_table:{update} permission on SELECT ... FOR SHARE OF,
>>> instead of db_table:{lock} permission.
>>
>> This again falls into the category of trying to have more fine-grained
>> permissions than vanilla PostgreSQL has. Just give up on the lock
>> permission, and let it check update permission instead. Yes, it can be
>> annoying that you need update-permission to do SELECT FOR SHARE, but
>> that's an existing problem and not in scope for this patch.
>
> Can I consider the term of "problem" means it can be resolved
> in the future (v8.5, if possible) version?
Sure, a patch to address that in 8.5 would be welcome.
I don't know why it's like that. Maybe no-one has just bothered. Or
maybe it's because of backwards-compatibility or SQL standard
compliance. In any case, it would seem useful to separate them in the
future.
-- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> KaiGai Kohei wrote:
>> * ACL_SELECT_FOR_UPDATE has same value with ACL_UPDATE, so SE-PostgreSQL
>> checks db_table:{update} permission on SELECT ... FOR SHARE OF,
>> instead of db_table:{lock} permission.
>
> This again falls into the category of trying to have more fine-grained
> permissions than vanilla PostgreSQL has. Just give up on the lock permission,
> and let it check update permission instead. Yes, it can be annoying that you
> need update-permission to do SELECT FOR SHARE, but that's an existing problem
> and not in scope for this patch.
Would it make sense to instead of removing and deferring pieces bit by bit to
instead work the other way around? Extract just the part of the patch that
maps SELinux capabilities to Postgres privileges as a first patch? Then
discuss any other parts individually at a later date?
That might relieve critics of the sneaking suspicion that there may be some
semantic change that hasn't been identified and discussed and snuck through?
Some of them are probably good ideas but if they are they're probably good
ideas even for non-SE semantics too.
-- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's PostGIS support!
Gregory Stark escribió:
> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
>
> > KaiGai Kohei wrote:
> >> * ACL_SELECT_FOR_UPDATE has same value with ACL_UPDATE, so SE-PostgreSQL
> >> checks db_table:{update} permission on SELECT ... FOR SHARE OF,
> >> instead of db_table:{lock} permission.
> >
> > This again falls into the category of trying to have more fine-grained
> > permissions than vanilla PostgreSQL has. Just give up on the lock permission,
> > and let it check update permission instead. Yes, it can be annoying that you
> > need update-permission to do SELECT FOR SHARE, but that's an existing problem
> > and not in scope for this patch.
>
> Would it make sense to instead of removing and deferring pieces bit by bit to
> instead work the other way around? Extract just the part of the patch that
> maps SELinux capabilities to Postgres privileges as a first patch? Then
> discuss any other parts individually at a later date?
I think that makes sense. Implement just a very basic core in a first
patch, and start adding checks slowly, one patch each. We have talked
about "incremental patches" in the past.
We wouldn't get "unbreakable PostgreSQL" in a single commit, but we
would at least start moving.
The good thing about having started in the opposite direction is that by
now we know that the foundation APIs are good enough to build the
complete feature.
--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera wrote: > Gregory Stark escribió: >> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes: >> >>> KaiGai Kohei wrote: >>>> * [..feature description..] >>> This again falls into the category of trying to have more fine-grained >>> permissions than vanilla PostgreSQL has.... >> Would it make sense to instead of removing and deferring pieces bit by bit to >> instead work the other way around? Extract just the part of the patch that >> maps SELinux capabilities to Postgres privileges as a first patch? Then >> discuss any other parts individually at a later date? > > I think that makes sense. Implement just a very basic core in a first > patch, and start adding checks slowly, one patch each. We have talked > about "incremental patches" in the past. +1 from an end-user's point of view too. I'm quite aware of the postgres privileges, and if there were a MAC system of enforcing those I'd be reasonably likely to enable them right away. On the other hand, if SEPostgres initially comes with a different set of privileges that don't map to what I'm already using, I'm much less likely to spend the time to figure out the two different systems. And I do see row-level restrictions (and the other access restrictions mentioned in this thread) as quite orthogonal to MAC vs DAC. Would it be cool to have row-level permissions in postgres? Sure, as an abstract concept. Do I have any use for them? Seeing that I'm getting by without them, the answer must be "not now". > We wouldn't get "unbreakable PostgreSQL" in a single commit, but we > would at least start moving. > > The good thing about having started in the opposite direction is that by > now we know that the foundation APIs are good enough to build the > complete feature.
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Gregory Stark escribi�:
>> Would it make sense to instead of removing and deferring pieces bit by bit to
>> instead work the other way around? Extract just the part of the patch that
>> maps SELinux capabilities to Postgres privileges as a first patch? Then
>> discuss any other parts individually at a later date?
> I think that makes sense.
That's pretty much the advice we gave KaiGai-san in January ... which
I gather he hasn't taken.
regards, tom lane
Ron Mayer wrote: > Alvaro Herrera wrote: >> Gregory Stark escribió: >>> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes: >>> >>>> KaiGai Kohei wrote: >>>>> * [..feature description..] >>>> This again falls into the category of trying to have more fine-grained >>>> permissions than vanilla PostgreSQL has.... >>> Would it make sense to instead of removing and deferring pieces bit by bit to >>> instead work the other way around? Extract just the part of the patch that >>> maps SELinux capabilities to Postgres privileges as a first patch? Then >>> discuss any other parts individually at a later date? >> I think that makes sense. Implement just a very basic core in a first >> patch, and start adding checks slowly, one patch each. We have talked >> about "incremental patches" in the past. > > +1 from an end-user's point of view too. > > I'm quite aware of the postgres privileges, and if there were a MAC > system of enforcing those I'd be reasonably likely to enable them > right away. > > On the other hand, if SEPostgres initially comes with a different set > of privileges that don't map to what I'm already using, I'm much less > likely to spend the time to figure out the two different systems. I cannot update whole of the wikipage yet, but updated some of descriptions in object classes and permission. http://wiki.postgresql.org/wiki/SEPostgreSQL#Object_classes_and_permission Some of permissions are mapped to the vanilla PostgreSQL privileges, and some of them are not so. * ACL_INSERT The db_table:{insert} and db_column:{insert} for all the target columns are checked. The table-level permissiondoes not override column-level permission, so the client need to have privileges for both of objects. It is sameas other permissions. * ACL_SELECT The db_table:{select} and db_column:{select} for all the target columns are checked. But it applies db_table:{lock}on LockTableCommand(). * ACL_UPDATE The db_table:{update} and db_column:{update} for all the target columns are checked. * ACL_DELETE The db_table:{delete} is also checked. No column-level checks here. * ACL_TRUNCATE The db_table:{delete} is also checked. SE-PostgreSQL does not discriminate between TRUNCATE and DELETE. * ACL_REFERENCES * ACL_TRIGGER SE-PostgreSQL does not care about these privileges. But, it checks db_procedure:{execute} on trigger invocationtime, and it also checks db_table:{select} on checks of FK constraint within its secondary SQL execution. * ACL_EXECUTE The db_procedure:{execute} is also checked. This check is embedded within pg_proc_ackcheck(). * ACL_USAGE * ACL_CREATE * ACL_CREATE_TEMP SE-PostgreSQL does not care about there privileges. * ACL_CONNECT The db_database:{access} is also checked. This check is embedded within pg_database_aclcheck(). * ACL_SELECT_FOR_UPDATE The db_table:{lock} should be also checked, but ... * database superuser privilege The db_database:{superuser} newly added should be also checked. In addition, SE-PostgreSQL defines and users some of new privileges. * db_xxx:{relabelfrom} and db_xxx:{relabelto} It is checked when the security context of database objects are changed. * db_xxx:{create} It is typically checked when CREATE TABLE and others. SE-PostgreSQL assigns a default security contexton the table and columns newly created, if user does not give any security context explicitly. Then, it checks whetherthe user have db_xxx:{create} privileges on the tables/columns/etc labeled as the security context, or not. * db_xxx:{setattr} * db_xxx:{drop} It is typically cheched when ALTER/DROP TABLE and others. The vanilla PostgreSQL checks user's privilegesbased on the ownership, but SE-PostgreSQL does not consider the concept of owner due to its MAC policy. These permissionare checked based on the security context assigned to the target objects. * db_procedure:{entrypoint} SE-PostgreSQL allows client to change its privilege during execution of certain procedures (calledas "trusted procedure"). It checks this permission when user tries to invoke trusted procedure. The vanilla PostgreSQLdoes not have similar ACL, but it concept it similar to security definer or setuid on operating system. > And I do see row-level restrictions (and the other access restrictions > mentioned in this thread) as quite orthogonal to MAC vs DAC. Would it > be cool to have row-level permissions in postgres? Sure, as an abstract > concept. Do I have any use for them? Seeing that I'm getting by > without them, the answer must be "not now". We defined six permissions for row-level, but not used (included) now. * db_tuple:{relabelfrom} * db_tuple:{relabelto} * db_tuple:{select} * db_tuple:{update} * db_tuple:{insert} * db_tuple:{delete} As SE-PostgreSQL doing on any other database object, it (can) assigns a default security context on the tuple newly inserted, if user does not given any security context explicitly. Then, it checks db_tuple:{insert} permission on them. Do you need explanation for any other permissions on db_tuple class? Thanks, >> We wouldn't get "unbreakable PostgreSQL" in a single commit, but we >> would at least start moving. >> >> The good thing about having started in the opposite direction is that by >> now we know that the foundation APIs are good enough to build the >> complete feature. -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
> * ACL_INSERT
> The db_table:{insert} and db_column:{insert} for all the target
> columns are checked. The table-level permission does not override
> column-level permission, so the client need to have privileges
> for both of objects. It is same as other permissions.
>
> * ACL_SELECT
> The db_table:{select} and db_column:{select} for all the target
> columns are checked.
> But it applies db_table:{lock} on LockTableCommand().
>
> * ACL_UPDATE
> The db_table:{update} and db_column:{update} for all the target
> columns are checked.
>
> * ACL_DELETE
> The db_table:{delete} is also checked. No column-level checks here.
I'm more or less with you up to this point.
> * ACL_TRUNCATE
> The db_table:{delete} is also checked.
> SE-PostgreSQL does not discriminate between TRUNCATE and DELETE.
But this seems wrong.
> * ACL_REFERENCES
> * ACL_TRIGGER
> SE-PostgreSQL does not care about these privileges.
> But, it checks db_procedure:{execute} on trigger invocation time,
> and it also checks db_table:{select} on checks of FK constraint
> within its secondary SQL execution.
And so do these. Why should there be any asymmetry with regular
PostgreSQL here?
> * ACL_EXECUTE
> The db_procedure:{execute} is also checked.
> This check is embedded within pg_proc_ackcheck().
Good...
> * ACL_USAGE
> * ACL_CREATE
> * ACL_CREATE_TEMP
> SE-PostgreSQL does not care about there privileges.
Again, there doesn't seem to be any reason for this asymmetry. I
think you should change it.
...Robert
Robert Haas wrote:
>> * ACL_INSERT
>> The db_table:{insert} and db_column:{insert} for all the target
>> columns are checked. The table-level permission does not override
>> column-level permission, so the client need to have privileges
>> for both of objects. It is same as other permissions.
>>
>> * ACL_SELECT
>> The db_table:{select} and db_column:{select} for all the target
>> columns are checked.
>> But it applies db_table:{lock} on LockTableCommand().
>>
>> * ACL_UPDATE
>> The db_table:{update} and db_column:{update} for all the target
>> columns are checked.
>>
>> * ACL_DELETE
>> The db_table:{delete} is also checked. No column-level checks here.
>
> I'm more or less with you up to this point.
>
>> * ACL_TRUNCATE
>> The db_table:{delete} is also checked.
>> SE-PostgreSQL does not discriminate between TRUNCATE and DELETE.
>
> But this seems wrong.
We consider these differences are just only the way to remove
all the tuples, not the target of the tables and its result.
>> * ACL_REFERENCES
>> * ACL_TRIGGER
>> SE-PostgreSQL does not care about these privileges.
>> But, it checks db_procedure:{execute} on trigger invocation time,
>> and it also checks db_table:{select} on checks of FK constraint
>> within its secondary SQL execution.
>
> And so do these. Why should there be any asymmetry with regular
> PostgreSQL here?
The ACL_REFERENCES is checked when we set up FK constraint, then
ri_PerformCheck() invokes another query to check constraint with
privileges of the table owner. It assumes the owner can access
on the table owned.
However, SE-PostgreSQL works orthogonally to the ownership.
We don't assume the client can access on the FK constrainted
table, and it apply appropriate checks on the secondary query
also, so it does not need to check on FK creation time.
The ACL_TRIGGER is also checked when CREATE TRIGGER.
If someone set a trigger function which is not allowed to execute
from other persons, the other person can invoke this function via
trigger mechanism.
I wonder why the vanilla PostgreSQL does not put pg_proc_aclcheck()
on the ExecCallTriggerFunc().
>> * ACL_EXECUTE
>> The db_procedure:{execute} is also checked.
>> This check is embedded within pg_proc_ackcheck().
>
> Good...
>
>> * ACL_USAGE
>> * ACL_CREATE
>> * ACL_CREATE_TEMP
>> SE-PostgreSQL does not care about there privileges.
>
> Again, there doesn't seem to be any reason for this asymmetry. I
> think you should change it.
The ACL_CREATE and ACL_CREATE_TEMP are checked on namespace
in which the object newly created belongs. And the ACL_USAGE
is checked on various kind of database objects, but they don't
have its security context.
Funfamentally, SELinux requires to check user's privileges
on the object newly created. The object is labeled as a default
security context (if user does not specify anything) on its
creation.
So, if we implement it now, a facility is necessary to store
a security context of namespace and others.
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
KaiGai Kohei wrote:
> I wonder why the vanilla PostgreSQL does not put pg_proc_aclcheck()
> on the ExecCallTriggerFunc().
I don't think we can assume any trigger functions are "trusted",
because normal users with ACL_TRIGGER privilege can set their
procedures on the allowed tables.
It also means someone without ACL_EXECUTE to invoke the functions,
but I cannot believe ACL_TRIGGER implicitly contains such a meaning.
Indeed, I put a hook to check db_procedure:{execute} permission
in SELinux, but putting pg_proc_aclcheck() here is meaningful
not only SE-PostgreSQL users.
I found another matter related to triggers.
I'll report it on another messages.
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
*** src/backend/commands/trigger.c (revision 1704)
--- src/backend/commands/trigger.c (working copy)
***************
*** 1560,1566 ****
--- 1560,1576 ----
* call.
*/
if (finfo->fn_oid == InvalidOid)
+ {
+ AclResult aclresult;
+
+ aclresult = pg_proc_aclcheck(trigdata->tg_trigger->tgfoid,
+ GetUserId(), ACL_EXECUTE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, ACL_KIND_PROC,
+ get_func_name(trigdata->tg_trigger->tgfoid));
+
fmgr_info(trigdata->tg_trigger->tgfoid, finfo);
+ }
Assert(finfo->fn_oid == trigdata->tg_trigger->tgfoid);
Heikki, I updated the SE-PostgreSQL patches: http://sepgsql.googlecode.com/files/sepgsql-core-8.4devel-r1714.patch http://sepgsql.googlecode.com/files/sepgsql-utils-8.4devel-r1714.patch http://sepgsql.googlecode.com/files/sepgsql-policy-8.4devel-r1714.patch http://sepgsql.googlecode.com/files/sepgsql-docs-8.4devel-r1714.patch http://sepgsql.googlecode.com/files/sepgsql-tests-8.4devel-r1714.patch - List of updates: * Removed the fixupSelectedColsByTrigger() which added ACL_SELECT on requiredPerms, and set a bit ofattno=0 on selectedCols. * Removed the invocatin of sepgsqlCheckProcedureExecute() from ExecCallTriggerFunc() These two changes make clear the attitude for trigger functions in SE-PostgreSQL. So, it now considers them as a part ofsystem internal operations. - Scale of patches: * r1714 (the latest revision) 59 files changed, 3622 insertions(+), 10 deletions(-), 4957 modifications(!)* r1710 (previous revision) 60 files changed, 3686 insertions(+), 10 deletions(-), 4952 modifications(!) ... about 70 lines were downsized. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
Alvaro Herrera wrote:
> Gregory Stark escribió:
>> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
>>
>>> KaiGai Kohei wrote:
>>>> * ACL_SELECT_FOR_UPDATE has same value with ACL_UPDATE, so SE-PostgreSQL
>>>> checks db_table:{update} permission on SELECT ... FOR SHARE OF,
>>>> instead of db_table:{lock} permission.
>>> This again falls into the category of trying to have more fine-grained
>>> permissions than vanilla PostgreSQL has. Just give up on the lock permission,
>>> and let it check update permission instead. Yes, it can be annoying that you
>>> need update-permission to do SELECT FOR SHARE, but that's an existing problem
>>> and not in scope for this patch.
>> Would it make sense to instead of removing and deferring pieces bit by bit to
>> instead work the other way around? Extract just the part of the patch that
>> maps SELinux capabilities to Postgres privileges as a first patch? Then
>> discuss any other parts individually at a later date?
>
> I think that makes sense. Implement just a very basic core in a first
> patch, and start adding checks slowly, one patch each. We have talked
> about "incremental patches" in the past.
>
> We wouldn't get "unbreakable PostgreSQL" in a single commit, but we
> would at least start moving.
>
> The good thing about having started in the opposite direction is that by
> now we know that the foundation APIs are good enough to build the
> complete feature.
What should I do for this matter?
At least, it is necessary to decide when we should fix it. v8.4? v8.5?
If we fix it soon, what strategy is desirable? 1) Add a new GRANT privilege something like "LOCK". It is straight
forwardapproach, but contains user visible change. In MySQL, it has an individual privilege for explicit table
locks.
2) Shrink ACL_SELECT_FOR_UPDATE to ACL_UPDATE in runtime. It is invisible from users, but GRANT UPDATE still
contains a meaning of explicit table locks.
3) "GRANT UPDATE ..." also allows users ACL_SELECT_FOR_UPDATE, not only ACL_UPDATE. It is similar to 2) option,
butit also modifies ACL side, not the requiredPerms side.
4) Other strategy?
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
Alvaro Herrera wrote: > > Would it make sense to instead of removing and deferring pieces bit by bit to > > instead work the other way around? Extract just the part of the patch that > > maps SELinux capabilities to Postgres privileges as a first patch? Then > > discuss any other parts individually at a later date? > > I think that makes sense. Implement just a very basic core in a first > patch, and start adding checks slowly, one patch each. We have talked > about "incremental patches" in the past. > > We wouldn't get "unbreakable PostgreSQL" in a single commit, but we > would at least start moving. > > The good thing about having started in the opposite direction is that by > now we know that the foundation APIs are good enough to build the > complete feature. Well, we have been trying to go simplify the SE-PostgreSQL patch since September, and while we have made progress, we still have work to do, and at this point I think we have run out of time. I think we have given it a fair shot, but I don't think it is going to make 8.4. KaiGai-san, the only option I can offer is perhaps to list a URL for your SE-PostgreSQL patch to be applied by people who want to use SE-PG. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > Alvaro Herrera wrote: >>> Would it make sense to instead of removing and deferring pieces bit by bit to >>> instead work the other way around? Extract just the part of the patch that >>> maps SELinux capabilities to Postgres privileges as a first patch? Then >>> discuss any other parts individually at a later date? >> I think that makes sense. Implement just a very basic core in a first >> patch, and start adding checks slowly, one patch each. We have talked >> about "incremental patches" in the past. >> >> We wouldn't get "unbreakable PostgreSQL" in a single commit, but we >> would at least start moving. >> >> The good thing about having started in the opposite direction is that by >> now we know that the foundation APIs are good enough to build the >> complete feature. > > Well, we have been trying to go simplify the SE-PostgreSQL patch since > September, and while we have made progress, we still have work to do, > and at this point I think we have run out of time. I think we have > given it a fair shot, but I don't think it is going to make 8.4. > > KaiGai-san, the only option I can offer is perhaps to list a URL for > your SE-PostgreSQL patch to be applied by people who want to use SE-PG. > Needless to say, I'm dissatisfied with this offer. But I can understand we're paying effort to release the v8.4 on schedule as far as possible, and we don't have infinite time for development all the items. Yes, it is possible to accept the offer for me. Meanwhile, I would not like to repeat same thing in the v8.5 development cycle again. At the head of this year, we have rest of three big new features (Sync-replication, Hot-standby and SE-PostgreSQL) but all of them have been bursted for the v8.4. In the v8.5 development cycle, I'll pay effort to propose this feature with smaller part, to build it up step-by-step, as suggested in this commit fest. So, I would like folks to review and commit it in the earlier phase. What is your opinion? Currently, we have the following action items for the SE-PostgreSQL with full-functionalities. I'll consider what components can be implemented as a separated patch again, and submit them at: http://wiki.postgresql.org/wiki/CommitFest_2009-First 0. Changes in security policy. - I got a few requirements for the SELinux security policy. It is necessary to discussin the SELinux community also. - *:{select} and *:{use} permission should be integrated - db_database:{get_paramset_param} to be removed - A new db_database:{superuser} permission 1. Security system columns support It contains a few sub-facilities, and they can be submitted in the different patches. 1-1. security system columns and pg_security system catalog 1-2. writable system columns support(security_labeland security_acl) 1-3. reclaim unused entries in pg_security system catalog 2. Basic tables/columns-level access controls (dependency: 1-1) It means the facility proposed in the v8.4 development. We also need to discuss an issue of ACL_UPDATE/ACL_SELECT_FOR_UPDATE. 3. Row-level access control facilities (dependency: 1-2, 2) It also provides permission checks in row-level granularity both of DAC and MAC. 4. Advanced permission support (dependency: 2) 4-1. db_procedure:{install} permission. Heikki also required similarstuff in the vanilla PostgreSQL. 4-2. db_database:{load_module install_module} permission. 4-3. file:{read write}permission for COPY TO/FROM and others. 4-4. permission checks on the large objects. (I guess vanilla PostgreSQLalso requires it.) 5. Audit support (dependency: 2) It is not a facility proposed in the v8.4. SE-PostgreSQL enables to generate audit recordsfor violated accesses. This facility write them out to system audit daemon. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai@ak.jp.nec.com>
Remaining items for 8.4 (was Re: Updates of SE-PostgreSQL 8.4devel patches (r1710))
From
Heikki Linnakangas
Date:
Bruce Momjian wrote: > Well, we have been trying to go simplify the SE-PostgreSQL patch since > September, and while we have made progress, we still have work to do, > and at this point I think we have run out of time. I think we have > given it a fair shot, but I don't think it is going to make 8.4. Agreed. At some point we just have to wrap up and cut the release. Tweaking indefinitely is not fair to all those patches that have already been pushed back, nor to those that have already been committed and are waiting to be released as part of 8.4. Apart from SE-PostgreSQL, we have four remaining items on the commitfest page: GIN fast insert I agree with Tom that we should just disable regular index scans for GIN. If someone misses it in 8.4, we can try to find a way to do it safely in 8.5. Removing existing capability is a bit worrisome, but I'm even less happy with the "out of memory" condition in this patch and in the partial match patch that has been committed already. That really needs to be cleaned up. B-Tree emulation for GIN I think this is in pretty good shape. Improve Performance of Multi-Batch Hash Join for Skewed Data Sets I believe everyone's happy with the performance testing that's been done. Some of the logic ought to be moved to the planner, and maybe there's some other cleanup to do. Proposal of PITR performance improvement Hmm. The first version of this was submitted back in October, as an external tool. There's still some outstanding issues: http://archives.postgresql.org//pgsql-hackers/2008-10/msg01387.php. I think we should push this to 8.5, for the same reasons as SE-PostgreSQL. On the positive side, the external tool is on pgFoundry for use with 8.4 (and earlier releases too?). -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Re: Remaining items for 8.4 (was Re: Updates of SE-PostgreSQL 8.4devel patches (r1710))
From
Tom Lane
Date:
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Improve Performance of Multi-Batch Hash Join for Skewed Data Sets
> I believe everyone's happy with the performance testing that's been
> done. Some of the logic ought to be moved to the planner, and maybe
> there's some other cleanup to do.
I'll take this up next. AFAIR refactoring to put that which should be
in the planner into the planner was the only significant issue.
regards, tom lane
Re: Remaining items for 8.4 (was Re: Updates of SE-PostgreSQL 8.4devel patches (r1710))
From
Koichi Suzuki
Date:
Hi, I believe all the issues pointed out in http://archives.postgresql.org//pgsql-hackers/2008-10/msg01387.php as been covered in the current patch, as discussed with Simon. I also understand that we're running out of time. I'd like to push this to pgFoundry first and then work again together with Sync.Rep and Hot Standby for 8.5. 2009/3/16 Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>: > Bruce Momjian wrote: >> >> Well, we have been trying to go simplify the SE-PostgreSQL patch since >> September, and while we have made progress, we still have work to do, >> and at this point I think we have run out of time. I think we have >> given it a fair shot, but I don't think it is going to make 8.4. > > Agreed. At some point we just have to wrap up and cut the release. Tweaking > indefinitely is not fair to all those patches that have already been pushed > back, nor to those that have already been committed and are waiting to be > released as part of 8.4. > > Apart from SE-PostgreSQL, we have four remaining items on the commitfest > page: > > GIN fast insert > > I agree with Tom that we should just disable regular index scans for GIN. If > someone misses it in 8.4, we can try to find a way to do it safely in 8.5. > Removing existing capability is a bit worrisome, but I'm even less happy > with the "out of memory" condition in this patch and in the partial match > patch that has been committed already. That really needs to be cleaned up. > > > B-Tree emulation for GIN > > I think this is in pretty good shape. > > > Improve Performance of Multi-Batch Hash Join for Skewed Data Sets > > I believe everyone's happy with the performance testing that's been done. > Some of the logic ought to be moved to the planner, and maybe there's some > other cleanup to do. > > > Proposal of PITR performance improvement > > Hmm. The first version of this was submitted back in October, as an external > tool. There's still some outstanding issues: > http://archives.postgresql.org//pgsql-hackers/2008-10/msg01387.php. I think > we should push this to 8.5, for the same reasons as SE-PostgreSQL. On the > positive side, the external tool is on pgFoundry for use with 8.4 (and > earlier releases too?). > > -- > Heikki Linnakangas > EnterpriseDB http://www.enterprisedb.com > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > -- ------ Koichi Suzuki
Re: Remaining items for 8.4 (was Re: Updates of SE-PostgreSQL 8.4devel patches (r1710))
From
Heikki Linnakangas
Date:
Koichi Suzuki wrote: > I believe all the issues pointed out in > http://archives.postgresql.org//pgsql-hackers/2008-10/msg01387.php as > been covered in the current patch, as discussed with Simon. I also > understand that we're running out of time. I pointed out a few more issues here: http://archives.postgresql.org/message-id/49B51791.5080303@enterprisedb.com > I'd like to push this to pgFoundry first and then work again together > with Sync.Rep and Hot Standby for 8.5. Great! -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Re: Remaining items for 8.4 (was Re: Updates of SE-PostgreSQL 8.4devel patches (r1710))
From
Koichi Suzuki
Date:
Sorry I see the comment. I'll continue the work to fulfill the comment. 2009/3/17 Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>: > Koichi Suzuki wrote: >> >> I believe all the issues pointed out in >> http://archives.postgresql.org//pgsql-hackers/2008-10/msg01387.php as >> been covered in the current patch, as discussed with Simon. I also >> understand that we're running out of time. > > I pointed out a few more issues here: > > http://archives.postgresql.org/message-id/49B51791.5080303@enterprisedb.com > >> I'd like to push this to pgFoundry first and then work again together >> with Sync.Rep and Hot Standby for 8.5. > > Great! > > -- > Heikki Linnakangas > EnterpriseDB http://www.enterprisedb.com > -- ------ Koichi Suzuki