Content-Length and Transfer-Encoding are mutually exclusive HTTP headers

Bug #981332 reported by Mike Lundy on 2012-04-14
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Glance
High
Mike Lundy
Essex
Undecided
Unassigned
glance (Ubuntu)
Undecided
Unassigned
Precise
Undecided
Chuck Short

Bug Description

If the glance api server is behind a conforming HTTP/1.1 reverse proxy (pound, in this example), all uploaded images will be corrupted. This is because glance.client sends both the "Content-Length" and "Transfer-Encoding: chunked" headers. The HTTP/1.1 spec (http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.4) says:

"If a message is received with both a Transfer-Encoding header field and a Content-Length header field, the latter MUST be ignored."

The glance client is sending Content-Length first, and pound sanitizes the request, so it rightfully strips out the Transfer-Encoding. This means that the chunk sizes in the body of the request are written as data to the image, resulting in a corrupted image.

This affects diablo, essex, and folsom, though it will only affect non-sendfile platforms on essex and folsom (since that codepath is not affected).

Related branches

Mike Lundy (novas0x2a) on 2012-04-14
description: updated

Fix proposed to branch: master
Review: https://review.openstack.org/6563

Changed in glance:
assignee: nobody → Mike Lundy (novas0x2a)
status: New → In Progress
Mike Lundy (novas0x2a) on 2012-04-14
description: updated

According to your quote from RFC2616, pound is violating the HTTP standard by not ignoring Content-Length in this case.
If it did, all would be working (because the chunked encoding itself specifies lengths)

Ah, the relevant section is this:
4.4. 3. If a Content-Length header field (section 14.13) is present, its decimal value in OCTETs represents both the entity-length and the transfer-length. The Content-Length header field MUST NOT be sent if these two lengths are different (i.e., if a Transfer-Encoding header field is present).

So the change is still needed to make glance-client standards-compliant.

Mike Lundy (novas0x2a) wrote :

Yeah, sorry, I should have included both the client-side and server-side directive when I quoted it.

It's also further complicated because Transfer-Encoding is a hop-by-hop header, so pound has to make an active effort to reuse it; since hop-by-hop headers are not implicitly passed through, "ignore" means "drop" in that case. The spec could be a little clearer there for proxies, but I think what it's doing is reasonable.

As I read the RFC, it says that, when there are both Transfer-Encoding and Content-Length headers, the latter (which means Content-Length, because it is listed last in this sentence) MUST be dropped. It is the only thing that makes sense in this situation.
So keeping Content-Length and dropping Transfer-Encoding is
a) the wrong thing to do and
b) violating this RFC.

Mike Lundy (novas0x2a) wrote :

Ah. Interesting. In the sentence "If a message is received with both a Transfer-Encoding header field and a Content-Length header field, the latter MUST be ignored." I interpreted "latter" as "latter header received", which pound also appears to have done, but I think your reading is correct.

Brian Waldon (bcwaldon) on 2012-04-23
Changed in glance:
importance: Undecided → High
Jay Pipes (jaypipes) on 2012-04-23
Changed in glance:
milestone: none → folsom-1
Jay Pipes (jaypipes) wrote :

I read the RFC to mean latter == Content-Length, since Content-Length was mentioned after Transfer-Encoding. And in the case of Pound with Glance, Pound is correctly ignoring the Content-Length header. Not sure I fully agree that "ignore" means "remove the header"... but c'est la vie.

Here's the problem: without a Content-Length header, the Glance API server is unable to correctly understand the total length of the bytestream to expect for an image, and therefore is unable to validate a number of things, including whether the image blob in the request is greater than the maximum image size Glance will accept.

I suppose we can live with this restriction -- but it will in effect be a security hole, as someone could send a gigantic stream of randomized data to Glance and there is no fail-fast short-circuit to prevent this type of runaway request.

There used to be issues in the Swift storage when an image's size could not previously be determined, but with Ewan Mellor's rewrite of the Swift chunking iterator, I believe that problem is since solved...

As an aside, this same issue exists in Swift (where some of the Glance client code comes from...). You will notice that the Swift put_object() call contains a content_length parameter that gets written into the HTTP headers -- even though the Transfer-Encoding: chunked header is written:

https://github.com/openstack/swift/blob/master/swift/common/client.py#L628

summary: - glance client corrupts images on upload if a proxy is involved
+ Content-Length and Transfer-Encoding are mutually exclusive HTTP headers

A different approach to solve this could be to keep the Content-Length header (if the size can be determined) and drop the Transfer-Encoding instead.

But, there still needs to be support for the streaming case, which is used for such calls
curl $URL | glance add ...

The downside is that this would make the code slightly more complex.
Another way would be to always use chunked encoding without Content-Length, but have only one big chunk for files of known size, so that the server could determine if the size is acceptable right after receiving the first bytes.

Mike Lundy (novas0x2a) wrote :

The content-length can only be considered a hint, you can never actually trust it. In the current code, using the filesystem store, you can send a content-length of 4 and then dump 100PB on glance and it'll accept it. The code that actually reads from the socket needs to stop when it hits IMAGE_SIZE_CAP; this is a minor security problem, but it requires auth, so it's not too bad.

In any case, I see the lack of input validation on content-length to be a separate bug; it will require some amount of rearchitecting because the store assumes the image_size/content-length can be trusted, but only the store knows how big the data actually is. Note also that chunked-encoding in glance is the fallback path; any performant implementation should be using the sendfile api anyway. I might even go so far as to say that glance should die immediately on startup when sendfile is not supported, unless the user passes a flag; sendfile has so much better performance that it's no contest.

Swift does not violate the HTTP spec that I can see, it only sends chunked if it doesn't send content-length: https://github.com/openstack/swift/blob/master/swift/common/client.py#L682 and 690. As far as I know, there's no max object size for swift, so there's no reasonable point at which the server can reject input ahead of time. Instead, it fallocates the content-length (https://github.com/openstack/swift/blob/master/swift/obj/server.py#L560) which would be rejected if it were larger than the filesystem could handle. It also validates that the upload_size matches the content-length (and refuses to add the object to the hash if violated: https://github.com/openstack/swift/blob/master/swift/obj/server.py#L576). I don't understand the intricacies of swift, but seeing that leads me to believe that someone specifically thought about it and handled it appropriately.

Does this set your fears to rest so you'll +1 the review?

Eoghan Glynn (eglynn) wrote :

Hi Mike,

One slightly related point we discussed at the summit was to require the presence of *some* header identifying the total image size.

This was intended to feed into some new API call-cost estimation logic to be used with smarter rate limiting.

Now this header doesn't necessarily have to be content-length, probably better to instead always set x-image-meta-size or a new header such as x-glance-image-size.

Cheers,
Eoghan

Jay Pipes (jaypipes) wrote :

I agree with you, Mike.. was just trying to add some background information to the bug, that's all :)

As for Swift, Swift always uses chunked-transfer-encoding if there is a readable file-like object that is passed to put_object(). It will also include a Content-Length header if the content_length parameter is not None:

https://github.com/openstack/swift/blob/master/swift/common/client.py#L676

Best,
-jay

Reviewed: https://review.openstack.org/6563
Committed: http://github.com/openstack/glance/commit/223fbee49a55691504623fa691bbb3e48048d5f3
Submitter: Jenkins
Branch: master

commit 223fbee49a55691504623fa691bbb3e48048d5f3
Author: Mike Lundy <email address hidden>
Date: Fri Apr 13 19:53:16 2012 -0700

    Omit Content-Length on chunked transfer

    Content-Length and Transfer-Encoding conflict according to the HTTP
    spec. This fixes bug 981332.

    This also adds the ability to test both the sendfile-present and
    sendfile-absent codepaths; the sendfile-present test will be skipped on
    sendfile-absent platforms.

    Change-Id: Ibb017b4a43d89ffdb6b868648296603cfec7780d

Changed in glance:
status: In Progress → Fix Committed
Mike Lundy (novas0x2a) wrote :

Jay, unless there's something I'm missing, swift does not set chunked when there is a content-length.
https://github.com/openstack/swift/blob/master/swift/common/client.py#L690

if hasattr(contents, 'read'):
    if content_length is None:
        conn.putheader('Transfer-Encoding', 'chunked')
        conn.endheaders()
    else:
        conn.endheaders()

Jay Pipes (jaypipes) wrote :

Mike, you're absolutely correct. Removed reference to Swift bug.

no longer affects: swift

Reviewed: https://review.openstack.org/6776
Committed: http://github.com/openstack/glance/commit/7a9e3a7dc53a20971dbd653f2061337efec136a2
Submitter: Jenkins
Branch: stable/essex

commit 7a9e3a7dc53a20971dbd653f2061337efec136a2
Author: Mike Lundy <email address hidden>
Date: Fri Apr 13 19:53:16 2012 -0700

    Omit Content-Length on chunked transfer

    Content-Length and Transfer-Encoding conflict according to the HTTP
    spec. This fixes bug 981332.

    This also adds the ability to test both the sendfile-present and
    sendfile-absent codepaths; the sendfile-present test will be skipped on
    sendfile-absent platforms.

    [ This is backported from 223fbee49a55691504623fa691bbb3e48048d5f3 ]

    Change-Id: I20856eb51ff66fe4b7145f796a540a832c3aa4d8

Reviewed: https://review.openstack.org/6565
Committed: http://github.com/openstack/glance/commit/bad506d840aede9c7bb263393efc495f287affed
Submitter: Jenkins
Branch: stable/diablo

commit bad506d840aede9c7bb263393efc495f287affed
Author: Mike Lundy <email address hidden>
Date: Fri Apr 13 19:53:16 2012 -0700

    Omit Content-Length on chunked transfer

    Content-Length and Transfer-Encoding conflict according to the HTTP
    spec. This fixes bug 981332.

    [ This is backported from 223fbee49a55691504623fa691bbb3e48048d5f3 ]

    Change-Id: I62f2bcabd9712361dd8837e39a577c4add052d0f

Thierry Carrez (ttx) on 2012-05-23
Changed in glance:
status: Fix Committed → Fix Released
Chuck Short (zulcss) on 2012-05-30
Changed in glance (Ubuntu):
status: New → In Progress
Changed in glance (Ubuntu Precise):
status: New → In Progress
Chuck Short (zulcss) wrote :

** Impact **

If the glance api server is behind a conforming HTTP/1.1 reverse proxy (pound, in this example), all uploaded images will be corrupted. This is because glance.client sends both the "Content-Length" and "Transfer-Encoding: chunked" headers. The HTTP/1.1 spec (http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.4) says:

"If a message is received with both a Transfer-Encoding header field and a Content-Length header field, the latter MUST be ignored."

The glance client is sending Content-Length first, and pound sanitizes the request, so it rightfully strips out the Transfer-Encoding. This means that the chunk sizes in the body of the request are written as data to the image, resulting in a corrupted image.

This affects diablo, essex, and folsom, though it will only affect non-sendfile platforms on essex and folsom (since that codepath is not affected).

** Development Fix **

This is fixed in the development trunk at: https://review.openstack.org/6563 and in quantal

** Stable Fix **

This is fixed in the stable/essex branch at: https://review.openstack.org/6776

** Test Case **

Run the glance unit tests

** Regression Potental **

Minimal, this code path is not used in Ubuntu by default.

Chuck Short (zulcss) on 2012-06-08
Changed in glance (Ubuntu Precise):
assignee: nobody → Chuck Short (zulcss)
milestone: none → ubuntu-12.04.1

Hello Mike, or anyone else affected,

Accepted glance into precise-proposed. The package will build now and be available in a few hours. Please test and give feedback here. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

Changed in glance (Ubuntu Precise):
status: In Progress → Fix Committed
tags: added: verification-needed

Please find the attached Jenkins job results from the Ubuntu Server Team's CI infrastructure. As part of the verification process for this bug, Glance has been deployed and configured across multiple nodes using precise-proposed as an installation source. After successful bring-up and configuration of the cluster, a number of exercises and smoke tests have be invoked to ensure the updated package did not introduce any regressions. A number of test iterations were carried out to catch any possible transient errors.

Please Note the list of installed packages at the top and bottom of the report.

For records of upstream test coverage of this update, please see the Jenkins links in the comments of the relevant upstream code-review(s):

Trunk review: https://review.openstack.org/6563
Stable review: https://review.openstack.org/6776

As per the provisional Micro Release Exception granted to this package by the Technical Board, we hope this contributes toward verification of this update.

Adam Gandelman (gandelman-a) wrote :

Test coverage log.

tags: added: verification-done
removed: verification-needed
Chris Halse Rogers (raof) wrote :

This looks ready to move to -updates. What is the status of this in Quantal? We need to be sure that it'll be addressed there so that upgraders don't suffer a regression.

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package glance - 2012.1+stable~20120608-5462295-0ubuntu2.2

---------------
glance (2012.1+stable~20120608-5462295-0ubuntu2.2) precise-proposed; urgency=low

  * New usptream snapshot. (LP: #1010473)
  * Resynchronize with stable/essex:
   - 5462295 Fix i18n in glance.notifier.notify_kombu. (LP: #983829)
   - 7a9e3a7 Omit Content-Length on chunked transfer. (LP: #981332)
   - 5838b63 Fix content type for qpid notifier. (LP: #980872)
   - 98913da search for logger in PATH. Fixes (LP: #978907)
   - f136e7e Ensure swift auth URL includes trailing slash. Fixes (LP: #979745)
  * debian/rules: Add ability to build tarballs from a git snapshot.
  * debian/patches/fix-pep8-ubuntu.patch: Fix pep8 errors.
 -- Chuck Short <email address hidden> Tue, 05 Jun 2012 10:43:12 -0400

Changed in glance (Ubuntu Precise):
status: Fix Committed → Fix Released
Chuck Short (zulcss) on 2012-07-18
Changed in glance (Ubuntu):
status: In Progress → Fix Released

The verification of this Stable Release Update has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regresssions.

Thierry Carrez (ttx) on 2012-09-27
Changed in glance:
milestone: folsom-1 → 2012.2
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers