Skip to content

fix(security): reject untrusted client certificates#710

Closed
WatchKitty wants to merge 1 commit into
AlkaidLab:masterfrom
WatchKitty:security-ghsa-ph75-mgxh-mv57
Closed

fix(security): reject untrusted client certificates#710
WatchKitty wants to merge 1 commit into
AlkaidLab:masterfrom
WatchKitty:security-ghsa-ph75-mgxh-mv57

Conversation

@WatchKitty
Copy link
Copy Markdown

Summary

  • Backport the upstream security fix for GHSA-ph75-mgxh-mv57 / CVE-2026-32253.
  • Stop treating X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY as a successful client-certificate verification result in src/crypto.cpp.
  • Keep the existing Moonlight clock-skew compatibility behavior for not-yet-valid and expired paired certificates, matching upstream v2026.516.143833.

Root Cause

Sunshine's HTTPS client-certificate authentication path uses cert_chain_t::verify from src/crypto.cpp. The custom OpenSSL verification callback previously converted an unknown local issuer error into success, which could allow an untrusted certificate chain to pass authentication.

Security Impact

This closes the authentication-bypass condition described in the upstream critical advisory for protected Sunshine HTTPS endpoints. The change mirrors the upstream fix commit 888a6bb0 and the patched release v2026.516.143833.

Validation

  • rg confirmed X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY is no longer allowed in src/crypto.cpp.
  • git diff --check passed.
  • CMake/unit tests were not run because cmake is not available in the current Windows PATH, and the repository preset expects an MSYS2 UCRT64 shell.

Notes

References

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 4, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 99248180-3c75-4be0-ad9e-93381f90eb32

📥 Commits

Reviewing files that changed from the base of the PR and between e5579b9 and 9f053ac.

📒 Files selected for processing (1)
  • src/crypto.cpp
💤 Files with no reviewable changes (1)
  • src/crypto.cpp

Summary by CodeRabbit

发布说明

  • Bug 修复
    • 优化了 SSL 证书验证的错误处理逻辑
    • 调整了在设备时钟偏差场景下证书有效期的验证处理

总体概览

在 OpenSSL 证书验证回调函数中,删除了 X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY 错误码的特定处理分支,同时调整了关于证书过期或未生效在设备时钟不准场景下的容错注释逻辑。

变更内容

证书验证回调错误处理

层级 / 文件 说明
错误码处理逻辑简化
src/crypto.cpp
openssl_verify_cb 回调的 switch 分支中移除对 X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY 的独立 case 处理;保留对 X509_V_ERR_CERT_NOT_YET_VALIDX509_V_ERR_CERT_HAS_EXPIRED 的可接受返回路径,并补充注释说明设备时钟不准场景下的容错机制。

🎯 2 (Simple) | ⏱️ ~8 分钟

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed 标题准确概括了主要变更:拒绝不受信任的客户端证书,与代码改动(移除不安全的证书验证逻辑)直接对应。
Description check ✅ Passed 描述清晰关联到变更集,详细说明了安全修复的目的、根本原因、影响范围和验证方式,完全相关于所做的代码改动。
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Yundi339
Copy link
Copy Markdown
Member

Yundi339 commented Jun 4, 2026

Thank you for paying attention to this issue. In fact, we discovered it last year and optimized it.

Here you can disable protection, let some old Moonlights continue to work.
By default, it is off, meaning protection mode is enabled. Turning it on means disabling protection mode

image

@WatchKitty
Copy link
Copy Markdown
Author

感谢您关注这个问题。事实上,我们去年就发现了这个问题并进行了优化。

您可以在此处禁用保护功能,让一些旧的 Moonlight 程序继续运行。 默认情况下,此功能处于关闭状态,表示已启用保护模式。启用此功能则表示已禁用保护模式。

图像

Thanks for the context. I agree that close_verify_safe exists and defaults to the safer path. However, the default verify_safe() path still installs openssl_verify_cb, and on master that callback still returns success for X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY.

That is the exact condition called out by GHSA-ph75-mgxh-mv57. This PR removes that case from the shared callback, so both verify() and verify_safe() stop accepting an untrusted issuer chain while preserving the old-client compatibility switch.

@WatchKitty
Copy link
Copy Markdown
Author

One important detail is that verify_safe() does not avoid the shared callback; it still calls X509_STORE_CTX_set_verify_cb(ctx.get(), openssl_verify_cb) before X509_verify_cert(). So the toggle changes which verification wrapper is used, but both paths still share the unsafe issuer-error override until this PR removes it.

感谢您关注这个问题。事实上,我们去年就发现了这个问题并进行了优化。
您可以在此处取消保护功能,让一些旧的月光程序继续运行。默认情况下,此功能处于关闭状态,表示启用保护模式。启用此功能已表示已取消保护模式。
图像

感谢您提供的背景信息。我同意确实close_verify_safe存在这种情况,并且默认会选择更安全的路径。但是,默认verify_safe()路径仍然会安装openssl_verify_cb,并且在主分支上,该回调仍然会返回成功X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY

这正是GHSA-ph75-mgxh-mv57所指出的情况。此 PR 从共享回调中移除了这种情况,因此两者都verify()verify_safe()不再接受不受信任的颁发者链,同时保留了旧客户端兼容性开关。

@Yundi339
Copy link
Copy Markdown
Member

Yundi339 commented Jun 4, 2026

@WatchKitty Thank you for your review.

OK, I just looked at last year's modification history, verify_safe will not trigger sharing. Because verify_safe every time will create new X509_STORE_CTX.

And verify_safe doesn't have X509_STORE_CTX_set_flags(), so verify_safe is safe function. I think
you should test it really. We've tested this manually several times over the last year.

@WatchKitty
Copy link
Copy Markdown
Author

@Yundi339 Thanks for the explanation. You’re right — I should have verified the actual verify_safe() behavior before opening this PR.

Since this case has already been handled and tested on your side, this PR is unnecessary. Sorry for the noise, and I’ll close it.

@WatchKitty WatchKitty closed this Jun 5, 2026
@WatchKitty WatchKitty deleted the security-ghsa-ph75-mgxh-mv57 branch June 5, 2026 04:50
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.

2 participants