Request body doubly written to upstream with file system buffer filter

[alxyzc]

If you are reporting any crash or any potential security issue, do not open an issue in this repo. Please report the issue via emailing envoy-security@googlegroups.com where the issue will be triaged appropriately.

Title: Request body doubly written to upstream with file system buffer filter

Description:

What issue is being seen? Describe what should be happening instead of the bug, for example: Envoy should not crash, the expected value isn't returned, etc.

We have observed the following unexpected behavior:

• When a retry policy is added to a route for a listener configured with file system buffer filter, Envoy would write the request body twice to the upstream connection. (Removing either the retry policy or the file system buffer filter will result in expected behavior of Envoy.)

• The echo server reads Content-Length from the socket for the request body, leaving the other copy of the request payload in the socket.

• The remainder bytes of the duplicated payload would be read as prefix to the subsequent request via the same connection (observed as prefix to the request method of the subsequent request in the response from the echo server):

  

• It is possible to forge a request $Re{q}_f$ ($f$ as in forged request) whose payload is a different, well-formed request $Re{q}_p$ ($p$ as in request in payload). In such cases, we've observed that the backend server will send 2 responses to Envoy — first $Res{p}_f$ then $Res{p}_p$ as the duplicated copy of $Re{q}_p$ was read as a new, stand-alone request.

• From our observation, $Res{p}_p$ is ignored in most cases. However, when we blast Envoy with $Re{q}_f$ concurrently, and in the meantime, we keep sending a different request $Re{q}_c$ ($c$ for control). Occasionally, we would observe that instead of the expected $Res{p}_c$, the client would receive $Res{p}_p$.

Repro steps:

Include sample requests, environment, etc. All data and inputs required to reproduce the bug.

This issue is most easily reproduced on a Kubernetes cluster:

1. We first deploy all resources to the leaky-body-repro namespace:

   kubectl apply -f - <<EOF
   apiVersion: v1
   kind: Namespace
   metadata:
     name: leaky-body-repro
   ---
   apiVersion: apps/v1
   kind: Deployment
   metadata:
     labels:
       app: echo
     name: echo
     namespace: leaky-body-repro
   spec:
     replicas: 1
     selector:
       matchLabels:
         app: echo
     template:
       metadata:
         labels:
           app: echo
       spec:
         containers:
           - image: gcr.io/k8s-staging-ingressconformance/echoserver:v20220815-e21d1a4
             name: echo
             ports:
               - containerPort: 3000
             env:
               - name: POD_NAME
                 valueFrom:
                   fieldRef:
                     fieldPath: metadata.name
               - name: NAMESPACE
                 valueFrom:
                   fieldRef:
                     fieldPath: metadata.namespace
             resources:
               limits: 
                 cpu: 200m
                 memory: 128Mi
   ---
   apiVersion: v1
   kind: Service
   metadata:
     labels:
       app: echo
     name: echo
     namespace: leaky-body-repro
   spec:
     ports:
       - port: 80
         protocol: TCP
         targetPort: 3000
     selector:
       app: echo
     sessionAffinity: None
     type: ClusterIP
   ---
   apiVersion: v1
   kind: ConfigMap
   metadata:
     name: envoy-config
     namespace: leaky-body-repro
   data:
     envoy.yaml: |
       admin:
         address:
           socket_address:
             address: 127.0.0.1
             port_value: 9901
       node:
         cluster: repro-envoy
         id: envoy
       static_resources:
         clusters:
           - name: echo_cluster
             type: STRICT_DNS
             lb_policy: ROUND_ROBIN
             load_assignment:
               cluster_name: echo_cluster
               endpoints:
                 - lb_endpoints:
                     - endpoint:
                         address:
                           socket_address:
                             address: echo.leaky-body-repro.svc.cluster.local
                             port_value: 80
         listeners:
           - address:
               socket_address:
                 address: 0.0.0.0
                 port_value: 80
             filter_chains:
             - filters:
               - name: envoy.filters.network.http_connection_manager
                 typed_config:
                   "@type": type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
                   stat_prefix: ingress_http
                   route_config:
                     name: local_route
                     virtual_hosts:
                     - name: service
                       domains:
                       - "*"
                       routes:
                       - match:
                           prefix: "/"
                         route:
                           cluster: echo_cluster
                       retry_policy:
                         retry_on: connect-failure
                         num_retries: 0
                   http_filters:
                   - name: file_system_buffer
                     typed_config:
                       "@type": type.googleapis.com/envoy.extensions.filters.http.file_system_buffer.v3.FileSystemBufferFilterConfig
                       manager_config:
                         thread_pool:
                           thread_count: 1
                       request:
                         behavior:
                           fully_buffer: {}
                       response:
                         behavior:
                           bypass: {}
                       storage_buffer_path: /tmp
                   - name: envoy.filters.http.router
                     typed_config:
                       "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router
   ---
   apiVersion: apps/v1
   kind: Deployment
   metadata:
     labels:
       app: envoy
     name: envoy
     namespace: leaky-body-repro
   spec:
     replicas: 1
     selector:
       matchLabels:
         app: envoy
     template:
       metadata:
         labels:
           app: envoy
         name: envoy
       spec:
         containers:
           - name: envoy-proxy
             image: envoyproxy/envoy:v1.33-latest
             imagePullPolicy: IfNotPresent
             command:
               - envoy
               - -c
               - /etc/envoy/envoy.yaml
               - --concurrency
               - "1"
             ports:
               - containerPort: 80
                 name: http
                 protocol: TCP
             resources:
               limits: 
                 cpu: 200m
                 memory: 128Mi
             volumeMounts:
               - mountPath: /etc/envoy/
                 name: envoy-config-volume
                 readOnly: true
         volumes:
           - configMap:
               defaultMode: 420
               items:
                 - key: envoy.yaml
                   path: envoy.yaml
               name: envoy-config
             name: envoy-config-volume
   EOF

2. We can now port-forward to the Envoy pod.

   kubectl port-forward $(kubectl get pod -l 'app=envoy' -n leaky-body-repro -o jsonpath='{.items[0].metadata.name}') -n leaky-body-repro 8080:80 9901

3. We can send the following request and verify that the payload of the first request gets leaked as the prefix of the second request:

   curl localhost:8080 -d 'foo' && curl localhost:8080

4. We can also verify that with this behavior of request body leakage, we could cause $Res{p}_p$ to be returned to a client sending $Re{q}_c$:

   # Send 1000 Req_f with Req_p in payload.
   for i in `seq 1000`; do
       curl -s localhost:8080 -H 'Content-Type: text/plain' --data-binary $'GET /bogus HTTP/1.1\r\nHost: bogushost\r\nHeader: bogus\r\n\r\n' > /dev/null &
   done &
   
   # Concurrent, keep sending Req_c until we get a Resp_p.
   seq=0
   while true; do
       response=$(curl -s localhost:8080 -H "Request-Seq: $seq" -v 2>&1)
       if [[ $response == *bogus* ]]; then
           echo $response
           break
       fi
       ((seq++))
   done

Admin and Stats Output:

Include the admin output for the following endpoints: /stats, /clusters, /routes, /server_info. For more information, refer to the
admin endpoint documentation.

Note: If there are privacy concerns, sanitize the data prior to sharing.

server_info.txt
routes.txt
clusters.txt
stats.txt

Config:

Include the config used to configure Envoy.

See step 1 in repro steps.

Logs:

Include the access logs and the Envoy logs.

Note: If there are privacy concerns, sanitize the data prior to sharing.

envoy.log (access logs not configured from repro config).

Call Stack:

If the Envoy binary is crashing, a call stack is required. Please refer to the Bazel Stack trace documentation.

No crash.

Comments:

We are uncertain of the exact security implications with this behavior. However, we speculate that depending on the backend server implementation, a forged request could contain a payload that tricks the backend server into sending a response to a non-suspecting client that could alter its behavior in a way desired by adversary. Thus, we are using this channel to report such an unexpected behavior when the file system buffer works in conjunction with route retry policies.

I have reached out to the Envoy Security channel, where @botengyao mentioned that since the behavior is only reproduceable with the file system buffer filter, the work-in-progress status implies that it will not be covered by the security team. I thus open the issue here.

[phlax]

cc @mattklein123 @ravenblackx

[ravenblackx]

I think this is a misbehavior of retries rather than a misbehavior of the filter - i.e. HTTP filters expect that decodeHeaders will be called once, and decodeData will be called enough times for the data to pass through once, but I think what's happening here is that a retry causes all the filters to run again, with their state intact, with all the inputs from a request that has already been processed by the filter being passed in.

Which is two things wrong - if a filter is stateless, on retry it still ends up doing its action on an already-modified request, e.g. a MutateHeaders filter with an 'add' mode mutation would end up, after a retry, with the upstream receiving the same header twice (or more).
And if a filter is not stateless, as in the case of the file system buffer filter, it gets surprised by the same request traveling through it a second time, which results in incorrect behavior here.

The latter could be fixed by instantiating a new filter chain for retries rather than reusing the existing filter chain, but the former would still be problematic IMO.

It's also possible that I'm wrong and HTTP filter instances are supposed to be stateless or only capture their state in a special way that's initialized per request, in which case the first problem still seems problematic, but that would make the second case an issue to be addressed in the buffer filter (and probably several other filters). I don't think that's the case, though I think maybe some years ago HTTP filters operated that way, and the misbehavior of retries might be an accidental holdover from that time.

If I'm right about how it's supposed to work, then I would suggest that decodeHeaders being called twice on the same filter instance should provoke an ENVOY_BUG, as part of addressing this.

Edit: and I don't even have a good idea for how to address the "non-idempotent filters applied twice" problem.

[alxyzc]

@ravenblackx One thing I forgot to highlight in the description is that this issue is reproducible in the presence of a retry policy, even if num_retries is 0 -- which I assume means that no retry has actually taken place.

[ravenblackx]

@ravenblackx One thing I forgot to highlight in the description is that this issue is reproducible in the presence of a retry policy, even if num_retries is 0 -- which I assume means that no retry has actually taken place.

But not reproducible in the absence of a retry policy, right? So now it sounds like two bugs in the retry behavior, in that if num_retries is 0 then the retry policy should have had no effect on anything.

[alxyzc]

@ravenblackx One thing I forgot to highlight in the description is that this issue is reproducible in the presence of a retry policy, even if num_retries is 0 -- which I assume means that no retry has actually taken place.

But not reproducible in the absence of a retry policy, right? So now it sounds like two bugs in the retry behavior, in that if num_retries is 0 then the retry policy should have had no effect on anything.

@ravenblackx Yes-- if we remove the retry policy, Envoy's behavior is correct.

[rabb1t-dev]

We are uncertain of the exact security implications with this behavior. However, we speculate that depending on the backend server implementation, a forged request could contain a payload that tricks the backend server into sending a response to a non-suspecting client that could alter its behavior in a way desired by adversary.

To clarify, the security impact here not only includes the behavior described above, but also the attacker recieves responses intended for other users. This would include sesson cookies or other secrets.

[ravenblackx]

Unfortunately it seems we no longer have anyone whose specialist subject includes retries - I've ended up assigned as one of the codeowners for that area because I'm willing to review, but I'm not going to have time to try to fix this, and the other codeowner for retries is @mattklein123 who I'm pretty sure also doesn't have time to dedicate to it. [correct me if I'm wrong Matt]

So there are three options remaining:

1. you could try to contribute a fix to how retries behave, which I would happily review.
2. you could hope someone else comes along who will do that.
3. if you need both of these features together, you could do a workaround for the problem in the config.

Specifically, I think this could be worked around by changing the config from

listener [retries + buffering] -> upstream

to

listener [buffering] -> internal_listener_a
internal_listener_a [retries] -> upstream

I realize this is a horrible solution and leaves the underlying bug intact, but it could at least unblock you.

[ravenblackx]

@yanavlasov @botengyao for maybe additional thoughts on security implications. I don't know if we'd want to maybe document somewhere "mixing retries with other filters has unpleasant security implications", if we can't get it fixed.
Edit: and I think internal redirects probably have the same issues, in that they too return to the start of the filter chain without resetting the filters' internal states.

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.