Project

General

Profile

Actions

Feature #3122

closed

Error messages should include client IP

Added by flynn 5 months ago. Updated 5 months ago.

Status:
Fixed
Priority:
Normal
Category:
core
Target version:
ASK QUESTIONS IN Forums:
No

Description

A lot of error messages do not include the client IP, e.g.

(request.c.278) GET/HEAD with content-length -> 400
(request.c.278) Invalid Hostname -> 400
(mod_openssl.c.3247) SSL: 1 error:1420918C:SSL routines:tls_early_post_process_client_hello:version too low
(mod_fastcgi.c.451) FastCGI-stderr:Primary script unknown
(connections.c.718) invalid request-line -> sending Status 400

Usually these requests are not logged in the access log and it is not possible to decide, whether to investigate this further.

Is this intended or can the client IP added to these log messages?

Actions #1

Updated by gstrauss 5 months ago

  • Category set to core
  • Target version changed from 1.4.64 to 1.4.xx

Is this intended ...

Historically, there has not been a formulated policy or enforced consistency to this level in the log messages.

Additionally, some error messages have the potential to produce lots and lots of noise in the error logs, and are enabled by config options such as debug.log-request-header-on-error = "enable" and debug.log-ssl-noise = "enable"

(mod_fastcgi.c.451) FastCGI-stderr:Primary script unknown is produced by your backend FastCGI script, and lighttpd forwards the FastCGI stderr to the lighttpd error log, prepending "FastCGI-stderr:"

Not every error message is directly caused by client activity, although you correctly point out that there are many error messages in lighttpd that are caused by client activity, yet many of those are not logged with client IP.

... or can the client IP added to these log messages?

The current logging interfaces require log_error_st object be passed. It would be a fairly invasive change to modify the logging interfaces to take an additional parameter (struct request_st or struct connection), and then to modify all code to pass those additional parameters. Specific classes of errors might be easier to modify to add IP, such as all errors enabled with debug.log-request-header-on-error = "enable"

For error investigation, is remote IP address sufficient, or is remote port also necessary? What about HTTP/2 stream-id? What about client certificate (if configured and provided)? What about authenticated username (if authenticated)? What about cookie identifier in request header (if present)? There are a multitude of different ways that this could be done, and there is no one-size-fits-all. (For ultimate customization, I could envision a lua handler which could be called to run custom lua code for the logging, but adding such a feature would also have some of the invasive prerequisites mentioned above.)


Are there specific, high priority error messages that you can identify which would benefit from logging the remote IP? Maybe those can be addressed more surgically and without a global change to error logging.

How important is it to be able to follow-up on these error messages, for which the remote IP is necessary? (e.g. Is this a must-have feature that would otherwise disqualify lighttpd from being used?) Please keep in mind that lighttpd does not directly compete with enterprise-level IDS systems. I can imagine that identifying remote IP of rogue clients on an internal network might be very useful, but possibly less so in a DDoS attack from the open internet.

Actions #2

Updated by gstrauss 5 months ago

I should note that obtaining remote IP for these error messages is already reasonably available by enabling high-precision timestamps in error log and access log, and cross-referencing by the high-precision timestamp (+ small delta).
You can log remote IP and potentially other info (e.g. a specific Cookie) in the accesslog.

server.errorlog-high-precision = "enable"

accesslog.format using %D or %{...}t with high-precision specifiers such as one of %{msec}t %{usec}t %{nsec}t %{msec_frac}t %{usec_frac}t %{nsec_frac}t, and prefix %{end:...}t for the ending time of the request instead of the begin time (as the ending time may be closer to the error in the error log).

Actions #3

Updated by flynn 5 months ago

My feature request concerns the "normal" error messages, not the ones that must be activated explicit (e.g. debug.log-request-header-on-error or debug.log-ssl-noise). I was several times in the situation, that somebody complains about a problem and I see nothing in the logs except an error message, which I cannot assign to this problem without e.g. a client IP.

The remote/client IP is the most important addition, because with this I can identify a request, that needs investigation. The port is not important to me. When the point is reached, that users are authenticated or client certifcates are decoded, then usually all is ok. Especially I'm interested int the very early failures in TLS-connections and the ones that generate a 400 response.

It is NOT a must-have, it is just a wish, and I understand now, that it is not so easy and lightweight as I thought.

The most important error message with remote/client IP for me are:
  • (request.c.278)
  • (mod_openssl.c.3247)
  • (connections.c.718)

in this order.

Actions #4

Updated by flynn 5 months ago

Correction: I use debug.log-request-header-on-error = "enable", where some of the error messages above may come from.

Actions #5

Updated by flynn 5 months ago

I only want the remote/client IPs for messages, which have no corresponding entry in one of the access logs. This true for (request.c.278) and can be deleted from my feature request and it remains:
  • (mod_openssl.c.3247)
  • (connections.c.718)
Actions #6

Updated by gstrauss 5 months ago

I only want the remote/client IPs for messages, which have no corresponding entry in one of the access logs.

That makes sense.

For established connections, con->dst_addr_buf.ptr is a readily-available formatted string of the remote IP, which might be changed by mod_extforward.

How does the format below look to you, or would you prefer the IP at the beginning? Should I prefix the IP with "(ip:%s)"? (It could also be a unix domain socket, not an IP.) The following patch adds the IP as a suffix since I tend to try to keep existing error strings the way they are since others, such as commercial log aggregators and processors, may have strict string matching to classify and translate errors.

--- a/src/connections.c
+++ b/src/connections.c
@@ -715,10 +715,11 @@ static int connection_handle_read_state(connection * const con)  {
             /* expecting ASCII method beginning with alpha char
              * or HTTP/2 pseudo-header beginning with ':' */
             /*(TLS handshake begins with SYN 0x16 (decimal 22))*/
-            log_error(r->conf.errh, __FILE__, __LINE__, "%s",
+            log_error(r->conf.errh, __FILE__, __LINE__, "%s (%s)",
                       c->mem->ptr[c->offset] == 0x16
                       ? "unexpected TLS ClientHello on clear port" 
-                      : "invalid request-line -> sending Status 400");
+                      : "invalid request-line -> sending Status 400",
+                      con->dst_addr_buf.ptr);
             r->http_status = 400; /* Bad Request */
             r->keep_alive = 0;
             connection_set_state(r, CON_STATE_REQUEST_END);
--- a/src/mod_openssl.c
+++ b/src/mod_openssl.c
@@ -3245,7 +3245,8 @@ connection_read_cq_ssl (connection * const con, chunkqueue * const cq, off_t max
                 }
                 /* get all errors from the error-queue */
                 log_error(hctx->errh, __FILE__, __LINE__,
-                  "SSL: %d %s", rc, ERR_error_string(ssl_err, NULL));
+                  "SSL: %d %s (%s)", rc, ERR_error_string(ssl_err, NULL),
+                  con->dst_addr_buf.ptr);
             }
             break;
         }
Actions #7

Updated by gstrauss 5 months ago

  • Status changed from New to Patch Pending
  • Target version changed from 1.4.xx to 1.4.64
Actions #8

Updated by flynn 5 months ago

I applied your patches and the format looks good for me, thanks a lot.

I scanned older error logs and found two more (but rare) log messages, which also should have a remote ip, if possible:
  • (connections.c.1072) unknown state: 53 272 (this message may have been triggered by tests with ticket #3118)
  • (mod_openssl.c.3135) SSL: renegotiation initiated by client, killing connection
Actions #9

Updated by gstrauss 5 months ago

(connections.c.1072) unknown state: 53 272 (this message may have been triggered by tests with ticket #3118)

Yeah, that should not happen and was probably memory corruption fixed in #3118

(mod_openssl.c.3135) SSL: renegotiation initiated by client, killing connection

Ok, I guess it is reasonable to consider TLSv1.2 renegotiation attempts as security scans or as attacks.

--- a/src/mod_openssl.c
+++ b/src/mod_openssl.c
@@ -3133,7 +3133,8 @@ connection_read_cq_ssl (connection * const con, chunkqueue * const cq, off_t max
         if (hctx->renegotiations > 1
             && hctx->conf.ssl_disable_client_renegotiation) {
             log_error(hctx->errh, __FILE__, __LINE__,
-              "SSL: renegotiation initiated by client, killing connection");
+              "SSL: renegotiation initiated by client, killing connection (%s)",
+              con->dst_addr_buf.ptr);
             return -1;
         }

Actions #10

Updated by gstrauss 5 months ago

  • Status changed from Patch Pending to Fixed
Actions

Also available in: Atom