WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#33626 closed defect (bug) (fixed)

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

Reported by: martiusweb Owned by: dd32
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.0
Component: General Keywords: has-patch dev-feedback
Focuses: Cc:

Description

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

Attachments (1)

33626.diff (397 bytes) - added by swissspidy 4 years ago.

Download all attachments as: .zip

Change History (7)

#1 @dd32
4 years ago

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.

Edit: If they're running with output buffering enabled, then BOM/whitespace won't cause those errors, but may cause some extra output on ajax requests or something.. So this would indeed fix those requests for those users, but not for others.

Last edited 4 years ago by dd32 (previous) (diff)

#2 @martiusweb
4 years ago

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.

#3 @dd32
4 years ago

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.

#4 @martiusweb
4 years ago

Removing the faulty header() works for me.

@swissspidy
4 years ago

#5 @swissspidy
4 years ago

  • Keywords has-patch dev-feedback added
  • Milestone changed from Awaiting Review to 4.4

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

#6 @dd32
4 years ago

  • Owner set to dd32
  • Resolution set to fixed
  • Status changed from new to closed

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

Note: See TracTickets for help on using tickets.