CVE-2024-23771: Observable Discrepancy (CWE-203)
value: darkhttpd before 1.15 uses strcmp (which is not constant time) to verify authentication, which makes it easier for remote attackers to bypass authentication via a timing side channel.
CWE description: The product behaves differently or sends different responses under different circumstances in a way that is observable to an unauthorized actor, which exposes security-relevant information about the state of the product, such as whether a particular operation was successful or not.
CVSS v3 base score: 9.8
Published: 2024-01-22T04:15Z
Repository: https://github.com/emikulic/darkhttpd
Language: Other
Files changed: darkhttpd.c, devel/Makefile, devel/run-tests, devel/test_password_equal.c
Commit date: 2024-01-18 10:22:41 +1100
Commit message: Don't use strcmp to compare passwords.

Add password_equal() which tries to avoid leaking the length of the secret
through timing side-channels.

Suggested-by: SUSE Security Team <security@suse.de>

Diff: diff --git a/darkhttpd.c b/darkhttpd.c
index 6f82400..7180fe1 100644
--- a/darkhttpd.c
+++ b/darkhttpd.c
@@ -302,11 +302,11 @@ static char *logfile_name = NULL;   /* NULL = no logging */
 static FILE *logfile = NULL;
 static char *pidfile_name = NULL;   /* NULL = no pidfile */
 static int want_chroot = 0, want_daemon = 0, want_accf = 0,
            want_keepalive = 1, want_server_id = 1;
 static char *server_hdr = NULL;
-static char *auth_key = NULL;
+static char *auth_key = NULL;       /* NULL or "Basic base64_of_password" */
 static char *custom_hdrs = NULL;
 static uint64_t num_requests = 0, total_in = 0, total_out = 0;
 static int accepting = 1;           /* set to 0 to stop accept()ing */
 static int syslog_enabled = 0;
 volatile int running = 0; /* signal handler sets this to false */
@@ -2289,10 +2289,37 @@ static void process_get(struct connection *conn) {
         );
         conn->http_code = 200;
     }
 }
 
+/* Returns 1 if passwords are equal, runtime is proportional to the length of
+ * user_input to avoid leaking the secret's length and contents through timing
+ * information.
+ */
+int password_equal(const char *user_input, const char *secret) {
+    size_t i = 0;
+    size_t j = 0;
+    char out = 0;
+
+    while (1) {
+        /* Out stays zero if the strings are the same. */
+        out |= user_input[i] ^ secret[j];
+
+        /* Stop at end of user_input. */
+        if (user_input[i] == 0) break;
+        i++;
+
+        /* Don't go past end of secret. */
+        if (secret[j] != 0) j++;
+    }
+
+    /* Check length after loop, otherwise early exit would leak length. */
+    out |= (i != j); /* Secret was shorter. */
+    out |= (secret[j] != 0); /* Secret was longer; j is not the end. */
+    return out == 0;
+}
+
 /* Process a request: build the header and reply, advance state. */
 static void process_request(struct connection *conn) {
     num_requests++;
 
     if (!parse_request(conn)) {
@@ -2303,12 +2330,11 @@ static void process_request(struct connection *conn) {
         redirect_https(conn);
     }
     /* fail if: (auth_enabled) AND (client supplied invalid credentials) */
     else if (auth_key != NULL &&
             (conn->authorization == NULL ||
-             strcmp(conn->authorization, auth_key)))
-    {
+             !password_equal(conn->authorization, auth_key))) {
         default_reply(conn, 401, "Unauthorized",
             "Access denied due to invalid credentials.");
     }
     else if (strcmp(conn->method, "GET") == 0) {
         process_get(conn);
diff --git a/devel/Makefile b/devel/Makefile
index 3a817cf..ae1e45c 100644
--- a/devel/Makefile
+++ b/devel/Makefile
@@ -6,8 +6,9 @@ clean:
 		test.out.log \
 		test.out.stderr \
 		test.out.stdout \
 		test.pyc \
 		test_make_safe_uri \
+		test_password_equal \
 		a.out darkhttpd.gcda darkhttpd.gcno \
 		fuzz_darkhttpd.o fuzz_llvm_make_safe_uri fuzz_socket
 	rm -rf tmp.fuzz tmp.httpd.tests
diff --git a/devel/run-tests b/devel/run-tests
index ce1810a..41331eb 100755
--- a/devel/run-tests
+++ b/devel/run-tests
@@ -151,19 +151,27 @@ runtests() {
   fi
 }
 
 # --- main ---
 
-# Unit test.
+# Unit tests.
 echo "===> test_make_safe_uri"
 $CC -g -O2 -fsanitize=address -fsanitize=undefined \
   test_make_safe_uri.c -o test_make_safe_uri || exit 1
 if ./test_make_safe_uri | egrep '^FAIL:'; then
   echo test_make_safe_uri failed >&2
   exit 1
 fi
 
+echo "===> test_password_equal"
+$CC -g -O2 -fsanitize=address -fsanitize=undefined \
+  test_password_equal.c -o test_password_equal || exit 1
+if ./test_password_equal | egrep '^FAIL:'; then
+  echo test_password_equal failed >&2
+  exit 1
+fi
+
 # Check that the code builds with various defines.
 echo "===> building without -DDEBUG"
 $CC -O2 -Wall ../darkhttpd.c || exit 1
 echo "===> building with -DNO_IPV6"
 $CC -O2 -Wall -DNO_IPV6 ../darkhttpd.c || exit 1
diff --git a/devel/test_password_equal.c b/devel/test_password_equal.c
new file mode 100644
index 0000000..b8c4c3e
--- /dev/null
+++ b/devel/test_password_equal.c
@@ -0,0 +1,30 @@
+#define main _main_disabled_
+#include "../darkhttpd.c"
+#undef main
+
+static void
+test(int equal, const char *user_input, const char *secret)
+{
+    int out = password_equal(user_input, secret);
+    printf("%s: \"%s\" \"%s\"\n",
+        (equal == out) ? "PASS" : "FAIL",
+        user_input, secret);
+}
+
+int
+main(void)
+{
+    test(1, "", "");
+    test(1, "a", "a");
+    test(1, "abc", "abc");
+
+    test(0, "a", "");
+    test(0, "ab", "");
+    test(0, "", "a");
+    test(0, "", "ab");
+    test(0, "abcd", "abc");
+    test(0, "abc", "abcd");
+    return 0;
+}
+
+/* vim:set tabstop=4 shiftwidth=4 expandtab tw=78: */

Vulnerable code: static char *auth_key = NULL;
             strcmp(conn->authorization, auth_key)))
    {
# Unit test.
Fixed code: static char *auth_key = NULL;       /* NULL or "Basic base64_of_password" */
/* Returns 1 if passwords are equal, runtime is proportional to the length of
 * user_input to avoid leaking the secret's length and contents through timing
 * information.
 */
int password_equal(const char *user_input, const char *secret) {
    size_t i = 0;
    size_t j = 0;
    char out = 0;

    while (1) {
        /* Out stays zero if the strings are the same. */
        out |= user_input[i] ^ secret[j];

        /* Stop at end of user_input. */
        if (user_input[i] == 0) break;
        i++;

        /* Don't go past end of secret. */
        if (secret[j] != 0) j++;
    }

    /* Check length after loop, otherwise early exit would leak length. */
    out |= (i != j); /* Secret was shorter. */
    out |= (secret[j] != 0); /* Secret was longer; j is not the end. */
    return out == 0;
}

             !password_equal(conn->authorization, auth_key))) {
		test_password_equal \
# Unit tests.
echo "===> test_password_equal"
$CC -g -O2 -fsanitize=address -fsanitize=undefined \
  test_password_equal.c -o test_password_equal || exit 1
if ./test_password_equal | egrep '^FAIL:'; then
  echo test_password_equal failed >&2
  exit 1
fi

#define main _main_disabled_
#include "../darkhttpd.c"
#undef main

static void
test(int equal, const char *user_input, const char *secret)
{
    int out = password_equal(user_input, secret);
    printf("%s: \"%s\" \"%s\"\n",
        (equal == out) ? "PASS" : "FAIL",
        user_input, secret);
}

int
main(void)
{
    test(1, "", "");
    test(1, "a", "a");
    test(1, "abc", "abc");

    test(0, "a", "");
    test(0, "ab", "");
    test(0, "", "a");
    test(0, "", "ab");
    test(0, "abcd", "abc");
    test(0, "abc", "abcd");
    return 0;
}

/* vim:set tabstop=4 shiftwidth=4 expandtab tw=78: */