Harden two hostile-input paths reaching the crawl (project name, UTF-8)#19
Merged
Conversation
The project name became the engine's -O destination with no validation, and File does not fold "..", so a name like "../../sdcard/evil" — typeable, or supplied by a crafted winprofile.ini that overwrites the map on load — steered writes outside the Websites tree. A pure StoragePaths.isValidProjectName now rejects empty, ".", ".." and any path separator, enforced when advancing past the name pane (with a message) and as a backstop in getTargetFile, where the crawl then aborts cleanly instead of escaping. Covered by a discriminating unit test. newStringSafe relied on NewStringUTF raising a Java exception on malformed modified UTF-8, which it does not, so its guard was dead and its "ignore invalid UTF-8" contract false; build_stats feeds it server-controlled bytes (Content-Type, charset, status, urls). It now sanitizes to well-formed modified UTF-8 first, so hostile bytes cannot reach NewStringUTF and trip a CheckJNI abort. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
Review follow-up. When sanitizeModifiedUtf8's malloc fails, newStringSafe fell back to the raw string, which under CheckJNI on hostile input is exactly the abort the sanitizer exists to prevent. Return null there instead: a null field beats an abort. Also dropped a redundant project-name test; the bare ".." case is already covered by refusesEmptyOrDotSegments. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
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
1 participant
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.
Two hostile-input paths that a crawled server can drive.
The project name becomes the engine's
-Odestination with no validation, andFiledoes not fold.., so a name like../../sdcard/evilwrites outside the Websites tree. It is typeable, and a craftedwinprofile.inisupplies it without the user typing anything, since the profile load overwrites the name in the map. A pureStoragePaths.isValidProjectNamenow rejects empty,.,.., and any path separator. It is enforced when advancing past the name pane (with a message) and as a backstop ingetTargetFile, where the crawl aborts cleanly rather than escaping. A unit test covers it, and it is mutation-checked: a naive/-only validator fails the bare-..case.newStringSaferelied onNewStringUTFraising a Java exception on malformed modified UTF-8, which it does not, so its guard was dead code and its "ignore invalid UTF-8" contract was false.build_stats()feeds it server-controlled bytes (Content-Type, charset, HTTP status, URLs). It now sanitizes to well-formed modified UTF-8 first (invalid sequences become?), so hostile bytes cannot reachNewStringUTFand trip a CheckJNI abort.From the pre-KVM audit (findings #7, #8, medium). The real abort-vs-clean-String behavior and the profile-import rejection need a device to confirm; the validator and the sanitizer are covered by the
-Werrornative build and the JUnit test.