Solr
  1. Solr
  2. SOLR-4882

Restrict SolrResourceLoader to only classloader accessible files and instance dir

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.3
    • Fix Version/s: 4.6, Trunk
    • Component/s: None
    • Labels:

      Description

      SolrResourceLoader currently allows to load files from any absolute/CWD-relative path, which is used as a fallback if the resource cannot be looked up via the class loader.

      We should limit this fallback to sub-dirs below the instanceDir passed into the ctor. The CWD special case should be removed, too (the virtual CWD is instance's config or root dir).

      The reason for this is security related. Some Solr components allow to pass in resource paths via REST parameters (e.g. XSL stylesheets, velocity templates,...) and load them via resource loader. By this it is possible to limit the whole thing to
      not allow loading e.g. /etc/passwd as a stylesheet.

      In 4.4 we should add a solrconfig.xml setting to enable the old behaviour, but disable it by default, if your existing installation requires the files from outside the instance dir which are not available via the URLClassLoader used internally. In Lucene 5.0 we should not support this anymore.

      1. SOLR-4882.patch
        14 kB
        Uwe Schindler
      2. SOLR-4882.patch
        12 kB
        Uwe Schindler
      3. SOLR-4882.patch
        10 kB
        Uwe Schindler

        Issue Links

          Activity

          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          113d 18h 55m 1 Uwe Schindler 21/Sep/13 15:59
          Uwe Schindler made changes -
          Comment [ I had to backport SOLR-3648 (fix Velocity template loading in SolrCloud mode), too. Otherwise it did not work. ]
          Uwe Schindler made changes -
          Attachment SOLR-4882-fix.patch [ 12618093 ]
          Uwe Schindler made changes -
          Attachment SOLR-4882-fix.patch [ 12618094 ]
          Uwe Schindler made changes -
          Attachment SOLR-4882-fix.patch [ 12618094 ]
          Uwe Schindler made changes -
          Attachment SOLR-4882-fix.patch [ 12618093 ]
          Uwe Schindler made changes -
          Labels security
          Hide
          ASF subversion and git services added a comment -

          Commit 1546958 from Uwe Schindler in branch 'dev/branches/lucene_solr_3_6'
          [ https://svn.apache.org/r1546958 ]

          SOLR-5520: Backport of SOLR-4882 (SolrResourceLoader was restricted to only allow access to resource files below the instance dir)

          Show
          ASF subversion and git services added a comment - Commit 1546958 from Uwe Schindler in branch 'dev/branches/lucene_solr_3_6' [ https://svn.apache.org/r1546958 ] SOLR-5520 : Backport of SOLR-4882 (SolrResourceLoader was restricted to only allow access to resource files below the instance dir)
          Uwe Schindler made changes -
          Link This issue is required by SOLR-5520 [ SOLR-5520 ]
          Uwe Schindler made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Fix Version/s 4.6 [ 12325000 ]
          Fix Version/s 4.5 [ 12324743 ]
          Resolution Fixed [ 1 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1525248 from Uwe Schindler in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1525248 ]

          Merged revision(s) 1525246 from lucene/dev/trunk:
          SOLR-4882: Restrict SolrResourceLoader to only allow access to resource files below the instance dir

          Show
          ASF subversion and git services added a comment - Commit 1525248 from Uwe Schindler in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1525248 ] Merged revision(s) 1525246 from lucene/dev/trunk: SOLR-4882 : Restrict SolrResourceLoader to only allow access to resource files below the instance dir
          Hide
          ASF subversion and git services added a comment -

          Commit 1525246 from Uwe Schindler in branch 'dev/trunk'
          [ https://svn.apache.org/r1525246 ]

          SOLR-4882: Restrict SolrResourceLoader to only allow access to resource files below the instance dir

          Show
          ASF subversion and git services added a comment - Commit 1525246 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1525246 ] SOLR-4882 : Restrict SolrResourceLoader to only allow access to resource files below the instance dir
          Uwe Schindler made changes -
          Attachment SOLR-4882.patch [ 12604396 ]
          Hide
          Uwe Schindler added a comment -

          Here is the final patch for trunk. I also added a test that checks for escaping instance dir

          Show
          Uwe Schindler added a comment - Here is the final patch for trunk. I also added a test that checks for escaping instance dir
          Hide
          Uwe Schindler added a comment -

          Hi,

          nobody commented on this issue, so I think the current patch is fine. I would like to commit this for 4.6.

          After that is resolved, we can also do SOLR-5234.

          Show
          Uwe Schindler added a comment - Hi, nobody commented on this issue, so I think the current patch is fine. I would like to commit this for 4.6. After that is resolved, we can also do SOLR-5234 .
          Steve Rowe made changes -
          Fix Version/s 4.5 [ 12324743 ]
          Fix Version/s 4.4 [ 12324324 ]
          Hide
          Steve Rowe added a comment -

          Bulk move 4.4 issues to 4.5 and 5.0

          Show
          Steve Rowe added a comment - Bulk move 4.4 issues to 4.5 and 5.0
          Uwe Schindler made changes -
          Description SolrResourceLoader currently allows to load files from any absolute/CWD-relative path, which is used as a fallback if the resource cannot be looked up via the class loader.

          We should limit this fallback to sub-dirs below the instanceDir passed into the ctor. The CWD special case should be removed, too (the virtual CWD is instance's config or root dir).

          The reason for this is security related. Some Solr components allow to pass in resource paths via REST parameters (e.g. XSL stalesheets,...) and load them via resource loader. By this it is possible to limit the whole thing to
          not allow loading e.g. /etc/passwd as a stylesheet.

          In 4.4 we should add a solrconfig.xml setting to enable the old behaviour, but disable it by default, if your existing installation requires the files from outside the instance dir which are not available via the URLClassLoader used internally. In Lucene 5.0 we should not support this anymore.
          SolrResourceLoader currently allows to load files from any absolute/CWD-relative path, which is used as a fallback if the resource cannot be looked up via the class loader.

          We should limit this fallback to sub-dirs below the instanceDir passed into the ctor. The CWD special case should be removed, too (the virtual CWD is instance's config or root dir).

          The reason for this is security related. Some Solr components allow to pass in resource paths via REST parameters (e.g. XSL stylesheets, velocity templates,...) and load them via resource loader. By this it is possible to limit the whole thing to
          not allow loading e.g. /etc/passwd as a stylesheet.

          In 4.4 we should add a solrconfig.xml setting to enable the old behaviour, but disable it by default, if your existing installation requires the files from outside the instance dir which are not available via the URLClassLoader used internally. In Lucene 5.0 we should not support this anymore.
          Uwe Schindler made changes -
          Attachment SOLR-4882.patch [ 12585618 ]
          Uwe Schindler made changes -
          Attachment SOLR-4882.patch [ 12585644 ]
          Uwe Schindler made changes -
          Attachment SOLR-4882.patch [ 12585618 ]
          Hide
          Uwe Schindler added a comment - - edited

          Patch that removes the legacy resource loader from velocity. It now only uses the SolrResourceLoader (wrapped) and the ParameterResourceLoader.

          As SolrResourceLoader can also read files from the instance dir, there is no backside, but you can no longer escape the sandbox. Also the special setting for the base dir to velocity default file loader was removed, as it allowed to change the "base" dir for loading vm files to be changed from the request URL -> same problem as the XSL issues

          Show
          Uwe Schindler added a comment - - edited Patch that removes the legacy resource loader from velocity. It now only uses the SolrResourceLoader (wrapped) and the ParameterResourceLoader. As SolrResourceLoader can also read files from the instance dir, there is no backside, but you can no longer escape the sandbox. Also the special setting for the base dir to velocity default file loader was removed, as it allowed to change the "base" dir for loading vm files to be changed from the request URL -> same problem as the XSL issues
          Hide
          Uwe Schindler added a comment -

          I found maybe another security problem with velocity (have to investigate). By default it uses SolrResourceLoader to find vm files, but it may also fall back to config directory (if not in zookeeper mode). We should probably simply disable this (the file resource loader) in VelocityResponseWriter#getEngine(). SolrResourceLoader and the special params resource loader should be way enough to find the templates, everything else makes it unmaintainable.

          Show
          Uwe Schindler added a comment - I found maybe another security problem with velocity (have to investigate). By default it uses SolrResourceLoader to find vm files, but it may also fall back to config directory (if not in zookeeper mode). We should probably simply disable this (the file resource loader) in VelocityResponseWriter#getEngine(). SolrResourceLoader and the special params resource loader should be way enough to find the templates, everything else makes it unmaintainable.
          Hide
          Uwe Schindler added a comment -

          How do paths work when under ZK? Is it allowed to do ../../some/path to read ZK data outside "conf"?

          No idea at all. I don't know how ZK works, I only know that ZK gets queried with the full path, I have no idea if ../ and stuff works at all with ZK. I think Mark can explain. I just fixed one small thing in the ZKResourceLoader, unrelated to ZK (the slashes when querying classloader must be forward).

          Probably good enough. An alternative to allow-everything could be a config option safePaths=/etc/solr/conf,/ext/other/path option to let RL see selected safe paths outside instanceDir. Windows users don't have symlinks so this could be nice for htem.

          Windows 7 and Server 2008 have symlinks. Nevertheless, this was the question to Mark miller. We have a hierarchy of SolrResourceLoaders (one for the startup and one for each core). Ideally, we would atomatically allow access to the directories of the parent ResourceLoader. Unfortunately, there is currently only a hierrarchy in the ClassLoaders not in the SolrResourceLoader as a whole (the core SolrResourceLoaders are not childs of the parent SolrResourceLoader, only the inner ClassLoaders use the hierarchy.

          Show
          Uwe Schindler added a comment - How do paths work when under ZK? Is it allowed to do ../../some/path to read ZK data outside "conf"? No idea at all. I don't know how ZK works, I only know that ZK gets queried with the full path, I have no idea if ../ and stuff works at all with ZK. I think Mark can explain. I just fixed one small thing in the ZKResourceLoader, unrelated to ZK (the slashes when querying classloader must be forward). Probably good enough. An alternative to allow-everything could be a config option safePaths=/etc/solr/conf,/ext/other/path option to let RL see selected safe paths outside instanceDir. Windows users don't have symlinks so this could be nice for htem. Windows 7 and Server 2008 have symlinks. Nevertheless, this was the question to Mark miller. We have a hierarchy of SolrResourceLoaders (one for the startup and one for each core). Ideally, we would atomatically allow access to the directories of the parent ResourceLoader. Unfortunately, there is currently only a hierrarchy in the ClassLoaders not in the SolrResourceLoader as a whole (the core SolrResourceLoaders are not childs of the parent SolrResourceLoader, only the inner ClassLoaders use the hierarchy.
          Hide
          Jan Høydahl added a comment -

          Probably good enough. An alternative to allow-everything could be a config option safePaths=/etc/solr/conf,/ext/other/path option to let RL see selected safe paths outside instanceDir. Windows users don't have symlinks so this could be nice for htem.

          How do paths work when under ZK? Is it allowed to do ../../some/path to read ZK data outside "conf"?

          Show
          Jan Høydahl added a comment - Probably good enough. An alternative to allow-everything could be a config option safePaths=/etc/solr/conf,/ext/other/path option to let RL see selected safe paths outside instanceDir. Windows users don't have symlinks so this could be nice for htem. How do paths work when under ZK? Is it allowed to do ../../some/path to read ZK data outside "conf"?
          Hide
          Uwe Schindler added a comment -

          Ah patch is against branch_4x, because I was expecting to remove the absolute path mode from trunk, so I wanted the merge this way.

          Show
          Uwe Schindler added a comment - Ah patch is against branch_4x, because I was expecting to remove the absolute path mode from trunk, so I wanted the merge this way.
          Uwe Schindler made changes -
          Field Original Value New Value
          Attachment SOLR-4882.patch [ 12585600 ]
          Hide
          Uwe Schindler added a comment - - edited

          Attached is the patch implementing this with a system property.

          The patch revisits the current logic of the open resource call of SolrResourceLoader and also fixes some possible bugs. It also uses the "hack" to make tests work only when the test mode of Solr is enabled. This closes more "leaks" that can be used.

          This should also improve security of velocity templates, i am not sure about their functionality but they are also loaded by SolrResourceLoader, as far as I see. This reduces more attack vectors.

          All tests pass, some tests had to be changed:

          • 2 tests needed the sysprop set, because they were explicitely testing the access to absolute paths
          • Some missing close() on SolrResourceLoader was added to get rid of warnings in Eclipse.
          • One test was changed to SolrTestCaseJ4, so the test mode was correctly enabled (this was not needed before, so it extended LuceneTestCase only)

          The Solr example starts successfully and I have seen no problems. If you are affected by this change you get an IOException warpped by the SolrException explaining that you may enable the mode for escaping the restricted resource loader.

          The test if the path is from inside the restricted path is done by transforming the file paths (absolute path, not canonic path, to enable playing with symlinks pointing from Solr conf directory to the outside) to URIs and then trying to relativize the URI, which is only possible if the normalized (./.. removed) URIs start with the same prefix and are not opaque. If this fails, the paths are considered as not related and the restriction is enforced.

          Tell me what you think, Hoss & others. We don't need to hurry.

          Show
          Uwe Schindler added a comment - - edited Attached is the patch implementing this with a system property. The patch revisits the current logic of the open resource call of SolrResourceLoader and also fixes some possible bugs. It also uses the "hack" to make tests work only when the test mode of Solr is enabled. This closes more "leaks" that can be used. This should also improve security of velocity templates, i am not sure about their functionality but they are also loaded by SolrResourceLoader, as far as I see. This reduces more attack vectors. All tests pass, some tests had to be changed: 2 tests needed the sysprop set, because they were explicitely testing the access to absolute paths Some missing close() on SolrResourceLoader was added to get rid of warnings in Eclipse. One test was changed to SolrTestCaseJ4, so the test mode was correctly enabled (this was not needed before, so it extended LuceneTestCase only) The Solr example starts successfully and I have seen no problems. If you are affected by this change you get an IOException warpped by the SolrException explaining that you may enable the mode for escaping the restricted resource loader. The test if the path is from inside the restricted path is done by transforming the file paths (absolute path, not canonic path, to enable playing with symlinks pointing from Solr conf directory to the outside) to URIs and then trying to relativize the URI, which is only possible if the normalized (./.. removed) URIs start with the same prefix and are not opaque. If this fails, the paths are considered as not related and the restriction is enforced. Tell me what you think, Hoss & others. We don't need to hurry.
          Hide
          Hoss Man added a comment -

          ... In Lucene 5.0 we should not support this anymore.

          FWIW: it's not hard to imagine situations where people have legitimate desire for using absolute paths like this. ie: loading synonyms or stop words from some central location outside of their solr home dir (eg: /etc/solr-common/stopwords/en.txt, used by multiple solr instances, with diff solr home dirs, running on diff ports.

          With that in mind, I don't think it makes sense to completely remove this ability – but it certainly makes sense to disable it by default and document the risks.

          In 4.4 we should add a solrconfig.xml setting to enable the old behaviour, but disable it by default...

          Given the lifecycle of the resource loaders, it may not be easy to have this configuration per-core in solrconfig.xml. I'm also not sure if it's worth adding as a solr.xml config option given the complexities in how that file is peristet after core operations (and how many times we've screwed ourselves adding things to that file)

          Given that this is something (i think) we should generally discourage, and something that i don't think we should be shy about making "hard" to turn on, it might be enough just to say that the only way you can enable it is with an explicit (and scary named) system property that affects the entire Solr instance?

          Show
          Hoss Man added a comment - ... In Lucene 5.0 we should not support this anymore. FWIW: it's not hard to imagine situations where people have legitimate desire for using absolute paths like this. ie: loading synonyms or stop words from some central location outside of their solr home dir (eg: /etc/solr-common/stopwords/en.txt, used by multiple solr instances, with diff solr home dirs, running on diff ports. With that in mind, I don't think it makes sense to completely remove this ability – but it certainly makes sense to disable it by default and document the risks. In 4.4 we should add a solrconfig.xml setting to enable the old behaviour, but disable it by default... Given the lifecycle of the resource loaders, it may not be easy to have this configuration per-core in solrconfig.xml. I'm also not sure if it's worth adding as a solr.xml config option given the complexities in how that file is peristet after core operations (and how many times we've screwed ourselves adding things to that file) Given that this is something (i think) we should generally discourage, and something that i don't think we should be shy about making "hard" to turn on, it might be enough just to say that the only way you can enable it is with an explicit (and scary named) system property that affects the entire Solr instance?
          Uwe Schindler created issue -

            People

            • Assignee:
              Uwe Schindler
              Reporter:
              Uwe Schindler
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development