summaryrefslogtreecommitdiffhomepage
diff options
context:
space:
mode:
authorRoland Reichwein <mail@reichwein.it>2023-01-26 19:14:05 +0100
committerRoland Reichwein <mail@reichwein.it>2023-01-26 19:14:05 +0100
commitfd3da82dd7772419c01bb751e5b5cb7f198b4752 (patch)
treea22034831ec31df68b4e9cb22a1c51949e41b613
parent9b0320c8b47c43adce1aa5d9821b67c1fa42e1d6 (diff)
websocket bugfix: socket leak
-rw-r--r--Makefile3
-rw-r--r--debian/changelog6
-rw-r--r--error.cpp32
-rw-r--r--error.h6
-rw-r--r--http.cpp27
-rw-r--r--tests/Makefile3
-rw-r--r--webserver.conf6
-rw-r--r--websocket.cpp30
-rw-r--r--websocket.h1
9 files changed, 59 insertions, 55 deletions
diff --git a/Makefile b/Makefile
index 49f3b6e..e5e6b11 100644
--- a/Makefile
+++ b/Makefile
@@ -39,7 +39,6 @@ LDFLAGS+=-pie
PROGSRC=\
auth.cpp \
config.cpp \
- error.cpp \
http.cpp \
plugin.cpp \
privileges.cpp \
@@ -150,8 +149,6 @@ DISTFILES= \
debian/webserver.install \
debian/webserver.manpages \
debian/webserver.service \
- error.cpp \
- error.h \
http.cpp \
http.h \
install-webserver.sh \
diff --git a/debian/changelog b/debian/changelog
index fda1b28..85d40eb 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,9 @@
+webserver (1.19) UNRELEASED; urgency=medium
+
+ * Websocket bugfix: close remaining connection, prevent leak
+
+ -- Roland Reichwein <mail@reichwein.it> Thu, 26 Jan 2023 19:11:21 +0100
+
webserver (1.18) unstable; urgency=medium
* Added websockets support (configurable forwarding proxy)
diff --git a/error.cpp b/error.cpp
deleted file mode 100644
index d7a26de..0000000
--- a/error.cpp
+++ /dev/null
@@ -1,32 +0,0 @@
-#include "error.h"
-
-#include <iostream>
-
-#include <boost/asio/ssl/error.hpp>
-
-// Report a failure
-void fail(boost::beast::error_code ec, char const* what)
-{
- // ssl::error::stream_truncated, also known as an SSL "short read",
- // indicates the peer closed the connection without performing the
- // required closing handshake (for example, Google does this to
- // improve performance). Generally this can be a security issue,
- // but if your communication protocol is self-terminated (as
- // it is with both HTTP and WebSocket) then you may simply
- // ignore the lack of close_notify.
- //
- // https://github.com/boostorg/beast/issues/38
- //
- // https://security.stackexchange.com/questions/91435/how-to-handle-a-malicious-ssl-tls-shutdown
- //
- // When a short read would cut off the end of an HTTP message,
- // Beast returns the error beast::http::error::partial_message.
- // Therefore, if we see a short read here, it has occurred
- // after the message has been completed, so it is safe to ignore it.
-
- if (ec == boost::asio::ssl::error::stream_truncated)
- return;
-
- std::cerr << what << ": " << ec.message() << "\n";
-}
-
diff --git a/error.h b/error.h
deleted file mode 100644
index f7a9da3..0000000
--- a/error.h
+++ /dev/null
@@ -1,6 +0,0 @@
-#pragma once
-
-#include <boost/beast/core/error.hpp>
-
-void fail(boost::beast::error_code ec, char const* what);
-
diff --git a/http.cpp b/http.cpp
index 893f1f0..2d78212 100644
--- a/http.cpp
+++ b/http.cpp
@@ -1,7 +1,6 @@
#include "http.h"
#include "config.h"
-#include "error.h"
#include "server.h"
#include "response.h"
#include "websocket.h"
@@ -46,6 +45,32 @@ using namespace Reichwein;
namespace {
+ // Report a failure
+void fail(boost::beast::error_code ec, char const* what)
+{
+ // ssl::error::stream_truncated, also known as an SSL "short read",
+ // indicates the peer closed the connection without performing the
+ // required closing handshake (for example, Google does this to
+ // improve performance). Generally this can be a security issue,
+ // but if your communication protocol is self-terminated (as
+ // it is with both HTTP and WebSocket) then you may simply
+ // ignore the lack of close_notify.
+ //
+ // https://github.com/boostorg/beast/issues/38
+ //
+ // https://security.stackexchange.com/questions/91435/how-to-handle-a-malicious-ssl-tls-shutdown
+ //
+ // When a short read would cut off the end of an HTTP message,
+ // Beast returns the error beast::http::error::partial_message.
+ // Therefore, if we see a short read here, it has occurred
+ // after the message has been completed, so it is safe to ignore it.
+
+ if (ec == boost::asio::ssl::error::stream_truncated)
+ return;
+
+ std::cerr << what << ": " << ec.message() << "\n";
+}
+
// Handles an HTTP server connection
template<class Derived>
class session
diff --git a/tests/Makefile b/tests/Makefile
index 898afbf..c88a285 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -36,7 +36,6 @@ LDFLAGS+=-pie
UNITS=\
auth.cpp \
config.cpp \
- error.cpp \
fastcgiprocess.cpp \
http.cpp \
plugin.cpp \
@@ -91,8 +90,6 @@ auth.o: ../auth.cpp
$(CXX) $(CXXFLAGS) $(CXXTESTFLAGS) -c $< -o $@
config.o: ../config.cpp
$(CXX) $(CXXFLAGS) $(CXXTESTFLAGS) -c $< -o $@
-error.o: ../error.cpp
- $(CXX) $(CXXFLAGS) $(CXXTESTFLAGS) -c $< -o $@
fastcgiprocess.o: ../plugins/fcgi/fastcgiprocess.cpp
$(CXX) $(CXXFLAGS) $(CXXTESTFLAGS) -c $< -o $@
http.o: ../http.cpp
diff --git a/webserver.conf b/webserver.conf
index a00549f..598a57c 100644
--- a/webserver.conf
+++ b/webserver.conf
@@ -99,9 +99,9 @@
<plugin>static-files</plugin>
<target>/home/ernie/code/whiteboard/html</target>
</path>
- <path requested="/whiteboard/whiteboard.fcgi">
- <plugin>fcgi</plugin>
- <target>127.0.0.1:9014</target>
+ <path requested="/whiteboard/websocket">
+ <plugin>websocket</plugin>
+ <target>::1:9014</target>
</path>
<path requested="/redirect1">
diff --git a/websocket.cpp b/websocket.cpp
index 68ff194..d361ec1 100644
--- a/websocket.cpp
+++ b/websocket.cpp
@@ -65,6 +65,20 @@ public:
relative_target_ = websocket_address.substr(slash_pos);
}
+ void fail(boost::beast::error_code ec, char const* what)
+ {
+ if (ec == websocket::error::closed)
+ return;
+
+ boost::beast::error_code ec2;
+ if (auto& ws_in{derived().ws_in()}; ws_in.is_open())
+ ws_in.close(beast::websocket::close_code::going_away, ec2);
+ if (auto& ws_app{ws_app_}; ws_app.is_open())
+ ws_app.close(beast::websocket::close_code::going_away, ec2);
+
+ std::cerr << what << ": " << ec.message() << "\n";
+ }
+
//
// The initial setup path
//
@@ -181,8 +195,10 @@ private:
if (ec == websocket::error::closed)
return;
- if (ec)
- fail(ec, "read in");
+ if (ec) {
+ // not error::closed as above, but no other strategy known
+ return fail(ec, "read in");
+ }
ws_app_.text(derived().ws_in().got_text());
@@ -202,7 +218,7 @@ private:
boost::ignore_unused(bytes_transferred);
if (ec)
- fail(ec, "write app");
+ return fail(ec, "write app");
buffer_in_.consume(buffer_in_.size());
@@ -231,8 +247,10 @@ private:
if (ec == websocket::error::closed)
return;
- if (ec)
- fail(ec, "read app");
+ if (ec) {
+ // not error::closed as above, but no other strategy known
+ return fail(ec, "read app");
+ }
do_write_out();
}
@@ -251,7 +269,7 @@ private:
{
boost::ignore_unused(bytes_transferred);
- if(ec)
+ if (ec)
return fail(ec, "write out");
// Clear the buffer
diff --git a/websocket.h b/websocket.h
index 87b5a04..5306b9b 100644
--- a/websocket.h
+++ b/websocket.h
@@ -3,7 +3,6 @@
//
#pragma once
-#include "error.h"
#include "response.h"
#include <boost/asio/buffer.hpp>