已合并
【模型分级可视化】调用命令前,增加安全校验 #722
sun-chao创建于 5月28日
【模型分级可视化】调用命令前,增加安全校验 #722
已合并
sun-chao创建于 5月28日
4 个文件变更+102-6
@@ -16,7 +16,7 @@ import os
16import json16import json
17import time17import time
18import threading18import threading
19-import subprocess19+import subprocess # nosec B404
20from abc import ABC, abstractmethod20from abc import ABC, abstractmethod
21from tensorboard.util import tb_logging21from tensorboard.util import tb_logging
22from ..utils.graph_utils import GraphUtils22from ..utils.graph_utils import GraphUtils
@@ -133,7 +133,13 @@ class GraphServiceStrategy(ABC):
133 run_list.extend([param, str(value_n)])133 run_list.extend([param, str(value_n)])
134 if value_b:134 if value_b:
135 run_list.append(str(value_b))135 run_list.append(str(value_b))
136- proc = subprocess.Popen(run_list, stdout=subprocess.PIPE, text=True)136+ success, result = GraphUtils.safe_run_command(run_list, stdout=subprocess.PIPE, text=True)
137+ if not success:
138+ logger.error(f"Failed to run command: {result}")
Menba
MenbaMenba5月28日

ProgressInfo.error_msg 类型不匹配 — 将字符串赋值给列表字段

  • 位置: plugins/tb_graph_ascend/hierarchy_plugin/server/app/service/graph_service_base.py:140
  • 严重级别: 严重
  • 说明: ProgressInfo 类的 error_msg 字段在代码中定义为 [](列表类型,见 python/msprobe/visualization/utils.py:409),且该类的 update_error_msg() 方法(utils.py:432-433) 通过 cls.error_msg.append(value) 操作其内容。本 PR 中 ProgressInfo.error_msg = result 直接赋值为字符串,改变了字段的类型。后续若有其他代码调用 update_error_msg()error_msg.append(),将触发 AttributeError: 'str' object has no attribute 'append',导致服务崩溃。建议改为 ProgressInfo.error_msg = [result] 以保持列表类型一致性。
likedislike
sun-chao
5月29日 评论:
sun-chao
5月29日 评论:
139+ ProgressInfo.error_msg = result
140+ ProgressInfo.process_running = False
141+ return
142+ proc = result
137 update_progress_info(proc, ProgressInfo)143 update_progress_info(proc, ProgressInfo)
138 144 
139 thread = threading.Thread(target=call_path_api)145 thread = threading.Thread(target=call_path_api)
@@ -14,6 +14,23 @@
14 14 
15from enum import Enum15from enum import Enum
16 16 
17+# 允许执行的命令白名单
18+ALLOWED_COMMANDS = {"msprobe"}
19+# 禁止的参数模式(注入攻击特征)
20+FORBIDDEN_ARG_PATTERNS = [
21+ r"\|", # 管道符
22+ r";", # 命令分隔符
23+ r"&&", # 逻辑与
24+ r"\|\|", # 逻辑或
25+ r"`", # 反引号命令替换
26+ r"\$\(", # 命令替换
27+ r"\n", # 换行符注入
28+ r"\r", # 回车符注入
29+ r">>", # 追加重定向
30+ r"[<>]", # 重定向
31+ r"\\x", # 十六进制转义
32+]
33+ 
17security_headers = {34security_headers = {
18 "Content-Security-Policy": (35 "Content-Security-Policy": (
19 "default-src 'self'; connect-src 'self'; script-src 'unsafe-inline'; "36 "default-src 'self'; connect-src 'self'; script-src 'unsafe-inline'; "
@@ -17,12 +17,15 @@ import os
17import json17import json
18import re18import re
19import stat19import stat
20+import shutil
21+import subprocess # nosec B404
20import threading22import threading
21from functools import cmp_to_key23from functools import cmp_to_key
22from pathlib import Path24from pathlib import Path
23from tensorboard.util import tb_logging25from tensorboard.util import tb_logging
24from .global_state import GraphState26from .global_state import GraphState
25-from .constant import DataType, FILE_NAME_REGEX, MAX_FILE_SIZE, PERM_GROUP_WRITE, PERM_OTHER_WRITE, COLOR_PATTERN27+from .constant import DataType, FILE_NAME_REGEX, MAX_FILE_SIZE, PERM_GROUP_WRITE, PERM_OTHER_WRITE
28+from .constant import COLOR_PATTERN, ALLOWED_COMMANDS, FORBIDDEN_ARG_PATTERNS
26from .i18n import language, ZH29from .i18n import language, ZH
27 30 
28# 创建一个全局锁31# 创建一个全局锁
@@ -181,6 +184,73 @@ class GraphUtils:
181 logger.error(f"An error occurred while parsing the nodeInfo parameter: {str(e)}")184 logger.error(f"An error occurred while parsing the nodeInfo parameter: {str(e)}")
182 return default_value185 return default_value
183 186 
187+ @staticmethod
188+ def safe_validate_command(cmd_list):
189+ """
190+ 安全校验外部传入的命令列表,防止命令注入攻击。
191+ 
192+ :param cmd_list: 命令列表,如 ["msprobe", "graph_visualize", "-tp", "/path/to/file"]
193+ :return: (success, error_message) 元组,校验通过返回 (True, None)
194+ """
MenbaMenba
MenbaMenba5月28日

FORBIDDEN_ARG_REGEX 中 $ 的二次检查重复/过于严格

  • 位置: plugins/tb_graph_ascend/hierarchy_plugin/server/app/utils/graph_utils.py:211
  • 严重级别: 严重
  • 说明: regex 已经用 r"\$\(" 匹配 $((命令替换),但 secondary check 中的 any(ch in arg for ch in ["$", ...]) 额外禁止了任何包含 $ 字符的参数。这意味着像 --param=value$1 这样的合法参数也会被拒绝。由于本场景不使用 shell=True,孤立的 $ 不会造成注入风险。建议移除 secondary check 中的 "$",仅保留 regex 级别的 $( 匹配即可,或至少添加注释说明此限制是故意收紧的。
likedislike
sun-chao
5月29日 评论:
MenbaMenba5月28日

secondary check 全面禁止反斜杠过于严格

  • 位置: plugins/tb_graph_ascend/hierarchy_plugin/server/app/utils/graph_utils.py:211
  • 严重级别: 一般
  • 说明: any(ch in arg for ch in ["\\", ...]) 禁止了任何参数中的反斜杠字符。虽然 Linux 路径用 /,但 msprobe 的参数可能包含转义序列或正则表达式中的 \。此外,由于 r"\\x" 正则已经专门匹配 \x 字符串,而 any() 中的 "\\" 完全覆盖了所有反斜杠场景,使 r"\\x" 正则变得事实上冗余。建议在注释中明确反斜杠禁止的安全模型,或考虑放宽为仅禁止 \x 等特定危险模式。
likedislike
sun-chao
5月29日 评论:
195+ if not isinstance(cmd_list, (list, tuple)):
196+ return False, "Command must be a list or tuple"
197+ 
198+ if len(cmd_list) == 0:
199+ return False, "Command list cannot be empty"
200+ FORBIDDEN_ARG_REGEX = re.compile("|".join(FORBIDDEN_ARG_PATTERNS))
201+ # 1. 校验命令名(白名单)
202+ cmd_name = cmd_list[0]
203+ if isinstance(cmd_name, str) and "/" in cmd_name:
204+ cmd_name = os.path.basename(cmd_name)
205+ if cmd_name not in ALLOWED_COMMANDS:
206+ return False, f"Command '{cmd_name}' is not in the allowed whitelist"
207+ 
208+ # 2. 校验命令可执行
209+ if not shutil.which(cmd_list[0]):
HowSir_X
HowSir_XHowSir_X5月28日

【review】【Bug】 【 文件和行号】plugins/tb_graph_ascend/hierarchy_plugin/server/app/utils/graph_utils.py:226-227 【检视意见】safe_validate_command 中白名单校验(第 222 行)使用 os.path.basename(cmd_list[0]) 提取命令名,而 shutil.which(cmd_list[0])(第 226 行)校验原始路径。两者逻辑不一致:如果 cmd_list[0] 为 "./msprobe" 或相对路径,白名单校验通过(提取 "msprobe"),但 shutil.which("./msprobe") 可能返回 None 导致误拦截。当前调用方始终传入裸命令名 "msprobe" 无影响,但若未来调用方传入路径形式,会出现校验不一致。建议统一校验逻辑:仅校验 os.path.basename(cmd_list[0]) 是否在白名单中,并用 shutil.which(cmd_name) 替换 shutil.which(cmd_list[0])。

likedislike
210+ return False, f"Command '{cmd_list[0]}' not found in PATH"
211+ 
212+ # 3. 校验每个参数
213+ for i, arg in enumerate(cmd_list):
214+ if not isinstance(arg, str):
215+ return False, f"Command argument at index {i} is not a string"
216+ 
217+ # 长度限制
218+ if len(arg) > FILE_PATH_MAX_LENGTH:
219+ return False, f"Command argument at index {i} exceeds length limit"
220+ 
221+ # 禁止注入字符
222+ if FORBIDDEN_ARG_REGEX.search(arg):
223+ return False, f"Command argument at index {i} contains forbidden characters"
224+ 
225+ # 禁止 shell 特殊字符组合
226+ if any(ch in arg for ch in ["$", "`", "\\", "\n", "\r"]):
227+ return False, f"Command argument at index {i} contains unsafe characters"
228+ 
229+ return True, None
230+ 
Menba
MenbaMenba5月28日

safe_run_command 缺少 stdin=DEVNULL

  • 位置: plugins/tb_graph_ascend/hierarchy_plugin/server/app/utils/graph_utils.py:247
  • 严重级别: 一般
  • 说明: subprocess.Popen(cmd_list, **kwargs) 未设置 stdin=subprocess.DEVNULL。子进程默认继承父进程的标准输入,而在 Web 服务场景中父进程的 stdin 可能连接着网络请求或 HTTP 连接。若子进程意外读取 stdin,可能导致死锁或数据泄露。建议在 Popen 调用中默认添加 stdin=subprocess.DEVNULL,除非 kwargs 中显式指定了 stdin。
likedislike
sun-chao
5月29日 评论:
231+ @staticmethod
232+ def safe_run_command(cmd_list, **kwargs):
233+ """
234+ 安全执行命令,先进行注入校验,再以非 shell 方式执行。
235+ 
236+ :param cmd_list: 命令列表
237+ :param kwargs: 传递给 subprocess.Popen 的额外参数(如 stdout, stderr, text 等)
238+ :return: (success, process_or_error) 校验失败返回 (False, error_msg);
239+ 校验成功返回 (True, Popen 实例)
240+ """
241+ success, error = GraphUtils.safe_validate_command(cmd_list)
HowSir_X
HowSir_XHowSir_X5月28日

【review】【Bug】 【文件和行号】plugins/tb_graph_ascend/hierarchy_plugin/server/app/utils/graph_utils.py:258-268 【检视意见】safe_run_command 在 subprocess.Popen 异常时返回 (False, str(e)),但调用方 graph_service_base.py:136-144 通过 if not success 检查并打印 result。当 Popen 抛出异常(如 FileNotFoundError),result 为异常消息字符串,但 ProgressInfo.error_msg 被赋值为异常消息,语义上 error_msg 可能被上层 UI 直接展示给用户,建议在开发/生产日志中记录详细异常,给用户返回简洁的通用错误信息。

likedislike
242+ if not success:
243+ logger.error(f"Command validation failed: {error}, cmd: {cmd_list}")
244+ return False, error
245+ 
246+ try:
247+ # The process must remain alive for the caller to monitor via update_progress_info
248+ proc = subprocess.Popen(cmd_list, **kwargs) # pylint: disable=consider-using-with # nosec B603
249+ return True, proc
250+ except Exception as e:
251+ logger.error(f"Failed to execute command: {e}, cmd: {cmd_list}")
252+ return False, str(e)
253+ 
184 @staticmethod254 @staticmethod
185 def safe_get_meta_data(data, default_value=None):255 def safe_get_meta_data(data, default_value=None):
186 meta_data = data.get("metaData")256 meta_data = data.get("metaData")
H
HHUsss8023475月28日

此条代码评论区间+251256

【review】【Bug】 【文件和行号】plugins/tb_graph_ascend/hierarchy_plugin/server/app/utils/graph_utils.py:267-272 【检视意见】safe_run_command 中 except Exception as e 捕获所有常规异常后仅记录日志并返回 (False, str(e)),异常消息 str(e) 可能包含敏感的系统路径、库版本或内部错误详情。注释 #8 已提及 Popen 异常路径的消息泄露问题,补充一点:若 subprocess.Popen 因 FileNotFoundError 失败(命令不在 PATH),str(e) 通常包含 No such file or directory: 'xxx',其中 xxx 是用户传入的命令名,属外部可控输入。若 ProgressInfo.error_msg 最终在 Web UI 中展示,建议对返回给前端的错误消息做通用化处理(如返回 Internal error, please check server logs),同时在日志中保留详细错误信息。

likedislike
@@ -54,11 +54,14 @@ def build_frontend(plugin_name):
54 raise RuntimeError(f"{failed_message} file 'package.json' is not exist!")54 raise RuntimeError(f"{failed_message} file 'package.json' is not exist!")
55 55 
56 # 安装依赖56 # 安装依赖
57- install_result = subprocess.run( # nosec57+ install_result = subprocess.run( # nosec B603, B607
58- ["npm", "install", "--force"], capture_output=True, text=True, check=False58+ ["npm", "ci"],
HowSir_X
HowSir_XHowSir_X5月28日

【review】【Bug】 【文件和行号】setup.py:57-60 【检视意见】将 npm install --force 替换为 npm ci 属于行为变更:npm ci 要求项目目录下存在且仅依赖 package-lock.json,且当 node_modules 已存在时会直接失败(除非删除),npm install --force 则更宽容。若构建环境的 package-lock.json 与 package.json 不同步,或 node_modules 残留未被清理,构建将失败。建议确认项目中存在 package-lock.json 且 CI 环境执行了 clean 步骤,否则应保留 npm install --force 或添加兼容判断。

likedislike
59+ capture_output=True,
60+ text=True,
61+ check=False,
59 )62 )
60 if install_result.returncode != 0:63 if install_result.returncode != 0:
61- raise RuntimeError(f"{failed_message} run 'npm install --force' failed!")64+ raise RuntimeError(f"{failed_message} run 'npm ci' failed!")
62 65 
63 # 执行构建66 # 执行构建
64 build_result = subprocess.run( # nosec67 build_result = subprocess.run( # nosec