Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-10031

ReplicationHandler path traversal vulnerability

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 6.4
    • Fix Version/s: 5.5.4, 6.4.1, master (7.0)
    • Component/s: replication (java)
    • Security Level: Public (Default Security Level. Issues are Public)
    • Labels:
      None

      Description

      Fra: Mark Thomas <markt@apache.org>
      Emne: Fwd: Apache Solr - security vulnerability (path traversal attack)
      Dato: 24. januar 2017 kl. 13.14.36 CET
      Til: private@lucene.apache.org
      Kopi: "security@apache.org" <security@apache.org>
      Svar til: private@lucene.apache.org

      Dear Apache Lucene PMC,

      The security vulnerability report has been received by the Apache
      Security Team and is being passed to you for action.

      Please take careful note of the following:

      • This information is private and should be treated accordingly. The
        issue must not be discussed on a public mailing list, it must not be
        added to a public bug tracker, etc.
      • The Lucene PMC is responsible for resolving this issue. The security
        team is here to provide help and advice but the responsibility to do the
        work lies with the Lucene PMC.

      You may find the "ASF Project Security for Committers" [1] a useful
      reference. This e-mail represents step three of that process. Step 4
      should be completed asap.

      Kind regards,

      Mark

      [1] http://www.apache.org/security/committers.html

      -------- Forwarded Message --------
      Subject: Apache Solr - security vulnerability (path traversal attack)
      Date: Mon, 23 Jan 2017 11:27:19 -0800
      From: Hrishikesh Gadre <gadre.solr@gmail.com>
      To: security@apache.org
      CC: Hrishikesh Gadre <gadre.solr@gmail.com>

      Hi,

      We found a path manipulation security vulnerability in Apache Solr after
      running HPE Fortify static code analyzer on the Solr codebase.

      Here is a brief description of this issue,

      • Apache Solr provides a "replication" handler which supports operations
        related to querying the state of an index as well as copying files
        associated with the index.

      https://cwiki.apache.org/confluence/display/solr/Index+Replication
      <https://cwiki.apache.org/confluence/display/solr/Index+Replication>

      This handler supports an HTTP API
      (/replication?command=filecontent&file=<file_name>) which is vulnerable
      to path traversal attack. Specifically, this API does not perform any
      validation of the user specified file_name parameter. This can allow an
      attacker to download any file readable to Solr server process even if
      it is not related to the actual Solr index state.
      https://www.owasp.org/index.php/Path_Traversal

      I have verified this with the Solr version 6.3. But I believe this
      vulnerability to be present for much longer (going back to v 4.10.x) . I
      am currently working on the fix. Please let me know the process to
      submit a patch for this.

      Thanks
      Hrishikesh

      1. path_traversal_fix.patch
        5 kB
        Jan Høydahl
      2. SOLR-10031_branch5_5.patch
        5 kB
        Jan Høydahl
      3. SOLR-10031.patch
        4 kB
        Jan Høydahl
      4. SOLR-10031.patch
        4 kB
        Jan Høydahl
      5. SOLR-10031.patch
        4 kB
        Jan Høydahl
      6. SOLR-10031.patch
        4 kB
        Jan Høydahl

        Activity

        Hide
        janhoy Jan Høydahl added a comment -

        Patch with test.

        I have not tested explicitly that you can actually get files from parent folders with an unpatched system, is that necessary? The fix throws an HTTP 403 if file name is absolute or contains "..", or if it is not a legal file name. Should we add other checks?

        Show
        janhoy Jan Høydahl added a comment - Patch with test. I have not tested explicitly that you can actually get files from parent folders with an unpatched system, is that necessary? The fix throws an HTTP 403 if file name is absolute or contains "..", or if it is not a legal file name. Should we add other checks?
        Hide
        janhoy Jan Høydahl added a comment -

        Just saw that the reporter also prepared a similar patch (attached path_traversal_fix.patch).

        Show
        janhoy Jan Høydahl added a comment - Just saw that the reporter also prepared a similar patch (attached path_traversal_fix.patch).
        Hide
        thetaphi Uwe Schindler added a comment -

        I like your patch more, especially the whole "legacy" Stack class looks not good. This one uses Path class, which would allow to implement the whole thing much easier.

        Show
        thetaphi Uwe Schindler added a comment - I like your patch more, especially the whole "legacy" Stack class looks not good. This one uses Path class, which would allow to implement the whole thing much easier.
        Hide
        janhoy Jan Høydahl added a comment -

        I see that my patch will throw an error for file names validly containing the string sequence ... It would be nicer to detect only if the whole path segment is exactly "..". I'll post a patch which uses Path.forEach( action ) for this

        Show
        janhoy Jan Høydahl added a comment - I see that my patch will throw an error for file names validly containing the string sequence .. . It would be nicer to detect only if the whole path segment is exactly "..". I'll post a patch which uses Path.forEach( action ) for this
        Hide
        janhoy Jan Høydahl added a comment -

        New patch which loops path components.

        Show
        janhoy Jan Høydahl added a comment - New patch which loops path components.
        Hide
        thetaphi Uwe Schindler added a comment -

        +1

        The only difference to the bug reporter's patch is that he uses a stack to look if it "really escapes". But I think we should simply forbid all ".." anywhere in patch. If somebody tries to use this it is always some break-in attack.

        Show
        thetaphi Uwe Schindler added a comment - +1 The only difference to the bug reporter's patch is that he uses a stack to look if it "really escapes". But I think we should simply forbid all ".." anywhere in patch. If somebody tries to use this it is always some break-in attack.
        Hide
        janhoy Jan Høydahl added a comment -

        Another difference is that Hrishikesh's test only tests the FILE param, while the same vulnerability is in cf and tlogFile too. And I also disallow absolute paths, though I'm not sure if that is allowed in new File(path, file) anyway. His checks are in the calling code while mine is in the DirectoryFileStream constructor, guess as low as possible is a good thing.

        Show
        janhoy Jan Høydahl added a comment - Another difference is that Hrishikesh's test only tests the FILE param, while the same vulnerability is in cf and tlogFile too. And I also disallow absolute paths, though I'm not sure if that is allowed in new File(path, file) anyway. His checks are in the calling code while mine is in the DirectoryFileStream constructor, guess as low as possible is a good thing.
        Hide
        thetaphi Uwe Schindler added a comment -

        Absolute paths should definitely be disallowed!

        Show
        thetaphi Uwe Schindler added a comment - Absolute paths should definitely be disallowed!
        Hide
        janhoy Jan Høydahl added a comment -

        New patch that does not reveal the vulnerability in the CHANGES entry

        Show
        janhoy Jan Høydahl added a comment - New patch that does not reveal the vulnerability in the CHANGES entry
        Hide
        janhoy Jan Høydahl added a comment -

        For the record, the vulnerability is ACK'ed and we have requested a CVE.

        It could be argued whether it is a real vulnerability since we recommend a firewalled setup, and the user running Solr should have limited privileges.. But there are enough installs where IT employees may have HTTP access to a Solr instance but are not allowed file-system access to the same box, so this one is a bit special.

        Show
        janhoy Jan Høydahl added a comment - For the record, the vulnerability is ACK'ed and we have requested a CVE. It could be argued whether it is a real vulnerability since we recommend a firewalled setup, and the user running Solr should have limited privileges.. But there are enough installs where IT employees may have HTTP access to a Solr instance but are not allowed file-system access to the same box, so this one is a bit special.
        Hide
        thetaphi Uwe Schindler added a comment - - edited

        FYI, I just made the following proposal for the future on the private@lao list, which should also prevent issues like this:

        I think this issue should be seen as a first request for running Apache Solr in a security manager with a suitable prolicy file (like we do in tests). Elasticsearch has gone this route and I like that very much!

        Solr should install the security manager and policy in its main() methods before Jetty is starting. There it should restrict access to the installation folder, maybe we can even go further. It should also prevent stuff like calling System.exit() from plugins. This is especially important for all stuff that may execute java code (like Javascript in DIH or similar).

        Show
        thetaphi Uwe Schindler added a comment - - edited FYI, I just made the following proposal for the future on the private@lao list, which should also prevent issues like this: I think this issue should be seen as a first request for running Apache Solr in a security manager with a suitable prolicy file (like we do in tests). Elasticsearch has gone this route and I like that very much! Solr should install the security manager and policy in its main() methods before Jetty is starting. There it should restrict access to the installation folder, maybe we can even go further. It should also prevent stuff like calling System.exit() from plugins. This is especially important for all stuff that may execute java code (like Javascript in DIH or similar).
        Hide
        janhoy Jan Høydahl added a comment -

        CVE received from Mark Cox:

        Hi, please use

        CVE-2017-3163

        Mark

        Show
        janhoy Jan Høydahl added a comment - CVE received from Mark Cox: Hi, please use CVE-2017-3163 Mark
        Hide
        janhoy Jan Høydahl added a comment -

        Trying to understand the next step in the process. The policy says the commit message should not reveal the security breach until after release. But can we mention it in CHANGES.txt? How else's will people be aware of the importance of the fix?

        Show
        janhoy Jan Høydahl added a comment - Trying to understand the next step in the process. The policy says the commit message should not reveal the security breach until after release. But can we mention it in CHANGES.txt? How else's will people be aware of the importance of the fix?
        Hide
        thetaphi Uwe Schindler added a comment -

        Have a more generic message including a reference to this issue. But don't say what exactly was wrong. Don't mention the CVE number.

        Show
        thetaphi Uwe Schindler added a comment - Have a more generic message including a reference to this issue. But don't say what exactly was wrong. Don't mention the CVE number.
        Hide
        janhoy Jan Høydahl added a comment -

        So if we don't mention that a security issue was fixed anywhere in the released artifacts, then we do the release and rewrite the commit message, fine. But how do we communicate to the users/downloaders that this release actually fixes the given CVE? We can do it in the release announcement of course, and perhaps in the refGuide which is (currently) released after the binaries (although later on the refGuide will be released together).

        I see that ES has a separate security fixes webpage, perhaps that is a solution, so that people monitoring whether they should upgrade Solr due to CVEs could have one place to look it up?

        Show
        janhoy Jan Høydahl added a comment - So if we don't mention that a security issue was fixed anywhere in the released artifacts, then we do the release and rewrite the commit message, fine. But how do we communicate to the users/downloaders that this release actually fixes the given CVE? We can do it in the release announcement of course, and perhaps in the refGuide which is (currently) released after the binaries (although later on the refGuide will be released together). I see that ES has a separate security fixes webpage, perhaps that is a solution, so that people monitoring whether they should upgrade Solr due to CVEs could have one place to look it up?
        Hide
        thetaphi Uwe Schindler added a comment -

        When doing the same with the issues about remote code injection (XXE attachs) in OpenOffice/MS-Office documents (it was SVN times), I did the following (was mainly on project POI, but also affected TIKA and Solr):

        I think the most important thing is to have the release notes especially mention the issue and CVE numbers and tell user: "hey upgrade, there is an issue".

        Show
        thetaphi Uwe Schindler added a comment - When doing the same with the issues about remote code injection (XXE attachs) in OpenOffice/MS-Office documents (it was SVN times), I did the following (was mainly on project POI, but also affected TIKA and Solr): I committed stuff using the issue ID, not mentioning the security issue. I just said: "Issue XXX. Restricted external entity resolving." In the announcement on the web page I added the CVE numbers, the same in the announcement mail (you can still see those in the Solr News page, POI News page): https://lucene.apache.org/solr/news.html#18-august-2014-recommendation-to-update-apache-poi-in-apache-solr-480-481-and-490-installations , https://poi.apache.org/changes.html#Summary-N11D34 After the release, I changed the SVN commit messages (not sure if you can do this in Git, without breaking checkouts). The CVE numbers were added to the issue (which were public) afterwards. I think the most important thing is to have the release notes especially mention the issue and CVE numbers and tell user: "hey upgrade, there is an issue".
        Hide
        janhoy Jan Høydahl added a comment -

        Ok, I have not run full tests and precommit with this yet. Have to go now, feel free to take over for a while so we can get it in 6.4.1 in time (without revealing that this JIRA contains a CVE on the public dev-list)

        Show
        janhoy Jan Høydahl added a comment - Ok, I have not run full tests and precommit with this yet. Have to go now, feel free to take over for a while so we can get it in 6.4.1 in time (without revealing that this JIRA contains a CVE on the public dev-list)
        Hide
        thetaphi Uwe Schindler added a comment - - edited

        OK, if you have some time you can commit the stuff, otherwise I will do this before Adrien spins 6.4.1.

        Show
        thetaphi Uwe Schindler added a comment - - edited OK, if you have some time you can commit the stuff, otherwise I will do this before Adrien spins 6.4.1.
        Hide
        janhoy Jan Høydahl added a comment -

        Passing precommit. Cannot get test suite to ever pass 100% but the failures are not related, and running only TestReplicationHandler succeeds.

        Show
        janhoy Jan Høydahl added a comment - Passing precommit. Cannot get test suite to ever pass 100% but the failures are not related, and running only TestReplicationHandler succeeds.
        Hide
        cpoerschke Christine Poerschke added a comment -

        The replication handler is one of the implicit plugins now, previously presumably it always needed to be configured in solrconfig.xml (and some of the test files still do that e.g. this one).

        Might it be worth mentioning somehow somewhere (afterwards) that the replication handler is configurable and that configuring a (custom) ReplicationHandler class with the fix would be an interim way of getting the fix without the upgrade?

        We would be sending a mixed message then though, whereas "upgrade recommended" is a clear message and anyone wishing to build and deploy a custom patched replication handler would probably have that idea by themselves anyhow.

        Show
        cpoerschke Christine Poerschke added a comment - The replication handler is one of the implicit plugins now, previously presumably it always needed to be configured in solrconfig.xml (and some of the test files still do that e.g. this one). Might it be worth mentioning somehow somewhere (afterwards) that the replication handler is configurable and that configuring a (custom) ReplicationHandler class with the fix would be an interim way of getting the fix without the upgrade? We would be sending a mixed message then though, whereas "upgrade recommended" is a clear message and anyone wishing to build and deploy a custom patched replication handler would probably have that idea by themselves anyhow.
        Hide
        janhoy Jan Høydahl added a comment -

        Hmm, are you thinking about people using 5.5.3, which is also a "supported" version. I think perhaps that once we acknowledge it as a security risk we should consider plugging the hole in 5.x with a 5.5.4 bugfix release. It's a small patch which should be easy enough to backport.

        Perhaps this should be coordinated so 5.x people have a true quick upgrade path once the announcement is made?

        We could also create a Wiki page for the CVE explaining what the risk is, how to exploit it and how to work around it if you cannot upgrade, such as turning off replication if you don't use it, making sure no untrusted clients have access to the /replication endpoint etc.

        Show
        janhoy Jan Høydahl added a comment - Hmm, are you thinking about people using 5.5.3, which is also a "supported" version. I think perhaps that once we acknowledge it as a security risk we should consider plugging the hole in 5.x with a 5.5.4 bugfix release. It's a small patch which should be easy enough to backport. Perhaps this should be coordinated so 5.x people have a true quick upgrade path once the announcement is made? We could also create a Wiki page for the CVE explaining what the risk is, how to exploit it and how to work around it if you cannot upgrade, such as turning off replication if you don't use it, making sure no untrusted clients have access to the /replication endpoint etc.
        Hide
        janhoy Jan Høydahl added a comment -

        Attaching patch backport to 5_5.

        Show
        janhoy Jan Høydahl added a comment - Attaching patch backport to 5_5.
        Hide
        janhoy Jan Høydahl added a comment -

        New patch crediting Hrishikesh Gadre and removing unnecessary positive test. Think this is ready to go in

        Show
        janhoy Jan Høydahl added a comment - New patch crediting Hrishikesh Gadre and removing unnecessary positive test. Think this is ready to go in
        Hide
        janhoy Jan Høydahl added a comment -

        Committed
        master: 6f598d24692a89da9b5b671be6cf4b947aa39266
        branch_6x: 7088137d52256354a52ed86547b9faa0e7042934
        branch_6_4: 3a4f885b18bc963a8326c752bd229497908f1dbc
        branch_5_5: ae789c252687dc8a18bfdb677f2e6cd14570e4db

        Show
        janhoy Jan Høydahl added a comment - Committed master: 6f598d24692a89da9b5b671be6cf4b947aa39266 branch_6x: 7088137d52256354a52ed86547b9faa0e7042934 branch_6_4: 3a4f885b18bc963a8326c752bd229497908f1dbc branch_5_5: ae789c252687dc8a18bfdb677f2e6cd14570e4db
        Hide
        janhoy Jan Høydahl added a comment -

        Resolving since all code has landed in the branches.

        Reminder: Post release we need to make a security announcement, make this JIRA public and rewrite the GIT commit log. msg.

        Show
        janhoy Jan Høydahl added a comment - Resolving since all code has landed in the branches. Reminder: Post release we need to make a security announcement, make this JIRA public and rewrite the GIT commit log. msg.
        Hide
        janhoy Jan Høydahl added a comment -

        Reopening to make public

        Show
        janhoy Jan Høydahl added a comment - Reopening to make public
        Hide
        janhoy Jan Høydahl added a comment -

        Issue made public now that ANNOUNCEMENT is made and 5.5.4 includes the fix.

        Show
        janhoy Jan Høydahl added a comment - Issue made public now that ANNOUNCEMENT is made and 5.5.4 includes the fix.

          People

          • Assignee:
            janhoy Jan Høydahl
            Reporter:
            janhoy Jan Høydahl
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development