FWD » Core Development » Runtime Infrastructure

================================================================================

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/<user>/.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.