From 15d4abf20bd476e7661447c9289b155a847d4fd9 Mon Sep 17 00:00:00 2001 From: Ben Shonaldmann Date: Mon, 17 Mar 2025 11:25:05 -0400 Subject: [PATCH 1/2] Use a random boundary value by default --- .../entity/mime/MultipartEntityBuilder.java | 57 +++++-------------- .../mime/TestMultipartEntityBuilder.java | 41 +++++++++---- .../mime/TestMultipartFormHttpEntity.java | 3 +- 3 files changed, 48 insertions(+), 53 deletions(-) diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java b/httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java index e3bce018c9..3cb4f1774c 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java @@ -47,18 +47,8 @@ /** * Builder for multipart {@link HttpEntity}s. *

- * This class constructs multipart entities with a boundary determined by either a fixed - * value ("httpclient_boundary_7k9p2m4x8n5j3q6t1r0vwyzabcdefghi") or a random UUID. If no - * boundary is explicitly set via {@link #setBoundary(String)}, it defaults to the fixed - * value unless {@link #withRandomBoundary()} is called to request a random UUID at build - * time. Users can provide a custom boundary with {@link #setBoundary(String)}. A warning - * is logged when no explicit boundary is set via {@link #setBoundary(String)}, encouraging - * deliberate choice. - *

- *

- * IMPORTANT: it is responsibility of the caller to validate / sanitize content of body - * parts, for instance, to ensure they do not contain the boundary value that can prevent - * the consumer of the entity from correctly parsing / processing the body parts. + * This class constructs multipart entities with a boundary determined by either a random UUID + * or an explicit boundary set via {@link #setBoundary(String)}. *

* * @since 5.0 @@ -74,7 +64,6 @@ public class MultipartEntityBuilder { private static final String BOUNDARY_PREFIX = "httpclient_boundary_"; - private boolean isRandomBoundaryRequested = false; /** * The logger for this class. */ @@ -125,12 +114,14 @@ public MultipartEntityBuilder setStrictMode() { /** * Sets a custom boundary string for the multipart entity. *

- * If {@code null} is provided, the builder reverts to its default boundary logic: - * either using a boundary from the {@code contentType} if present, or falling back - * to a fixed or random boundary (depending on {@link #withRandomBoundary()}). + * If {@code null} is provided, the builder reverts to its default logic of using a random UUID. + *

+ *

+ * IMPORTANT: when setting an explicit boundary, it is responsibility of the caller to validate / sanitize content + * of body parts to ensure they do not contain the boundary value. *

* - * @param boundary the boundary string, or {@code null} to use the default boundary logic + * @param boundary the boundary string, or {@code null} to use a random UUID. * @return this builder instance */ public MultipartEntityBuilder setBoundary(final String boundary) { @@ -234,14 +225,12 @@ public MultipartEntityBuilder addBinaryBody(final String name, final InputStream } /** - * Returns the fixed default boundary value. - */ - private String getFixedBoundary() { - return BOUNDARY_PREFIX + "7k9p2m4x8n5j3q6t1r0vwyzabcdefghi"; - } - - /** - * Generates a random boundary using UUID. + * Generates a random boundary using UUID. The UUID is a v4 random UUID generated from a cryptographically-secure + * random source. + *

+ * A cryptographically-secure random number source is used to avoid security issues similar to + * CVE-2025-22150 (affecting the Node.JS ecosystem). + *

*/ private String getRandomBoundary() { return BOUNDARY_PREFIX + UUID.randomUUID(); @@ -274,29 +263,13 @@ public MultipartEntityBuilder addEpilogue(final String epilogue) { return this; } - /** - * Configures the builder to request a random boundary generated by UUID.randomUUID() - * at build time if no explicit boundary is set via {@link #setBoundary(String)}. - * - * @return this builder instance - * @since 5.5 - */ - public MultipartEntityBuilder withRandomBoundary() { - this.isRandomBoundaryRequested = true; - this.boundary = null; - return this; - } - MultipartFormEntity buildEntity() { String boundaryCopy = boundary; if (boundaryCopy == null && contentType != null) { boundaryCopy = contentType.getParameter("boundary"); } if (boundaryCopy == null) { - boundaryCopy = isRandomBoundaryRequested ? getRandomBoundary() : getFixedBoundary(); - if (LOG.isWarnEnabled()) { - LOG.warn("No boundary explicitly set; using generated default: {}", boundaryCopy); - } + boundaryCopy = getRandomBoundary(); } Charset charsetCopy = charset; if (charsetCopy == null && contentType != null) { diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartEntityBuilder.java b/httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartEntityBuilder.java index 5cbc878881..79e00e39e3 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartEntityBuilder.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartEntityBuilder.java @@ -313,9 +313,8 @@ void testMultipartWriteToRFC7578ModeWithFilenameStar() throws Exception { @Test void testRandomBoundary() { final MultipartFormEntity entity = MultipartEntityBuilder.create() - .withRandomBoundary() .buildEntity(); - final NameValuePair boundaryParam = extractBoundary(entity.getContentType()); + final NameValuePair boundaryParam = extractBoundary(entity.getContentType(), "multipart/mixed"); final String boundary = boundaryParam.getValue(); Assertions.assertNotNull(boundary); Assertions.assertEquals(56, boundary.length()); @@ -324,21 +323,43 @@ void testRandomBoundary() { } @Test - void testExplicitBoundaryOverridesRandom() { - final String customBoundary = "my_custom_boundary"; + void testRandomBoundaryWriteTo() throws Exception { + final String helloWorld = "hello world"; + final List parameters = new ArrayList<>(); + parameters.add(new BasicNameValuePair(MimeConsts.FIELD_PARAM_NAME, "test")); + parameters.add(new BasicNameValuePair(MimeConsts.FIELD_PARAM_FILENAME, helloWorld)); final MultipartFormEntity entity = MultipartEntityBuilder.create() - .withRandomBoundary() - .setBoundary(customBoundary) + .setStrictMode() + .addPart(new FormBodyPartBuilder() + .setName("test") + .setBody(new StringBody("hello world", ContentType.TEXT_PLAIN)) + .addField("Content-Disposition", "multipart/form-data", parameters) + .build()) .buildEntity(); - final NameValuePair boundaryParam = extractBoundary(entity.getContentType()); - Assertions.assertEquals(customBoundary, boundaryParam.getValue()); + + final NameValuePair boundaryParam = extractBoundary(entity.getContentType(), "multipart/form-data"); + final String boundary = boundaryParam.getValue(); + Assertions.assertNotNull(boundary); + Assertions.assertEquals(56, boundary.length()); + Assertions.assertTrue(boundary.startsWith("httpclient_boundary_")); + Assertions.assertTrue(boundary.substring(20).matches("[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}")); + + final ByteArrayOutputStream out = new ByteArrayOutputStream(); + entity.writeTo(out); + out.close(); + Assertions.assertEquals("--" + boundary + "\r\n" + + "Content-Disposition: multipart/form-data; name=\"test\"; filename=\"hello world\"\r\n" + + "Content-Type: text/plain; charset=UTF-8\r\n" + + "\r\n" + + helloWorld + "\r\n" + + "--" + boundary + "--\r\n", out.toString(StandardCharsets.US_ASCII.name())); } - private NameValuePair extractBoundary(final String contentType) { + private NameValuePair extractBoundary(final String contentType, final String expectedName) { final BasicHeaderValueParser parser = BasicHeaderValueParser.INSTANCE; final ParserCursor cursor = new ParserCursor(0, contentType.length()); final HeaderElement elem = parser.parseHeaderElement(contentType, cursor); - Assertions.assertEquals("multipart/mixed", elem.getName()); + Assertions.assertEquals(expectedName, elem.getName()); return elem.getParameterByName("boundary"); } diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartFormHttpEntity.java b/httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartFormHttpEntity.java index 3e4d79b1fb..1e5d8ecffa 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartFormHttpEntity.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/entity/mime/TestMultipartFormHttpEntity.java @@ -78,7 +78,8 @@ void testImplicitContractorParams() { final String boundary = p1.getValue(); Assertions.assertNotNull(boundary); - Assertions.assertEquals(52, boundary.length()); + // "httpclient_boundary_" + UUID (32 characters + 4 dashes) + 56 characters + Assertions.assertEquals(56, boundary.length()); final NameValuePair p2 = elem.getParameterByName("charset"); Assertions.assertNull(p2); } From c4e7baee7dc1f9b8fc5bd38f61a2682f80badf12 Mon Sep 17 00:00:00 2001 From: Ben Shonaldmann Date: Tue, 18 Mar 2025 10:01:18 -0400 Subject: [PATCH 2/2] Adjust comments --- .../http/entity/mime/MultipartEntityBuilder.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java b/httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java index 3cb4f1774c..de16e98c2e 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/entity/mime/MultipartEntityBuilder.java @@ -50,6 +50,12 @@ * This class constructs multipart entities with a boundary determined by either a random UUID * or an explicit boundary set via {@link #setBoundary(String)}. *

+ *

+ * IMPORTANT: it is responsibility of the caller to validate / sanitize content of body + * parts. For instance, when using an explicit boundary, it's the caller's responsibility to + * ensure the body parts do not contain the boundary value, which can prevent the consumer of + * the entity from correctly parsing / processing the body parts. + *

* * @since 5.0 */ @@ -228,8 +234,8 @@ public MultipartEntityBuilder addBinaryBody(final String name, final InputStream * Generates a random boundary using UUID. The UUID is a v4 random UUID generated from a cryptographically-secure * random source. *

- * A cryptographically-secure random number source is used to avoid security issues similar to - * CVE-2025-22150 (affecting the Node.JS ecosystem). + * A cryptographically-secure random number source is used to generate the UUID, to avoid a malicious actor crafting + * a body part that contains the boundary value to tamper with the entity structure. *

*/ private String getRandomBoundary() {