Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sanitise the http requests #3316

Merged
merged 4 commits into from
Oct 14, 2022
Merged

Sanitise the http requests #3316

merged 4 commits into from
Oct 14, 2022

Conversation

vascoguita
Copy link
Contributor

Add http.ServeMux to http server to sanitise http requests and mitigate XSS.
Use html.EscapeString whenever responding to an http request with user-provided values (internal/http/services/ocmd/invites.go and pkg/siteacc/siteacc.go).

@update-docs
Copy link

update-docs bot commented Oct 6, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@vascoguita vascoguita requested review from ishank011 and a team as code owners October 6, 2022 09:57
@lgtm-com
Copy link

lgtm-com bot commented Oct 6, 2022

This pull request fixes 3 alerts when merging defa86a into db57930 - view on LGTM.com

fixed alerts:

  • 3 for Reflected cross-site scripting

@lgtm-com
Copy link

lgtm-com bot commented Oct 6, 2022

This pull request fixes 3 alerts when merging c1dd14e into db57930 - view on LGTM.com

fixed alerts:

  • 3 for Reflected cross-site scripting

pkg/rhttp/rhttp.go Outdated Show resolved Hide resolved
pkg/rhttp/rhttp.go Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Oct 6, 2022

This pull request fixes 3 alerts when merging ff4312e into e58cd30 - view on LGTM.com

fixed alerts:

  • 3 for Reflected cross-site scripting

@lgtm-com
Copy link

lgtm-com bot commented Oct 7, 2022

This pull request fixes 3 alerts when merging 1802b1e into 1e4948b - view on LGTM.com

fixed alerts:

  • 3 for Reflected cross-site scripting

@lgtm-com
Copy link

lgtm-com bot commented Oct 7, 2022

This pull request fixes 3 alerts when merging f463253 into 1e4948b - view on LGTM.com

fixed alerts:

  • 3 for Reflected cross-site scripting

@phil-davis
Copy link
Contributor

https://drone.cernbox.cern.ch/cs3org/reva/9096/10/6

Scenario: send DELETE requests to webDav endpoints with 2 slashes                                        # /drone/src/tmp/testrunner/tests/acceptance/features/apiAuthWebDav/webDavSpecialURLs.feature:13
    When user "Alice" requests these endpoints with "DELETE" using password "%regular%" about user "Alice" # OCSContext::userSendsRequestToTheseEndpointsWithOutBodyUsingPassword()
      | endpoint                                            |
      | //remote.php/webdav/textfile0.txt                   |
      | //remote.php//dav/files/%username%/textfile1.txt    |
      | /remote.php//dav/files/%username%/PARENT/parent.txt |
      | /remote.php//webdav/PARENT                          |
      | //remote.php/dav//files/%username%//FOLDER          |
    Then the HTTP status code of responses on all endpoints should be "204"                                # FeatureContext::theHTTPStatusCodeOfResponsesOnAllEndpointsShouldBe()
      Expected same but found different http status codes of last requested responses.Found status codes: 200,200,200,501,501 (Exception)

Those are examples of URLs that have multiple / in various places. They all pass on ownCloud10. The routing/URL-parsing manages to cope with it. But with the ServeMux changes here, it seems that ServeMux is not coping with the last 2 examples - it returns 501

In real life a client that sends such requests should be fixed anyway! So maybe you are OK with these odd-looking-URL tests failing. If so, then add them to the expected-failures files:
https://github.com/cs3org/reva/blob/master/tests/acceptance/expected-failures-on-OCIS-storage.md
https://github.com/cs3org/reva/blob/master/tests/acceptance/expected-failures-on-S3NG-storage.md

If not, then work out how ServeMux has to be set up so that it can understand and route these requests.

@phil-davis
Copy link
Contributor

  Scenario: send PUT requests to webDav endpoints with 2 slashes                                                                       # /drone/src/tmp/testrunner/tests/acceptance/features/apiAuthWebDav/webDavSpecialURLs.feature:184
    When user "Alice" requests these endpoints with "PUT" including body "doesnotmatter" using password "%regular%" about user "Alice" # OCSContext::userSendsRequestToTheseEndpointsWithBodyUsingPassword()
      | endpoint                                             |
      | //remote.php/webdav/textfile0.txt                    |
      | /remote.php//webdav/textfile1.txt                    |
      | //remote.php//dav/files/%username%/textfile1.txt     |
      | /remote.php/dav/files/%username%/textfile7.txt       |
      | //remote.php/dav/files/%username%/PARENT//parent.txt |
    Then the HTTP status code of responses on all endpoints should be "204" or "201"                                                   # FeatureContext::theHTTPStatusCodeOfResponsesOnAllEndpointsShouldBeOr()
      Unexpected status code received 200

This one is strange - these PUT requests were returning 204 or 201, but now they return 200. Why is ServeMux processing these "OK" but returning a different 2xx HTTP status then the previous way it was done?

Again, add to expected failures if you do not care about this.

@phil-davis
Copy link
Contributor

https://drone.cernbox.cern.ch/cs3org/reva/9096/12/6

Scenario: Copy file within a public link folder new public WebDAV API                         # /drone/src/tmp/testrunner/tests/acceptance/features/apiSharePublicLink2/copyFromPublicLink.feature:8
    Given user "Alice" has uploaded file with content "some data" to "/PARENT/testfile.txt"     # FeatureContext::userHasUploadedAFileWithContentTo()
    And user "Alice" has created a public link share with settings                              # FeatureContext::userHasCreatedAPublicLinkShareWithSettings()
      | path        | /PARENT                   |
      | permissions | read,update,create,delete |
    When the public copies file "/testfile.txt" to "/copy1.txt" using the new public WebDAV API # PublicWebDavContext::thePublicCopiesFileUsingTheWebDAVApi()
    Then the HTTP status code should be "201"                                                   # FeatureContext::thenTheHTTPStatusCodeShouldBe()
      HTTP status code 200 is not the expected value 201
      Failed asserting that 200 matches expected '201'.

The failures in this test pipeline are of real things. In this example the returned status has changed from 201 to 200 - maybe that is a real problem, or maybe no client will care.

But:

Scenario: Copy folder within a public link folder new public WebDAV API                                 # /drone/src/tmp/testrunner/tests/acceptance/features/apiSharePublicLink2/copyFromPublicLink.feature:20
    Given user "Alice" has created folder "/PARENT/testFolder"                                            # FeatureContext::userHasCreatedFolder()
    And user "Alice" has uploaded file with content "some data" to "/PARENT/testFolder/testfile.txt"      # FeatureContext::userHasUploadedAFileWithContentTo()
    And user "Alice" has created a public link share with settings                                        # FeatureContext::userHasCreatedAPublicLinkShareWithSettings()
      | path        | /PARENT                   |
      | permissions | read,update,create,delete |
    When the public copies folder "/testFolder" to "/testFolder-copy" using the new public WebDAV API     # PublicWebDavContext::thePublicCopiesFileUsingTheWebDAVApi()
    Then the HTTP status code should be "201"                                                             # FeatureContext::thenTheHTTPStatusCodeShouldBe()
      HTTP status code 501 is not the expected value 201
      Failed asserting that 501 matches expected '201'.

In this case, the receiver of a public link share is copying a folder that is inside the public link share, to create another folder inside the public link share. A 501 is returned, so the request is broken. That looks like it really needs to be fixed.

In a lot of the other failures in that pipeline, the request is returning 200 instead of a more-specific 2xx HTTP status code. So maybe there is something in ServeMux that is causing a "generic" 200 code to get returned?

@lgtm-com
Copy link

lgtm-com bot commented Oct 14, 2022

This pull request fixes 3 alerts when merging 181e1b4 into 0a5d64b - view on LGTM.com

fixed alerts:

  • 3 for Reflected cross-site scripting

@labkode labkode merged commit 9bc72e6 into cs3org:master Oct 14, 2022
vascoguita added a commit to vascoguita/reva that referenced this pull request Oct 18, 2022
vascoguita added a commit to vascoguita/reva that referenced this pull request Oct 18, 2022
@vascoguita vascoguita deleted the patch_xss branch October 19, 2022 12:02
vascoguita added a commit to vascoguita/reva that referenced this pull request Oct 19, 2022
vascoguita added a commit to vascoguita/reva that referenced this pull request Oct 20, 2022
vascoguita added a commit to vascoguita/reva that referenced this pull request Oct 21, 2022
vascoguita added a commit to vascoguita/reva that referenced this pull request Oct 26, 2022
vascoguita added a commit to vascoguita/reva that referenced this pull request Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants