• Do not register here on develop.twiki.org, login with your twiki.org account.
• Use View topic Item7848 for generic doc work for TWiki-6.1.1. Use View topic Item7851 for doc work on extensions that are not part of a release. More... Close
• Anything you create or change in standard webs (Main, TWiki, Sandbox etc) will be automatically reverted on every SVN update.
Does this site look broken?. Use the LitterTray web for test cases.

The TWiki::Access:checkAccessPermission method accepts the optional topic text. If the topic text is provided, it will only look in that text for Set (ALLOW|DENY)TOPIC$mode lines, ignoring any permissions in the META of the topic. If no topic text is given, permissions are retreived properly.


As a result (a.o.) the %SEARCH% function which uses the TWiki::Access:checkAccessPermission with a prefetched topic text will show topics that the user does not have view access to in the result list, usually along with a snippet of text. This could thus lead to unwanted disclosure of sensitive material.


The patch below (also attached) fixes the issue, introducing an implicit '* Set permission definitions override META permission definitions if the topic text is prefetched' definition, while the reverse is true for getTopicPreferencesValue (the method used if no prefetched text is supplied). The simple solution would be to change the order in TWiki::Prefs::PrefsCache::loadPrefsFromTopic. Another solution would be to reverse the order in Access.pm, but then you will always check using getTextPreferencesValue and we might as well drop the $text prefetched text option to checkAccessPermission completely..


--- lib/TWiki/Access.pm Wed Aug  2 13:39:12 2006
+++ lib/TWiki/Access.pm Wed Aug  2 14:22:06 2006
@@ -136,16 +136,21 @@

     # extract the * Set (ALLOWTOPIC|DENYTOPIC)$mode
     if( $text ) {
-        # override topic permissions. Note: ignores embedded metadata
-        # SMELL: this is horrible! But it's inevitable given the dreadful
-        # business of storing access controls embedded in topic text.
         $allowText = $prefs->getTextPreferencesValue( 'ALLOWTOPIC'.$mode,
                                                       $text, $web, $topic );
         $denyText = $prefs->getTextPreferencesValue( 'DENYTOPIC'.$mode,
                                                      $text, $web, $topic );
-    } elsif( $topic ) {
+    }
+
+    # if the (ALLOWTOPIC|DENYTOPIC)$mode are not defined (because no $text
+    # was supplied or because $text was supplied but didn't contain them)
+    # read them 'the proper way' (note: therefore permissions definitions
+    # in the topic text override those in the META data)
+    if(not defined($allowText)) {
         $allowText = $prefs->getTopicPreferencesValue( 'ALLOWTOPIC'.$mode,
                                                        $web, $topic );
+    }
+    if(not defined($denyText)) {
         $denyText = $prefs->getTopicPreferencesValue( 'DENYTOPIC'.$mode,
                                                       $web, $topic );
     }

-- TWiki:Main.KoenMartens

This is on the 4.0.4 hotfix 3 candidate list. Elevated it to requirement.

Since this is a patch for code that has a huge impact on TWiki we will need several core developers to review the proposed fix.

Both that it resolves the problem and that it does not break anything. Access rights is a long fragile string of rules which is easy to break.

-- KJL

I have confirmed that the bug above makes it easy to make a formatted search that will show the content of any topic in which the topic is protected from view by placing the ALLOWTOPICVIEW in "Edit Settings".

It is rediculously easy. It means that ALLOWTOPICVIEW in "Edit Settings" or META is worthless if people have write access to any other topic.

I have also tested with ALLOWTOPICVIEW in the topic and that works. You cannot view a protected topic with a formatted search when the ALLOWTOPICVIEW is in the topic. This was the only way to do it in Cairo so people that just upgraded or did not discover the new "Edit Settings" feature will not have a problem.

This bug really needs a fix fast which we can provide in KnownIssues on t.o.

-- KJL


As I understand it, the original reason for the $text parameter was performance; viz. to avoid re-reading a topic (to extract access controls) after it had already been read once. A better solution would have been to cache topics in the store, but given the complexity of the store code I'm not surprised that approach wasn't taken. I kept the interface the same because it is published to plugins through Func.

Anyway, there a number of different scenarios that have to be considered here:

  1. Access controls in text
    1. View latest rev of topic
    2. View old rev of topic
    3. Include topic in another topic
    4. Search for topic
    5. Search for protected topic content also hits topic that contains conflicting access rights
    6. TWiki::Func::checkAccessPermissions is passed $text
  2. Access controls in META
    • List as for 1
  3. Access controls mixed in text and meta
    • List as for 1
I fear that Koen's patch may break at least 1.2 and 1.6 above. This problem needs to be researched more completely - by writing a unit test that tests all the scenarios described above - before making any code changes. The unit test would also provide a platform for verifying any other concerns people may have.

CC


Yes, these are things I did not consider yet. I made the patch right after I detected the problem to protect a clients twiki where it is essential that certain topics are not publicly available.

However, I now see this is quite a quagmire.

Apparently, the Prefs API (TWiki::Prefs / TWiki::Prefs::PrefsCache) does not even support getting other revisions, so i think that is one area that needs extensive patching.

It makes me wonder what the current state is, with Set style permissions. If on the current revision you are not allowed to read the topic, but you were allowed on one of the earlier revisions, does that mean you may view those versions??

Actually, I just checked on a fresh 4.0.4 install on my laptop, and apparently this is not the case. I made a Sandbox topic that was publicly viewable. Then I created a new revision, revision 2, and put an ALLOWTOPICVIEW on that in the text. I was then not able to read revision 1 as TWikiGuest.

So, the problem is not that my patch breaks current behaviour, but actually the old behaviour was inconsistent. When you use text-based permissions, and pass the text of my revision 1 for example into checkAccessPermissions, you would get an 'ok to view' while in fact it is not ok to view!

Anyway, unit tests should be implemented indeed, especially on a subject as important as checking access permissions. I'm willing to get into this. However, I do need some pointers about unit tests in TWiki, and what is proper behaviour anyway.

KM

One detail.

Should the access rights be per version of a topic or should the latest access rights (or lack of) override earlier versions?

Put yourself in the users position and you have the answer. If someone wrote something secret and forgot to protect the content. And then come back and add the ALLOWTOPICVIEW - he wants to protect also the previous versions!

And if something was secret. And then you decide that now it is OK to share - you remove or change the ALLOWTOPICVIEW. And then you expect to share the entire history I am sure. So the current access rights should apply to older versions also. That is what is most natual and logical and useful.

The issue with this bug is that I can write a simple search in any topic on a TWiki and read a topic which is protected by ALLOWTOPICVIEW in "Edit settings".

And this security hole needs to be plugged in a hurry. I will add it to the KnownIssues page now. People need to know that the edit settings cannot be used for access rights. I will not keep them in the dark any longer.

I hope the resolution of this bug is not going to be pending Koen learning how to write unit tests. This is a security issue!!!

-- KJL

It may "need to be plugged in a hurry" but it has been in existance since permissions were first added to TWiki. I tried changing the scheme to set permissions per-revision, but the obvious security problem you note above stopped me.

My view is that permissions should be moved out of band, so that they are not tied to a rev of a topic. This is far too big a fix for a hotfix.

The issue with SEARCH results affecting permissions clearly needs to be remedied, of course, and ASAP.

CC

It is good that we agree on everything smile

  • The spec, ie. permissions based on latest revision only. I only commented because Koen asked the question and I wanted to help so a fix does not change the behavour.
  • That "permissions out of band" is for sure not the expected action in a hotfix. That is a roadmap feature. And probably needed to speed up TWiki.
  • That the original problem reported in this bug report is the hot issue: To plug the hole that a simple SEARCH can reveal the contents of a protected topic if the protection is inside "Edit Settings" (META). And hopefully without killing performance on searches too much.

KJL

Changed the order so that plagueins come second.

CC

Closed in 4.0.5 KJL

ItemTemplate
Summary TWiki::Access::checkAccessPermission ignores permissions in META if $text supplied (hotfix candidate)
ReportedBy TWiki:Main.KoenMartens
Codebase 4.0.0, 4.0.2, 4.0.4, ~twiki4, 4.0.1, 4.0.3
SVN Range not sure
AppliesTo Engine
Component

Priority Urgent
CurrentState Closed
WaitingFor

Checkins 11345 11390
TargetRelease patch
Topic attachments
I Attachment History Action Size Date Who Comment
Unknown file formatpatch Access.patch r1 manage 1.4 K 2006-08-02 - 13:00 KoenMartens Patch
Edit | Attach | Watch | Print version | History: r13 < r12 < r11 < r10 < r9 | Backlinks | Raw View | Raw edit | More topic actions
Topic revision: r13 - 2006-11-14 - KennethLavrsen
 
This site is powered by the TWiki collaboration platform Powered by PerlCopyright © 2008-2018 by the contributing authors. All material on this collaboration platform is the property of the contributing authors.
Ideas, requests, problems regarding TWiki? Send feedback