Skip to content

Fix/security hardening - #3877

Closed
8079-debug wants to merge 3 commits into
modelscope:mainfrom
8079-debug:fix/security-hardening
Closed

8079-debug wants to merge 3 commits into
modelscope:mainfrom
8079-debug:fix/security-hardening

Conversation

@8079-debug

Copy link
Copy Markdown

Summary

Replaces unsafe dynamic-code parsing (eval, yaml.Loader) with safe alternatives and hardens the runtime HTTP/WebSocket/Flask services against malformed client input and resource exhaustion. All changes are behavior-preserving for valid inputs.

  • eval() on dataset/config/manifest strings → ast.literal_eval; data2vec conv-layer expressions use a restricted AST parser (literals and +/* only)
  • yaml.load(Loader=yaml.Loader) → yaml.safe_load in 6 runtime config/vocab loaders
  • Flask service: debug mode disabled on 0.0.0.0, unique temp files instead of fixed /tmp paths, collision-safe upload names, empty-result and form-JSON guards
  • HTTP service: upload size limit (--max_upload_mb, default 100, returns 413), temp-file cleanup in finally, extension sanitization, empty-result guard, inference offloaded to a thread pool
  • WebSocket service: chunk_interval clamped to ≥1 (division by zero), chunk_size type validation without session teardown, wav_name path-traversal sanitization including Windows separators
  • Download: request timeouts, mkdtemp instead of a GC-reclaimed TemporaryDirectory().name, URL-path basename for saved filenames

Type of change

  • Bug fix
  • Runtime or deployment

Validation

  • python -m compileall funasr examples tests (exit 0)

  • Runtime/deployment command tested

  • pytest tests/test_data2vec_compute_var.py tests/test_websocket_file_finalization.py tests/test_release_runtime_downloads.py tests/test_runtime_sdk_download_tool.py → 56 passed

  • 20 additional behavior checks: malicious expressions and YAML tags are rejected; valid model configs, bytes literals and fairseq-style conv expressions still parse correctly

  • tests/test_training_docs_contract.py has 3 failures verified identical on a clean baseline worktree (pre-existing, unrelated)

User impact

Production deployers of the runtime services: no debug-console exposure, no disk/memory exhaustion via uploads, no session crashes from malformed config messages. Toolkit users: parsing of dataset/manifest/config files can no longer execute arbitrary code.

Notes for reviewers

  • No functional change for valid inputs; existing websocket test contracts preserved (including the intentional invalid-mode discard behavior).
  • The Kaldi piped-command feature in kaldi_data.py and torch.load checkpoint loading are intentionally left unchanged.
  • Parsing issues are described defensively here; happy to follow the SECURITY.md disclosure channel if maintainers prefer.

@LauraGPT LauraGPT left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Changes requested for the upload cleanup path in runtime/python/websocket/funASR-Service.py.

speech_recognition_Timestamp_cam_identify_speakers() saves the upload before parsing/validating the form. The new malformed-JSON except returns 400 before reaching the inference cleanup, and the empty/invalid speaker database 400 path also returns with the saved file still present. With the new UUID filenames, rejected requests leave separate files behind instead of cleaning up the upload.

Independently reproduced against exact head bb2e520acadd6484f509c6ec5b6045de0a597b55, file blob 9a4a00df893ccaa306e868d22ba5556ba3ab3ea2, using real Flask/Werkzeug multipart requests and a private filesystem:

  • identify_speakers=not-json, speaker_db={}: 400, one uploaded file remains.
  • identify_speakers=false, speaker_db={: 400, one uploaded file remains.
  • identify_speakers=true, speaker_db={}: 400, one uploaded file remains.
  • Valid false/{} with an empty inference result: 200 and the upload is removed (passing control).

Each fixture was only 22 bytes. Cleanup assertions give 3 failed, 1 passed; no model inference is reached in the rejected cases. Environment: Python 3.12.3, Flask 3.1.2, Werkzeug 3.1.9, pytest 8.1.1. ASR/speaker model construction and inference were replaced at their boundary; Flask, multipart parsing and file operations were real. This is a scoped HTTP/filesystem result, not GPU/ASR validation or acceptance of all 17 changed files. The initial missing-Flask setup errors were retained separately and are not counted as this reproduction.

Please validate all form fields before persisting the upload, or place persistence, validation and inference under a cleanup finally that covers every return/exception. Add permanent route-level regressions for the rejected cases above plus a successful control. The pre-existing empty-database early return should be covered by the same cleanup rather than left outside it. Other changes in this PR still need their own relevant tests before merge.

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