Zammad's Misconfigured Rack_Attack.rb Does Not Appropriately Protect Against Brute Force Attacks in zammad/zammad

Valid

Reported on

Jun 22nd 2022


Description

Zammad relies on the rack_attack.rb file to defend the application against various brute force attacks, including forgotten password requests, ticket submissions, etc. The currently utilized Rack_Attack.rb file's configuration attempts to prevent password reset requests per IP to 3 per minute. This resulted in 429 errors being issued after the 3rd attempt, as declared in the Rack_Attack file. This works appropriately until the tester placed a random string after the /api/v1/users/password_reset path location in a captured Proxy request. Appending the characters ".json" to the end of /api/v1/users/password_reset (/api/v1/users/password_reset.json) allowed the tester to run hundreds of password reset requests against the server, bypassing the Rack_Attack restrictions.

The /api/v1/form_submit path was also found to be vulnerable.

Proof of Concept

Password Reset PoC - Note .json appended to end of path

POST /api/v1/users/password_reset.json HTTP/1.1
Host: localhost:8080
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:101.0) Gecko/20100101 Firefox/101.0
Accept: application/json, text/javascript, */*; q=0.01
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
Referer: http://localhost:8080/
Content-Type: application/json
X-Requested-With: XMLHttpRequest
X-CSRF-Token: tF2RQB380o7ulITTsKuhSzZKgtqqMkbILL+gIpCMi0p7g0wN+lC/oA3lnIH0FOi17kCiO5DrJ6G4fm4Q9i8FZg==
Content-Length: 33
Origin: http://localhost:8080
DNT: 1
Connection: close
Cookie: _zammad_session_a138cfd0f37=9a80633fcf555db2687c7f22b114d42f
Sec-Fetch-Dest: empty
Sec-Fetch-Mode: cors
Sec-Fetch-Site: same-origin

{"username":"def@mayorsec%2ecom"}

Impact

The lack of properly configured constraints in the Rack_Attack file allows for unlimited amounts of form and reset password submissions, bypassing the application's intended logic.

We are processing your report and will contact the zammad team within 24 hours. a month ago
Joe Helle modified the report
a month ago
We created a GitHub Issue asking the maintainers to create a SECURITY.md a month ago
We have contacted a member of the zammad team and are waiting to hear back a month ago
zammad/zammad maintainer
a month ago

Maintainer


Test comment

zammad/zammad maintainer
a month ago

Maintainer


Hello Joe, thank you so much for your report. I can confirm the issues regarding appending .json to the configured request URLs, and we will provide a fix for this one. My current idea would be using start_with? rather than == to check the request path, would you agree to this?

However, regarding authentication brute force, Zammad does contain a prevention mechanism against this which is not based on rack_attack. Failed log-in attempts will be recorded on user records, and after a certain small amount of such failed attempts users will be invalidated, preventing further log-ins.

Therefore I would ask you for more details about this part of your report. How can we reproduce it? Right now I'm assuming that this is not a valid concern. This would also mean the criticality of your report would be lower.

Joe Helle
a month ago

Researcher


Correct on the rack_attack fix.

In the case of brute forcing the authentication, I am able to make a significant number of attempts without being invalidated (180 listed in this report, but it was really hundreds in additional testing). If the application uses a different mechanism as you said, I would question both the appropriateness of the number of allowed attempts, or if it's actually working at all. Based on that, I would not reduce the severity and would even go as far as recommending a rack_attack configuration to prevent the brute force authentication attacks.

If you would prefer I could modify this report to address the rack attack vulnerability for the password resets (which with the resets I would reduce severity on), and submit a separate report on the authentication issue. But again, the default installation allowed this large amount of attempts to go unchecked, and the issue is resolvable with a rack_attack modification. I'm happy to help you with the code to implement that if desired.

zammad/zammad maintainer
a month ago

Maintainer


Would you be so kind to provide us with more details on how exactly this can be reproduced? E.g. a sample request?

Zammad has a password_max_login_failed setting which defaults to 5. After 5 failed logins, the user is prevented from logging in, and we do have tests to cover that (e.g. spec/lib/auth/user_spec.rb:55, spec/lib/auth_spec.rb:72.

Joe Helle
a month ago

Researcher


Sure, so I apologize for not including more in-depth instructions for the brute force authentication. I've included a link here to help visually see the process - https://www.notion.so/themayor/Zammad-Authentication-Brute-Force-4b9bb936ba344e9fb6b562e71d9d7a0f.

I just tested the issue extensively and am finding that I can consistently get results up to request #50, which would be out of range of what you've specified. Requests after 50 appear to be inconsistent, and as such I won't comment much on it, however I was able to reproduce a valid request at line 184, which was similar to the original finding.

Note that I've been using a macro that automates the CSRF response in order to bypass CSRF token error issues. This doesn't appear to have an actual effect on the issue but was helping me with testing to bypass those errors. I included screenshots of this process as well for information, but again, I don't believe it had a verifiable effect on the process as it works without it.

Please let me know if you have any questions.

zammad/zammad maintainer
a month ago

Maintainer


Thank you so much for the details Joe. I will analyze this in detail early next week. One question though, if I may. Is it possible that your tool performs the requests in a massively parallel manner? That might explain the issue - concurrent web server processed handle them more or less at the same time, which means that not after 5 failed requests the log-ins become prevented, but after way more. That might explain the behaviour you are seeing, while still being consistent how Zammad is designed to work up to this point.

Joe Helle
a month ago

Researcher


I think that's actually a good idea and a distinct possibility. I just tested with a single thread, and the lockout worked as expected. Testing with 10 concurrent threads bypasses it and allows for more attempts than permitted.

At this point I think it may be of value for us to segregate the two issues presented in this report. @admin is there an efficient way to do this being as it's apparent that there are two separate issues here? I'm happy to clean this one up, low the CVSS for the rack_attack vulnerability slightly, and submit a new report for the concurrency issue.

I believe the new finding would be something like CWE-362 Concurrent Execution Using Shared Resource with Improper Synchronization, or CWE-440 Expected Behavior Violation. I would report the criticality of the issue as is for the current report.

Please let me know if this is acceptable to you as well @maintainer. Thank you for working with me on determining the root cause.

Joe Helle
a month ago

Researcher


And just to confirm the overall vuln, the higher the concurrent threads, the deeper into the wordlist I can go. I'm consistently finding the passwords in the high 100's, and low 200's. The tool doesn't support any more threads than I am trying unfortunately.

We have sent a follow up to the zammad team. We will try again in 7 days. a month ago
Jamie Slome
a month ago

Admin


@Joe - thanks for ping ☎️

Feel free to split your reports, if the maintainer is happy. You can edit this report and reduce the severity as mentioned, plus submit a new report focusing more specifically on the other issue.

Just be aware that this is only possible if the other issue is a different vulnerability/CWE. We do not allow the stacking of reports for different vulnerabilities, as we encourage researchers to submit this as multiple occurrences or permalinks into a single report.

zammad/zammad maintainer
a month ago

Maintainer


@jamieslome @dievus I agree to the split. We are talking about different issues with different fixes and classifications.

@dievus so if you could update the current report to reflect only the parts about the wrong URL matching in rack_attack with a lower severity and a different CWE, plus opening a new report for the brute force prevention of concurrent requests, please? I will acknowledge both reports. The severity of the second one should not be that high, though, as it does not allow unrestricted brute force attacks even in the unfixed state.

zammad/zammad maintainer
a month ago

Maintainer


@dievus we decided to try and fix the brute force prevention issue by proper DB synchronization rather than rack_attack.

To verify, would you be able to test the changes? Just exchange the valid? method in lib/auth.rb with the following code:

  def valid?
    validated = auth_user&.user&.with_lock do
      if auth_user.can_login? && backends.valid?
        auth_user.update_last_login
        true
      else
        auth_user.increase_login_failed if increase_login_failed_attempts
        false
      end
    end

    avoid_brute_force_attack if !validated
    validated == true
  end

That should prevent the issue no matter how much concurrency is involved. I would highly appreciate your feedback on this.

Joe Helle
a month ago

Researcher


I'll get to work on splitting them. Both are CVE eligible issues in my opinion, and I'll ask @admin to issue for both. My updated CVSS for the rack_attack specific issue is 8.2 (AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:L/A:H). I will change this report's CWE to Expected Behavior Violations as the expectation was for the application to prevent these, and the underlying code was not managed correctly.

As for the code, are you fixing the rack_attack vulns mentioned in the report through that implementation, or is this specific to the authentication portal only?

Based on my testing, my only limitation in the brute force authentication issue was the tool itself. I could probably spend a bit of time writing out a script that would allow for increased threads, which would continue to affect the allowed amount. As referenced before, it seems more like a RACE condition to me than anything else. This is covered under CWE-366. I have evidence that NIST scores these between 7 and 8.

@admin when we clean the report up, is it possible to remove comments?

Jamie Slome
a month ago

Admin


Sure, we can remove comments 👍

Joe Helle modified the report
a month ago
Joe Helle
a month ago

Researcher


This report is modified to reflect the rack_attack issue.

Joe Helle
a month ago

Researcher


@maintainer I submitted the race condition as a separate issue. https://huntr.dev/bounties/1b5bf096-fc67-42eb-a556-8fc2b3fe9233

zammad/zammad maintainer
a month ago

Maintainer


@dievus the code above for valid? is for the issue you split out, of course. The original issue will be fixed by replacing the == checks with start_with? in rack_attack.rb. Config as follows:

# Copyright (C) 2012-2022 Zammad Foundation, https://zammad-foundation.org/

#
# Throttle password reset requests
#
API_V1_USERS__PASSWORD_RESET_PATH = '/api/v1/users/password_reset'.freeze
Rack::Attack.throttle('limit password reset requests per username', limit: 3, period: 1.minute.to_i) do |req|
  if req.path.start_with?(API_V1_USERS__PASSWORD_RESET_PATH) && req.post?
    # Normalize to protect against rate limit bypasses.
    req.params['username'].to_s.downcase.gsub(%r{\s+}, '')
  end
end
Rack::Attack.throttle('limit password reset requests per source IP address', limit: 3, period: 1.minute.to_i) do |req|
  if req.path.start_with?(API_V1_USERS__PASSWORD_RESET_PATH) && req.post?
    req.ip
  end
end

#
# Throttle form submit requests
#
API_V1_FORM_SUBMIT_PATH = '/api/v1/form_submit'.freeze
form_limit_by_ip_per_hour_proc = proc { Setting.get('form_ticket_create_by_ip_per_hour') || 20 }
Rack::Attack.throttle('form submits per IP and hour', limit: form_limit_by_ip_per_hour_proc, period: 1.hour.to_i) do |req|
  if req.path.start_with?(API_V1_FORM_SUBMIT_PATH)
    req.ip
  end
end
form_limit_by_ip_per_day_proc = proc { Setting.get('form_ticket_create_by_ip_per_day') || 240 }
Rack::Attack.throttle('form submits per IP and day', limit: form_limit_by_ip_per_day_proc, period: 1.day.to_i) do |req|
  if req.path.start_with?(API_V1_FORM_SUBMIT_PATH)
    req.ip
  end
end
form_limit_per_day_proc = proc { Setting.get('form_ticket_create_per_day') || 5000 }
Rack::Attack.throttle('form submits per day', limit: form_limit_per_day_proc, period: 1.day.to_i) do |req|
  if req.path.start_with?(API_V1_FORM_SUBMIT_PATH)
    req.path
  end
end
zammad/zammad maintainer
a month ago

Maintainer


@dievus we usually request CVE IDs for our security advisories. How may we credit you there? Suggestion:

  • N: Joe Helle
  • W: https://themayor.tech/
zammad/zammad maintainer has acknowledged this report a month ago
Joe Helle
a month ago

Researcher


Huntr can issue them through their CNA status here on the platform so you wouldn't have to handle any of that overhead. I'll ping the poor @admin team one more time for that.

As for the security advisory, are you referring to that placement on Github? If so, here's an example of another maintainer who has done the same. https://github.com/inventree/InvenTree/security/advisories/GHSA-mmm6-rwf8-ghv3

My name on Github is @dievus. I also have a Twitter if you would like to reference it - @joehelle.

As for the rack_attack modification, I believe that needs to be starts_with? rather than start_with?. I've tested the starts_with? changes locally with other projects and it has worked appropriately to resolve the problem.

Jamie Slome
a month ago

Admin


Same here @maintainer - if you would like a CVE for this report, please adjust CVE at the top right of the report to "Yes" 👍

zammad/zammad maintainer
a month ago

Maintainer


@dievus: start_with should be correct, please see https://stackoverflow.com/questions/34945669/do-starts-with-and-start-with-have-the-same-function-in-ruby - starts_with? is no longer in the Rails version we use (6.1).

Joe Helle
a month ago

Researcher


Understood. Thanks for the update with it. I've got next to no experience with Rails beyond this specific vulnerability and trying to remediate it. I'll make a note of it for my other reports pending.

Joe Helle modified the report
a month ago
Joe Helle modified the report
a month ago
Joe Helle modified the report
a month ago
Joe Helle modified the report
a month ago
zammad/zammad maintainer validated this vulnerability a month ago
Joe Helle has been awarded the disclosure bounty
The fix bounty is now up for grabs
The researcher's credibility has increased: +7
Joe Helle
a month ago

Researcher


Thank you for validating. If you agree, can you select the CVE option on your side please?

zammad/zammad maintainer
a month ago

Maintainer


We usually request CVE numbers directly when our advisories go public, and for the moment we'd like to keep it this way.

Jamie Slome
a month ago

Admin


@maintainer - understood! We are happy to arrange the CVE once the report goes public. Let me know how this sounds.

We have sent a fix follow up to the zammad team. We will try again in 7 days. a month ago
zammad/zammad maintainer
a month ago

Maintainer


@admin how can we stop receiving mails for the comments of these reports? I did mark them as read, but all new comments are still posted to us via email.

Jamie Slome
a month ago

Admin


@maintainer - we continue notifying all chat participants of new comments. The "seen" button stops all follow-up reminders of the report submission.

If you had an idea for a feature improvement here, I'd love to get your thoughts so that we can track it for you. Perhaps an unsubscribe from thread action or...?

zammad/zammad maintainer confirmed that a fix has been merged on ccff13 a month ago
The fix bounty has been dropped
zammad/zammad maintainer gave praise a month ago
Dear Joe @researcher, today we released Zammad 5.2.1 with a fix for this issue. You can find our advisory with your credits at https://zammad.com/en/advisories/zaa-2022-05, CVE assignment is still pending. Thank you for your efforts and for handling this security issue in a responsible way! Best regards, Martin
The researcher's credibility has slightly increased as a result of the maintainer's thanks: +1
zammad/zammad maintainer
a month ago

Maintainer


Sorry, the commit ID should actually be ccff1354babd620a3beaf2414c9fe7e85a68927c.

Jamie Slome
a month ago

Admin


I'll sort out the commit SHA for you now 👍

Jamie Slome
a month ago

Admin


Could we also include a reference to this report URL in the advisory + CVE?

to join this conversation