I'll look into it

I'm through with the code review. This is a self-sufficient C coding style
that I mostly like. It won't get much better than this when using C
programming.

There are a couple of areas that seemed brittle:

- the possibility of log spoofing by passing control characters
- the possibility to cause out of bound memory accesses by passing bad byte
  range values

Log spoofing is actively prevent though by escaping anything that is not
alphanumerical.

The byte range worry stems from the fact that a cast from "long long" to
"off_t" is performed in one spot. If sizeof(long long) > sizeof(off_t) then
this could result in a negative off_t value being used for reaching into
memory.

By default when compiling on Linux for 32-bit then off_t has only 4 bytes and
the problem would arise. The code defines `_FILE_OFFSET_BITS 64`, though, thus
off_t becomes 8 bytes wide.

In all other constellations both data types are always 8 bytes these days
though, even on i386. I even checked some BSDs, but none are affected.

All that remains is that there is a weak http basic auth password comparison
that could be used for timing attacks. I will approach upstream privately
about this.

I approached the upstream author about two security problems in the web
server's code base:

- strcmp() is used to check HTTP basic auth. This is subject to timing
  attacks.
- the `--auth` command line switch leaks the HTTP basic auth credentials in
  the process list of the local system.

The upstream author does not want an embargo so we can publish immediately. I
will also request CVEs from Mitre. Until this is all done let's keep this
private for a couple of days longer.

This is the formal report that I shared with upstream.

Basic Auth Timing Attack
========================

The issue is found in darkhttpd.c line 2272 [4]. Here the HTTP basic
authentication string supplied by a client is compared against the
secret configured via the `--auth` command line parameter. For this
comparison a regular `strcmp()` function call is used.

Since `strcmp()` performs an efficient linear comparison it will
terminate earlier if the first bytes of the supplied authentication
string don't match compared to if they do match. This difference in
runtime can be used for timing attacks to try and find out the correct
authentication parameter to access the web server.

To fix this, a constant-time string comparison function needs to be used
that always takes the same amount of computation time for the comparison
independently of how many bytes of the provided data match the actual
authentication secret. An example for such a function is the
`CRYPTO_memcmp()` function provided by the openSSL library [5].

If darkhttpd is used for unencrypted http:// over the Internet then one
could argue that the authentication data will be sent unencrypted over
an untrusted channel anyway. If darkhttpd is used behind a reverse proxy
that uses SSL and thus uses a secure channel, then a major security
property will be violated by this issue though.

Local Leak of Authentication Parameter in Process List
======================================================

The only way to configure the HTTP basic auth string is to pass it via
the `--auth` command line parameter. On Linux all local users can view
the parameters of other programs running on the system. This means if
there are other users or programs running in different security domains
then these can obtain the authentication credentials for the web server.

To fix this an alternative mechanism needs to be provided to pass the
authentication credentials in a safe way. Typically this can be solved
by using an environment variable or a protected configuration file. If
the existing `--auth` command line switch is kept around then the fact
that this leaks the authentication credentials on Linux systems should
be documented.

Review Summary
==============

Apart from these basic authentication related issues I have not found
any problematic spots in the code base of darkhttpd. I focused on the
potential for log file spoofing, escaping the web root via crafted URLs
and memory corruption e.g. through specifying bad byte ranges. The code
is robust in these areas.

References
==========

[1]: https://github.com/emikulic/darkhttpd
[2]: https://en.opensuse.org/openSUSE:Security_disclosure_policy
[3]: https://www.openwall.com/lists/oss-security/
[4]: https://github.com/emikulic/darkhttpd/blob/v1.14/darkhttpd.c#L2272
[5]: https://www.openssl.org/docs/man1.1.1/man3/CRYPTO_memcmp.html

This seems to be fixed upstream with 1.15, to which Dominik has already submitted to Factory.
I'd have personally preferred to see auth supporting an environment variable. However, it does now warn about --auth's security shortcomings, so that may be enough to consider this resolved.

Thanks as usual Matthias :)

Yes, there was quite some back and forth between us and the upstream author.
For some reason he was not too eager to implement this as an environment
variable.

I'm still keeping this bug private for a bit longer until I get CVE references
from Mitre and then I will publish on multiple channels.

I now published the full report on the oss-security mailing list and on our
blog:

https://security.opensuse.org/2024/01/22/darkhttpd-basic-auth-issues.html

Mitre still did not respond to my CVE requests.

Since we package this only in openSUSE:Factory, and the version has already
been bumped to the fixed v1.15 we can close this review bug.

Mitre finally assigned the CVEs for these issues:

> darkhttpd through 1.15 allows local users to discover credentials (for --auth) by listing processes and their arguments:

Use CVE-2024-23770

> 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.

Use CVE-2024-23771