Skip to content

fix: 清理 CodeQL 告警,修复兑换守门、配置并发等问题 - #11

Open
heylumen wants to merge 3 commits into
uvwt:mainfrom
heylumen:main
Open

heylumen wants to merge 3 commits into
uvwt:mainfrom
heylumen:main

Conversation

@heylumen

Copy link
Copy Markdown

跑了一遍完整审查(CodeQL 扫出 4 条 high 告警),修掉了所有确认的问题。分三块说明。

一、CodeQL 的 4 条告警

1 条是真问题,已经修了:

  • allocation-size-overflowclink/protocol.go):帧长度用 int 累加没有上限,32 位平台上可能先溢出再按错误的 size 分配内存。加了 16 MiB 上限,超了直接 panic(外层有 RecoverPanic 接住)。消息负载都是本地构造的短数据,正常路径碰不到这个上限。

另外 3 条看过之后确认不是漏洞,加了注释说明原因(我在自己的 fork 上 dismiss 了,这边合并后还需要处理一次):

  • disabled-certificate-checkclink/tls.go):Clink 是拿 IP 直连 wss 的,服务端只发 *.ctyun.cn 证书,而且线上有已经过期的旧证书。Go 没法只忽略有效期,必须关掉默认校验再在 VerifyConnection 里自己验——链签名、ctyun.cn 域名、还没生效的证书这些都会被拒绝,不存在跳过校验的路径。
  • 两处 weak-sensitive-data-hashingauth/sign.go 的 SHA256、auth/clink_login.go 的 MD5):都是天翼服务端协议定死的算法,改了就登录不上,这里是传输摘要不是本地存密码。

提醒一下:源码里的 // codeql[...] 注释只在 advanced setup(启用 AlertSuppression.ql)时生效,默认设置下这 3 条还会报,需要在界面里 dismiss 一下。

二、修复的缺陷

  1. 兑换守门有漏洞app/automation.go):手动点"检查兑换"会先确认"使用1小时"任务完成才继续,但自动调度那条路在等满 80 分钟后不管任务完没完成都会继续下单。现在两条路走同一个检查,任务没完成就跳过并提示原因。注意这是行为变化:以前超时后会硬着头皮兑换,现在不会了。
  2. 兑换状态会"漂移"automation/redeem_job.go):commitState 原来先改内存再写盘,写盘失败两边就不一致了,重启后 pending 标记会丢。改成先落盘、成功后再改内存,跟 ResolvePending 的做法对齐。
  3. 两处后台 goroutine 没 recoverapp/runtime.goapp/automation.go):包里其他地方都有 RecoverPanic,就这两处漏了。刷积分链路 panic 一次,整个保活进程就没了。
  4. 配置保存互相覆盖storage/config.goUpdateConfig,登录/设置/兑换三处调用点统一改走):原来都是自己 Load→改→Save,并发时后写的会把先写的冲掉。
  5. 兑换设置保存会卡住定时任务app/redeem_settings.go):原来一进 Save 就拿写锁,然后才发网络请求加载目录,网络慢的时候所有定时任务都要干等。目录加载挪到锁外了,拿到锁之后会重新校验一遍账号状态。

三、顺手改的小问题

  • 登录/绑定/兑换设置这几个对话框,关闭之后后台还在飞的网络回调会继续操作已经销毁的控件。现在 dlg.Run() 返回后置个 closed 标志,回调开头检查一下。
  • 主日志初始化失败时程序直接起不来,改成降级运行——crash 日志本来就是允许失败的,主日志没道理更严格。
  • UI 判断"上次兑换结果不确定"原来靠文案字符串比较,改成 State 里的 RedeemPending 字段。
  • 三处随机字符串生成有轻微模偏差(256 % 62 = 8),改成拒绝采样。
  • storage/protected.go 的文件名校验和 statePath 规则不一致(少了 ./..),统一掉。
  • Windows 凭据写入后把内存里的明文清零。
  • 代理的 UTF-16 解析换成 windows.UTF16PtrToString,原来的手写循环没上界。
  • 打包脚本里 finally 的 throw 会覆盖 try 里的原始异常,排障看不到根因,改成了 warning。

影响和验证

正常路径的行为基本不变,唯一的例外就是上面说的兑换守门:超时后不再硬兑换。现有的测试语义都没动(pending 持久化、settings 回滚、跨账号守门这些用例都核对过)。

写这个 PR 的环境里没有 Go 工具链,没跑过 build/test,麻烦合并前跑一下:

go build ./... && go vet ./... && go test ./...

代码扫描(CodeQL 默认设置)共 4 条告警,逐条复核后分别处理:

1. go/disabled-certificate-check @ internal/ctyun/clink/tls.go
   Clink 以 IP 作为 wss endpoint,服务端只提供 *.ctyun.cn 证书且线上存在已过期
   旧证书,Go 的 crypto/tls 没有“仅忽略有效期”的开关,因此必须关闭默认 verifier
   再由 VerifyConnection 复刻兼容策略。校验逻辑仍完整(链签名、ctyun.cn 归属、
   尚未生效一律拒绝),不存在无条件放行路径。此处加定点抑制注释并补充说明。

2. go/weak-sensitive-data-hashing @ internal/ctyun/auth/sign.go
   SHA256Hex 是通用摘要工具,同时服务请求签名与天翼登录协议。登录流程只把它作为
   一次性传输摘要提交,算法由服务端固定要求,并非本地口令存储。补充说明并抑制。

3. go/weak-sensitive-data-hashing @ internal/ctyun/auth/clink_login.go
   天翼旧 Clink 鉴权协议固定使用 MD5 签名,客户端无法更换算法。补充说明并抑制。

4. go/allocation-size-overflow @ internal/ctyun/clink/protocol.go
   帧长度用 int 累加,32 位平台上可能先溢出再按错误大小分配缓冲区。新增
   maxMessagePayloadBytes(16 MiB)上限校验后 panic,由上层 RecoverPanic 边界
   记录,避免静默截断或错误分配。这是真实修复,未使用抑制。

审查中发现并一并修复的缺陷:

- internal/app/runtime.go:refreshPointsAsync 的后台 goroutine 缺少 panic 恢复,
  单次刷新 panic 会直接终结整个保活进程。补齐 RecoverPanic 边界。
- internal/app/automation.go:TaskAutomation.Start 的启动刷新 goroutine 存在同样
  问题,补齐 RecoverPanic 边界。
- internal/automation/redeem_job.go:commitState 原先先改内存再写盘,落盘失败时
  进程内状态与磁盘状态漂移(重启后 pending 丢失)。改为先落盘、成功后再更新内存,
  与 ResolvePending 的既有原则保持一致。

未纳入本次改动(需产品决策,见审查报告):PointsJob.WaitUsageAndRefresh 超时后
仍继续兑换的行为、storage 层 config.json 读-改-写的跨服务竞态。
首次提交把 // codeql[...] 写在告警行行尾,实测未被 CodeQL 识别(行尾追加无效),
且带 -- 说明后缀不符合抑制注释的解析要求。现改为独占一行、紧邻告警行上方、
不含任何后缀的规范形式;解释性文字移到相邻普通注释行。
- 兑换守门统一:自动调度在“使用1小时”等待超时后同样跳过兑换,
  不再出现未达成条件仍消耗积分下单的情况(行为变更,见 PR 说明)
- storage 新增 UpdateConfig 原子读改写,登录/设置/兑换三处配置保存
  不再互相覆盖;兑换设置保存的目录加载移出写锁,不再阻塞定时任务
- 登录/绑定/兑换设置对话框关闭后,丢弃仍在飞行中的网络回调,
  避免回调触碰已销毁的控件
- 主日志初始化失败改为降级运行,不再阻止保活程序启动
-其余:模型状态新增 RedeemPending 字段替代文案字符串判断;随机串
  改用拒绝采样消除模偏差;受保护文件名校验与 statePath 统一;
  凭据写入后清零内存明文;代理 UTF-16 解析改用 windows.UTF16PtrToString;
  图标资源脚本 finally 不再覆盖原始异常
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.

1 participant