[issue38216] Fix for issue30458 prevents crafting invalid requests

[Tim Burke]

Tim Burke  added the comment:

> 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 https://github.com/python/cpython/commit/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 : 
> https://github.com/urllib3/urllib3/pull/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 
https://github.com/openstack/swift/commit/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. :-/

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue38216] Fix for issue30458 prevents crafting invalid requests

[Jason R. Coombs]

Jason R. Coombs  added the comment:

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 issue36274 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue38216] Fix for issue30458 prevents crafting invalid requests

[Gregory P. Smith]

Gregory P. Smith  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue38216] Fix for issue30458 prevents crafting invalid requests

[Matej Cepl]

Change by Matej Cepl :


--
nosy: +mcepl

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue38216] Fix for issue30458 prevents crafting invalid requests

[Karthikeyan Singaravelan]

Karthikeyan Singaravelan  added the comment:

For reference this is how urllib handled the same issue in their test suite : 
https://github.com/urllib3/urllib3/pull/1673.

golang also received similar regression report as the change landed in 1.11.6 
and discussion : https://github.com/golang/go/issues/30903

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue38216] Fix for issue30458 prevents crafting invalid requests

[Ammar Askar]

Ammar Askar  added the comment:

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

--
nosy: +ammar2

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue38216] Fix for issue30458 prevents crafting invalid requests

[Ned Deily]

Ned Deily  added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue38216] Fix for issue30458 prevents crafting invalid requests

[Gregory P. Smith]

Gregory P. Smith  added the comment:

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.

--
nosy: +gregory.p.smith

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue38216] Fix for issue30458 prevents crafting invalid requests

[Larry Hastings]

Larry Hastings  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue38216] Fix for issue30458 prevents crafting invalid requests

[Tim Burke]

Change by Tim Burke :


--
nosy: +tburke

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue38216] Fix for issue30458 prevents crafting invalid requests

[Ned Deily]

Ned Deily  added the comment:

Thanks for identifying this issue and breaking it out into a separate bpo, 
Jason.  If I understand correctly, the problematic fix for Issue30458 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 Issue30458 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.

--
nosy: +benjamin.peterson, larry, lukasz.langa, ned.deily
priority: normal -> release blocker
versions: +Python 2.7, Python 3.5, Python 3.6, Python 3.7, Python 3.8, Python 
3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue38216] Fix for issue30458 prevents crafting invalid requests

[Jason R. Coombs]

Change by Jason R. Coombs :


--
keywords: +3.6regression, 3.7regression

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue38216] Fix for issue30458 prevents crafting invalid requests

[Sviatoslav Sydorenko]

Change by Sviatoslav Sydorenko :


--
nosy: +webknjaz

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue38216] Fix for issue30458 prevents crafting invalid requests

[Karthikeyan Singaravelan]

Change by Karthikeyan Singaravelan :


--
nosy: +xtreak

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue38216] Fix for issue30458 prevents crafting invalid requests

[Jason R. Coombs]

New submission from Jason R. Coombs :

The fix for issue30458 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](https://github.com/cherrypy/cherrypy/issues/1781#issuecomment-507836873),
 where tests in CherryPy were disabled due to emergent failures against nightly 
builds.

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

In [this comment](https://bugs.python.org/issue36274#msg352711), 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.

--
messages: 352738
nosy: jaraco
priority: normal
severity: normal
status: open
title: Fix for issue30458 prevents crafting invalid requests

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com