Item10097: SEARCH not checking ACLs of group topics

SEARCH should be checking ACLs before displaying search results, but fails to do so with group topics.

Steps to reproduce:

1. Create a TopSecretGroup
2. Create a TopSecretTopic in Main
3. Set ALLOWTOPICVIEW = AdminGroup in both newly created topics
4. Set up a SEARCH finding every topic in Main in a topic accessible to guest users
5. Log out and look at the search topic as WikiGuest: SEARCH correctly omits TopSecretTopic but shows TopSecretGroup.

WikiGuest can't actually access the TopSecretGroup topic, but its title is exposed through SEARCH.

-- HolstenerLiesel - 30 Nov 2010

Unable to reproduce on Foswiki 1.1.2 or trunk.

Please tell us more about your environment: loginmanager, passwordmanager, plugins, web server (apache?) operating system (linux?) etc.

-- PaulHarvey - 02 Dec 2010

Also, do you have {PageCache} enabled?

-- PaulHarvey - 02 Dec 2010

Thank you for testing and responding; it's reassuring to hear this is not a general problem.

Login manager is TemplateLogin, password manager HtPasswdUser, web server is apache, OS is currently Ubuntu 9.10, page cache is not enabled. I'll sift through the plugins and try to think of some tests over the weekend if I get around to it.

-- HolstenerLiesel - 03 Dec 2010

I should add that WikiGuest can't actually access the TopSecretGroup topic, it's just displayed in SEARCH, if that was unclear.

-- HolstenerLiesel - 03 Dec 2010

Just found something interesting. In Main I set up a search like this:

%SEARCH{
   search="Group"
   web="%WEB%"
   scope="topic"
   nonoise="on"
   format="   1. $topic"
}%

All existing group topics are restricted to be viewed only by members of ContributorsGroup, which WikiGuest is not. SEARCH shows every group to WikiGuest except AdminGroup which is the first hit.

When I create AbGroup it becomes the first hit instead. Now this group is the only one hidden from WikiGuest, but AdminGroup is visible. I create AaGroup – AbGroup becomes visible.

So it seems on my install SEARCH is only checking ACLs of the first group topic it finds. Does this make any sense to you at all?

-- HolstenerLiesel - 03 Dec 2010

Test:

1. AMStudyGroup
2. AdminGroup
3. AntiWikiSpamBypassGroup
4. AssociationBoardGroup
5. BlogAuthorGroup
6. Main.CDBCGroup
7. ConsistGroup
8. ContentMigrationGroup
9. CoreDevelopersGroup
10. DevelopersGroup
11. DocumentationGroup
12. ExtensionDevelopersGroup
13. GroupTemplate
14. GroupViewTemplate
15. InfrastructureTaskTeamGroup
16. MaintainGroup
17. NanoLundTritonGroup
18. NewNameGroup
19. NobodyGroup
20. ReleaseManagersGroup
21. Main.SQIM-Test_Group
22. SecurityGroup
23. WikiGroups

-- PaulHarvey - 03 Dec 2010

We weren't able to reproduce here on trunk.foswiki.org. Setting back to new - can anybody make some suggestions?

-- PaulHarvey - 04 Dec 2010

I can easily reproduce it in 1.1.2 (Release01x01 branch).

I create two group topics and have already some other group topics.

In the two new I put ALLOWTOPICVIEW = AdminGroup

And I can confirm that the first of the two is not listed. The 2nd is. If I remove the ALLOWTOPICVIEW from the first then the one that was 2nd is not listed in the search result.

Somehow the search code only respect the ALLOWTOPICVIEW for the first topic but not the ones that follow.

You cannot open the topics and see the content so it is not a huge security issue. But it is a bug that needs to be fixed.

Paul I do not know what it is you say you cannot reproduce on trunk.foswiki.org. The secret groups you created are not view protected. Only change protected. So they should all be visible. Not sure you understood the bug report.

-- KennethLavrsen - 05 Dec 2010

I did a similar test with normal non-group topics in another web and then the view protected topics are not listed. So it is related to Groups.

-- KennethLavrsen - 05 Dec 2010

I think I can shed some light on why Paul couldn't reproduce this: I remember him saying he created some test groups and set them to ALLOWTOPICVIEW = PaulHarvey. This way the issue could not be reproduced.

I tested this on my install now, result: The reported behaviour occurs only when the ACLs are passed a group, not an individual user.

Thus setting group topics to ALLOWTOPICVIEW = JohnDoe will work as expected, while setting them to ALLOWTOPICVIEW = AdminGroup will expose all but the first hit in a SEARCH.

-- HolstenerLiesel - 05 Dec 2010

I just tried to reproduce this on Release01x01 and trunk. I can't. Here's what I tried. Login manager Template login.

1. Logged in
2. Created Main.IdiotsGroup
   • Set ALLOWTOPICVIEW = AdminGroup
   • Set GROUP = SomeUser
3. Create Main.NumptiesGroup
   • Set ALLOWTOPICVIEW = AdminGroup
   • Set GROUP = SomeUser
4. Created Main.BlahBlah
   • Pasted the search Holstner described above.
5. Worked fine - neither topic visible in the list.

So, what am I doing that you are not (or vice versa)?

-- CrawfordCurrie - 06 Dec 2010

I used the new UI for group topics (which uses META:PREFERENCE). Since you mention Set GROUP, maybe you did not.

-- HolstenerLiesel - 06 Dec 2010

I also used the new UI so the ALLOWTOPICCHANGE and the GROUP are in meta.

I defined the ALLOWTOPICVIEW as a normal in-topic set statement.

-- KennethLavrsen - 07 Dec 2010

I changed the GROUP settings to META:PREFERENCE; still no problem.

Can you please attach the raw .txt for the two test topic, as well as your search topic?

-- CrawfordCurrie - 07 Dec 2010

Here's a set for you:

• AaGroup.txt
• AbGroup.txt
• TestGroup.txt
• SearchTopic.txt

Both AaGroup and AbGroup rely on TestGroup for ALLOWTOPICVIEW. WikiGuest is not a member of TestGroup. SearchTopic shows AbGroup as first hit when logged in (or rather logged out) as WikiGuest.

-- HolstenerLiesel - 07 Dec 2010

It took several episodes of a TV show I'm half watching, but I finally managed to reproduce this on my own PC. The search:

%SEARCH{
   search="Group$"
   type="regex"
   web="%WEB%"
   scope="topic"
   nonoise="on"
   format="   1. $topic"
}%

The result I get, both authenticated and unauthenticated: AbGroup, AdminGroup, NobodyGroup, TestGroup

Expected: AdminGroup, NobodyGroup

The topics of interest, which trigger the fault:

• AaGroup, ALLOWTOPICVIEW = OtherGroup (non-existent group/topic)
• AbGroup, ALLOWTOPICVIEW = AdminGroup
• TestGroup, ALLOWTOPICVIEW = AdminGroup

Here's the weird thing:

• If AaGroup is ALLOWTOPICVIEW = NonExistentGroup, the fault DOES occur.
• If AaGroup is ALLOWTOPICVIEW = NonExistentUser, the fault does not occur.
• If AaGroup is ALLOWTOPICVIEW = AdminGroup, the fault does not occur.
• If AaGroup is ALLOWTOPICVIEW = SomeUser, the fault does not occur.

I have attached the entire Main web (in case there's something really strange with an exact combination of specific topics).

I was running from a clean checkout (git reset --hard && git clean -f, followed by pseudo-install.pl developer and a fairly vanilla run at interactive configure).

-- PaulHarvey - 11 Dec 2010

I need to pay more attention. Didn't notice that HolstenerLiesel had already attached the test case

-- PaulHarvey - 11 Dec 2010

The problem is the fix to Item9809. In order to allow the USERSWEB to be protected, we set the {session}->{user} to admin 'temporarily'. However, this primes the Foswiki::MetaCache with *Group topics that aren't necessarily supposed to be visible to the real user.

Restoring the {session}->{user} back to the real user doesn't help, as the cache has already been primed with protected topics whose ACLs need to be checked again against the real user.

I've added the Meta/Search gurus Crawford and Sven for suggestions on how to avoid contaminating the cache when we temporarily escalate privileges like this. And Olivier, because he added the line of code

I am adding a unit test.

-- PaulHarvey - 12 Dec 2010

Better to ask Michael - I don't know any more about this than you do. I would suggest that the cache should be disabled when kicking into "privileged" mode, but I'm only guessing.

-- CrawfordCurrie - 12 Dec 2010

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 pushed the leaking of group info that the sessoin user is DENYed into Item10176.

this task is fixed for 1.1.3

-- SvenDowideit - 18 Dec 2010

Thanks Sven!

-- OlivierRaginel - 18 Dec 2010

No commits against release branch

-- GeorgeClark - 27 Feb 2011

Never mind - fixed in a different task. Actually not fixed. The unit tests were not brought to 1.1.3 from trunk so this appeared fixed. Opened Item10421

-- GeorgeClark - 28 Feb 2011

Commenting is disabled while not logged in