Fix for issue30458 (HTTP Header Injection) prevents crafting invalid requests

[jaraco]

BPO	38216
Nosy	@gpshead, @jaraco, @larryhastings, @benjaminp, @ned-deily, @mcepl, @ambv, @webknjaz, @ammaraskar, @miss-islington, @tipabu, @tirkarthi
PRs	bpo-38216, bpo-36274: Allow subclasses to override validation and encoding behavior #16321 bpo-38216, bpo-36274: Allow subclasses to separately override validation and encoding behavior #16448 [3.8] bpo-38216, bpo-36274: Allow subclasses to separately override validation and encoding behavior (GH-16448) #16460 [3.7] bpo-38216, bpo-36274: Allow subclasses to separately override v… #16461 [3.6] bpo-38216, bpo-36274: Allow subclasses to separately override v… #16462 [3.5] bpo-38216, bpo-36274: Allow subclasses to separately override validation and encoding behavior (GH-16448) #16475 [2.7] bpo-38216, bpo-36274: Allow subclasses to separately override validation and encoding behavior (GH-16448) #16476
Files	0001-bpo-38216-Only-forbid-CR-LF-and-SP-in-http-URLs.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-10-08.17:37:30.548>
created_at = <Date 2019-09-18.16:14:30.701>
labels = ['release-blocker']
title = 'Fix for issue30458 (HTTP Header Injection) prevents crafting invalid requests'
updated_at = <Date 2019-10-08.17:37:30.547>
user = 'https://github.com/jaraco'

bugs.python.org fields:

activity = <Date 2019-10-08.17:37:30.547>
actor = 'larry'
assignee = 'none'
closed = True
closed_date = <Date 2019-10-08.17:37:30.548>
closer = 'larry'
components = []
creation = <Date 2019-09-18.16:14:30.701>
creator = 'jaraco'
dependencies = []
files = ['48618']
hgrepos = []
issue_num = 38216
keywords = ['patch', '3.6regression', '3.7regression']
message_count = 30.0
messages = ['352738', '352759', '352802', '352827', '352830', '352831', '352839', '352847', '352850', '352870', '352899', '352912', '352930', '352937', '352982', '352983', '352988', '352990', '353001', '353040', '353344', '353447', '353449', '353453', '353464', '353481', '353484', '353501', '354159', '354225']
nosy_count = 12.0
nosy_names = ['gregory.p.smith', 'jaraco', 'larry', 'benjamin.peterson', 'ned.deily', 'mcepl', 'lukasz.langa', 'webknjaz', 'ammar2', 'miss-islington', 'tburke', 'xtreak']
pr_nums = ['16321', '16448', '16460', '16461', '16462', '16475', '16476']
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue38216'
versions = ['Python 2.7', 'Python 3.5']

[jaraco]

The fix for bpo-30458 prevents any request line from transmitting non-ascii characters. In some cases, it's useful to transmit these illegal bytes in order to simulate a maliciously-crafted request (such as to ensure a web server responds correctly to such a request). This limitation is a regression from previous behavior that allowed transmission of such invalid requests.

For example, consider this comment, where tests in CherryPy were disabled due to emergent failures against nightly builds.

Originally, I reported this issue in bpo-36274, but I believe this issue is distinct and has a different timeline from that issue.

In this comment, xtreak suggests that the proper solution might be not to transmit raw invalid characters but to actually rely on URL quoting to "transmit" those bytes. While that does seem to exercise CherryPy's error detection properly, I believe it still fails to exercise the receipt and handling of raw invalid bytes, but it does provide a possible satisfactory mitigation to this issue.

[ned-deily]

Thanks for identifying this issue and breaking it out into a separate bpo, Jason. If I understand correctly, the problematic fix for bpo-30458 has already been released in maintenance release 3.7.4 and security release 3.6.9, is in the current security release candidate 3.5.8rc1, as well as 3.8.0b4, and, without further action, will be in 2.7.17rc1 and continue to be in 3.7.5rc1. In other words, this issue potentially affects all currently maintained Python branches and/or releases. (In addition, there appear to be still unresolved questions about the original bpo-30458 and the CVE's associated with it. But let's ignore those here. My brain hurts enough already.)

The immediate question for me is what to do about 3.7.5. We could:

1. hold 3.7.5rc1 for a mitigation fix
2. release 3.7.5rc1 and accept a fix for 3.7.5final or for an unplanned 3.7.5rc2
3. fix in 3.7.6
4. do nothing other than possibly a doc change

Since 3.5.8rc1 is already released for testing, a similar decision needs to be made for it.

And 3.8.0rc1 and 2.7.17rc1 are schedulded for tagging om the coming weeks.

Since the problem. as best I understand, is most likely to impact tests rather than legitimate user cases (is that correct?) and, since at least some projects and users of 3.7.4 impacted by the change have developed workarounds, and since 3.7.5rc1 is being delayed pending a resolution of this, I think the best options for 3.7.5 at this point are either 2 or 3 above. So, unless someone expresses a major objection in the next few hours, I am going to proceed with 3.7.5rc1 as is with the hope that we will have final resolution prior to 3.7.5 final.

Decisions will still have to be made by the other RMs for their branches.

[larryhastings]

FWIW I planned to tag and release 3.5.8 final early next week. I don't have the domain knowledge to assess the severity of this bug--much less pitch in and help fix it--so I suspect this will simply hold up 3.5.8 final.

Depending on the complexity of the fix for this issue, I may also insert a second rc into the 3.5.8 schedule.

[gpshead]

What's needed here is a Decision. (release managers and steering councils make those)

IMNSHO, this regression is intentional and does not feel like a bug.

The Python HTTP APIs were never designed with an explicit intent to allow violations of the protocol. That it was "allowed" was an accident because the very old original API design didn't go out of its way to check much. That doesn't mean it was intended. Hyrum's Law applies as the (lack of) behavior has been around forever. So of course someone has written code that depended on that. But it doesn't make said code right.

We either "be strict in what we produce" or we ignore abuse of the APIs and effectively close all CVEs filed against them as "not a bug, this library is intended to allow abuse when given untrusted input."

If we take the latter route and intended to allow such things, we'd need to explicitly document it as such and guarantee their behaviors. As noted, disallowing it has already shipped in two stable releases.

When writing tests of particular out of spec queries such that servers can implement good "be lenient in what you accept" behavior, it is better not to depend on specific behaviors of a given http client library to allow you to do so for such tests.

If the decision requires code changes, do not expect me to spend time creating them. Please feel free to loop me in on PR reviews but I'd rather not review anything without release manager approval of the direction they take things in the first place.

[ned-deily]

Thanks for your comments, Greg. Here's my take as release manager for 3.7 (and for 3.6). What bothers me here is that we apparently changed de facto behavior between maintenance releases, in the middle of 3.7's lifecycle, without warning, no doubt because we didn't realize it would break third-party packages. But it has and that's a big no-no. A very important part of our maintenance strategy is that we implicitly promise our users that they can easily upgrade from any older release in a release family to the most current release without fear of incompatibilities (e.g. 3.7.0 to 3.7.4). In return for that, we will only provide fixes for the most recent maintenance release in a family (e.g. once 3.7.5 is released, 3.7.4 is dead and 2.7.3 through 3.7.0 were already dead). So it seems here we have violated that compatibility promise for 3.7 and 3.6 and are about to do so for 3.5 and 2.7. Since at least one project is known to have been impacted, it's not unreasonable to expect that more will be. So I think we should avoid such breakage and undo the change in behavior for 3.7 (and the older releases as well).

Now, as for 3.8, as it hasn't released yet, we have more latitude and, although we're close to producing an RC, it may still be OK to document the changed behavior there. That should be Łukasz's call.

Does that make sense?

[ammaraskar]

What bothers me here is that we apparently changed de facto behavior between maintenance releases, in the middle of 3.7's lifecycle, without warning, no doubt because we didn't realize it would break third-party packages.

Arguably, I think the programs that are affected by this vulnerability far outnumber the amount of third-party packages that will be broken. The trade-off here seems to be between the promise of compatibility and the promise of security, choosing compatibility strikes me as odd.

[tirkarthi]

For reference this is how urllib handled the same issue in their test suite : urllib3/urllib3#1673.

golang also received similar regression report as the change landed in 1.11.6 and discussion : golang/go#30903

[gpshead]

All bug fixes are behavior changes. Any broken behavior can be relied upon by someone.

So far the only ones who have popped up with this change as being a problem is one project's test suite where the behavior was used by a test because it was a convenient hack. Not by application code. Reverting mitigations due to the undesired behavior being convenient for someones protocol-abuse tests seems over cautious. Those can be updated to not use our client library for their abuse scenarios.

[jaraco]

Thanks for all the comments. I agree the current (secure by default) implementation is desirable. I also agree that such usage was never explicitly supported, so the "regression" here is perhaps over-stated. What I seek is to avoid the Go recommendation of "fork the implementation" when a lightweight hook could be provided as a means to achieve a reasonable, if unusual and discouraged, behavior. In particular, I'd like to support:

Sending a single request with invalid bytes for the path.
Allowing all requests for a client to support invalid bytes on the path.

Ideally, the solution should allow sending other non-control bytes as well, supporting the use case reported in bpo-36274 also.

I'll draft a patch. I'll try to get to it this weekend, but if I don't, don't feel like this needs to block releases.

[tipabu]

Since at least one project is known to have been impacted, it's not unreasonable to expect that more will be.

I can confirm at least one other: OpenStack Swift's stable jobs have been broken by bb8071a since 11 Sep; see https://bugs.launchpad.net/swift/+bug/1843816 . (Why we're testing against an untagged version of py27, I still need to investigate...)

Now, the broken tests *do* represent something of a long-standing bug in application code: our front-end server was lenient in what it accepted but not strict in what it emitted -- it would forward on whatever crazy query params the client sent, directly to the back-end servers. Of course, as long as the back-end servers are *also* lenient in what they accept, it doesn't actually present a problem. Still, I'm happy to fix the issue.

Actually *addressing* this has proven problematic, however, as we have numerous functional tests that send raw UTF-8 request paths -- to the point that I can only assume there exist clients sending such requests. Changing the tests to URL-encode the path would do a disservice to those clients, as my fix (or some future fix) may break them and we'd be none the wiser.

We either "be strict in what we produce" or we ignore abuse of the APIs and effectively close all CVEs filed against them as "not a bug, this library is intended to allow abuse when given untrusted input."

I think this is a false dichotomy; in https://bugs.python.org/issue36274#msg351834 Jason proposed a few alternatives that allow for a secure and obvious default API while adding a new, explicitly unsafe API.

I'd like to add yet another option that may be useful specifically for maintenance releases: forbid only the problematic characters -- namely LF (and potentially CR and SP). This seems like a much more surgical fix for maintenance releases, allowing the null byte for CherryPy or the raw UTF-8 bytes for Swift, while still mitigating the CVE.

3.8 and later can still have the more-robust check to ensure callers have done any requisite URL-escaping. Ideally, there would also be a reasonable hook in place to allow application developers to verify behavior in the presence of invalid requests; something short of writing raw bytes to a socket.

For reference this is how urllib handled the same issue in their test suite : urllib3/urllib3#1673.

I think the test suites developed by clients (like urllib3) and servers (like CherryPy or Swift) inherently have very different goals -- where a client would want to ensure that it *produces valid bytes* on the wire, a server ought to check that it doesn't completely fall over when it *receives invalid bytes* on the wire.

What I seek is to avoid the Go recommendation of "fork the implementation" ...

This is *exactly* the way we worked around https://bugs.python.org/issue36274 in Swift: swap out putrequest() in our func tests. (See openstack/swift@c0ae48b .) It looks like a similar change will be required to fix our stable branches.

I agree, I'd much rather *not* have to do that. :-/