Skip to content

Enforce secure redirects#22368

Merged
MikeMcQuaid merged 1 commit into
mainfrom
fix-post-insecure-redirect
May 21, 2026
Merged

Enforce secure redirects#22368
MikeMcQuaid merged 1 commit into
mainfrom
fix-post-insecure-redirect

Conversation

@MikeMcQuaid
Copy link
Copy Markdown
Member

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

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them? Performance claims (e.g. "this is faster") must include Hyperfine benchmarks.
  • Have you written new tests (excluding integration tests) for your changes? Here's an example.
  • Have you successfully run brew lgtm (style, typechecking and tests) with your changes locally?

  • AI was used to generate or assist with generating this PR. Please specify below how you used AI to help you, and what steps you have taken to manually verify the changes. Non-maintainers may only have one AI-assisted/generated PR open at a time.

OpenAI Codex 5.5 xhigh with local review, tweaking and testing.


- 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.
Copilot AI review requested due to automatic review settings May 21, 2026 13:29
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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::Curl for detecting insecure redirects and for enforcing --proto-redir =https on 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_download to 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.

Comment thread Library/Homebrew/utils/curl.rb
Comment thread Library/Homebrew/test/utils/curl_spec.rb
@MikeMcQuaid MikeMcQuaid enabled auto-merge May 21, 2026 14:17
@MikeMcQuaid MikeMcQuaid added this pull request to the merge queue May 21, 2026
Merged via the queue into main with commit 50af657 May 21, 2026
41 checks passed
@MikeMcQuaid MikeMcQuaid deleted the fix-post-insecure-redirect branch May 21, 2026 15:37
@singetu0096
Copy link
Copy Markdown

Thanks for fixing and publishing GHSA-7699-qf8c-q47m.

Could you please assign a CVE for this published advisory?

@Bo98
Copy link
Copy Markdown
Member

Bo98 commented May 21, 2026

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.

@singetu0096
Copy link
Copy Markdown

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.

@woodruffw
Copy link
Copy Markdown
Member

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.

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:

  1. Homebrew's primary cask repository requires Gatekeeper signatures on applications. Unless I'm missing something that means that the attacker here wouldn't actually succeed in getting a MITM'd application to launch here, unless they also have a valid Apple developer certificate. That's not inconceivable, but it's a step beyond an opportunistic MITM and is (in terms of threat model) similar to the HTTPS origin becoming malicious.
  2. I'd need to check and confirm this, but I believe macOS places an additional quarantine bit on applications (i.e. .app bundles) downloaded over HTTP rather than HTTPS. Users would need to explicitly go and override macOS's security warnings there. I believe Homebrew propagates that quarantine, but I'm not sure off the top of my head.

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.

@singetu0096
Copy link
Copy Markdown

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.

@p-linnane
Copy link
Copy Markdown
Member

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.

@singetu0096
Copy link
Copy Markdown

Understood. The "Low" part is a bit strange, but thank you for your assistance.

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.

9 participants