Bug: Diffs Function Does Not Authenticate Properly

• problem occurs with a web that has ALLOWWEBVIEW set to a specific group

• in TWikiRelease01Dec2001, view authentication for the topic a diff was requested for was not checked in bin/rdiff
  thus, it was possible to view diffs for any topic in a web for everyone, once the topic name was known, independent of the ALLOWWEBVIEW setting

• in TWikiRelease01Feb2003, an authentication check was added in bin/rdiff
  however, a user already authenticated (using viewauth) is not able to use rdiff, it will fail with "View Access Denied". It seems that rdiff is requested for Main.Guest

• The problem remains in Cairo release

• the authenticate mechanism used by viewauth and rdiff seem to be different (as well as the debug mechnism...)

Test case

before I build a test case: I would like to learn what rdiff is supposed to do, in order to let it write useful debug info

Environment

-- FlorenzKley - 18 Feb 2003

Follow up

My users got hit by this. I made a quick and dirty workaround by symlinking the rdiff script to rdiffauth and requesting authentification for rdiffauth. You can thus view diffs for protected webs via rdiffauth.

This tends to indicate that the same mecanism could be implemented for rdiff than the one with view/viewauth. I will try to experiment today.

-- ColasNahaboo - 28 Mar 2003

Fix record

I made this patch which seems to work OK. Please test and provide feedback. You need to copy (or better symlink) a bin/rdiffauth script from the bin/rdiff, and ad to your bin/.htaccess the declaration:

<Files "rdiffauth">
       require valid-user
</Files>

• DiffsFunctionDoesNotAuthenticateProperly.patch: tentative patch for Feb 2003 version to bin/rdiff
• DiffsFunctionDoesNotAuthenticateProperly.patch2: patch to add rdiffauth to bin/.htaccess

-- ColasNahaboo - 28 Mar 2003

Is now in TWikiAlphaRelease and at TWiki.org. Docs need to get updated.

I made a slightly different patch to account for proper authentication of included topics:

*** ../../b/bin/rdiff   Sat Feb  1 00:57:32 2003
--- rdiff       Thu Apr 10 22:51:40 2003
***************
*** 173,179 ****
      # check access permission
      my $wikiUserName = &TWiki::userToWikiName( $userName );
      my $viewAccessOK = &TWiki::Access::checkAccessPermission( "view", $wikiUserName, "", $topic, $webName );
!     if( ( $TWiki::readTopicPermissionFailed ) || ( ! $viewAccessOK ) ) {
          my $url = &TWiki::getOopsUrl( $webName, $topic, "oopsaccessview" );
          TWiki::redirect( $query, $url );
          return;
--- 173,199 ----
      # check access permission
      my $wikiUserName = &TWiki::userToWikiName( $userName );
      my $viewAccessOK = &TWiki::Access::checkAccessPermission( "view", $wikiUserName, "", $topic, $webName );
!     if( $TWiki::readTopicPermissionFailed ) {
!         # Can't read requested topic and/or included (or other accessed topics)
!         # user could not be authenticated, may be not logged in yet?
!         my $rdiffauthFile = $ENV{'SCRIPT_FILENAME'};
!         $rdiffauthFile =~ s|/rdiff|/rdiffauth|o;
!         if( ( ! $theRemoteUser ) && (-e $rdiffauthFile ) ) {
!             # try again with authenticated rdiffauth script
!             # instead of non authenticated rdiff script
!             my $url = $ENV{"REQUEST_URI"};
!             if( $url ) {
!                 # $url i.e. is "twiki/bin/rdiff.cgi/Web/Topic?cms1=val1&cmd2=val2"
!                 $url =~ s|/rdiff|/rdiffauth|o;
!                 $url = "$TWiki::urlHost$url";
!             } else {
!                 $url = "$TWiki::urlHost$scriptUrlPath/$rdiffauthFile/$webName/$topic";
!             }
!             TWiki::redirect( $query, $url );
!             return;
!         }
!     }
!     if( ! $viewAccessOK ) {
          my $url = &TWiki::getOopsUrl( $webName, $topic, "oopsaccessview" );
          TWiki::redirect( $query, $url );
          return;

-- PeterThoeny - 11 Apr 2003

RdiffCgiScript with ColasNahaboo's modified patch applied (revision 1.30 in CVS) doesn't handle view authentication quite right. Specifically:

• Authenticated mode activation (see ImproveViewAuthentication) does not depend on topic's view permissions; it may depend on web's view permissions due to CheckAccessPermissionBrokenForMainAndTWikiWebs bug, if SmiliesPlugin (or any other plug-in that doesn't use internal flag when reading its preferences) is installed.
  • This is because auth. mode activation is triggered with $TWiki::readTopicPermissionFailed variable, which is not affected by functions that RdiffCgiScript calls for the topic it is processing.
• The intended (I think it is?) activation of auth. mode in the case of failed permission check for included topics doesn't work.
  • This is because auth. mode activation code is executed before actual text processing (and therefore before reading any included topics).

Here is the patch for RdiffCgiScript, generated against Beijing release; it fixes all these issues. Some related notes/questions:

• I moved a part of auth. mode activation code into the function; maybe this function should be placed in some .pm file (it makes real sence in context of ImproveViewAuthentication).
• I changed some regular expressions, so that they became more robust. Also, I added RE quotation (\Q\E) where appropriate (specifically, "\Q$TWiki::scriptSuffix\E") - here quotation is not important until $scriptSuffix contains something uncommon, but what about other places? I guess there are quite a lot of places in TWiki where variable values are included in REs!

-- PavelGoran - 06 Jul 2003

The patch I uploaded contained a small error; now it's fixed.

-- PavelGoran - 13 Jul 2003

Installing TWiki:Plugins/SmartSessionPlugin solves all of these problems without the need for patching. And it prevents other problems. For example:

• Lots of templates link to edit and rdiff; what to do about them?
• So now the URL has changed; what happens when someone copies and pastes the URL to someone who has not authenticated?
• What happens when future plugins or additions add scripts also need authentication?
• It seems like a mess to patch TWiki, especially patching this much of TWiki (scripts, templates, etc.)
• You could always turn on $doRememberRemoteUser, but that fails on IP change, etc.

So my first suggestion was TWiki:Codev/BetterThandoRememberRemoteUser, which requires a small (six line) patch to TWiki.pm and a little bit of a change of directory structure, and for anyone who supports symlinks, solves all problems above without any changes.

But I didn't like patching TWiki, and it's a pain that it depends on symlinks (a pain for Windows users). Plus, I knew in all of the other sites I've administered, I've used cookies to store login information via sessions.

Seeing the already existing TWiki:Plugins/SessionPlugin, I figured someone else had already thought of this, but that plugin doesn't solve these problems, and it doesn't even use a session library from CPAN (like CGI::Session, PHP::Session, or Apache::Session).

So I wrote TWiki:Plugins/SmartSessionPlugin, which is a complete rewrite of TWiki:Plugins/SessionPlugin, but it is much more configurable, requires (but still supports) no bin/logon script, and will soon support transparent session ID passing (which keeps it from needing cookies, but also causes problems... (i.e., there are tradeoffs)).

So basically, if you're looking for a number of good ways to solve this problem, I'd recommend taking a look at (in order of complexity, with simplest first):

• TWiki:Plugins/SmartSessionPlugin (TWiki:Plugins/SmartSessionPluginDev)
• TWiki:Codev/BetterThandoRememberRemoteUser
• TWiki:Codev/ImproveViewAuthentication

-- TedPavlic - 17 Jul 2003

Whilst a plugin that fixes this is nice (means people can close this hole now), having a security flaw in the core code is BAD.

-- MichaelSparks - 20 Jul 2003

The plug-in doesn't actually fix the bug, it only makes it show up less frequently - but it would still appear when following the direct URL to RdiffCgiScript (from notification message, for example).

(See also my comment from 18 Jul in ImproveViewAuthentication.)

-- PavelGoran - 20 Jul 2003

This looks like something that definitely needs to go in, but due to the recent TWiki::UI refactoring it's not going to be all that clean a merge. I'm going to mark it as PatchAdjustmentRequired and hope someone comes along and applies the patch to the UI module, and uploads a diff from that, preferably 'cvs diff -u' output. Of course, maybe someone else in the core is more interested in fixing this than I, in which case they're welcome to handle all the details and commit. I'm not really cut out for a TWikiPatchCoordinatorRole, I guess.

-- WalterMundt - 20 Apr 2004

It looks like the correct test would be if( $TWiki::readTopicPermissionFailed ne "" ). It turns out that nonempty strings except "0" count as "true", with only "0", undef, or "" counting as false in Perl conditions (this is in one of the perldoc pages), so I take that back - since the var in question will never be set to "0", the old test of just if( $TWiki::readTopicPermissionFailed ) was fine.

-- JonathanGraehl - 21 Jun 2004

Bumped to Dakaar due to incomplete implementation and documentation. See IRC logs of 28 June 2004 for rationale/discussion.

-- CrawfordCurrie - 30 Jun 2004

The bug susbsists in Cairo. I made a quick fix in my case (no SessionPlugin) but it is not a general fix.

-- ColasNahaboo - 03 Jan 2005

This is a bug impacting security, and should be fixed in Dakar.

Category: TWikiPatches

See also: DiffDoesNotAuthenticate

Since access control is now done at the topic access level for the logged-in user, and I can't reproduce this, I'm marking it as fixed.

-- CrawfordCurrie - 25 Mar 2005