websocket bugfix: socket leak

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>