fix: prevent path traversal via user_id in playground /clear_memory (CWE-22)#67
Open
sebastiondev wants to merge 1 commit intoBAI-LAB:mainfrom
Open
fix: prevent path traversal via user_id in playground /clear_memory (CWE-22)#67sebastiondev wants to merge 1 commit intoBAI-LAB:mainfrom
sebastiondev wants to merge 1 commit intoBAI-LAB:mainfrom
Conversation
…-22) User-supplied user_id was used unsanitized in filesystem path construction (os.path.join(data_path, "users", user_id)). An attacker could set user_id to "../../etc" via POST /init_memory, then POST /clear_memory would call shutil.rmtree() on the traversed path, deleting arbitrary directories. Changes: - Add validate_identifier() with strict allowlist regex that permits only alphanumeric chars, hyphens, underscores, and dots (1-128 chars). - Call validation on user_id in /init_memory before it reaches Memoryos. - Add defense-in-depth check in /clear_memory: verify resolved paths are within the expected data_storage_path before calling shutil.rmtree. - Move "import re" to module top-level (was previously inside function).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a path traversal vulnerability (CWE-22) in
memoryos-playground/memdemo/app.pywhere a user-supplieduser_idflows unsanitized into filesystem path construction and is later passed toshutil.rmtree(), allowing arbitrary directory deletion as the Flask process user.Vulnerability details
memoryos-playground/memdemo/app.pyPOST /init_memory(entry point) →POST /clear_memory(sink)0.0.0.0:5019with no authenticationData flow
POST /init_memoryaccepts a JSON body and readsuser_iddirectly fromrequest.json.user_idis passed toMemoryos(...), which buildsdata_storage_path/users/<user_id>(and a parallelassistants/...path) without sanitization.user_data_dir/assistant_data_dirare stored on the in-memorymemory_systemsdict keyed byuser_id.POST /clear_memorylooks up the entry and callsshutil.rmtree(user_data_dir)andshutil.rmtree(assistant_data_dir).A payload such as
{"user_id": "../../etc", ...}(along with the requiredapi_key/base_url/modelfields) escapes the intendeddata/users/directory. The subsequentclear_memorycall then deletes the traversed path recursively.Fix
Two layers of protection in
app.py:validate_identifier()helper enforces an allowlist regex (^[A-Za-z0-9][A-Za-z0-9._-]{0,127}$), rejects non-strings, empty/whitespace-only values, and null bytes.init_memory()returns HTTP 400 for anyuser_idthat fails validation, before aMemoryosinstance is created.clear_memory()resolves both the user and assistant directories withos.path.realpath()and verifies they live underrealpath(memory_system.data_storage_path) + os.sepbefore invokingshutil.rmtree(). If a path escapes the storage root (e.g. via an unexpected symlink or a future regression), the request is rejected with HTTP 400 instead of deleting files.Also moves
import reto module top-level (it was previously imported inside an unrelated helper).Tests
A new
tests/test_cwe22_path_traversal.pyexercisesvalidate_identifier()against representative inputs:"../../etc","foo/bar","/absolute","ab\x00c",""," ", non-string values."alice","user.1","user-1","user_1".I ran the validator manually against the same payloads and all behave as expected. The change is self-contained to
app.pyplus a new test file; no existing functionality is removed.Why this is exploitable in practice
The playground app has no authentication and listens on all interfaces by default (
app.run(host='0.0.0.0', port=5019)). Any user with network reachability to the port can send the malicious JSON. Flask does not sanitize JSON body fields used in filesystem operations, and there is no other layer (no reverse-proxy validation in the bundled config, nosecure_filename-style call) between the request andshutil.rmtree(). The only constraint on the deletion target is the OS-level permission of the Flask process.Adversarial review
Before submitting I tried to disprove this finding. I checked whether the
Memoryosconstructor or any helper betweeninit_memoryandclear_memoryalready normalises or rejects traversal segments — it doesn't; it just callsos.path.joinandos.makedirs(..., exist_ok=True), which happily accept..segments. I also considered whetherclear_memoryrequires prior state that an attacker couldn't reach — but the same unauthenticatedinit_memorycall seeds that state, so a single attacker controls both ends of the flow. Finally, I verified that the playground is intended to be runnable as-is (the README documents thepython app.pyentrypoint), so this isn't dead/example-only code that maintainers would expect to harden separately before deployment.Diff stat
Happy to adjust the allowlist regex (e.g. tighten dot handling to disallow leading dots beyond the anchor, or widen to support unicode usernames) if that better matches your intended user model.
cc @lewiswigmore