From d923111f50e59196ec4b84d943ccfb652a9d77d8 Mon Sep 17 00:00:00 2001
From: Francois Kritzinger <francois@codesynthesis.com>
Date: Wed, 28 Feb 2024 10:52:08 +0200
Subject: Verify webhook request HMACs

---
 mod/mod-ci-github.cxx | 110 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 104 insertions(+), 6 deletions(-)

(limited to 'mod/mod-ci-github.cxx')

diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index e19a41b..7cdc01d 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -7,6 +7,7 @@
 #include <libbutl/json/parser.hxx>
 
 #include <mod/jwt.hxx>
+#include <mod/hmac.hxx>
 #include <mod/module-options.hxx>
 
 #include <stdexcept>
@@ -64,10 +65,12 @@ namespace brep
 
     HANDLER_DIAG;
 
-    // @@ TODO: disable service if HMAC is not specified in config.
-    //
-    if (false)
+    if (!options_->ci_github_app_webhook_secret_specified ())
+    {
+      error << "webhook secret not configured";
+
       throw invalid_request (404, "CI request submission disabled");
+    }
 
     // Process headers.
     //
@@ -80,15 +83,26 @@ namespace brep
     //        we already do for the most part), and "webhook event" only when
     //        more context would be useful?
     //
-    string event; // Webhook event.
+    string event;  // Webhook event.
+    string r_hmac; // Received HMAC.
     {
       bool content_type (false);
 
       for (const name_value& h: rq.headers ())
       {
+        // HMAC authenticating this request. Note that it won't be present
+        // unless a webhook secret has been set in the GitHub app's settings.
+        //
+        if (icasecmp (h.name, "x-hub-signature-256") == 0)
+        {
+          if (!h.value)
+            throw invalid_request (400, "missing x-hub-signature-256 value");
+
+          r_hmac = *h.value;
+        }
         // This event's UUID.
         //
-        if (icasecmp (h.name, "x-github-delivery") == 0)
+        else if (icasecmp (h.name, "x-github-delivery") == 0)
         {
           // @@ TODO Check that delivery UUID has not been received before
           //         (replay attack).
@@ -118,6 +132,21 @@ namespace brep
         }
       }
 
+      // Parse the x-hub-signature-256 header value. For example:
+      //
+      // sha256=5e822587094c68e646db8b916da1db2056d92f1dea4252136a533b4147a30cb7
+      //
+      // Check for the presence of the "sha256=" prefix and then strip it to
+      // leave only the HMAC.
+      //
+      if (r_hmac.find ("sha256=", 0, 7) == string::npos)
+      {
+        throw invalid_request (400,
+                               "invalid x-hub-signature-256 value: '" +
+                                   r_hmac + '\'');
+      }
+      r_hmac = r_hmac.substr (7);
+
       if (!content_type)
         throw invalid_request (400, "missing content-type header");
 
@@ -125,6 +154,75 @@ namespace brep
         throw invalid_request (400, "missing x-github-event header");
     }
 
+    // Read the entire request body into a buffer because we need to compute
+    // an HMAC over it and then parse it as JSON. The alternative of reading
+    // from the stream twice works out to be more complicated and expensive.
+    //
+    // @@ TMP Although it is possible (with a bit of work) to rewind the
+    //        request's input stream, in that case we'd have to read from it
+    //        twice (once for the HMAC and once for the JSON parsing) instead
+    //        of just this once, temp buffer(s) would be necessary, and we'd
+    //        still be storing the entire request body in memory anyway (just
+    //        not here).
+    //
+    vector<char> body;
+    {
+      // Note: buffer=0 disables caching.
+      //
+      istream& is (rq.content (64 * 1024, 0 /* buffer */));
+
+      // Note that istream::read() sets failbit if unable to read the
+      // requested number of bytes.
+      //
+      is.exceptions (istream::badbit);
+
+      try
+      {
+        size_t n (0); // Total bytes read.
+
+        while (!eof (is))
+        {
+          body.resize (n + 8192);
+          is.read (body.data () + n, 8192);
+          n += is.gcount ();
+        }
+
+        body.resize (n);
+      }
+      catch (const io_error& e)
+      {
+        fail << "unable to read request body: " << e;
+      }
+    }
+
+    // Verify the received HMAC.
+    //
+    // Compute the HMAC over the request body using the configured webhook
+    // secret as key and compare it to the received HMAC.
+    //
+    try
+    {
+      string hmac (
+          compute_hmac (*options_,
+                        body,
+                        options_->ci_github_app_webhook_secret ().c_str ()));
+
+      // @@ TODO GitHub recommends constant-time comparison (timing attack).
+      //
+      if (!icasecmp (hmac, r_hmac))
+      {
+        string m ("computed HMAC does not match received HMAC");
+
+        error << m;
+
+        throw invalid_request (400, move (m));
+      }
+    }
+    catch (const system_error& e)
+    {
+      fail << "unable to compute request HMAC: " << e;
+    }
+
     // There is a webhook event (specified in the x-github-event header) and
     // each event contains a bunch of actions (specified in the JSON request
     // body).
@@ -140,7 +238,7 @@ namespace brep
       check_suite_event cs;
       try
       {
-        json::parser p (rq.content (64 * 1024), "check_suite event");
+        json::parser p (body.data (), body.size (), "check_suite event");
 
         cs = check_suite_event (p);
       }
-- 
cgit v1.1