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

Public share protect (4) #123

Closed
wants to merge 7 commits into from
Closed

Public share protect (4) #123

wants to merge 7 commits into from

Conversation

phil-davis
Copy link
Contributor

After rebase of PR #90 just now, this is on top with the acceptance test commit cherry-picked from PR #111

@phil-davis
Copy link
Contributor Author

phil-davis commented May 19, 2020

I tried making a public share of just a file, then trying to download that 3 times with the wrong password.
The brute force protection kicks in on the 3rd download attempt good.
But another request with the correct password downloads the file, when I expect it to be blocked.

    And the administrator has set the bruteforceprotection settings to: # BruteForceProtectionContext::setTheBruteforceprotectionSettings()
      | threshold-time | 60  |
      | fail-tolerance | 2   |
      | ban-period     | 300 |

  Scenario: access to public link is (not) blocked after too many invalid requests                                                                              # /home/phil/git/owncloud/core/apps-external/brute_force_protection/tests/acceptance/features/apiBruteForceProtection/bruteforceprotection.feature:91
[Tue May 19 17:59:33 2020] 127.0.0.1:40196 Accepted
[Tue May 19 17:59:33 2020] 127.0.0.1:40196 [201]: PUT /remote.php/webdav/randomfile.txt
[Tue May 19 17:59:33 2020] 127.0.0.1:40196 Closing
    Given user "Alice" has uploaded file with content "user1 file" to "randomfile.txt"                                                                        # FeatureContext::userHasUploadedAFileWithContentTo()
[Tue May 19 17:59:33 2020] 127.0.0.1:40198 Accepted
[Tue May 19 17:59:34 2020] 127.0.0.1:40198 [200]: POST /ocs/v1.php/apps/files_sharing/api/v1/shares
[Tue May 19 17:59:34 2020] 127.0.0.1:40198 Closing
    When user "Alice" creates a public link share using the sharing API with settings                                                                         # FeatureContext::userCreatesAPublicLinkShareWithSettings()
      | path     | randomfile.txt |
      | password | %public%       |
[Tue May 19 17:59:34 2020] 127.0.0.1:40200 Accepted
[Tue May 19 17:59:34 2020] Login failed: 'public' (Remote IP: '127.0.0.1')
[Tue May 19 17:59:34 2020] 127.0.0.1:40200 [401]: GET /remote.php/dav/public-files/creh7Far9YK7C12/randomfile.txt
[Tue May 19 17:59:34 2020] 127.0.0.1:40200 Closing
    And the public download of the last publicly shared file using the new public WebDAV API with password "12345" should fail with HTTP status code "401"    # PublicWebDavContext::theLastPublicSharedFileShouldNotBeAbleToBeDownloadedWithPassword()
[Tue May 19 17:59:34 2020] 127.0.0.1:40202 Accepted
[Tue May 19 17:59:34 2020] Login failed: 'public' (Remote IP: '127.0.0.1')
[Tue May 19 17:59:34 2020] 127.0.0.1:40202 [401]: GET /remote.php/dav/public-files/creh7Far9YK7C12/randomfile.txt
[Tue May 19 17:59:34 2020] 127.0.0.1:40202 Closing
    And the public download of the last publicly shared file using the new public WebDAV API with password "12345" should fail with HTTP status code "401"    # PublicWebDavContext::theLastPublicSharedFileShouldNotBeAbleToBeDownloadedWithPassword()
[Tue May 19 17:59:34 2020] 127.0.0.1:40204 Accepted
[Tue May 19 17:59:34 2020] Caused by: {"Exception":"OC\\User\\LoginException","Message":"Too many failed login attempts. Try again in  5 minutes.","Code":0,"Trace":"#0 \/home\/phil\/git\/owncloud\/core\/apps-external\/brute_force_protection\/lib\/Hooks.php(100): OCA\\BruteForceProtection\\Throttle->applyBruteForcePolicyForLogin()\n#1 \/home\/phil\/git\/owncloud\/core\/lib\/composer\/symfony\/event-dispatcher\/EventDispatcher.php(264): OCA\\BruteForceProtection\\Hooks->preLoginCallback()\n#2 \/home\/phil\/git\/owncloud\/core\/lib\/composer\/symfony\/event-dispatcher\/EventDispatcher.php(239): Symfony\\Component\\EventDispatcher\\EventDispatcher->doDispatch()\n#3 \/home\/phil\/git\/owncloud\/core\/lib\/composer\/symfony\/event-dispatcher\/EventDispatcher.php(73): Symfony\\Component\\EventDispatcher\\EventDispatcher->callListeners()\n#4 \/home\/phil\/git\/owncloud\/core\/lib\/private\/User\/Session.php(517): Symfony\\Component\\EventDispatcher\\EventDispatcher->dispatch()\n#5 \/home\/phil\/git\/owncloud\/core\/lib\/private\/User\/Session.php(333): OC\\User\\Session->loginWithPassword(*** sensitive parameters replaced ***)\n#6 \/home\/phil\/git\/owncloud\/core\/lib\/private\/User\/Session.php(362): OC\\User\\Session->login(*** sensitive parameters replaced ***)\n#7 \/home\/phil\/git\/owncloud\/core\/apps\/dav\/lib\/Connector\/Sabre\/Auth.php(131): OC\\User\\Session->logClientIn(*** sensitive parameters replaced ***)\n#8 \/home\/phil\/git\/owncloud\/core\/lib\/composer\/sabre\/dav\/lib\/DAV\/Auth\/Backend\/AbstractBasic.php(103): OCA\\DAV\\Connector\\Sabre\\Auth->validateUserPass(*** sensitive parameters replaced ***)\n#9 \/home\/phil\/git\/owncloud\/core\/apps\/dav\/lib\/Connector\/Sabre\/Auth.php(239): Sabre\\DAV\\Auth\\Backend\\AbstractBasic->check()\n#10 \/home\/phil\/git\/owncloud\/core\/apps\/dav\/lib\/Connector\/Sabre\/Auth.php(156): OCA\\DAV\\Connector\\Sabre\\Auth->auth()\n#11 \/home\/phil\/git\/owncloud\/core\/lib\/composer\/sabre\/dav\/lib\/DAV\/Auth\/Plugin.php(182): OCA\\DAV\\Connector\\Sabre\\Auth->check()\n#12 \/home\/phil\/git\/owncloud\/core\/lib\/composer\/sabre\/dav\/lib\/DAV\/Auth\/Plugin.php(137): Sabre\\DAV\\Auth\\Plugin->check()\n#13 \/home\/phil\/git\/owncloud\/core\/lib\/composer\/sabre\/event\/lib\/WildcardEmitterTrait.php(89): Sabre\\DAV\\Auth\\Plugin->beforeMethod()\n#14 \/home\/phil\/git\/owncloud\/core\/lib\/composer\/sabre\/dav\/lib\/DAV\/Server.php(454): Sabre\\DAV\\Server->emit()\n#15 \/home\/phil\/git\/owncloud\/core\/lib\/composer\/sabre\/dav\/lib\/DAV\/Server.php(251): Sabre\\DAV\\Server->invokeMethod()\n#16 \/home\/phil\/git\/owncloud\/core\/apps\/dav\/lib\/Server.php(329): Sabre\\DAV\\Server->start()\n#17 \/home\/phil\/git\/owncloud\/core\/apps\/dav\/appinfo\/v2\/remote.php(31): OCA\\DAV\\Server->exec()\n#18 \/home\/phil\/git\/owncloud\/core\/remote.php(165): require_once('\/home\/phil\/git\/...')\n#19 {main}","File":"\/home\/phil\/git\/owncloud\/core\/apps-external\/brute_force_protection\/lib\/Throttle.php","Line":102}
[Tue May 19 17:59:34 2020] 127.0.0.1:40204 [401]: GET /remote.php/dav/public-files/creh7Far9YK7C12/randomfile.txt
[Tue May 19 17:59:34 2020] 127.0.0.1:40204 Closing
    And the public download of the last publicly shared file using the new public WebDAV API with password "12345" should fail with HTTP status code "401"    # PublicWebDavContext::theLastPublicSharedFileShouldNotBeAbleToBeDownloadedWithPassword()
[Tue May 19 17:59:34 2020] 127.0.0.1:40206 Accepted
[Tue May 19 17:59:34 2020] 127.0.0.1:40206 [200]: GET /remote.php/dav/public-files/creh7Far9YK7C12/randomfile.txt
    And the public download of the last publicly shared file using the new public WebDAV API with password "%public%" should fail with HTTP status code "401" # PublicWebDavContext::theLastPublicSharedFileShouldNotBeAbleToBeDownloadedWithPassword()
      response body is not valid XML, maybe download did work
      response body: 
      user1 file
      
      Failed asserting that false is not false.

The 4 "interesting" requests are all:

GET /remote.php/dav/public-files/creh7Far9YK7C12/randomfile.txt

accompanied by different authentication.

The 3rd call with a bad password triggers all that "Exception":"OC\\User\\LoginException","Message":"Too many failed login attempts. Try again in 5 minutes." stuff. But failedLoginCallback never happens, so addFailedLoginAttempt does not happen. So "the system" does not remember the 3rd bad password attempt. So it never really triggers blocking for 5 minutes.

failedLoginCallback happens in the first 2 API calls, but not in the 3rd call.

IMO this does not work the way it should.

@karakayasemi your thoughts?

Scenario: access to public link is not blocked after too many invalid requests
Given user "Alice" has uploaded file with content "user1 file" to "/PARENT/randomfile.txt"
When user "Alice" creates a public link share using the sharing API with settings
| path | PARENT |
Copy link
Contributor

@karakayasemi karakayasemi May 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phil-davis I am trying to figure out the issue, this one is the problematic scenario right?
Is there a mistake on column names in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made some changes locally to convince myself that there really is a problem.
But they are on my desktop computer, and we have had big storms and power issues here, so that is currently off and I have no power for it!

You should be able to to manually create a link share of a file, then do API calls to try and download the file, using an endpoint like:

GET /remote.php/dav/public-files/creh7Far9YK7C12/randomfile.txt

And provide a wrong password a few times.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I manually created randomfile.txt containing the text "some data" in the UI of a user and created a public link with password "pass". Then tried to access it multiple times with the wrong password:

$ curl -u public:pwd123 http://172.17.0.1:8080/remote.php/dav/public-files/3M5BhjTb4wCLQTC/randomfile.txt
<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception\NotAuthenticated</s:exception>
  <s:message>Username or password was incorrect, No public access to this resource., Username or password was incorrect, Username or password was incorrect</s:message>
</d:error>
$ curl -u public:pwd123 http://172.17.0.1:8080/remote.php/dav/public-files/3M5BhjTb4wCLQTC/randomfile.txt
<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception\NotAuthenticated</s:exception>
  <s:message>Username or password was incorrect, No public access to this resource., Username or password was incorrect, Username or password was incorrect</s:message>
</d:error>
$ curl -u public:pwd123 http://172.17.0.1:8080/remote.php/dav/public-files/3M5BhjTb4wCLQTC/randomfile.txt
<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception\NotAuthenticated</s:exception>
  <s:message>Username or password was incorrect, No public access to this resource., Username or password was incorrect, Username or password was incorrect</s:message>
</d:error>
$ curl -u public:pwd123 http://172.17.0.1:8080/remote.php/dav/public-files/3M5BhjTb4wCLQTC/randomfile.txt
<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception\NotAuthenticated</s:exception>
  <s:message>Too many failed login attempts. Try again in  5 minutes.</s:message>
</d:error>

And then access it with the correct password:

$ curl -u public:pass http://172.17.0.1:8080/remote.php/dav/public-files/3M5BhjTb4wCLQTC/randomfile.txt
some data

The file downloaded. IMO the download should have been blocked.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the brute force protection logic is relying on events that emitted from core endpoints. If it is not working in any endpoint, we may have missed emitting some events from this endpoint.

I will look at it and report back here. By the way, thanks for driving this pr to forward.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phil-davis As I suspect, DAV accesses are not emitting the necessary events for link share auth. I opened a core PR to fix it in here: owncloud/core#37430

@phil-davis
Copy link
Contributor Author

Updated tests have been added to #90

@phil-davis phil-davis closed this May 27, 2020
@phil-davis phil-davis deleted the public-share-protect-4 branch May 27, 2020 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants