connection: don't pass read data to filters when in delayed close.#6852
Merged
htuch merged 6 commits intoenvoyproxy:masterfrom May 14, 2019
Merged
connection: don't pass read data to filters when in delayed close.#6852htuch merged 6 commits intoenvoyproxy:masterfrom
htuch merged 6 commits intoenvoyproxy:masterfrom
Conversation
Fuzzing revealed that HCM is not happy when it think it has performed a close(), e.g. due to CodecProtocolException, and new data arrives in onData(). It basically tries to dispatch that to the codec, which might be in reset state. This situation can arise when the connection (at the network level) is in delayed close and it continues to dispatch to HCM. The fix is to not forward data in ConnectionImpl::onRead(). This seems the safest place to short circuit during delayed close, since L4 filters (including HCM) should be able to rely on the fact that they don't receive any data after a close(). Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=14516. Risk level: Medium (one line change, sensitive area of code base) Testing: TBD Signed-off-by: Harvey Tuch <htuch@google.com>
Member
Author
|
@AndresGuedez @alyssawilk @mattklein123 LMK if you think this is the right approach and I'll add some tests beyond the corpus entry. There's actually a bunch of bad stuff that happens in HCM if you pump in data after it thinks it has closed; I think other L4 filters could also suffer from this. |
AndresGuedez
suggested changes
May 8, 2019
Contributor
AndresGuedez
left a comment
AndresGuedez
left a comment
There was a problem hiding this comment.
Thanks for looking into this. I suggested an alternate approach to your fix which avoids the extra conditional.
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
alyssawilk
previously approved these changes
May 8, 2019
Contributor
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
Though optional (me being me) I'd love an addition to unit tests as well as the fuzz test :-)
mattklein123
requested changes
May 8, 2019
Member
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Definitely looks like the right approach.
/wait
Signed-off-by: Harvey Tuch <htuch@google.com>
htuch
force-pushed
the
hcm-fix-delayed-close
branch
from
bdcc185 to
06d8363
Compare
mattklein123
previously approved these changes
May 13, 2019
Member
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks, this looks like a solid fix. Would definitely like @AndresGuedez and @alyssawilk to take a look.
AndresGuedez
suggested changes
May 13, 2019
Contributor
There was a problem hiding this comment.
Thanks for all the extra test coverage!
A few general comments regarding the tests:
- I think it would be helpful to
EXPECT_CALL(.., closeSocket(...))since the socket's state explains whytransport_socket_->doRead()does or does not trigger in the tests. transport_socket_->canFlushClose()is returningfalsewhich means that some corner cases regarding write buffers not being depleted after writes are not being exercised. Consider adding a test to cover that edge case.
alyssawilk
previously approved these changes
May 13, 2019
Contributor
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
LGTM pending Andres' comments being addressed
Signed-off-by: Harvey Tuch <htuch@google.com>
mattklein123
approved these changes
May 13, 2019
duderino
pushed a commit
to istio/envoy
that referenced
this pull request
Aug 13, 2019
…nvoyproxy#6852) Fuzzing revealed that HCM is not happy when it think it has performed a close(), e.g. due to CodecProtocolException, and new data arrives in onData(). It basically tries to dispatch that to the codec, which might be in reset state. This situation can arise when the connection (at the network level) is in delayed close and it continues to dispatch to HCM. The fix is to not forward data in ConnectionImpl::onRead(). This seems the safest place to short circuit during delayed close, since L4 filters (including HCM) should be able to rely on the fact that they don't receive any data after a close(). Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=14516. Risk level: Medium (one line change, sensitive area of code base) Testing: Unit tests added. Signed-off-by: Harvey Tuch <htuch@google.com> Signed-off-by: Piotr Sikora <piotrsikora@google.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fuzzing revealed that HCM is not happy when it think it has performed a
close(), e.g. due to CodecProtocolException, and new data arrives in
onData(). It basically tries to dispatch that to the codec, which might
be in reset state. This situation can arise when the connection (at the
network level) is in delayed close and it continues to dispatch to HCM.
The fix is to not forward data in ConnectionImpl::onRead(). This seems
the safest place to short circuit during delayed close, since L4 filters
(including HCM) should be able to rely on the fact that they don't
receive any data after a close().
Fixes oss-fuzz issue
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=14516.
Risk level: Medium (one line change, sensitive area of code base)
Testing: Unit tests added.
Signed-off-by: Harvey Tuch htuch@google.com