From b01d45ccd3a402d9ae85bdf9218befee9b73e693 Mon Sep 17 00:00:00 2001 From: Alexander Dahl Date: Sat, 29 Jan 2022 11:14:27 +0100 Subject: [PATCH 1/7] 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: #255 --- src/webserver.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/webserver.cpp b/src/webserver.cpp index a1392bdc..4c4a034a 100644 --- 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(); From ab1755928a47a18eb0c573ec757096c62d0a9e79 Mon Sep 17 00:00:00 2001 From: Alexander Dahl Date: Thu, 27 Jan 2022 11:30:36 +0100 Subject: [PATCH 2/7] 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 f9b769106435 ("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: #255 --- src/file_response.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/file_response.cpp b/src/file_response.cpp index 3bfdcec3..9703af1b 100644 --- a/src/file_response.cpp +++ b/src/file_response.cpp @@ -31,7 +31,11 @@ namespace httpserver { MHD_Response* file_response::get_raw_response() { 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 { From 103aedbc330c84e647c432c3d2d64e963fedbca5 Mon Sep 17 00:00:00 2001 From: Alexander Dahl Date: Tue, 25 Jan 2022 17:28:07 +0100 Subject: [PATCH 3/7] 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: #255 --- test/integ/basic.cpp | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/test/integ/basic.cpp b/test/integ/basic.cpp index 05b771ab..8a719632 100644 --- a/test/integ/basic.cpp +++ b/test/integ/basic.cpp @@ -221,6 +221,13 @@ class file_response_resource_default_content_type : public http_resource { } }; +class file_response_resource_missing : public http_resource { + public: + const shared_ptr render_GET(const http_request&) { + return shared_ptr(new file_response("missing", 200)); + } +}; + class exception_resource : public http_resource { public: const shared_ptr render_GET(const http_request&) { @@ -896,6 +903,29 @@ 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, exception_forces_500) exception_resource resource; ws->register_resource("base", &resource); From 0f43a84511a91873f91c2b35efda91472fa481d6 Mon Sep 17 00:00:00 2001 From: Alexander Dahl Date: Mon, 31 Jan 2022 17:51:13 +0100 Subject: [PATCH 4/7] 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: #255 --- src/file_response.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/file_response.cpp b/src/file_response.cpp index 9703af1b..669f5e8b 100644 --- a/src/file_response.cpp +++ b/src/file_response.cpp @@ -22,6 +22,8 @@ #include #include #include +#include +#include #include #include @@ -30,6 +32,15 @@ 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); if (fd == -1) return nullptr; From 3ff2047bdc98c0a5ca16e248f446278945f42b10 Mon Sep 17 00:00:00 2001 From: Alexander Dahl Date: Sun, 30 Jan 2022 09:35:19 +0100 Subject: [PATCH 5/7] file_response: Add API doc to constructor This documents a possible pitfall for users when passing filename of not existing files. References: #255 --- src/httpserver/file_response.hpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/httpserver/file_response.hpp b/src/httpserver/file_response.hpp index 71e8984f..9e03a0c9 100644 --- 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, From 81bcc4d7bafd2d4c0d0a7a3774cc089ac9df5971 Mon Sep 17 00:00:00 2001 From: Alexander Dahl Date: Tue, 1 Feb 2022 20:40:43 +0100 Subject: [PATCH 6/7] 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: #255 --- test/integ/basic.cpp | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/test/integ/basic.cpp b/test/integ/basic.cpp index 8a719632..3d2aca50 100644 --- a/test/integ/basic.cpp +++ b/test/integ/basic.cpp @@ -228,6 +228,13 @@ class file_response_resource_missing : public http_resource { } }; +class file_response_resource_dir : public http_resource { + public: + const shared_ptr render_GET(const http_request&) { + return shared_ptr(new file_response("integ", 200)); + } +}; + class exception_resource : public http_resource { public: const shared_ptr render_GET(const http_request&) { @@ -926,6 +933,29 @@ LT_BEGIN_AUTO_TEST(basic_suite, file_serving_resource_missing) 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); From 41d9eeb5b3b2b0be6e5b2fdbea67ef30e18a9cbb Mon Sep 17 00:00:00 2001 From: Alexander Dahl Date: Thu, 3 Feb 2022 07:20:41 +0100 Subject: [PATCH 7/7] readme: Document requirements and behaviour of file_response Suggested-by: Sebastiano Merlino References: #255 --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 5acd3c13..d76c84a6 100644 --- 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(*cycle_callback_ptr)(shared_ptr<T>, char*, 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).