Fix the webserver class to return 500 on invalid MHD Response

LeSpocky · web-flow · commit 5cb0be15745b · 2022-02-03T08:50:50.000-08:00
This particularly applies in case of a file_response (in case the file doesn't exist or is a directory). * webserver: Return 500 on invalid MHD response If `http_response::get_raw_response()` returns nullptr instead of a valid `MHD_Response*` for whatever reason, that pointer would be passed on to `http_response::decorate_response()` and `http_response::enqueue_response()` eventually, leading to different API calls to libmicrohttpd with NULL as argument `struct MHD_Response *response`. MHD does not guarantee any form of behaviour for invalid input, so we have to consider it undefined behaviour and avoid passing invalid input to MHD. HTTP status 500 (Internal Server Error) is returned for consistency with surrounding error handling, we don't know what caused `http_response::get_raw_response()` to return nullptr, so we can not give a better answer here. Fixes: etr#255 * file_response: Add return value checks Both `open()` and `lseek()` might fail depending on how `filename` is set in object of class `httpserver::file_response()`. In the case of a missing file *fd* got -1 and lseek set *size* (which had the wrong type btw.) to 0xffffffff aka (off_t) -1. Passing an invalid file descriptor and a massively huge size value on to `MHD_create_response_from_fd()` might lead to unpredictable results depending how well libmicrohttpd treats such invalid values. Note: Before f9b7691 ("Use MHD_create_response_from_fd for files") `httpserver::http::load_file()` was used, which throws an exception in case a file can not be opened successfully. That exception would have lead to returning HTTP status 500 (Internal Server Error). References: etr#255 * test: Add unit test for missing file response The constructor of class `httpserver::file_response` can be called with a `filename` pointing into the void, to a file which does not actually exist. The webserver should fail predictably in that case. References: etr#255 * file_response: Test on regular file It was possible before to pass a path to a directory, or a to a device file, or to basically any path. `open()` would happily open it. In case of a directory, `lseek()` returns LONG_MAX (0x7FFFFFFFFFFFFFFF) and `MHD_create_response_from_fd()` is invoked. To avoid such nonsense, we test the path now and allow regular files only. References: etr#255 * file_response: Add API doc to constructor This documents a possible pitfall for users when passing filename of not existing files. References: etr#255 * test: Add unit test for file_response pointing to directory The constructor of class `httpserver::file_response` can be called with a `filename` pointing to a directory instead of a regular file. The webserver should fail predictably in that case. References: etr#255 * readme: Document requirements and behaviour of file_response Suggested-by: Sebastiano Merlino <sebastiano@hey.com> References: etr#255
diff --git a/README.md b/README.md
@@ -603,7 +603,7 @@ As seen in the documentation of [http_resource](#the-resource-object), every ext
 
 There are 5 types of response that you can create - we will describe them here through their constructors:
 * _string_response(**const std::string&** content, **int** response_code = `200`, **const std::string&** content_type = `"text/plain"`):_ The most basic type of response. It uses the `content` string passed in construction as body of the HTTP response. The other two optional parameters are the `response_code` and the `content_type`. You can find constant definition for the various response codes within the [http_utils](https://github.com/etr/libhttpserver/blob/master/src/httpserver/http_utils.hpp) library file.
-* _file_response(**const std::string&** filename, **int** response_code = `200`, **const std::string&** content_type = `"text/plain"`):_ Uses the `filename` passed in construction as pointer to a file on disk. The body of the HTTP response will be set using the content of the file. The other two optional parameters are the `response_code` and the `content_type`. You can find constant definition for the various response codes within the [http_utils](https://github.com/etr/libhttpserver/blob/master/src/httpserver/http_utils.hpp) library file.
+* _file_response(**const std::string&** filename, **int** response_code = `200`, **const std::string&** content_type = `"text/plain"`):_ Uses the `filename` passed in construction as pointer to a file on disk. The body of the HTTP response will be set using the content of the file. The file must be a regular file and exist on disk. Otherwise libhttpserver will return an error 500 (Internal Server Error). The other two optional parameters are the `response_code` and the `content_type`. You can find constant definition for the various response codes within the [http_utils](https://github.com/etr/libhttpserver/blob/master/src/httpserver/http_utils.hpp) library file.
 * _basic_auth_fail_response(**const std::string&** content, **const std::string&** realm = `""`, **int** response_code = `200`, **const std::string&** content_type = `"text/plain"`):_ A response in return to a failure during basic authentication. It allows to specify a `content` string as a message to send back to the client. The `realm` parameter should contain your realm of authentication (if any). The other two optional parameters are the `response_code` and the `content_type`. You can find constant definition for the various response codes within the [http_utils](https://github.com/etr/libhttpserver/blob/master/src/httpserver/http_utils.hpp) library file.
 * _digest_auth_fail_response(**const std::string&** content, **const std::string&** realm = `""`, **const std::string&** opaque = `""`, **bool** reload_nonce = `false`, **int** response_code = `200`, **const std::string&** content_type = `"text/plain"`):_ A response in return to a failure during digest authentication. It allows to specify a `content` string as a message to send back to the client. The `realm` parameter should contain your realm of authentication (if any). The `opaque` represents a value that gets passed to the client and expected to be passed again to the server as-is. This value can be a hexadecimal or base64 string. The `reload_nonce` parameter tells the server to reload the nonce (you should use the value returned by the `check_digest_auth` method on the `http_request`. The other two optional parameters are the `response_code` and the `content_type`. You can find constant definition for the various response codes within the [http_utils](https://github.com/etr/libhttpserver/blob/master/src/httpserver/http_utils.hpp) library file.
 * _deferred_response(**ssize_t(&ast;cycle_callback_ptr)(shared_ptr&lt;T&gt;, char&ast;, size_t)** cycle_callback, **const std::string&** content = `""`, **int** response_code = `200`, **const std::string&** content_type = `"text/plain"`):_ A response that obtains additional content from a callback executed in a deferred way. It leaves the client in pending state (returning a `100 CONTINUE` message) and suspends the connection. Besides the callback, optionally, you can provide a `content` parameter that sets the initial message sent immediately to the client. The other two optional parameters are the `response_code` and the `content_type`. You can find constant definition for the various response codes within the [http_utils](https://github.com/etr/libhttpserver/blob/master/src/httpserver/http_utils.hpp) library file. To use `deferred_response` you need to have the `deferred` option active on your webserver (enabled by default).
diff --git a/src/file_response.cpp b/src/file_response.cpp
@@ -22,6 +22,8 @@
 #include <fcntl.h>
 #include <microhttpd.h>
 #include <stddef.h>
+#include <sys/types.h>
+#include <sys/stat.h>
 #include <unistd.h>
 #include <iosfwd>
 
@@ -30,8 +32,21 @@ struct MHD_Response;
 namespace httpserver {
 
 MHD_Response* file_response::get_raw_response() {
+    struct stat sb;
+
+    // Deny everything but regular files
+    if (stat(filename.c_str(), &sb) == 0) {
+        if (!S_ISREG(sb.st_mode)) return nullptr;
+    } else {
+        return nullptr;
+    }
+
     int fd = open(filename.c_str(), O_RDONLY);
-    size_t size = lseek(fd, 0, SEEK_END);
+    if (fd == -1) return nullptr;
+
+    off_t size = lseek(fd, 0, SEEK_END);
+    if (size == (off_t) -1) return nullptr;
+
     if (size) {
         return MHD_create_response_from_fd(size, fd);
     } else {
diff --git a/src/httpserver/file_response.hpp b/src/httpserver/file_response.hpp
@@ -37,6 +37,19 @@ class file_response : public http_response {
  public:
      file_response() = default;
 
+     /**
+      * Constructor of the class file_response.  You usually use this to pass a
+      * filename to the instance.
+      * @param filename Name of the file which content should be sent with the
+      *                 response.  User must make sure file exists and is a
+      *                 regular file, otherwise libhttpserver will return a
+      *                 generic response with HTTP status 500 (Internal Server
+      *                 Error).
+      * @param response_code HTTP response code in good case, optional,
+      *                      default is 200 (OK).
+      * @param content_type Mime type of the file content, e.g. "text/html",
+      *                     optional, default is "application/octet-stream".
+     **/
      explicit file_response(
              const std::string& filename,
              int response_code = http::http_utils::http_ok,
diff --git a/src/webserver.cpp b/src/webserver.cpp
@@ -623,6 +623,10 @@ MHD_Result webserver::finalize_answer(MHD_Connection* connection, struct details
     try {
         try {
             raw_response = mr->dhrs->get_raw_response();
+            if (raw_response == nullptr) {
+                mr->dhrs = internal_error_page(mr);
+                raw_response = mr->dhrs->get_raw_response();
+            }
         } catch(const std::invalid_argument& iae) {
             mr->dhrs = not_found_page(mr);
             raw_response = mr->dhrs->get_raw_response();
diff --git a/test/integ/basic.cpp b/test/integ/basic.cpp
@@ -221,6 +221,20 @@ class file_response_resource_default_content_type : public http_resource {
      }
 };
 
+class file_response_resource_missing : public http_resource {
+ public:
+     const shared_ptr<http_response> render_GET(const http_request&) {
+         return shared_ptr<file_response>(new file_response("missing", 200));
+     }
+};
+
+class file_response_resource_dir : public http_resource {
+ public:
+     const shared_ptr<http_response> render_GET(const http_request&) {
+         return shared_ptr<file_response>(new file_response("integ", 200));
+     }
+};
+
 class exception_resource : public http_resource {
  public:
      const shared_ptr<http_response> render_GET(const http_request&) {
@@ -896,6 +910,52 @@ LT_BEGIN_AUTO_TEST(basic_suite, file_serving_resource_default_content_type)
     curl_easy_cleanup(curl);
 LT_END_AUTO_TEST(file_serving_resource_default_content_type)
 
+LT_BEGIN_AUTO_TEST(basic_suite, file_serving_resource_missing)
+    file_response_resource_missing resource;
+    ws->register_resource("base", &resource);
+    curl_global_init(CURL_GLOBAL_ALL);
+
+    string s;
+    CURL *curl = curl_easy_init();
+    CURLcode res;
+    curl_easy_setopt(curl, CURLOPT_URL, "localhost:8080/base");
+    curl_easy_setopt(curl, CURLOPT_HTTPGET, 1L);
+    curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, writefunc);
+    curl_easy_setopt(curl, CURLOPT_WRITEDATA, &s);
+    res = curl_easy_perform(curl);
+    LT_ASSERT_EQ(res, 0);
+    LT_CHECK_EQ(s, "Internal Error");
+
+    int64_t http_code = 0;
+    curl_easy_getinfo(curl, CURLINFO_RESPONSE_CODE, &http_code);
+    LT_ASSERT_EQ(http_code, 500);
+
+    curl_easy_cleanup(curl);
+LT_END_AUTO_TEST(file_serving_resource_missing)
+
+LT_BEGIN_AUTO_TEST(basic_suite, file_serving_resource_dir)
+    file_response_resource_dir resource;
+    ws->register_resource("base", &resource);
+    curl_global_init(CURL_GLOBAL_ALL);
+
+    string s;
+    CURL *curl = curl_easy_init();
+    CURLcode res;
+    curl_easy_setopt(curl, CURLOPT_URL, "localhost:8080/base");
+    curl_easy_setopt(curl, CURLOPT_HTTPGET, 1L);
+    curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, writefunc);
+    curl_easy_setopt(curl, CURLOPT_WRITEDATA, &s);
+    res = curl_easy_perform(curl);
+    LT_ASSERT_EQ(res, 0);
+    LT_CHECK_EQ(s, "Internal Error");
+
+    int64_t http_code = 0;
+    curl_easy_getinfo(curl, CURLINFO_RESPONSE_CODE, &http_code);
+    LT_ASSERT_EQ(http_code, 500);
+
+    curl_easy_cleanup(curl);
+LT_END_AUTO_TEST(file_serving_resource_dir)
+
 LT_BEGIN_AUTO_TEST(basic_suite, exception_forces_500)
     exception_resource resource;
     ws->register_resource("base", &resource);
