Contain public resource output paths

[M0nd0R]

Contain public resource output paths

Public resource paths can be supplied by classpath resources and are later written into compiler and CodeServer output directories. This change resolves output paths against the intended output root and rejects any path that canonicalizes outside that root before creating parent directories or writing bytes.

Changes:

• Contain OutputFileSetOnDirectory writes inside dir + prefix.
• Contain CodeServer public-resource writes inside the module output directory.
• Add regression coverage for harmless in-root normalization and escaping traversal attempts.
• Add the new CodeServer test to the Bazel and JUnit suite metadata.

Validation:

• git diff --check
• ant dev -Dtarget=test -Dgwt.junit.testcase.dev.core.includes="**/OutputFileSetOnDirectoryTest.class"
• ant codeserver -Dtarget=test -Dtest.dev.disable=true

No linked issue.

[niloc132]

What's the actual bug or feature you're addressing here? If I'm reading it right, this looks like validation that the project's source isn't maliciously reading other files on disk as if they are source - but specific only to the codeserver, rather than the compiler or running tests?

[M0nd0R]

The bug this addresses is path traversal on output writes, not source reads.

Public resource path names (and other output paths) can contain segments like ../. When those get joined onto an output directory and then canonicalized, GWT can create parent dirs / write bytes outside the intended output root. That’s the failure mode this PR is closing.

It’s also not CodeServer-only:

1. OutputFileSetOnDirectory — used when the compiler (and related tooling) writes into a directory output. That path now resolves against dir + prefix and rejects anything that canonicalizes outside that root.
2. LauncherDir.writePublicResources — CodeServer’s copy of public resources into the module output directory, with the same containment check.

So the shared write-path hardening is in OutputFileSetOnDirectory; the CodeServer change is the matching sink for public resources during Super Dev Mode. I didn’t try to cover every possible filesystem interaction in the test runners — just the places where classpath/public resource paths are turned into on-disk writes under an output root.

Happy to broaden coverage further if there’s another write sink you think should get the same treatment.

[zbynek]

The Bazel build files are not used, no need to update them.

[niloc132]

The bug this addresses is path traversal on output writes, not source reads.

Understood - but my question is where is the path read from such that output is written to the incorrect location? Or does the compiler generate ../ paths somewhere itself?

Public resource path names (and other output paths) can contain segments like ../.

How can that happen, specifically, in a real project?

Public resources are resources in a directory specified by a <public /> source tag in a .gwt.xml file, which means that the paths are read from the directory's contents, or created as an EmittedArtifact by a Generator at compile time. I'm not aware of a mechanism by which public resources can be contributed by other means, except EmittedArtifacts, which are explicitly created by a Generator at compile time.

You didn't file or link an issue to go with this, so it seems like you're speculating based on code that looks wrong at a glance outside the context of the rest of the project. I could imagine that a Generator might be able to achieve this case, but someone would need to go out of their way to do it that way, and I'm not sure how we reasonably guard against that without breaking existing code. If that is the specific use case you're after, it would be far better to move that validation closer to where the path is emitted so that the user gets a stack trace that reflects the issue.

[M0nd0R]

Fair question — thanks for pushing on the concrete path.

Where the path comes from (not the compiler inventing ../ on its own)

For public resources, path names are the rerooted classpath resource paths from ResourceOracleImpl / PathPrefix.getRerootedPath. That method is a plain prefix substring — it does not normalize or reject .. segments.

So the realistic case is a JAR/ZIP on the compile classpath whose entry name starts with the module’s <public> prefix and then continues with .. segments. Example:

• <public path="com/example/app/public"/>
• JAR entry: com/example/app/public/../../../outside.txt
• includesResource accepts it (prefix match)
• getRerootedPath yields ../../../outside.txt
• LauncherDir.writePublicResources / linker emission then does new File(outputDir, pathName) and writes outside the module/war output root

That’s classic zip-slip into the output tree. A normal on-disk <public> directory walk won’t produce this (file names don’t contain ../), which is why it doesn’t show up in typical projects. It shows up if a dependency JAR (or intentionally crafted test JAR) contributes public resources.

OutputFileSetOnDirectory is a separate sink

That path is not limited to public resources. StandardLinkerContext emits EmittedArtifact.getPartialPath() straight into OutputFileSet.openForWrite. Those partial paths come from Generators/Linkers. The pre-existing unit test even asserted that /../path/... with prefix test/ would write to work/to/file — i.e. escaping the prefix was previously accepted behavior.

On validating closer to emission

Agreed for the Generator/Linker case: rejecting at emitArtifacts / artifact creation gives a stack that points at the emitter, which is more useful than failing later at mkdir/write. I’ll move/add that validation next to emission and keep the write-sink check as defense-in-depth for the JAR public-resource case above (where the bad path is already in the resource oracle before any Generator runs).

Also noting @zbynek’s comment: Bazel dev/codeserver/BUILD isn’t used — reverting that suite/test wiring.

Happy to add a small JAR-based regression that packs a ../-escaping public entry if that would make the threat model clearer.

[M0nd0R]

Follow-up pushed:

1. Reverted the unused dev/codeserver/BUILD LauncherDirTest wiring (@zbynek).
2. Emit-time check in OutputFileSet.openForWrite via pathEscapesRoot() — rejects relative paths that walk above the output root (allows in-root a/../b). That fails at the emit API used by StandardLinkerContext.produceOutput, so the artifact logger / stack points at emission rather than a later mkdir.
3. Left the canonicalization guards in OutputFileSetOnDirectory / LauncherDir as defense-in-depth (covers prefix escape and any FS-oddities the logical check misses).

Ant/JUnit LauncherDirTest + CodeServerSuite wiring is unchanged.

[niloc132]

So to confirm, "The bug this addresses is path traversal on output writes, not source reads" actually means "yes, this is a bug with malicious sources". I agree this is a plausible issue, but this is what I was hoping for you to clarify in the issue, in the PR title/description, or when I asked the first time.

If the preexisting unit test confirms that the behavior works, (ping @jnehlmeier, the author of that test), perhaps we shouldn't be changing this?

Instead, when there is an issue of unsafe inputs, we should validate the inputs themselves, right?

For example, ModuleDef lets the user build up .gwt.xml files and handles any <public> tags, and uses DefaultFilters to handle includes/excludes. DefaultFilters also has i`sDefaultExcluded(String), which has some default excludes that we could append to here, to search for ".." path segments? There could be other places her to check for appropriate excludes.

The ModuleDef.lazyPublicOracle field is of type ResourceOracleImpl, and has a method scanResources - this is what would trigger the checks. This delegates to the abstract ClassPathEntry.findApplicableResources to actually look for resources - ZipFileClassPathEntry#computeApplicableResources looks to be the zip implementation here that would end up resolving those, and could itself discover (and warn?) when any ".." path segment is found in the zip's directory record.

If a classpath jar has apparently malicious contents, warning when we find the issue with a specific pointer to the jar in question seems more appropriate than logging where we would have written the output - if there is something malicious on the classpath, it should be removed outright. That said, if you've already included something malicious like this on the classpath and included its .gwt.xml, they could have just written a Generator that runs arbitrary code at compile time, so its a weird way to try to attack.

--

We already have a few tests that use checked-in jars/zips for resource scanning and such - ClassPathEntryTest, ResourceOracleImplRealClasspathTest, and ResourceOracleImplTest each extend from AbstractResourceOrientedTestBase which offers helper methods to find these existing test jars. You might want to model any tests off of this, to demonstrate detection and handling.

[jnehlmeier]

The test exists because there was a regression long ago which is discussed in #8483
The patch restored the previous behavior of allowing .. in paths and added a test for that.

However in the linked issue there was already discussion within the Google team to restrict such paths, either completly or only allowing them within a sane boundery like the current project. So they asked back why .. has been used in the first place and it was to better organize generated files produced by a custom GWT generator.

But even if disallowed a malicious library can still literally to anything on your host. As @niloc132 pointed out you usually have to inherit a GWT module and once you do that, this GWT module can mess with public resource paths but also replace a linker or generator and sneak in additional malicious code that uses java.nio to write anywhere. It is similar to adding a new maven or gradle plugin to the build. You have to trust that new external library and in the best case verify its hash before each build if you use a build system that potentially downloads that library automatically.

There is only one way without inheriting a GWT module manually and that is if a classpath jar provides a GWT module that GWT SDK also provides. In that case classpath ordering decides which GWT module will be loaded. GWT could potentially check if GWT SDK modules have been indeed loaded from GWT SDK jars but in general it is much better to secure your build via your build tool so you can trust your libraries based on signatures.

[niloc132]

Any more thoughts here or should we close this?

[zbynek]

Let's close this, can always reopen if something changes.