wp_favicon_request() may set a bad value for Content-Length header

[martiusweb]

wp_favicon_request() catches /favicon.ico requests early and sends an empty response, avoiding the core to be loaded.

The current implementation sets the header Content-Length to 0 then exits. However, the ob_cache will be flushed and may sent data (for instance, a BOM - Byte Order Mark left in wp-config.php).

With Apache 2.2 and probably several other common servers implementations (including nginx), it lead to a bad response, since the client should not expect a body but will receive the 3 bytes of the BOM. Some proxies might get confused.

This can be fixed by calling ob_clean() or ob_end_clean() in wp_favicon_request().

[dd32]

Correct me if I'm wrong, but a BOM or other whitespace will cause other issues such as headers already sent errors? If that's the case, then accounting for it in the favicon request handler doesn't seem like it'd gain much anyway.

[martiusweb]

Hi, thanks for replying so fast :)

If output buffering (what I incorrectly called ob_cache in my first message) is disabled, Content-Length won't be set, and the HTTP message will not contain an erroneous Content-Length header value. I think only a warning will be logged (and/or displayed).

I thought wordpress activated output buffering by default, but in the case I investigated it may be because of a plugin or user's php configuration.

The very issue here is Content-Length set to 0, which is received by Apache from the fcgi handler and sent to the client without verification.

The HTTP exchange looked like:

GET /favicon.ico HTTP/1.1
Host: whatever.com
Connection: keep-alive

HTTP/1.1 200 OK
Content-Length: 0
Content-Type: image/vnd.microsoft.icon

GET /wp-admin/or-any-next-request/ HTTP/1.1
Host: whatever.com
Connection: keep-alive

<eeff>HTTP/1.1 200 OK  <- the first 3 bytes are the BOM sent by the first response
...

The last line cause confusions, for instance:

• ngnix as a reverse proxy had to issue a second TCP connection for the second request because the first exchange it received is invalid (I wonder if it can issue the 2nd request twice, which might be a problem if not idempotent).
• a strict proxy issue a "bad gateway" because it failed to parse the first line.

[dd32]

If output buffering (what I incorrectly called ob_cache in my first message) is disabled, Content-Length won't be set, and the HTTP message will not contain an erroneous Content-Length header value. I think only a warning will be logged (and/or displayed).

Correct, A Warning is issued as PHP had already started outputting the page (So the Content-Length & Content-Type headers can't be sent). This effectively breaks all Location: redirects & cookies used by WordPress.

WordPress doesn't enable output buffering, but it's sometimes enabled from within PHP (which it will be in this case I expect).
So while clearing the output buffer before exiting in this function might help, it doesn't actually fix the issue for a lot of users.

I feel that we should instead remove the Content-Length: 0 header allowing the web server to determine the appropriate length.

[martiusweb]

Removing the faulty header() works for me.

[swissspidy]

33626.diff​ removes the Content-Length header. I tested the change with different browsers and behaviour hasn't changed.

[dd32]

In 33935:

Favicon: Do not specify a Content-Length: 0 header for our "empty" response to fail more gracefully on environments with extra whitespace on output.

This allows for the web server to generate the appropriate Content-Length header for the request, allowing for strict clients/proxies/servers to process the response.

Props swissspidy.
Fixes #33626