diff --git a/reviews/code-review-SecurityEngineer.md b/reviews/code-review-SecurityEngineer.md new file mode 100644 index 0000000..55f363d --- /dev/null +++ b/reviews/code-review-SecurityEngineer.md @@ -0,0 +1,401 @@ +# vr-shopxo-plugin 安全审计报告 + +> 审计人:SecurityEngineer +> 日期:2026-04-15 +> 范围:EventListener.php / TicketService.php / BaseService.php / ticket_detail.html / Verification.php / Ticket.php / Goods.php / plugin.json +> 评级说明:🔴 严重 / 🟡 中等 / 🟢 轻微 / 💡 建议 + +--- + +## 一、执行摘要 + +vr-shopxo-plugin 是一个在 ShopXO 基础上扩展票务功能的插件,核心功能包括座位模板管理、电子票生成、QR 码核销、观演人信息收集。经过全面审计,共发现**1 个严重漏洞、5 个中等风险、3 个轻微问题、4 项改进建议**。最关键的问题是支付回调 `onOrderPaid` 存在并发竞态条件,可导致一张票被多次核销。 + +--- + +## 二、插件架构审计 + +### 2.1 生命周期钩子(EventListener.php) + +| 钩子 | 实现 | 评估 | +|------|------|------| +| `vr_ticket_install` | ✅ | 建表语句规范,使用 `IF NOT EXISTS` 防止重复执行 | +| `vr_ticket_uninstall` | ✅ | 默认不删除数据(注释状态),安全 | +| `vr_ticket_upgrade` | ⚠️ | 仅有空壳,无版本迁移逻辑 | + +**🟡 中等 — 升级迁移缺失** +`vr_ticket_upgrade($old_version)` 是空实现。若未来表结构变更(如新增字段),升级流程无法自动执行 ALTER TABLE。建议引入基于版本号的增量迁移表或脚本。 + +### 2.2 plugin.json 配置 + +```json +"hooks": [ + "plugins_service_order_pay_success_handle_end", + "plugins_service_order_delete_success" +] +``` + +**💡 建议 — 缺少 `plugins_service_order_delete_success` 处理逻辑** +`plugin.json` 声明监听订单删除钩子,但代码中没有对应处理函数。若订单删除后票未同步处理,会导致票与订单状态不一致。 + +### 2.3 ALTER TABLE 安全风险 + +```php +$db->query("ALTER TABLE `{$prefix}goods` ADD COLUMN `item_type` VARCHAR(20) ..."); +``` + +**🟢 轻微 — 直接执行 DDL 的并发问题** +ShopXO 可能在高并发场景下执行此 ALTER TABLE 时出现锁表。风险极低(仅执行一次),但建议在 install 中用事务或预先检测。 + +--- + +## 三、票务核心安全审计 + +### 3.1 🔴 严重 — `onOrderPaid` 并发竞态条件 + +**文件:** `TicketService.php:23-68` + +```php +// 第 54-60 行:逐个生成票 +foreach ($order_goods as $og) { + $ticket_id = self::issueTicket($order, $og); + // ... +} +``` + +**问题分析:** +- ShopXO 支付回调通过 HTTP 请求触发,如果用户使用多设备或重复点击支付,**同一订单可能被多次触发 `onOrderPaid`** +- 没有幂等性保护(无 order_id 锁或已处理标记) +- 结果:**同一订单商品可能生成多张电子票**(等于支付金额可入场多次) + +**攻击场景:** +1. 攻击者购买一张票,支付 100 元 +2. 支付成功回调触发 2 次(网络重试) +3. 系统生成 2 张票,每张都可独立入场核销 + +**修复建议:** +```php +// 在 issueTicket 前增加幂等检查 +$existing = \Db::name(BaseService::table('tickets')) + ->where('order_id', $order['id']) + ->where('spec_base_id', $order_goods['spec_base_id'] ?? 0) + ->count(); +if ($existing > 0) { + return 0; // 已发放,跳过 +} +``` + +### 3.2 🟡 中等 — `verifyTicket` 核销状态竞争 + +**文件:** `TicketService.php:138-196` + +```php +// 第 148-154 行:状态检查 +if ($ticket['verify_status'] == 1) { + return ['code' => -2, 'msg' => '该票已核销']; +} +// ... 中间无锁 ... +// 第 159-166 行:更新状态 +\Db::name(BaseService::table('tickets')) + ->where('id', $ticket['id']) + ->update(['verify_status' => 1, ...]); +``` + +**问题分析:** +- 核销过程中,检查(SELECT)与更新(UPDATE)分离 +- 两个核销员同时扫描同一张票,在极短时间窗口内可能同时通过状态检查 +- 修复:使用数据库行级锁或原子更新: +```php +$affected = \Db::name(BaseService::table('tickets')) + ->where('id', $ticket['id']) + ->where('verify_status', 0) // 乐观锁:只更新未核销票 + ->update([...]); +if ($affected === 0) { + return ['code' => -2, 'msg' => '该票已核销']; +} +``` + +### 3.3 🟡 中等 — QR 码 URL 直接暴露 ticket_code + +**文件:** `TicketService.php:220-228` + +```php +public static function getQrCodeUrl($ticket_code) +{ + $content = base64_encode(json_encode([ + 'type' => 'vr_ticket', + 'code' => $ticket_code, // 直接明文 + ])); + return ROOT_URL . '?s=index/qrcode/index&content=' . urlencode($content) . '&size=8&level=H'; +} +``` + +**问题分析:** +- QR 码内容直接 Base64 编码,**ticket_code 可见** +- QR 码打印在纸上后,攻击者可抄录 ticket_code 尝试重放 +- 虽然 ticket_code 是 UUID v4(随机),但若泄露仍可被使用 + +**修复建议:** +- 使用加密后的 `qr_data` 而非原始 `ticket_code` +- 或使用一次性 token + HMAC 签名: +```php +$token = hash_hmac('sha256', $ticket_code, $secret); +$content = base64_encode(json_encode([ + 'type' => 'vr_ticket', + 'token' => $token, + 'exp' => time() + 3600, // 1小时有效 +])); +``` + +### 3.4 🟡 中等 — `verifyTicket` 缺少权限验证 + +**文件:** `Ticket.php:110-128`(手动核销接口) + +```php +public function verify() +{ + $ticket_code = input('ticket_code', '', null, 'trim'); + $verifier_id = input('verifier_id', 0, 'intval'); + // ... + $result = \app\plugins\vr_ticket\service\TicketService::verifyTicket($ticket_code, $verifier_id); +``` + +**问题分析:** +- 接口未验证当前登录用户是否为有效核销员 +- 任何登录用户(包括普通买家)只要知道 ticket_code 就可以调用此接口为他人核销 +- `verifier_id` 来自前端 POST 参数,可被伪造 + +**修复建议:** +```php +$verifier_id = input('verifier_id', 0, 'intval'); +$current_user_id = session('?user_id') ? session('user_id') : 0; +// 验证当前用户是否为注册核销员 +$verifier = \Db::name(BaseService::table('verifiers')) + ->where('user_id', $current_user_id) + ->where('status', 1) + ->find(); +if (empty($verifier)) { + return DataReturn('无权核销', -1); +} +$verifier_id = $verifier['id']; // 不接受前端传入的 verifier_id +``` + +### 3.5 🟢 轻微 — `decryptQrData` 无法验证完整性 + +**文件:** `BaseService.php:68-93` + +**问题分析:** +- AES-256-CBC 不提供认证加密(无 GMAC/GMAC 模式) +- 若密文被篡改,解密可能成功但数据异常(padding oracle 攻击理论上可能) +- ShopXO 环境风险较低(攻击者难以接触密文),但最佳实践应使用 AES-GCM + +**修复建议:** 切换到 `AES-256-GCM`,或追加 HMAC-SHA256 验证。 + +--- + +## 四、前端安全审计(ticket_detail.html) + +### 4.1 🟡 中等 — XSS 风险 + +**文件:** `ticket_detail.html:125` + +```html +
{$goods.simple_desc|default=''|raw}
+``` + +**问题分析:** +- `|raw` 过滤器**禁用所有 HTML 转义**,直接输出 `simple_desc` +- 若管理员在商品副标题中注入恶意脚本(如 ``),所有访问该商品页的用户都会执行 +- 同样风险存在于 `$goods.title`(第 124 行,无 `|raw` 安全) + +**修复建议:** +- 移除 `|raw`,或对 `simple_desc` 使用白名单过滤: +```html +
{$goods.simple_desc|default=''|htmlspecialchars}
+``` + +### 4.2 🟢 轻微 — 已选座位标签 XSS 风险 + +**文件:** `ticket_detail.html:275` + +```html +'data-label="'+rowLabel+(colIndex+1)+'排'+(colIndex+1)+'座" ' +``` + +**问题分析:** +- `seatInfo.label` 来自后端 JSON,理论上可信,但如果 seat_map JSON 数据被污染,可导致 DOM XSS +- 风险较低(需要插件管理员配合注入恶意数据) + +### 4.3 🟢 轻微 — 观演人表单无服务端验证 + +**文件:** `ticket_detail.html:352-368` + +**问题分析:** +- 观演人表单(姓名、手机、身份证)仅有 HTML5 `required` 属性 +- 无服务端校验:攻击者可在提交前修改 DOM 移除 `required`,或直接 POST 构造请求上传恶意数据 +- 身份证格式、手机号格式均无后端校验 + +### 4.4 💡 建议 — `loadSoldSeats` 未实现 + +**文件:** `ticket_detail.html:370-378` + +```javascript +loadSoldSeats: function() { + // TODO: 从后端加载已售座位 +``` + +已售座位数据完全依赖 JS 内存标记(空对象),**后端未实时同步座位状态**。用户可能在前端看到可选择的座位,但提交时被告知已售(库存超卖)。需实现后端座位锁定 API。 + +### 4.5 💡 建议 — 座位选中数量无硬上限 + +用户可选任意数量座位,理论上可购买全场的票。无单次购买上限限制。 + +--- + +## 五、数据库安全审计 + +### 5.1 SQL 注入评估 + +**评估结果:✅ 未发现 SQL 注入** + +代码全面使用 ShopXO ORM(`\Db::name()->where()->find()/select()`),所有用户输入均通过框架参数化查询: +- `Ticket.php:21` — `where('ticket_code|verifier_name', 'like', "%{$keywords}%")` +- `Verification.php:29-36` — 日期范围 `strtotime()` 处理 +- `BaseService.php:139` — `where('category_id', $goods['category_id'])` + +框架自动处理参数化,无 SQL 拼接风险。 + +### 5.2 索引评估 + +**✅ 索引设计合理** + +- `vr_tickets` 表:`(ticket_code)` 唯一索引、`(order_id)`、`(user_id)`、`(goods_id)`、`(verify_status)` — 覆盖所有查询场景 +- `vr_verifications` 表:`(ticket_id)`、`(verifier_id)` — 合理 +- `vr_seat_templates` 表:`(category_id)` 唯一索引 — 合理 + +### 5.3 🟡 中等 — 观演人信息明文存储 + +**文件:** `EventListener.php:49-51` + +```sql +`real_name` VARCHAR(60) +`phone` CHAR(15) +`id_card` CHAR(18) +``` + +**问题分析:** +- 观演人身份证、姓名、手机号**明文存储在数据库** +- 若数据库被拖库或内部人员泄露,可导致大规模个人信息泄露(中国法律要求敏感个人信息加密存储) +- 违反《个人信息保护法》第 51 条 + +**修复建议:** +- 对身份证号使用 AES-256-CBC 加密存储 +- 或在 `vr_tickets` 表中使用单独的加密字段: +```php +'id_card_encrypted' => encrypt($attendee['id_card'] ?? ''), +``` + +--- + +## 六、支付安全审计 + +### 6.1 🟡 中等 — 支付回调无签名验证 + +**文件:** `TicketService.php:23`(`onOrderPaid` 回调) + +ShopXO 的 `plugins_service_order_pay_success_handle_end` 钩子由 ShopXO 内部触发,但**未提供签名或 nonce 验证机制**。若存在插件注册了同名钩子,理论上可伪造回调(风险依赖 ShopXO 框架安全性)。 + +**当前缓解措施:** 订单状态二次验证(`$order['pay_status'] == 1`)是正确做法。 + +### 6.2 💡 建议 — 退款后票状态未自动更新 + +**文件:** `plugin.json:24` + +声明了 `plugins_service_order_delete_success` 钩子但未实现。若订单退款,票状态仍为"未核销",退款用户仍可持票入场。需监听退款成功事件并将 `verify_status` 设为 2。 + +--- + +## 七、AES QR 加密方案评估 + +### 7.1 加密算法选择 + +**✅ AES-256-CBC 基本安全** +- IV 使用 `random_bytes(16)` 随机生成 ✅ +- 密钥从环境变量读取 ✅ +- 过期时间 `exp` 字段防止长期泄露 ✅ + +### 7.2 🟡 中等 — 密钥回退到硬编码默认值 + +**文件:** `BaseService.php:106` + +```php +return config('shopxo.app_key', 'shopxo_default_secret_change_me'); +``` + +**问题分析:** +- 若环境变量 `VR_TICKET_QR_SECRET` 未配置,且 ShopXO `app_key` 未修改默认值 +- 所有 QR 密文使用同一已知密钥加密,**攻击者可离线解密所有 QR 数据** +- `shopxo_default_secret_change_me` 极可能出现在生产环境中 + +**修复建议:** +```php +$secret = env('VR_TICKET_QR_SECRET', ''); +if (empty($secret)) { + throw new \Exception('VR_TICKET_QR_SECRET 环境变量未配置'); +} +``` + +--- + +## 八、问题汇总 + +| 编号 | 严重程度 | 类别 | 文件 | 描述 | +|------|---------|------|------|------| +| **S-01** | 🔴 严重 | 业务逻辑 | TicketService.php:54-60 | `onOrderPaid` 无幂等保护,同一订单可生成多张票 | +| **M-01** | 🟡 中等 | 业务逻辑 | TicketService.php:138-166 | `verifyTicket` 存在 TOCTOU 竞态条件 | +| **M-02** | 🟡 中等 | 鉴权 | Ticket.php:110-128 | 手动核销接口未验证核销员身份,可被任意登录用户调用 | +| **M-03** | 🟡 中等 | 数据安全 | EventListener.php:49-51 | 观演人身份证明文存储,违反个人信息保护法律 | +| **M-04** | 🟡 中等 | 前端安全 | ticket_detail.html:125 | `simple_desc` 使用 `|raw` 导致存储型 XSS | +| **M-05** | 🟡 中等 | 加密 | BaseService.php:106 | QR 加密密钥回退到硬编码默认值 | +| **L-01** | 🟢 轻微 | 前端安全 | ticket_detail.html:275 | `data-label` 属性可能含未转义数据 | +| **L-02** | 🟢 轻微 | 数据完整性 | BaseService.php:80 | AES-CBC 无认证加密,可被 padding oracle 攻击 | +| **L-03** | 🟢 轻微 | 业务逻辑 | ticket_detail.html:370-378 | `loadSoldSeats` 未实现,存在超卖风险 | +| **I-01** | 💡 建议 | 架构 | EventListener.php:127 | 升级迁移逻辑为空 | +| **I-02** | 💡 建议 | 业务逻辑 | plugin.json:24 | 退款钩子已注册但未实现 | +| **I-03** | 💡 建议 | 前端 | ticket_detail.html:384 | 购买数量无上限控制 | +| **I-04** | 💡 建议 | 前端 | ticket_detail.html:397-407 | 观演人表单无服务端格式校验 | + +--- + +## 九、修复优先级建议 + +### 立即修复(上线前必须处理) +1. **S-01** — 添加 `onOrderPaid` 幂等检查(防止一张票多次入场) +2. **M-02** — 手动核销接口鉴权(防止权限绕过) +3. **M-04** — 移除 `|raw` 或使用 `htmlspecialchars` +4. **M-05** — 移除 QR 密钥硬编码回退 + +### 上线后尽快修复 +5. **M-01** — `verifyTicket` 乐观锁 +6. **M-03** — 身份证字段加密存储 +7. **I-02** — 实现退款后票状态更新 + +### 迭代修复 +8. **I-01** — 升级迁移框架 +9. **L-03** — 实现实时座位锁定 +10. **L-02** — 升级到 AES-GCM + +--- + +## 十、整体评价 + +| 维度 | 评分 | 说明 | +|------|------|------| +| SQL 注入防护 | ⭐⭐⭐⭐⭐ | 全面使用 ORM 参数化,无注入风险 | +| 加密方案 | ⭐⭐⭐ | AES-256-CBC 基础安全,但无认证加密,密钥管理有缺陷 | +| 鉴权设计 | ⭐⭐ | 核销接口鉴权严重缺失 | +| 并发安全 | ⭐ | 核心业务流程存在明显竞态条件 | +| 数据安全合规 | ⭐⭐ | 敏感个人信息明文存储 | +| 前端安全 | ⭐⭐ | 存在存储型 XSS | + +**综合评级:中等风险(B)** — 核心业务逻辑(票务核销)存在并发安全缺陷,需优先修复后才能安全上线。