You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by ch...@apache.org on 2023/06/19 02:19:23 UTC
[bookkeeper] branch master updated: Fix arbitrary file upload vulnerability with httpServerEnabled (#3982)
This is an automated email from the ASF dual-hosted git repository.
chenhang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/bookkeeper.git
The following commit(s) were added to refs/heads/master by this push:
new 1809e69c6d Fix arbitrary file upload vulnerability with httpServerEnabled (#3982)
1809e69c6d is described below
commit 1809e69c6d4eeced49e76b76e979e9da96942231
Author: 萧易客 <km...@live.com>
AuthorDate: Mon Jun 19 10:19:15 2023 +0800
Fix arbitrary file upload vulnerability with httpServerEnabled (#3982)
### Motivation
There is a potential arbitrary file upload vulnerability with `httpServerEnabled=true`, it's caused by `BodyHandler.create()` which returns a BodyHandler that automatically processes file upload requests.
https://github.com/apache/bookkeeper/blob/7f64246ad38981126cc8dd929ff448805a738b8f/bookkeeper-http/vertx-http-server/src/main/java/org/apache/bookkeeper/http/vertx/VertxHttpServer.java#L82
This simple command will upload a file into the `file-uploads` directory under the bookkeeper server process `CWD`.
```shell
$ curl -i --request POST \
--url http://localhost:8000/api/v1/bookie/info \
--header 'Content-Type: multipart/form-data' \
--form file=@<a-path-of-the-file>
$ ls
LICENSE NOTICE README.md bin conf deps file-uploads lib logs scripts
$ ls file-uploads
758801ba-ea1e-49e3-85d6-e510f539ea0d
```
### Changes
Create the `BodyHandler` with handleFileUploads disabled (`BodyHandler.create(false)`).
---
.../bookkeeper/http/vertx/VertxHttpServer.java | 2 +-
.../bookkeeper/http/vertx/TestVertxHttpServer.java | 76 ++++++++++++++++++++++
2 files changed, 77 insertions(+), 1 deletion(-)
diff --git a/bookkeeper-http/vertx-http-server/src/main/java/org/apache/bookkeeper/http/vertx/VertxHttpServer.java b/bookkeeper-http/vertx-http-server/src/main/java/org/apache/bookkeeper/http/vertx/VertxHttpServer.java
index ceaaf7d02d..331ed6566b 100644
--- a/bookkeeper-http/vertx-http-server/src/main/java/org/apache/bookkeeper/http/vertx/VertxHttpServer.java
+++ b/bookkeeper-http/vertx-http-server/src/main/java/org/apache/bookkeeper/http/vertx/VertxHttpServer.java
@@ -79,7 +79,7 @@ public class VertxHttpServer implements HttpServer {
CompletableFuture<AsyncResult<io.vertx.core.http.HttpServer>> future = new CompletableFuture<>();
VertxHttpHandlerFactory handlerFactory = new VertxHttpHandlerFactory(httpServiceProvider);
Router router = Router.router(vertx);
- router.route().handler(BodyHandler.create());
+ router.route().handler(BodyHandler.create(false));
HttpRouter<VertxAbstractHandler> requestRouter = new HttpRouter<VertxAbstractHandler>(handlerFactory) {
@Override
public void bindHandler(String endpoint, VertxAbstractHandler handler) {
diff --git a/bookkeeper-http/vertx-http-server/src/test/java/org/apache/bookkeeper/http/vertx/TestVertxHttpServer.java b/bookkeeper-http/vertx-http-server/src/test/java/org/apache/bookkeeper/http/vertx/TestVertxHttpServer.java
index 24a4bf9185..a0c6c4ae69 100644
--- a/bookkeeper-http/vertx-http-server/src/test/java/org/apache/bookkeeper/http/vertx/TestVertxHttpServer.java
+++ b/bookkeeper-http/vertx-http-server/src/test/java/org/apache/bookkeeper/http/vertx/TestVertxHttpServer.java
@@ -20,13 +20,20 @@
*/
package org.apache.bookkeeper.http.vertx;
+import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
+import com.google.common.io.Files;
+import io.vertx.ext.web.handler.BodyHandler;
import java.io.BufferedReader;
+import java.io.File;
+import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.OutputStream;
+import java.io.OutputStreamWriter;
+import java.io.PrintWriter;
import java.net.HttpURLConnection;
import java.net.URL;
import java.nio.charset.StandardCharsets;
@@ -109,6 +116,22 @@ public class TestVertxHttpServer {
httpServer.stopServer();
}
+ @Test
+ public void testArbitraryFileUpload() throws IOException {
+ VertxHttpServer httpServer = new VertxHttpServer();
+ HttpServiceProvider httpServiceProvider = NullHttpServiceProvider.getInstance();
+ httpServer.initialize(httpServiceProvider);
+ assertTrue(httpServer.startServer(0));
+ int port = httpServer.getListeningPort();
+ File tempFile = File.createTempFile("test-" + System.currentTimeMillis(), null);
+ Files.asCharSink(tempFile, StandardCharsets.UTF_8).write(TestVertxHttpServer.class.getName());
+ String[] filenamesBeforeUploadRequest = listFiles(BodyHandler.DEFAULT_UPLOADS_DIRECTORY);
+ HttpResponse httpResponse = sendFile(getUrl(port, HttpRouter.BOOKIE_INFO), tempFile);
+ assertEquals(HttpServer.StatusCode.OK.getValue(), httpResponse.responseCode);
+ assertArrayEquals(filenamesBeforeUploadRequest, listFiles(BodyHandler.DEFAULT_UPLOADS_DIRECTORY));
+ httpServer.stopServer();
+ }
+
private HttpResponse send(String url, HttpServer.Method method) throws IOException {
return send(url, method, "");
}
@@ -143,6 +166,59 @@ public class TestVertxHttpServer {
return new HttpResponse(responseCode, response.toString());
}
+ private HttpResponse sendFile(String url, File file) throws IOException {
+ URL obj = new URL(url);
+ HttpURLConnection con = (HttpURLConnection) obj.openConnection();
+ String boundary = "---------------------------" + System.currentTimeMillis();
+ // optional, default is GET
+ con.setRequestMethod("POST");
+ con.setDoOutput(true);
+ con.setRequestProperty("Content-Type", "multipart/form-data; boundary=" + boundary);
+ try (
+ OutputStream outputStream = con.getOutputStream();
+ PrintWriter writer = new PrintWriter(new OutputStreamWriter(outputStream, StandardCharsets.UTF_8),
+ true);
+ FileInputStream fileInputStream = new FileInputStream(file);
+ ) {
+ writer.append("--" + boundary).append("\r\n");
+ writer.append("Content-Disposition: form-data; name=\"file\"; filename=\"file.txt\"").append("\r\n");
+ writer.append("Content-Type: text/plain").append("\r\n");
+ writer.append("\r\n");
+
+ byte[] buffer = new byte[4096];
+ int bytesRead;
+ while ((bytesRead = fileInputStream.read(buffer)) != -1) {
+ outputStream.write(buffer, 0, bytesRead);
+ }
+
+ writer.append("\r\n");
+ writer.append("--" + boundary + "--").append("\r\n");
+ }
+ int responseCode = con.getResponseCode();
+ StringBuilder response = new StringBuilder();
+ BufferedReader in = null;
+ try {
+ in = new BufferedReader(new InputStreamReader(con.getInputStream()));
+ String inputLine;
+ while ((inputLine = in.readLine()) != null) {
+ response.append(inputLine);
+ }
+ } finally {
+ if (in != null) {
+ in.close();
+ }
+ }
+ return new HttpResponse(responseCode, response.toString());
+ }
+
+ private String[] listFiles(String directory) {
+ File directoryFile = new File(directory);
+ if (!directoryFile.exists() || !directoryFile.isDirectory()) {
+ return new String[0];
+ }
+ return directoryFile.list();
+ }
+
private String getUrl(int port, String path) {
return "http://localhost:" + port + path;
}