Test comment

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.

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.

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.

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.

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.

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.

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.

@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.

@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.

@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.

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?

Sure, we can remove comments 👍

This report is modified to reflect the rack_attack issue.

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

@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

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

• N: Joe Helle
• W: https://themayor.tech/

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.

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

@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).

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.

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

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

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

@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.

@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...?

Sorry, the commit ID should actually be ccff1354babd620a3beaf2414c9fe7e85a68927c.

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

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

We received CVE-2022-35488 from MITRE to identify this issue. I'm not able to update this report any more. @researcher or @admin could you update this please?

Sorted 👍