Enforce secure redirects#22368
Conversation
- Share the `HOMEBREW_NO_INSECURE_REDIRECT` predicate with `CurlPostDownloadStrategy` and URL audits. - Apply `--proto-redir =https` in the shared curl execution path so redirect-following calls use curl-level enforcement. - Strip caller-provided `--proto-redir` values while the policy is enabled so they cannot weaken the redirect restriction. - Route GitHub artifact downloads through `curl_download` for the same policy.
There was a problem hiding this comment.
Pull request overview
This PR strengthens Homebrew’s redirect security controls by centralizing the HOMEBREW_NO_INSECURE_REDIRECT logic and pushing enforcement down into the shared curl execution path, including GitHub artifact downloads.
Changes:
- Add shared helpers in
Utils::Curlfor detecting insecure redirects and for enforcing--proto-redir =httpson redirect-following curl calls. - Reuse the shared redirect predicate in URL auditing logic and in
CurlDownloadStrategy/CurlPostDownloadStrategy. - Route GitHub Actions artifact downloads through
Utils::Curl.curl_downloadto inherit the shared curl policy.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Library/Homebrew/utils/github/artifacts/github_artifact_download_strategy.rb | Switch artifact downloads to curl_download to inherit shared curl enforcement. |
| Library/Homebrew/utils/curl.rb | Introduce shared insecure-redirect predicate and inject --proto-redir =https for redirect-following curl calls. |
| Library/Homebrew/test/utils/curl_spec.rb | Add unit tests for no_insecure_redirect_curl_args and curl enforcement behavior. |
| Library/Homebrew/test/download_strategies/curl_post_spec.rb | Add test coverage for CurlPostDownloadStrategy rejecting insecure redirects. |
| Library/Homebrew/download_strategy/curl_post_download_strategy.rb | Enforce insecure redirect checks for POST-based downloads. |
| Library/Homebrew/download_strategy/curl_download_strategy.rb | Centralize redirect policy enforcement in ensure_no_insecure_redirect!. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for fixing and publishing GHSA-7699-qf8c-q47m. Could you please assign a CVE for this published advisory? |
|
CVSS probably needs to be discussed first - I don't think it's High for a feature that's disabled by default, requires compromise of the remote server and can't really be exploited in any first-party cask (as the download strategy is used in 2 casks, both which are checksummed). In the case of attacker-controlled cask files (third-party taps), they could just point to malware directly even via HTTPS. |
|
I agree that arbitrary attacker-controlled third-party taps should not be used to justify the severity. A malicious third-party tap could simply point to malware directly, so that is not the boundary I’m relying on. The High argument is for the trusted install path case: Homebrew’s own documented HTTPS-to-HTTP redirect protection is enabled, the metadata is trusted, and the POST strategy still allows the real artifact fetch to cross to HTTP. For checksum-protected downloads, the checksum remains an important backstop and the impact is lower. But for a trusted checksumless executable cask, or an equivalent trusted install path without an artifact integrity check after the downgrade, the same bug permits executable artifact substitution over the HTTP leg. I also don’t think “disabled by default” should automatically make the Base CVSS non-High. This is an opt-in security protection, and the vulnerable behavior is that Homebrew fails to enforce that protection after the user explicitly enables it. In CVSS terms, that seems closer to a specific vulnerable configuration / environmental consideration than a reason to remove the high-impact case from the Base assessment entirely. On “requires compromise of the remote server”: I would frame the required attacker control more narrowly. The attacker does not need to control the user’s local machine or the cask metadata. The relevant attack condition is control of, or influence over, the HTTP leg that Homebrew should not have followed once "HOMEBREW_NO_INSECURE_REDIRECT=1" was set. If an HTTPS endpoint redirects to HTTP, the protection is specifically meant to prevent the installer from relying on that plaintext leg. I agree the current first-party snapshot matters for practical exploitability. If all currently reachable first-party "using: :post" casks are checksummed, then the immediately demonstrated first-party impact is the documented protection bypass. But I still think High is defensible for the advisory when the impact is scoped to the trusted checksumless executable-cask/equivalent trusted install path case, because the vulnerable component is the shared Homebrew download strategy and the missing enforcement is not specific to a malicious third-party tap. |
Can you explain how the attacker in this scenario ends up with end-user code execution? I see a number of things that would preclude it here:
Given (1) in particular, I'm inclined to consider this a medium at best (and most likely a low): our threat model assumes that HTTPS origins are actually kept secure by their owners, and (correspondingly) those owners also keep their signing materials for applications secure. It's definitely a bug that we allowed an insecure redirect in this instance, but I think macOS's layered defenses here largely mitigate the most concerning variant we'd imagine here. |
|
I agree that an unsigned arbitrary ".app" should not be treated as automatic code execution, because Gatekeeper/quarantine materially mitigates that case. The High case I’m trying to preserve is narrower: a trusted Homebrew install path, "HOMEBREW_NO_INSECURE_REDIRECT=1", no checksum backstop after the downgrade, and a substituted artifact on the HTTP leg that is still accepted by macOS execution policy, for example because it is signed/notarized. That is a stronger precondition than opportunistic MITM, so I agree AC:H is more appropriate than AC:L. I do not think that collapses to “the HTTPS origin became malicious,” because the original HTTPS origin and vendor signing material do not need to be compromised. The bug is that Homebrew follows and consumes an HTTP leg that the user explicitly configured it not to follow; if there is no checksum or expected signing-identity pinning after that point, Gatekeeper only checks platform acceptability, not that the bytes are the intended vendor artifact. So I’m fine with lowering the High vector to something like: "CVSS:3.1/AV:A/AC:H/PR:N/UI:R/S:U/C:H/I:H/A:H = 7.5 High" For current checksum-protected first-party "using: :post" casks, I agree the practical impact is lower. |
|
We're not going to request a CVE for this. Homebrew's practice is to publish GHSAs rather than request CVEs, and that's especially appropriate for an opt-in feature whose failure mode degrades to Homebrew's default behavior. We've set the GHSA to Low severity, which is fairly generous as it stands. It doesn't warrant the scanner noise and user alarm a CVE would generate. |
|
Understood. The "Low" part is a bit strange, but thank you for your assistance. |
HOMEBREW_NO_INSECURE_REDIRECTpredicate withCurlPostDownloadStrategyand URL audits.--proto-redir =httpsin the shared curl execution path so redirect-following calls use curl-level enforcement.--proto-redirvalues while the policy is enabled so they cannot weaken the redirect restriction.curl_downloadfor the same policy.brew lgtm(style, typechecking and tests) with your changes locally?OpenAI Codex 5.5 xhigh with local review, tweaking and testing.