Project

General

Profile

Actions

Bug #3147

closed

mod_proxy breaks X-Forwarded-For on requests via unix sockets

Added by fstelzer 2 months ago. Updated 2 months ago.

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

Description

In the case of lighttpd accepting requests via a unix socket and forwarding it via mod_proxy the X-Forwarded-For and Forwarded headers break. I think I see garbage memory content in them in my network trace for this case. (Their content always changes slightly. Usually short strings like: _..., N..., _.)

I've debugged this into: https://github.com/lighttpd/lighttpd1.4/blob/master/src/mod_proxy.c#L692
It appends &con->dst_addr_buf for everything not AF_INET|INET6 which I'm not what it contains for a unix socket.

The backend webserver will deny these requests because of an invalid header.

In general I'm not sure what or if anything at all should be in the header when a unix socket is used.

Actions #1

Updated by gstrauss 2 months ago

It appends &con->dst_addr_buf for everything not AF_INET|INET6 which I'm not what it contains for a unix socket.

It should contain the path to the unix socket if you are not using mod_extforward or if the request was sent directly to the unix socket without forwarding headers in the original request.

&con->dst_addr_buf is initialized at the beginning of each and every request in https://github.com/lighttpd/lighttpd1.4/blob/master/src/connections.c#L937 and is used all over lighttpd code for logging purposes. Do you have mod_accesslog enabled? What is logged for the remote address (%a)?

What version of lighttpd are you using?
If you enable debug.log-request-header = "enable" in lighttpd.conf, can you share what your request headers look like?
If you strace -s 4096 lighttpd, can you confirm that lighttpd is sending garbage in the request headers to the backend?

Actions #2

Updated by fstelzer 2 months ago

What version of lighttpd are you using?

1.4.64 and I also tried current master while debugging

If you enable debug.log-request-header = "enable" in lighttpd.conf, can you share what your request headers look like?

request headers show the correct X-Forwarded-For header passed by the frontend (a varnish proxy) via the unix socket.

If you strace -s 4096 lighttpd, can you confirm that lighttpd is sending garbage in the request headers to the backend?

Here is the relevant strace part:
Incoming request (anonymized)
write(6, "2022-03-03 17:33:55: (connections.c.771) fd:10 rqst: GET /test.jpg?123-1234 HTTP/1.1\n", 126) = 126
write(6, "2022-03-03 17:33:55: (connections.c.771) fd:10 rqst: User-Agent: curl/7.79.1\n", 77) = 77
write(6, "2022-03-03 17:33:55: (connections.c.771) fd:10 rqst: Accept: */*\n", 65) = 65
write(6, "2022-03-03 17:33:55: (connections.c.771) fd:10 rqst: X-Forwarded-For: 111.111.111.111\n", 86) = 86
write(6, "2022-03-03 17:33:55: (connections.c.771) fd:10 rqst: Host: a.valid.host.com\n", 82) = 82
write(6, "2022-03-03 17:33:55: (connections.c.771) fd:10 rqst: Accept-Encoding: gzip\n", 75) = 75
write(6, "2022-03-03 17:33:55: (connections.c.771) fd:10 rqst: X-Varnish: 32771\n", 70) = 70
write(6, "2022-03-03 17:33:55: (connections.c.771) fd:10 rqst: \n", 54) = 54

and the outgoing mod_proxy call:
writev(11, [{iov_base="GET /test.jpg?123-1234 HTTP/1.1\r\nHost: a.valid.host.com\r\nUser-Agent: curl/7.79.1\r\nAccept: */*\r\nX-Forwarded-For: \203\1\r\nAccept-Encoding: gzip\r\nX-Varnish: 32771\r\nX-Host: a.valid.host.com\r\nX-Forwarded-Host: a.valid.host.com\r\nX-Forwarded-Proto: http\r\nConnection: close\r\n\r\n", iov_len=351}], 1) = 351

You can see the \203\1 garbage in the forwarded for. the access log has the same in it when logging %h (I assume %a was just a typo?)

When i switch the incoming connection to localhost instead of the unix socket the headers get set correctly.

Actions #3

Updated by fstelzer 2 months ago

The access log has the same in it when logging %h (I assume %a was just a typo?)

Or maybe you meant %A (uppercase) which shows the correctly used unix socket in my access logs.

Actions #4

Updated by fstelzer 2 months ago

fstelzer wrote in #note-3:

The access log has the same in it when logging %h (I assume %a was just a typo?)

Or maybe you meant %A (uppercase) which shows the correctly used unix socket in my access logs.

sorry, its getting late.. Just saw that there is actually a %a. If i set this to log then i get random garbage in the logfile as well.
For a single request it seems to be always the same until I restart lighttpd. Then i get some different garbage. So just some uninitialized stuff I guess?

Actions #5

Updated by gstrauss 2 months ago

Thanks for the additional info. I'll see if I can reproduce it.

Actions #6

Updated by gstrauss 2 months ago

If you can share (with modification to hide private info), please share your lighttpd.conf proxy.* settings, as well as extforward.*, if you are using mod_extforward.

Actions #7

Updated by gstrauss 2 months ago

What is the name of the unix domain socket on which lighttpd is listening? Is that unix domain socket path generated by Varnish? Does it contain those unprintable characters? lighttpd uses the unix domain socket path as-is.

If you are not already doing so, I recommend using lighttpd mod_extforward and configuring it to trust the Varnish proxy in front of it. If you can configure the Varnish proxy to use the PROXY protocol, then I recommend configuring lighttpd mod_extforward to receive and use the PROXY protocol from the Varnish proxy.

Actions #8

Updated by gstrauss 2 months ago

  • Status changed from New to Patch Pending
  • Target version changed from 1.4.xx to 1.4.65

In my testing using curl --unix-socket /path/to/lighttpd.sock ..., I can see that in network.c:network_server_handle_fdevent() that after the call to accept(), the sun_path is not what is expected. Some quick internet searches suggest that it might be filled in only if the client called bind() on the unix domain socket before connect(). e.g. https://stackoverflow.com/questions/17090043/unix-domain-sockets-accept-not-setting-sun-path

lighttpd should do something different here to fill in sun_path to something sane. lighttpd already has checked the length of the bound listen socket path saved in srv_socket->srv_token.

--- a/src/network.c
+++ b/src/network.c
@@ -74,6 +74,8 @@ static handler_t network_server_handle_fdevent(void *context, int revents) {

         if (nagle_disable)
             network_accept_tcp_nagle_disable(fd);
+        else if (addrlen <= 2) /*(AF_UNIX if !nagle_disable)*/
+            memcpy(addr.un.sun_path, BUF_PTR_LEN(srv_socket->srv_token)+1);

         connection *con = connection_accepted(srv, srv_socket, &addr, fd);
         if (__builtin_expect( (!con), 0)) return HANDLER_GO_ON;

Actions #9

Updated by fstelzer 2 months ago

gstrauss wrote in #note-8:

In my testing using curl --unix-socket /path/to/lighttpd.sock ..., I can see that in network.c:network_server_handle_fdevent() that after the call to accept(), the sun_path is not what is expected. Some quick internet searches suggest that it might be filled in only if the client called bind() on the unix domain socket before connect(). e.g. https://stackoverflow.com/questions/17090043/unix-domain-sockets-accept-not-setting-sun-path

lighttpd should do something different here to fill in sun_path to something sane. lighttpd already has checked the length of the bound listen socket path saved in srv_socket->srv_token.
[...]

I can confirm that this patch lets the socket show up in the accesslog for %a and also sets the X-Forwarded-For header correctly in the mod_proxy:
X-Forwarded-For: /run/lighttpd.sock
I'm still not sure if it is actually a good idea to put this path into the header. If I understand the rfc correctly this would not be valid (though my golang app behind mod_proxy accepts it). https://datatracker.ietf.org/doc/html/rfc7239#section-6
The socket path could contain all kinds of invalid header chars, no? Maybe just putting in a static "_unixsocket" identifier or similar would be better?

It seems a bit weird to me for this to depend on the var "nagle_disable" without the context of its value just above. I would rename the const to sth like "not_af_unix" to make this immediately clear.

Thanks for your help on this!

Actions #10

Updated by gstrauss 2 months ago

It seems a bit weird to me for this to depend on the var "nagle_disable" without the context of its value just above. I would rename the const to sth like "not_af_unix" to make this immediately clear.

Look all of 10 lines higher in the code where nagle_disable is assigned.

I'm still not sure if it is actually a good idea to put this path into the header.

It is not a great idea. Again, please refer to recommendations to use mod_extforward or, alternatively, lua mod_extforward

Actions #11

Updated by fstelzer 2 months ago

gstrauss wrote in #note-10:

I'm still not sure if it is actually a good idea to put this path into the header.

It is not a great idea. Again, please refer to recommendations to use mod_extforward or, alternatively, lua mod_extforward

Even when using mod_extforward, wouldn't lighttpd then still add its socket into the Header when forwarding via mod_proxy?

I don't actually need the socket info in the header at all. I only noticed since my backend rejected these requests as having invalid headers so I wanted to make sure lighttpd does not send them.

Actions #12

Updated by gstrauss 2 months ago

  • Status changed from Patch Pending to Fixed
Actions

Also available in: Atom