Item10176: Groups expansion leaks supposedly DENYed information

from Item10097:

Paul wrote some unit tests that helped me see the issue, and it seems there are at least 3

1. Item9809 foolishly continues the mess where code changes session->{user} without any extra protection to do so safely (we start with reading the BASEWEB WebPreferences that way, rather than bailing out
   • worse, is that Item was resolved without a unit test (I wrote one before i started work on fixing this one, as they are somewhat opposite in their needs)
2. the admin based expansion of groups then shows that we pollute the MetaCache with 'allowView' info under the incorrect session user
   • I've made a change to Metacache to attempt to avoid that, by changing the allowView cached bool to be per-user
   • for 2.0, it may well make more sense to make MetaCache a cross session persist able multi-user safe cache
3. the mapper->eachGroup and groupMember where forced by Item9809 to reveal internal and supposedly hidden information (ie, how that hidden GROUP is defined)
   • which is why Paul's verify_getListOfGroups* tests still fail - and perhaps they should?

In short, I've commited a fix, but I'm not really happy about the implications of Item9809 , as we have defined group memebrship to be a leaky abstraction - I suspect we shall have to live with them though.

added a few more unit tests that show how we leak info that is contained in the hidden group topic.

-- SvenDowideit - 13 Dec 2010

I'm just wondering how we used to be doing the same in 1.0. For me, the metaCache caches the wrong information, which is a bug, because it was designed to run only as one user, which is silly.

The bug I fixed (and thanks for writing the unit test) is that one couldn't use groups with a protected USERSWEB. I wrote and said at the time that the fix I did wasn't right. For me, the best way would have been to add some parameter to the methods reading the topics, to bypass the ACLs, and use those when checking for groups. But that might mean we bypass the cache, which is bad. Ergo... I got stuck, and committed something which worked, but just like Sven did, I wasn't happy with (except that Sven wrote unit tests :))

Any better idea is more than welcomed

-- OlivierRaginel - 13 Dec 2010

Item9809 Kind of freaks me out. I used to assume our prefs/ACL system was consistent and logical, but the more I read the more I begin to doubt. This kind of exception to support an edge use-case is concerning, but I realise why it was done.

Another way to deal with that task is to set the ALLOWTOPICVIEW on each group topic to override the WebPreferences, or roll your own UserMapperContrib. Or, isolate Main/Users/Group webs, or let's at least make TopicUserMapperContrib's escelation to admin a configure thing, disabled by default.

I don't understand why we want the group list method to bypass ACLs, surely that should be up to the caller?

-- PaulHarvey - 13 Dec 2010

It's not a given that a group member should be able to view a group topic. As an example, I have a client who wants to add customers to a group, so they can grant view access to some topics, but customers must not be able to find out who other customers are.

It seems to (without fully understanding allt he detail above) that we need access control checking to operate outside of the "normal" topic access control, reading (and caching) mechanisms in the TopicUserMappingContrib. Right?

-- CrawfordCurrie - 13 Dec 2010

excellent - thankyou, you've confirmed both the unusualness, and the importance of not leaking. And in the process of dumping to irc, I have an idea how to resolve it ,but i need sleep first.

cheers!

-- SvenDowideit - 13 Dec 2010

Don't mind me. I guess I'm just having a brief crisis over nothing.. I guess, as long as this is contained to the user mapper, that's probably okay.

The only other idea I have is that we incorporate the GROUP pref on a *Group topic as part of the ACL inheritance for said *Group topic.

-- PaulHarvey - 13 Dec 2010

Yeah, that was the reason I did my fix the way I did it. I just didn't realise the topic would get cached, as I get this cache was new too.

As I wrote somewhere, my idea to fix the bug I fixed was to have some parameter to the checkGroup which would use some super user, or not check ACLs. As the code is all nested in sub-calls, it was a hell of a task to pass all this along, hence I thought over-riding the user was the easiest, even though some people (Sven) expressed some concerns that this could go wrong. Turns out they were right, but not the way they thought

-- OlivierRaginel - 13 Dec 2010

I've separated out the second part of this issue from the other task, as that one is fixed.

The work remaining, is to try to prevent the Group expansion code from showing a user information derived from topics that they don't have permission to view.

in essence, I think the way we can mitigate this, is to use the 'unexpanded' groups list if there are any topics in the expansion that the session user does not have view permission for.

-- SvenDowideit - 18 Dec 2010

I looked at fixing this in %GROUPINFO% but since the test needs to be made for every topic at every level of the nested groups, it really needs to be down in the recursive expansion code in TopicUserMapping.

But the fix for expanding groups is ugly. I'm not sure how to do it without major reworking of the Mapper. The problem is the same code is used for access checks, including isAdmin checks. If I check if access is allowed for a group, ultimately the AdminGroup is expanded to see if the cUID is an admin, which ends up checking access, ... deep recursion. The same issue will occur if a group is ALLOW to the same or some other group. Expand checks access, which has to expand to check access.

We probably need to differentiate between access checks - which expands and caches the group, and a group list, which needs to check access for each nested group and user topic, and only show those groups and topics where the session user has access.

But this is all inter-related between UserMapping, TopicUserMapping, LdapMapping, etc. and adding to the API to separate access checks from group lists for display will broadly impact the various Mapper implementations. I don't think I can fix this for a patch release. Is this leakage also present in 1.0.x?

-- GeorgeClark - 21 Dec 2010

There are probably some superficial changes that can help

• %GROUPINFO - verify that each member is viewable when not expanding.
• Require admin rights in %GROUPINFO to request expansion? May need to be configurable since this is a bit drastic.
• Similar changes for %GROUP.

-- GeorgeClark - 21 Dec 2010

Found one minor error in GROUPINFO - the access check for group members was checking the group itself for each member. Committed that fix along with a failing unit test that also exposes this nested hidden group issue.

-- GeorgeClark - 21 Dec 2010

its way, not that simple, as there are Func calls to protect too.

-- SvenDowideit - 21 Dec 2010

I've committed changes to GROUP and GROUPINFO macros

• A top group denied to the session user will not be revealed
• Any user in a top or nested group denied to the session user will not be revealed
• A nested group VIEW denied to the session user, with expand=no, will not be revealed to the user

However a nested group denied for VIEW containing users who permit VIEW will be revealed. As of these changes

• GROUP and GROUPINFO are mostly protected with the one exception of a hidden group containing non-hidden users.
• Func API is not protected.

-- GeorgeClark - 21 Dec 2010

Do we split this task into a 2nd API related task that handles this internal to the Mapper implementations? I believe that my changes have resolved the more significant issue of the GROUP* macros revealing user topics that are not readable. The casual user will no longer see users that they would not be able to view.

-- GeorgeClark - 04 Jan 2011

I've documented this with SMELLs and added a detailed explanation of the operation of hidden groups to the release notes.

-- GeorgeClark - 08 Jan 2011

Updated unit test to no longer have expected failures. I'm not sure how it would be possible to hide the secret group membership from the Func API and still allows plugins and contribs like MailerContrib and ImmediateNotify, etc. to work. We'd have to add back in another unprotected API to get emails from secret groups so notification would continue to work. I believe we need to differentiate between internal access to these groups, and presentation of the group contents to users. If an extension is going to render or otherwise expose group membership it should expand the %GROUPINFO% macro to get the filtered information.

-- GeorgeClark - 24 Mar 2011

Made one more commit to trunk on this post close. The unit tests for trunk were a bit different than release branch, so the tests were still failing.

-- GeorgeClark - 29 Apr 2011

Commenting is disabled while not logged in