CVE-2018-1000850: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') (CWE-22)
value: Square Retrofit version versions from (including) 2.0 and 2.5.0 (excluding) contains a Directory Traversal vulnerability in RequestBuilder class, method addPathParameter that can result in By manipulating the URL an attacker could add or delete resources otherwise unavailable to her.. This attack appear to be exploitable via An attacker should have access to an encoded path parameter on POST, PUT or DELETE request.. This vulnerability appears to have been fixed in 2.5.0 and later.
CWE description: The product uses external input to construct a pathname that is intended to identify a file or directory that is located underneath a restricted parent directory, but the product does not properly neutralize special elements within the pathname that can cause the pathname to resolve to a location that is outside of the restricted directory.
CVSS v3 base score: 7.5
Published: 2018-12-20T15:29Z
Repository: https://github.com/square/retrofit
Language: Java
Files changed: retrofit/src/main/java/retrofit2/RequestBuilder.java, retrofit/src/test/java/retrofit2/RequestFactoryTest.java
Commit date: 2018-10-22 22:03:02 -0400
Commit message: Forbid path traversal ('.' and '..') as @Path parameters.

They're likely to have the unintended effect. For example, passing ".."
to @DELETE /account/book/{isbn}/ yields @DELETE /account/.

Diff: diff --git a/retrofit/src/main/java/retrofit2/RequestBuilder.java b/retrofit/src/main/java/retrofit2/RequestBuilder.java
index 98dd6669..79fac3bc 100644
--- a/retrofit/src/main/java/retrofit2/RequestBuilder.java
+++ b/retrofit/src/main/java/retrofit2/RequestBuilder.java
@@ -14,10 +14,11 @@
  * limitations under the License.
  */
 package retrofit2;
 
 import java.io.IOException;
+import java.util.regex.Pattern;
 import javax.annotation.Nullable;
 import okhttp3.FormBody;
 import okhttp3.Headers;
 import okhttp3.HttpUrl;
 import okhttp3.MediaType;
@@ -30,10 +31,25 @@ import okio.BufferedSink;
 final class RequestBuilder {
   private static final char[] HEX_DIGITS =
       { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F' };
   private static final String PATH_SEGMENT_ALWAYS_ENCODE_SET = " \"<>^`{}|\\?#";
 
+  /**
+   * Matches strings that contain {@code .} or {@code ..} as a complete path segment. This also
+   * matches dots in their percent-encoded form, {@code %2E}.
+   *
+   * <p>It is okay to have these strings within a larger path segment (like {@code a..z} or {@code
+   * index.html}) but when alone they have a special meaning. A single dot resolves to no path
+   * segment so {@code /one/./three/} becomes {@code /one/three/}. A double-dot pops the preceding
+   * directory, so {@code /one/../three/} becomes {@code /three/}.
+   *
+   * <p>We forbid these in Retrofit paths because they're likely to have the unintended effect.
+   * For example, passing {@code ..} to {@code DELETE /account/book/{isbn}/} yields {@code DELETE
+   * /account/}.
+   */
+  private static final Pattern PATH_TRAVERSAL = Pattern.compile("(.*/)?(\\.|%2e|%2E){1,2}(/.*)?");
+
   private final String method;
 
   private final HttpUrl baseUrl;
   private @Nullable String relativeUrl;
   private @Nullable HttpUrl.Builder urlBuilder;
@@ -89,11 +105,17 @@ final class RequestBuilder {
   void addPathParam(String name, String value, boolean encoded) {
     if (relativeUrl == null) {
       // The relative URL is cleared when the first query parameter is set.
       throw new AssertionError();
     }
-    relativeUrl = relativeUrl.replace("{" + name + "}", canonicalizeForPath(value, encoded));
+    String replacement = canonicalizeForPath(value, encoded);
+    String newRelativeUrl = relativeUrl.replace("{" + name + "}", replacement);
+    if (PATH_TRAVERSAL.matcher(newRelativeUrl).matches()) {
+      throw new IllegalArgumentException(
+          "@Path parameters shouldn't perform path traversal ('.' or '..'): " + value);
+    }
+    relativeUrl = newRelativeUrl;
   }
 
   private static String canonicalizeForPath(String input, boolean alreadyEncoded) {
     int codePoint;
     for (int i = 0, limit = input.length(); i < limit; i += Character.charCount(codePoint)) {
diff --git a/retrofit/src/test/java/retrofit2/RequestFactoryTest.java b/retrofit/src/test/java/retrofit2/RequestFactoryTest.java
index 97d17867..602d5cd8 100644
--- a/retrofit/src/test/java/retrofit2/RequestFactoryTest.java
+++ b/retrofit/src/test/java/retrofit2/RequestFactoryTest.java
@@ -876,10 +876,92 @@ public final class RequestFactoryTest {
     assertThat(request.headers().size()).isZero();
     assertThat(request.url().toString()).isEqualTo("http://example.com/foo/bar/baz/pong/");
     assertThat(request.body()).isNull();
   }
 
+  @Test public void pathParametersAndPathTraversal() {
+    class Example {
+      @GET("/foo/bar/{ping}/") //
+      Call<ResponseBody> method(@Path(value = "ping") String ping) {
+        return null;
+      }
+    }
+
+    assertMalformedRequest(Example.class, ".");
+    assertMalformedRequest(Example.class, "..");
+
+    assertThat(buildRequest(Example.class, "./a").url().encodedPath())
+        .isEqualTo("/foo/bar/.%2Fa/");
+    assertThat(buildRequest(Example.class, "a/.").url().encodedPath())
+        .isEqualTo("/foo/bar/a%2F./");
+    assertThat(buildRequest(Example.class, "a/..").url().encodedPath())
+        .isEqualTo("/foo/bar/a%2F../");
+    assertThat(buildRequest(Example.class, "../a").url().encodedPath())
+        .isEqualTo("/foo/bar/..%2Fa/");
+    assertThat(buildRequest(Example.class, "..\\..").url().encodedPath())
+        .isEqualTo("/foo/bar/..%5C../");
+
+    assertThat(buildRequest(Example.class, "%2E").url().encodedPath())
+        .isEqualTo("/foo/bar/%252E/");
+    assertThat(buildRequest(Example.class, "%2E%2E").url().encodedPath())
+        .isEqualTo("/foo/bar/%252E%252E/");
+  }
+
+  @Test public void encodedPathParametersAndPathTraversal() {
+    class Example {
+      @GET("/foo/bar/{ping}/") //
+      Call<ResponseBody> method(@Path(value = "ping", encoded = true) String ping) {
+        return null;
+      }
+    }
+
+    assertMalformedRequest(Example.class, ".");
+    assertMalformedRequest(Example.class, "%2E");
+    assertMalformedRequest(Example.class, "%2e");
+    assertMalformedRequest(Example.class, "..");
+    assertMalformedRequest(Example.class, "%2E.");
+    assertMalformedRequest(Example.class, "%2e.");
+    assertMalformedRequest(Example.class, ".%2E");
+    assertMalformedRequest(Example.class, ".%2e");
+    assertMalformedRequest(Example.class, "%2E%2e");
+    assertMalformedRequest(Example.class, "%2e%2E");
+    assertMalformedRequest(Example.class, "./a");
+    assertMalformedRequest(Example.class, "a/.");
+    assertMalformedRequest(Example.class, "../a");
+    assertMalformedRequest(Example.class, "a/..");
+    assertMalformedRequest(Example.class, "a/../b");
+    assertMalformedRequest(Example.class, "a/%2e%2E/b");
+
+    assertThat(buildRequest(Example.class, "...").url().encodedPath())
+        .isEqualTo("/foo/bar/.../");
+    assertThat(buildRequest(Example.class, "a..b").url().encodedPath())
+        .isEqualTo("/foo/bar/a..b/");
+    assertThat(buildRequest(Example.class, "a..").url().encodedPath())
+        .isEqualTo("/foo/bar/a../");
+    assertThat(buildRequest(Example.class, "a..b").url().encodedPath())
+        .isEqualTo("/foo/bar/a..b/");
+    assertThat(buildRequest(Example.class, "..b").url().encodedPath())
+        .isEqualTo("/foo/bar/..b/");
+    assertThat(buildRequest(Example.class, "..\\..").url().encodedPath())
+        .isEqualTo("/foo/bar/..%5C../");
+  }
+
+  @Test public void dotDotsOkayWhenNotFullPathSegment() {
+    class Example {
+      @GET("/foo{ping}bar/") //
+      Call<ResponseBody> method(@Path(value = "ping", encoded = true) String ping) {
+        return null;
+      }
+    }
+
+    assertMalformedRequest(Example.class, "/./");
+    assertMalformedRequest(Example.class, "/../");
+
+    assertThat(buildRequest(Example.class, ".").url().encodedPath()).isEqualTo("/foo.bar/");
+    assertThat(buildRequest(Example.class, "..").url().encodedPath()).isEqualTo("/foo..bar/");
+  }
+
   @Test public void pathParamRequired() {
     class Example {
       @GET("/foo/bar/{ping}/") //
       Call<ResponseBody> method(@Path("ping") String ping) {
         return null;
@@ -2781,6 +2863,14 @@ public final class RequestFactoryTest {
         .baseUrl("http://example.com/")
         .addConverterFactory(new ToStringConverterFactory());
 
     return buildRequest(cls, retrofitBuilder, args);
   }
+
+  static void assertMalformedRequest(Class<?> cls, Object... args) {
+    try {
+      Request request = buildRequest(cls, args);
+      fail("expected a malformed request but was " + request);
+    } catch (IllegalArgumentException expected) {
+    }
+  }
 }

Vulnerable code:     relativeUrl = relativeUrl.replace("{" + name + "}", canonicalizeForPath(value, encoded));
Fixed code: import java.util.regex.Pattern;
  /**
   * Matches strings that contain {@code .} or {@code ..} as a complete path segment. This also
   * matches dots in their percent-encoded form, {@code %2E}.
   *
   * <p>It is okay to have these strings within a larger path segment (like {@code a..z} or {@code
   * index.html}) but when alone they have a special meaning. A single dot resolves to no path
   * segment so {@code /one/./three/} becomes {@code /one/three/}. A double-dot pops the preceding
   * directory, so {@code /one/../three/} becomes {@code /three/}.
   *
   * <p>We forbid these in Retrofit paths because they're likely to have the unintended effect.
   * For example, passing {@code ..} to {@code DELETE /account/book/{isbn}/} yields {@code DELETE
   * /account/}.
   */
  private static final Pattern PATH_TRAVERSAL = Pattern.compile("(.*/)?(\\.|%2e|%2E){1,2}(/.*)?");

    String replacement = canonicalizeForPath(value, encoded);
    String newRelativeUrl = relativeUrl.replace("{" + name + "}", replacement);
    if (PATH_TRAVERSAL.matcher(newRelativeUrl).matches()) {
      throw new IllegalArgumentException(
          "@Path parameters shouldn't perform path traversal ('.' or '..'): " + value);
    }
    relativeUrl = newRelativeUrl;
  @Test public void pathParametersAndPathTraversal() {
    class Example {
      @GET("/foo/bar/{ping}/") //
      Call<ResponseBody> method(@Path(value = "ping") String ping) {
        return null;
      }
    }

    assertMalformedRequest(Example.class, ".");
    assertMalformedRequest(Example.class, "..");

    assertThat(buildRequest(Example.class, "./a").url().encodedPath())
        .isEqualTo("/foo/bar/.%2Fa/");
    assertThat(buildRequest(Example.class, "a/.").url().encodedPath())
        .isEqualTo("/foo/bar/a%2F./");
    assertThat(buildRequest(Example.class, "a/..").url().encodedPath())
        .isEqualTo("/foo/bar/a%2F../");
    assertThat(buildRequest(Example.class, "../a").url().encodedPath())
        .isEqualTo("/foo/bar/..%2Fa/");
    assertThat(buildRequest(Example.class, "..\\..").url().encodedPath())
        .isEqualTo("/foo/bar/..%5C../");

    assertThat(buildRequest(Example.class, "%2E").url().encodedPath())
        .isEqualTo("/foo/bar/%252E/");
    assertThat(buildRequest(Example.class, "%2E%2E").url().encodedPath())
        .isEqualTo("/foo/bar/%252E%252E/");
  }

  @Test public void encodedPathParametersAndPathTraversal() {
    class Example {
      @GET("/foo/bar/{ping}/") //
      Call<ResponseBody> method(@Path(value = "ping", encoded = true) String ping) {
        return null;
      }
    }

    assertMalformedRequest(Example.class, ".");
    assertMalformedRequest(Example.class, "%2E");
    assertMalformedRequest(Example.class, "%2e");
    assertMalformedRequest(Example.class, "..");
    assertMalformedRequest(Example.class, "%2E.");
    assertMalformedRequest(Example.class, "%2e.");
    assertMalformedRequest(Example.class, ".%2E");
    assertMalformedRequest(Example.class, ".%2e");
    assertMalformedRequest(Example.class, "%2E%2e");
    assertMalformedRequest(Example.class, "%2e%2E");
    assertMalformedRequest(Example.class, "./a");
    assertMalformedRequest(Example.class, "a/.");
    assertMalformedRequest(Example.class, "../a");
    assertMalformedRequest(Example.class, "a/..");
    assertMalformedRequest(Example.class, "a/../b");
    assertMalformedRequest(Example.class, "a/%2e%2E/b");

    assertThat(buildRequest(Example.class, "...").url().encodedPath())
        .isEqualTo("/foo/bar/.../");
    assertThat(buildRequest(Example.class, "a..b").url().encodedPath())
        .isEqualTo("/foo/bar/a..b/");
    assertThat(buildRequest(Example.class, "a..").url().encodedPath())
        .isEqualTo("/foo/bar/a../");
    assertThat(buildRequest(Example.class, "a..b").url().encodedPath())
        .isEqualTo("/foo/bar/a..b/");
    assertThat(buildRequest(Example.class, "..b").url().encodedPath())
        .isEqualTo("/foo/bar/..b/");
    assertThat(buildRequest(Example.class, "..\\..").url().encodedPath())
        .isEqualTo("/foo/bar/..%5C../");
  }

  @Test public void dotDotsOkayWhenNotFullPathSegment() {
    class Example {
      @GET("/foo{ping}bar/") //
      Call<ResponseBody> method(@Path(value = "ping", encoded = true) String ping) {
        return null;
      }
    }

    assertMalformedRequest(Example.class, "/./");
    assertMalformedRequest(Example.class, "/../");

    assertThat(buildRequest(Example.class, ".").url().encodedPath()).isEqualTo("/foo.bar/");
    assertThat(buildRequest(Example.class, "..").url().encodedPath()).isEqualTo("/foo..bar/");
  }


  static void assertMalformedRequest(Class<?> cls, Object... args) {
    try {
      Request request = buildRequest(cls, args);
      fail("expected a malformed request but was " + request);
    } catch (IllegalArgumentException expected) {
    }
  }