================================================================================ PROPOSAL major-security-1 Unvalidated client-supplied filename reaches a file-write sink (path traversal) ================================================================================ Branch : 11645a (task #11645), trunk base r16695 Files : src/com/goldencode/p2j/ui/client/gui/DragDropHelper.java (sink) src/com/goldencode/p2j/ui/client/gui/driver/web/GuiWebDriver.java src/com/goldencode/p2j/ui/client/gui/driver/web/GuiWebSocket.java Severity : MAJOR as a defect in its own right; scoped MINOR for THIS diff because the sink is pre-existing and untouched here Category : pre-existing defect, NOT introduced by this branch 1. SCOPE NOTE - READ FIRST -------------------------------------------------------------------------------- This is not a defect in the 11645 changes. The vulnerable sink and its two twins predate the branch entirely and are untouched by it; the branch only reworks the MSG_PARTIAL reassembly that happens to be the delivery route. It surfaced during this review and is written up here so it is not lost. Recommendation: raise as its own ticket and fix outside this task's branch. Do not bundle it into 11645 - it changes a method signature used by three call sites and deserves its own review and test pass. 2. PROBLEM -------------------------------------------------------------------------------- A client-supplied filename flows verbatim into a file-write sink with no normalization, no canonicalization and no containment check, giving arbitrary file write on the host running the spawned web client. Verified chain (all in the post-diff tree): 1. WebClientProtocol.java:2230 AppendMessageTask.run() on the last piece calls processChannel(channel). 2. GuiWebSocket.java:2419-2441 messageType == MSG_FILE_UPLOADING fileName = readMessageText(...) <- verbatim from the peer callbacks.saveDropTargetFile(fileName, channel, fileSize) No validation. Also no gating: the handler does not require a pending file dialog or an active drop target. (Contrast chooseFiles / FileUploadInfo at :2383-2394, which the HTTP upload path does consult.) 3. GuiWebDriver.java:1935 DragDropHelper.readDataAndSaveAsFile(tmpDir.concat(name), channel, length) tmpDir is set unconditionally for every GUI client (ThinClient.java:4327 -> AbstractGuiDriver.java:399) and ENDS WITH File.separator (Utils.getOrCreateTemporaryDirectory, Utils.java:4487) - so concat yields a traversable prefix, not a mangled sibling name. 4. DragDropHelper.java:254-261 Paths.get(name) -> Files.newByteChannel(fileToWrite, OPEN_OPTIONS) No normalize(), no getFileName(), no startsWith(tmpDir) check. A crafted peer sending a UTF-16 name field of "../../../../home//.bashrc" writes attacker-chosen bytes to that path. Two more sinks share the flaw, both pre-existing: - GuiWebSocket.java:3540 (small-message MSG_FILE_UPLOADING) -> GuiWebDriver.java:1918 -> DragDropHelper.saveFile:214 - GuiWebDriver.startDropTarget:1890, which also concatenates an unvalidated name 3. WHY THIS CROSSES A PRIVILEGE BOUNDARY -------------------------------------------------------------------------------- The obvious objection - "that is the client JVM the end user already owns" - does not hold for the web GUI driver. The GuiWebSocket / GuiWebDriver process is the spawned FWD web client running SERVER-SIDE: WebDriverHandler authenticates, spawns the client, and the browser is then redirected to that process's embedded Jetty (VirtualDesktopWebHandler:98). That websocket endpoint is registered with GenericWebSocketCreator at the fixed AJAX_TARGET path with NO Origin check (GenericWebServer.addWebSocketHandler:938-955). The OS account it runs as comes from WebDriverHandler.java:986-1006: with OS_USER_OVERRIDE set, or with no SSO OS-user mapping, every user's client runs as the single shared DEFAULT_OS_USER. So an authenticated FWD *application* user - who normally has no shell and no filesystem rights on the application server - gains arbitrary file write on the server host as that frequently-shared OS account, which also reaches other users' sessions. That is a real boundary crossing. Mitigating factors (why not CRITICAL): - It is gated behind authentication; the spawned client only exists post-login. - A stock browser cannot produce a File.name containing path separators (p2j.socket.js:1746 createFileUploadingMessage just forwards file.name), so the trigger needs a crafted websocket peer or injected JS, not an ordinary drag-and-drop. 4. PROPOSED FIX -------------------------------------------------------------------------------- FWD already contains the correct pattern - the HTTP multipart upload path does exactly the missing check: UploadHandler.java:231-233 Paths.get(filename); file = file.getFileName(); uploadDir.resolve(file); Apply the same containment at the SINK so all three callers are covered by one change, rather than patching each caller. Step 1 - change DragDropHelper to take the directory and the untrusted name as separate arguments instead of a pre-concatenated string. This is the crux: as long as the API takes one joined string, every caller is one concat away from reintroducing the bug. private static Path resolveInside(String dir, String untrustedName) throws IOException { Path base = Paths.get(dir).toAbsolutePath().normalize(); Path name = Paths.get(untrustedName).getFileName(); if (name == null) { throw new IOException("Illegal upload file name: " + untrustedName); } Path target = base.resolve(name).normalize(); if (!target.startsWith(base)) { throw new IOException("Upload path escapes the target directory: " + untrustedName); } return target; } Notes on the implementation: - Paths.get(untrustedName) can itself throw InvalidPathException on a NUL byte or an illegal Windows name; catch it and convert to the same IOException so callers see one failure mode. - getFileName() alone defeats "../" traversal; the startsWith check is kept as belt-and-braces and to catch platform-specific surprises (e.g. Windows alternate data streams, drive-relative paths). - Reject or rewrite an empty result and the reserved names "." and "..". Step 2 - route readDataAndSaveAsFile and saveFile through resolveInside, and update the three call sites (GuiWebDriver:1918, :1890, :1935) to pass tmpDir and the raw name separately instead of concatenating. Step 3 - add the missing gate in GuiWebSocket.processChannel: only accept MSG_FILE_UPLOADING when a file dialog or drop target is actually pending, the way the HTTP path consults FileUploadInfo. Defence in depth - it removes the unsolicited-upload primitive entirely, independent of the name handling. Step 4 - decide the policy for a name collision inside tmpDir (overwrite vs unique-ify). Current behaviour overwrites; preserve it unless the team wants a change, but make it explicit rather than incidental. 5. ALTERNATIVES CONSIDERED -------------------------------------------------------------------------------- (i) Sanitize in GuiWebSocket.processChannel where the name is read. Rejected as the primary fix: it leaves the sink dangerous for the other two callers and for any future one. Do it in addition if cheap, not instead. (ii) Reject any name containing a separator or "..". Weaker than getFileName() (encoding tricks, platform quirks) and rejects names a browser can legitimately produce on some platforms. Prefer normalization to blacklisting. 6. RISK -------------------------------------------------------------------------------- Low functionally, but it is a signature change across three call sites, so it must compile-and-run clean on both the GUI and CHUI web drivers. Behavioural change to be aware of: names that previously wrote outside tmpDir (only reachable maliciously) now fail with an IOException - confirm the error surfaces sensibly to the user rather than killing the drawing thread. 7. TEST PLAN -------------------------------------------------------------------------------- 1. Ordinary drag-and-drop file upload in the web GUI client: unchanged, file lands in tmpDir. 2. Ordinary file-dialog upload (both the websocket and the HTTP multipart path): unchanged. 3. Crafted websocket frame with name "../../../../tmp/pwned": expect an IOException and no file written outside tmpDir. Verify via the filesystem, not just the log. 4. Name with a NUL byte, an empty name, "." and "..": expect clean rejection, no crash. 5. Unsolicited MSG_FILE_UPLOADING with no pending dialog or drop target: expect rejection once Step 3 is in. 6. Windows and Linux both, since the path semantics differ.