From 0901db2a5fa0e110f11f1c0855113afc1556a194 Mon Sep 17 00:00:00 2001 From: Xavier Roche Date: Sat, 18 Jul 2026 19:03:31 +0200 Subject: [PATCH 1/2] native+app: harden two hostile-input paths reaching the crawl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Signed-off-by: Xavier Roche --- .../com/httrack/android/HTTrackActivity.java | 10 ++++ .../com/httrack/android/StoragePaths.java | 22 ++++++++ app/src/main/jni/htslibjni-private.h | 51 ++++++++++++++++--- app/src/main/res/values/strings.xml | 1 + .../com/httrack/android/ProjectNameTest.java | 38 ++++++++++++++ 5 files changed, 115 insertions(+), 7 deletions(-) create mode 100644 app/src/test/java/com/httrack/android/ProjectNameTest.java diff --git a/app/src/main/java/com/httrack/android/HTTrackActivity.java b/app/src/main/java/com/httrack/android/HTTrackActivity.java index 3e138ec..f108425 100755 --- a/app/src/main/java/com/httrack/android/HTTrackActivity.java +++ b/app/src/main/java/com/httrack/android/HTTrackActivity.java @@ -801,6 +801,11 @@ protected String[] getProjectNames() { protected File getTargetFile() { final String name = mapper.getProjectName(); if (name != null && name.length() != 0) { + // Backstop: a name that is not a single in-root segment must never become an -O path. + if (!StoragePaths.isValidProjectName(name)) { + Log.w(getClass().getSimpleName(), "rejecting unsafe project name: " + name); + return null; + } return new File(getProjectRootFile(), name); } else { return null; @@ -2140,6 +2145,11 @@ protected boolean validatePane() { dirtyNamePane = false; } } + // A crafted winprofile.ini can overwrite the name with a path that escapes the root. + if (!StoragePaths.isValidProjectName(mapper.getProjectName())) { + showNotification(getString(R.string.invalid_project_name), true); + return false; + } return true; } return false; diff --git a/app/src/main/java/com/httrack/android/StoragePaths.java b/app/src/main/java/com/httrack/android/StoragePaths.java index fc24f5a..3d61b70 100644 --- a/app/src/main/java/com/httrack/android/StoragePaths.java +++ b/app/src/main/java/com/httrack/android/StoragePaths.java @@ -40,4 +40,26 @@ static Boolean isWritable(final File path, final File external, final File inter } return external != null ? Boolean.FALSE : null; } + + /** + * Whether {@code name} is safe to append to a root as one directory. File does not fold "..", so + * a crafted winprofile.ini name like "../../sdcard/evil" would otherwise steer the engine's -O + * outside the Websites tree. Rejects empty, ".", ".." and anything carrying a path separator. + * + * @param name + * the project name, already whitespace-collapsed + * @return true if it denotes a single, in-root directory segment + */ + static boolean isValidProjectName(final String name) { + if (name == null || name.isEmpty() || name.equals(".") || name.equals("..")) { + return false; + } + for (int i = 0; i < name.length(); i++) { + final char c = name.charAt(i); + if (c == '/' || c == '\\' || c == '\0') { + return false; + } + } + return true; + } } diff --git a/app/src/main/jni/htslibjni-private.h b/app/src/main/jni/htslibjni-private.h index 6edfa55..645777d 100644 --- a/app/src/main/jni/htslibjni-private.h +++ b/app/src/main/jni/htslibjni-private.h @@ -137,15 +137,52 @@ typedef struct hts_state_t { const char *message; } hts_state_t; -/* NewStringUTF, but ignore invalid UTF-8 or NULL input. */ +/* Sanitize server-controlled bytes to well-formed modified UTF-8 (invalid sequences, including + 4-byte forms, become '?'), so hostile input cannot reach NewStringUTF, which aborts under + CheckJNI. Caller frees the result. */ +static char *sanitizeModifiedUtf8(const char *s) { + const size_t len = strlen(s); + char *const out = malloc(len + 1); + if (out != NULL) { + const unsigned char *const in = (const unsigned char *) s; + size_t i = 0, j = 0; + while (i < len) { + const unsigned char c = in[i]; + unsigned int seq = 0; + if (c <= 0x7F) { + seq = 1; + } else if (c >= 0xC2 && c <= 0xDF) { + if (i + 1 < len && (in[i + 1] & 0xC0) == 0x80) { + seq = 2; + } + } else if (c >= 0xE0 && c <= 0xEF) { + /* Reject overlong E0 80..9F; surrogate halves (ED A0..BF) stay, they are valid here. */ + const unsigned char lo = (c == 0xE0) ? 0xA0 : 0x80; + if (i + 2 < len && in[i + 1] >= lo && (in[i + 1] & 0xC0) == 0x80 + && (in[i + 2] & 0xC0) == 0x80) { + seq = 3; + } + } + if (seq != 0) { + for (unsigned int k = 0; k < seq; k++) { + out[j++] = (char) in[i++]; + } + } else { + out[j++] = '?'; + i++; + } + } + out[j] = '\0'; + } + return out; +} + +/* NewStringUTF over sanitized bytes, so hostile input cannot reach it; NULL input yields NULL. */ static jobject newStringSafe(JNIEnv *env, const char *s) { if (s != NULL) { - const int ne = ! (*env)->ExceptionOccurred(env); - jobject str = (*env)->NewStringUTF(env, s); - /* Silently ignore UTF-8 exception. */ - if (str == NULL && (*env)->ExceptionOccurred(env) && ne) { - (*env)->ExceptionClear(env); - } + char *const safe = sanitizeModifiedUtf8(s); + jobject str = (*env)->NewStringUTF(env, safe != NULL ? safe : s); + free(safe); return str; } return NULL; diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 06eb7ef..9b488e0 100755 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -184,6 +184,7 @@ Could not write to: Read-only media (SDCARD) No storage media (SDCARD) + Invalid project name: it must not contain \'/\', \'\\\' or \'..\' HTTrack may not be able to download websites until this problem is fixed HTTrack: mirror \'%s\' stopped! Click on this notification to restart the interrupted mirror diff --git a/app/src/test/java/com/httrack/android/ProjectNameTest.java b/app/src/test/java/com/httrack/android/ProjectNameTest.java new file mode 100644 index 0000000..7e3c61e --- /dev/null +++ b/app/src/test/java/com/httrack/android/ProjectNameTest.java @@ -0,0 +1,38 @@ +package com.httrack.android; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import org.junit.Test; + +/** Attacks StoragePaths.isValidProjectName: a name is one in-root directory segment, nothing more. */ +public class ProjectNameTest { + @Test + public void acceptsAPlainName() { + assertTrue(StoragePaths.isValidProjectName("My Website")); + assertTrue(StoragePaths.isValidProjectName("..dotdot..name")); + assertTrue(StoragePaths.isValidProjectName("a.b.c")); + } + + @Test + public void refusesEmptyOrDotSegments() { + assertFalse(StoragePaths.isValidProjectName(null)); + assertFalse(StoragePaths.isValidProjectName("")); + assertFalse(StoragePaths.isValidProjectName(".")); + assertFalse(StoragePaths.isValidProjectName("..")); + } + + /** The discriminating case: no separator, yet still a traversal a naive '/'-only check would miss. */ + @Test + public void refusesABareParentSegment() { + assertFalse(StoragePaths.isValidProjectName("..")); + } + + @Test + public void refusesAnyPathSeparator() { + assertFalse(StoragePaths.isValidProjectName("../../../../sdcard/Android/data/evil")); + assertFalse(StoragePaths.isValidProjectName("a/b")); + assertFalse(StoragePaths.isValidProjectName("a\\b")); + assertFalse(StoragePaths.isValidProjectName("evil\0hidden")); + } +} From 67dae7a815add6e8bc04f5d7e9e56ba8927676e2 Mon Sep 17 00:00:00 2001 From: Xavier Roche Date: Sat, 18 Jul 2026 23:39:33 +0200 Subject: [PATCH 2/2] native: return null from newStringSafe on OOM, not raw bytes 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) Signed-off-by: Xavier Roche --- app/src/main/jni/htslibjni-private.h | 9 ++++++--- .../test/java/com/httrack/android/ProjectNameTest.java | 6 ------ 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/app/src/main/jni/htslibjni-private.h b/app/src/main/jni/htslibjni-private.h index 645777d..6f67c38 100644 --- a/app/src/main/jni/htslibjni-private.h +++ b/app/src/main/jni/htslibjni-private.h @@ -181,9 +181,12 @@ static char *sanitizeModifiedUtf8(const char *s) { static jobject newStringSafe(JNIEnv *env, const char *s) { if (s != NULL) { char *const safe = sanitizeModifiedUtf8(s); - jobject str = (*env)->NewStringUTF(env, safe != NULL ? safe : s); - free(safe); - return str; + /* On OOM, a null field beats feeding raw (possibly hostile) bytes to NewStringUTF. */ + if (safe != NULL) { + jobject str = (*env)->NewStringUTF(env, safe); + free(safe); + return str; + } } return NULL; } diff --git a/app/src/test/java/com/httrack/android/ProjectNameTest.java b/app/src/test/java/com/httrack/android/ProjectNameTest.java index 7e3c61e..890ab8b 100644 --- a/app/src/test/java/com/httrack/android/ProjectNameTest.java +++ b/app/src/test/java/com/httrack/android/ProjectNameTest.java @@ -22,12 +22,6 @@ public void refusesEmptyOrDotSegments() { assertFalse(StoragePaths.isValidProjectName("..")); } - /** The discriminating case: no separator, yet still a traversal a naive '/'-only check would miss. */ - @Test - public void refusesABareParentSegment() { - assertFalse(StoragePaths.isValidProjectName("..")); - } - @Test public void refusesAnyPathSeparator() { assertFalse(StoragePaths.isValidProjectName("../../../../sdcard/Android/data/evil"));