wasm: crashes when removing ":status" header in the response.

[mathetake]

Currently Envoy does not limit which headers can be modified from Wasm plugins, so if a plugin removes, for example, :status header from the response headers, then Envoy crashes. Ideally, we should verify the final headers in the Wasm plugin in Envoy. In any way, Envoy should not crash even when the processed headers from filters are invalid in terms of the "contents".

This is a potential security risk when we start running untrusted Wasm binaries. I think this might not be a Wasm specific issue, but rather a generic point we can improve to make Envoy protected against a crash path.

I guess there's already a similar GH issue, but I couldn't find. Thanks!

cc @PiotrSikora @lizan

[PiotrSikora]

There should be an issue for that already, but I cannot find it either.

I recall that @asraa was working on Envoy-wide guards against misbehaving filters/plugins, including removing :status header, but that was a while ago, I'm not sure what's the status of that work. Perhaps, that's the reason for the crash.

[mathetake]

^ @asraa any comments?

[asraa]

That's noted right here and it hasn't been fixed :( #14960

Could you confirm that this is where Envoy crashes?

envoy/source/common/http/http2/codec_impl.cc

Line 223 in d5e8634

ASSERT(headers.Status() != nullptr);

[asraa]

Found the issue
#13756

[PiotrSikora]

@asraa thanks!

@mathetake we should probably fix this at the Wasm level anyway, not only to avoid the crash (since that's going to be fixed in #13756), but also to notify the plugin.

The issue is that currently we return WasmResult::BadArgument in case of bad map_type value, so I'm not convinced if we should reuse the same value in case of missing :status (or :authority, :path or :scheme in case of requests), since it changes the failure modes in the SDKs... although, I could be probaly convinced that we have to do it.

The other option is to simply ignore the bad headers update and log something at the error level to notify the user.

Any thoughts?

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

[PiotrSikora]

Not stale.

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

[github-actions]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.

[github-actions]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.

[mathetake]

not stale

[mathetake]

#16745 I reworked the Envoy-wide solution