XWiki Platform
  1. XWiki Platform
  2. XWIKI-12912

Improve authorization checks based on the current user for accessing a VFS attachment archive

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 7.4-milestone-2
    • Fix Version/s: 8.3, 8.4-rc-1, 7.4.6
    • Component/s: VFS
    • Labels:
      None
    • Tests:
      Unit
    • Difficulty:
      Unknown
    • Documentation in Release Notes:
      N/A
    • Similar issues:

      Description

      Right now we have 2 problems:

      • we don't create any xwiki context and we don't authenticate the user
      • the authorization check we do is cached and thus if a user has access to a vfs node and then a user without access permission tries to access it, he'll be allowed...

        Issue Links

          Activity

          Show
          Vincent Massol added a comment - problem explained at https://java.net/projects/truezip/lists/users/archive/2015-12/message/8
          Hide
          Vincent Massol added a comment -
          Show
          Vincent Massol added a comment - Added integration test to prove the problem: https://github.com/xwiki/xwiki-platform/commit/bba49a4e7ff675be8cc4a885affc7f93fb5dea9f
          Hide
          Vincent Massol added a comment -

          Since there's no solution at the level of TrueVFS I'll implement the following:

          • Remove the view permission check from the Attach VFS Driver code (since TrueVFS cache its calls to it we cannot check permissions in there)
          • Add a view permission check in the PathConverter, i.e. when converting from a String to a NIO2 Path object. Since this conversion is called from velocity for any VFS API call, that should protect it (example of call: $stringtool.toString($niotool.readAllBytes("attach:Sandbox.WebHome@test.zip/test.txt"), "utf-8")).
          • This means that when using Java we won't have the permission check which means it'll be up to the Java code calling performing the call to the VFS API to add a check if need be.
          Show
          Vincent Massol added a comment - Since there's no solution at the level of TrueVFS I'll implement the following: Remove the view permission check from the Attach VFS Driver code (since TrueVFS cache its calls to it we cannot check permissions in there) Add a view permission check in the PathConverter, i.e. when converting from a String to a NIO2 Path object. Since this conversion is called from velocity for any VFS API call, that should protect it (example of call: $stringtool.toString($niotool.readAllBytes("attach:Sandbox.WebHome@test.zip/test.txt"), "utf-8") ). This means that when using Java we won't have the permission check which means it'll be up to the Java code calling performing the call to the VFS API to add a check if need be.
          Hide
          Vincent Massol added a comment -

          I've implemented a stopgap solution. Far from perfect but that should remove the security issue.

          Note that when the user doesn't have the permission and the VFS API is used from Velocity, no error is printed on the screen. The VFS code is just not executed. This is because I've implemented the check in the PathConverter and when an exception is thrown in a Converter it's currently silently ignored (there's not even a warning message in the console - Maybe something to improve in our Converter Manager?)

          Show
          Vincent Massol added a comment - I've implemented a stopgap solution. Far from perfect but that should remove the security issue. Note that when the user doesn't have the permission and the VFS API is used from Velocity, no error is printed on the screen. The VFS code is just not executed. This is because I've implemented the check in the PathConverter and when an exception is thrown in a Converter it's currently silently ignored (there's not even a warning message in the console - Maybe something to improve in our Converter Manager?)
          Hide
          Vincent Massol added a comment -

          FTR I've also introduced a new java API (VfsPathFactory) for java code wishing to test permissions while creating a TPath instance. Example:

          {{groovy}}
          import org.xwiki.vfs.*
          import java.nio.file.*
          
          def factory = services.component.getInstance(VfsPathFactory.class)
          def path = factory.create(URI.create("attach:VfsTest.testVfsMacro@test.zip/"))
          def ds = Files.newDirectoryStream(path)
          ds.each() {
            println "* ${it.getFileName().toString()}"
          }
          {{/groovy}}
          
          Show
          Vincent Massol added a comment - FTR I've also introduced a new java API (VfsPathFactory) for java code wishing to test permissions while creating a TPath instance. Example: {{groovy}} import org.xwiki.vfs.* import java.nio.file.* def factory = services.component.getInstance(VfsPathFactory.class) def path = factory.create(URI.create( "attach:VfsTest.testVfsMacro@test.zip/" )) def ds = Files.newDirectoryStream(path) ds.each() { println "* ${it.getFileName().toString()}" } {{/groovy}}

            People

            • Assignee:
              Vincent Massol
              Reporter:
              Vincent Massol
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: