mirror of
https://github.com/MobSF/Mobile-Security-Framework-MobSF.git
synced 2026-10-11 22:50:09 +00:00
[HOTFIX][Security] Security Updates July 5 2026 (#2627)
Security Fixes [GHSA-8j49-mmcx-4mp5] Arbitrary File Read via Path Traversal in ZIP/APK Icon Extraction The android:icon attribute from an APK manifest was interpolated into file paths without validation, allowing a crafted APK to read arbitrary files from the server. Fixed by adding is_path_traversal() and is_safe_path() guards in find_icon_path_zip. Dependency bump included. [GHSA-3p54-567p-2wpr] CSRF Checks Not Enforced After Django Middleware Migration The migration from the deprecated MIDDLEWARE_CLASSES to MIDDLEWARE omitted CsrfViewMiddleware, SecurityMiddleware, and XFrameOptionsMiddleware, leaving CSRF, HSTS, and clickjacking protections silently disabled. All three have been restored to the active MIDDLEWARE tuple. The now-dead MIDDLEWARE_CLASSES block has been removed to prevent future confusion. [GHSA-x768-8642-mmq9] Zip Bomb Denial of Service via Per-File Size Limit Bypass The per-file size check in ZIP extraction logged a warning on oversized members but was missing a continue, allowing files exceeding ZIP_MAX_UNCOMPRESSED_FILE_SIZE to be extracted anyway as long as the aggregate limit was not reached. Fixed by adding the missing continue. [GHSA-95px-34x5-p37h] SSRF Port Restriction Bypass in assetlinks_check valid_host() was called with only the bare hostname, allowing android:port to be appended afterward without going through port validation, bypassing the HTTP/HTTPS-only restriction. Fixed with a two-layer approach: primary port allowlist check before URL assembly in get_browsable_activities, and a defence-in-depth re-check in _check_url. Hardening The hand-rolled safe_extract used os.path.abspath (symlink-blind) instead of os.path.realpath, creating a TOCTOU window where a symlink could redirect extraction outside the destination directory. Replaced with Python 3.12's tarfile.extractall(filter='data') (PEP 706), which rejects symlinks, hardlinks, absolute paths, and traversal members per-member before extraction. A robust realpath-based fallback is included for older Python versions. Both download_app_data call sites in the iOS dynamic analyser now wrap app_container and tarfile with shlex.quote when building the SSH tar command, as defensive coding hygiene. Refactoring is_path_traversal, is_safe_path, clean_filename, cmd_injection_check, is_pipe_or_link, and is_attack_pattern were scattered across utils.py and shared.py. All have been moved to mobsf/MobSF/security.py as the single authoritative location for security primitives. Developer Tooling AGENTS.md / CLAUDE.md - AI agent guidelines Added a coding-standards document for AI coding agents (Cursor, Claude, Codex) covering MobSF-specific secure-by-default patterns: path traversal guards, archive extraction safety, Django middleware active-tuple hygiene, split-validation anti-patterns (SSRF port bypass), guard completeness (continue/return/raise after every security check), and Django-specific security features (form validators, decorators, middleware). CLAUDE.md is a symlink to AGENTS.md.
This commit is contained in:
Ajin Abraham
@@ -0,0 +1,234 @@
|
||||
# MobSF Agent Guidelines
|
||||
|
||||
MobSF is a security analysis platform. Every code path processes attacker-supplied
|
||||
input (APKs, ZIPs, IPAs, manifests) from authenticated but potentially malicious
|
||||
users. Security must be the default, not an afterthought.
|
||||
|
||||
---
|
||||
|
||||
## Code Quality — Mandatory Before Every Commit
|
||||
|
||||
Run lint and fix all errors before finishing any task:
|
||||
|
||||
```bash
|
||||
tox -e lint
|
||||
```
|
||||
|
||||
Never leave a task with a non-zero exit code from this command.
|
||||
|
||||
---
|
||||
|
||||
## Security Architecture
|
||||
|
||||
Centralized security helpers live in **`mobsf/MobSF/security.py`**. When adding new
|
||||
security checks, prefer adding them there. Some legacy validators still live in
|
||||
`mobsf/MobSF/utils.py`; use existing helpers where they are already established.
|
||||
|
||||
### Available Security Functions
|
||||
|
||||
Import only the helpers needed for the change:
|
||||
|
||||
```python
|
||||
from mobsf.MobSF.security import (
|
||||
# Path safety
|
||||
is_path_traversal, # Check raw string for .. sequences, absolute paths, URL encoding tricks
|
||||
is_safe_path, # Containment check after path construction via realpath()
|
||||
|
||||
# Input validation
|
||||
is_attack_pattern, # Detect shell injection: ;, $(), ||, &&
|
||||
cmd_injection_check, # Detect OS command injection characters
|
||||
is_pipe_or_link, # Detect symlinks and named FIFOs before reading files
|
||||
|
||||
# Output sanitization
|
||||
sanitize_filename, # Safe filename for Content-Disposition headers
|
||||
sanitize_for_logging,# Strip newlines and control chars before logging user input
|
||||
sanitize_redirect, # Allow only relative paths in redirects
|
||||
sanitize_svg, # Strip XSS vectors from SVG content (bleach-based)
|
||||
clean_filename, # Windows-safe filename (unicode normalization)
|
||||
|
||||
# Network / SSRF
|
||||
valid_host, # DNS-resolves host; rejects private/loopback/multicast IPs
|
||||
)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Past Vulnerabilities and Insecure Patterns
|
||||
|
||||
Read `.github/SECURITY.md` to understand the full history of security issues in this
|
||||
codebase. Use it as a guide for what classes of bugs to watch for and what patterns
|
||||
have been exploited before. When in doubt about whether a pattern is safe, check
|
||||
whether a similar pattern has appeared in the advisory history.
|
||||
|
||||
---
|
||||
|
||||
## Incomplete Fix Anti-Pattern
|
||||
|
||||
The most common source of security regressions in this codebase is applying a fix to
|
||||
one code path but not its siblings. Before closing any security fix:
|
||||
|
||||
1. Search for all functions or patterns that perform the same operation (e.g., every
|
||||
place that resolves an icon path, every place that extracts an archive entry).
|
||||
2. Verify the fix is applied consistently across **all** of them.
|
||||
3. Check both the APK binary flow and the source-ZIP flow — they are separate code paths
|
||||
with separate callsites and have diverged in the past.
|
||||
|
||||
---
|
||||
|
||||
## Input Trust Model
|
||||
|
||||
- `request.GET` / `request.POST`: untrusted. Validate with forms or explicit checks;
|
||||
escape on output.
|
||||
- File uploads: untrusted. Validate magic bytes, size limits, and extension allowlists.
|
||||
- Archive entries (`zip`, `tar`, `ar`): untrusted. Check each entry before extraction.
|
||||
- `AndroidManifest.xml` values: untrusted. Treat as attacker-controlled before using
|
||||
them in filesystem operations or rendering them.
|
||||
- `Info.plist` values: untrusted. Apply the same treatment as manifest values.
|
||||
- `md5` / `hash` URL parameters: semi-trusted only after validation. Always validate
|
||||
with `is_md5()` before using them in paths.
|
||||
- Device identifiers: untrusted. Use command-injection checks plus format validation.
|
||||
|
||||
---
|
||||
|
||||
## Django-Specific Security Features
|
||||
|
||||
### Form Validation — The Primary Input Sanitization Layer
|
||||
|
||||
Prefer Django forms for new request validation. If a view does not use a form, validate
|
||||
every `request.GET[...]` or `request.POST[...]` value explicitly before using it.
|
||||
|
||||
The project uses a mixin composition pattern. Combine the appropriate mixins rather than
|
||||
writing ad-hoc validation in view code:
|
||||
|
||||
```python
|
||||
# StaticAnalyzer/forms.py — mixins to compose from
|
||||
AttackDetect # is_path_traversal + extension allowlist on a 'file' param
|
||||
APIChecks # MD5 format check on a 'hash' param (API mode)
|
||||
WebChecks # MD5 format check on an 'md5' param (HTML mode)
|
||||
AndroidChecks # ChoiceField allowlist for Android scan type
|
||||
IOSChecks # ChoiceField allowlist for iOS scan type
|
||||
```
|
||||
|
||||
Custom field validators belong in a `clean_<field>()` method that raises
|
||||
`forms.ValidationError` on rejection — never return a partial result and check it in
|
||||
the view. `FormUtil.errors_message(form)` produces the standard error envelope to return
|
||||
to the caller when `form.is_valid()` is False.
|
||||
|
||||
**Use `ChoiceField` for any parameter with a finite set of valid values.** This
|
||||
eliminates an entire class of injection risk at the form layer with no extra code.
|
||||
Never use `CharField` and then manually compare the value against an allowlist in the
|
||||
view — let the form do it.
|
||||
|
||||
### View Decorators — Apply All Three
|
||||
|
||||
Views that handle sensitive operations should use the applicable Django decorators for
|
||||
authentication, authorization, and method restriction:
|
||||
|
||||
```python
|
||||
@login_required
|
||||
@permission_required(Permissions.SCAN) # or DELETE, SUPPRESS, etc.
|
||||
@require_http_methods(['POST']) # or ['GET'] — never omit this
|
||||
def my_view(request, api=False):
|
||||
...
|
||||
```
|
||||
|
||||
- `@login_required` blocks unauthenticated access.
|
||||
- `@permission_required` enforces role-based access beyond authentication.
|
||||
- `@require_http_methods` rejects wrong HTTP verbs before any logic runs,
|
||||
preventing CSRF-via-GET and other method-confusion issues.
|
||||
|
||||
### Template Auto-Escaping
|
||||
|
||||
Django's template engine escapes variables by default. Do **not** use `{% autoescape off %}`
|
||||
or the `|safe` filter on any value derived from scan data, manifests, or user input.
|
||||
When rendering user-controlled strings outside of templates (e.g., in a JSON response
|
||||
built by hand), use `django.utils.html.escape()` explicitly.
|
||||
|
||||
### ORM — No Raw SQL
|
||||
|
||||
Use the Django ORM for all database access. Never use `.raw()` or string-formatted SQL.
|
||||
When a queryset filter value comes from user input, pass it as a keyword argument
|
||||
(the ORM parameterizes it automatically):
|
||||
|
||||
```python
|
||||
# Correct
|
||||
RecentScansDB.objects.filter(MD5=checksum)
|
||||
|
||||
# Wrong
|
||||
RecentScansDB.objects.raw(f'SELECT * FROM ... WHERE MD5 = "{checksum}"')
|
||||
```
|
||||
|
||||
### CSRF
|
||||
|
||||
Django's `CsrfViewMiddleware` is enabled globally. Do not use `@csrf_exempt` on any
|
||||
view that modifies state. API endpoints that accept an `X-Csrftoken` header or use
|
||||
token-based auth are the only legitimate exception, and that pattern is already
|
||||
established in the existing API views.
|
||||
|
||||
---
|
||||
|
||||
## Archive Extraction Safety
|
||||
|
||||
### TAR
|
||||
|
||||
Never use a hand-rolled name-only check with `os.path.abspath`. The symlink +
|
||||
nested-entry combination bypasses it: a symlink member named `escape` passes the
|
||||
name check, gets extracted to disk, and then a file member named `escape/pwned.txt`
|
||||
is written through the symlink to an arbitrary location.
|
||||
|
||||
`os.path.abspath` normalises `..` but does **not** resolve symlinks.
|
||||
`os.path.realpath` resolves both — but even `realpath`-based checks that run before
|
||||
extraction have a TOCTOU window.
|
||||
|
||||
Use Python 3.12's built-in filter instead (MobSF requires `python = "^3.12"`):
|
||||
|
||||
```python
|
||||
# Correct — per-member, type-aware, symlink-aware
|
||||
tar.extractall(dest, members=safe_members_generator, filter='data')
|
||||
|
||||
# Wrong — abspath-based name check; blind to symlinks
|
||||
for member in tar.getmembers():
|
||||
if not os.path.abspath(join(dest, member.name)).startswith(dest):
|
||||
raise ...
|
||||
tar.extractall(dest, members=...)
|
||||
```
|
||||
|
||||
`filter='data'` rejects: symlinks outside destination, hardlinks outside destination,
|
||||
absolute paths, path traversal, and device files — per member, before extraction.
|
||||
|
||||
For code that must support Python < 3.12, fall back to: skip all symlink and hardlink
|
||||
members (`member.issym()` / `member.islnk()`), then use `realpath` for the boundary
|
||||
check, and validate-then-extract per member rather than batch-validate-then-extractall.
|
||||
|
||||
### ZIP
|
||||
|
||||
Python's `zipfile` module does not create real filesystem symlinks from Unix symlink
|
||||
entries — it writes the link target as plain file bytes. The TAR symlink attack does
|
||||
not apply to ZIP extraction. Use `is_path_traversal` + `is_safe_path` for member name
|
||||
validation and validate per-member before calling `zip_ref.extract(member, dest)`.
|
||||
|
||||
---
|
||||
|
||||
## Import Conventions
|
||||
|
||||
When adding new imports, maintain alphabetical order within each import group to satisfy
|
||||
`flake8-import-order`. Group order: stdlib → third-party → Django → local MobSF.
|
||||
|
||||
---
|
||||
|
||||
## Checklist for Any Change That Touches File I/O or User Input
|
||||
|
||||
- [ ] Raw input validated with `is_path_traversal` before path construction
|
||||
- [ ] Constructed filesystem paths verified with `is_safe_path` when a safe root exists
|
||||
- [ ] Symlinks and FIFOs rejected with `is_pipe_or_link` before file reads
|
||||
- [ ] Shell arguments passed as a list, not a formatted string
|
||||
- [ ] User-controlled strings escaped with `django.utils.html.escape` before rendering
|
||||
- [ ] SVG content piped through `sanitize_svg`
|
||||
- [ ] Outbound URLs checked with `valid_host`
|
||||
- [ ] Redirects wrapped in `sanitize_redirect`
|
||||
- [ ] Log statements use `sanitize_for_logging` on any user-derived value
|
||||
- [ ] TAR extraction uses `filter='data'` — not a hand-rolled `abspath` check
|
||||
- [ ] ZIP extraction validates each member path with `realpath` before `extract()`
|
||||
- [ ] Every security guard has `continue` / `return` / `raise` — logging alone is not a guard
|
||||
- [ ] Fix applied symmetrically to all equivalent code paths
|
||||
- [ ] `tox -e lint` passes with exit code 0
|
||||
Reference in New Issue
Block a user