listener_manager: detect and reject effectively equivalent filter cha…#6022
listener_manager: detect and reject effectively equivalent filter cha…#6022htuch merged 2 commits intoenvoyproxy:masterfrom
Conversation
…ins. Previously, it was possible to have two filter chains that didn't have proto equivalent FilterChainMatches, yet had semantically equivalent matchers. E.g. when one filter chain has a not-yet-implemented field. This led to a situation where the first filter chain might register for SDS (and corresponding initialization callbacks), then the second equivalent filter chain would replace it, freeing up the callback target. When SDS initialized, the stale callback would be invoked, resulting in heap-user-after-free. Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=13221. Risk level: Low Testing: Corpus entry and unit test added. Signed-off-by: Harvey Tuch <htuch@google.com>
PiotrSikora
left a comment
There was a problem hiding this comment.
Shouldn't we reject configs that use unimplemented proto fields instead? While this PR fixes the issue for FilterChainMatch, there are 51 occurrences of not-implemented-hide in the api/, so perhaps a fix at the proto validation layer would be a bit more useful?
|
I can do that, we should at least add an ASSERT here though, catching this
via fuzzers is not ideal.
…On Thu, Feb 21, 2019 at 6:33 PM Piotr Sikora ***@***.***> wrote:
***@***.**** commented on this pull request.
Shouldn't we reject configs that use unimplemented proto fields instead?
While this PR fixes the issue for FilterChainMatch, there are 51
occurrences of not-implemented-hide in the api/, so perhaps a fix at the
proto validation layer would be a bit more useful?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#6022 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKaLv14aBE93ELFHXEuscF8JsbKrPtLfks5vPyzbgaJpZM4bHZwt>
.
|
|
Thinking about this a bit more, it's a bit tricky to do in general. Here are some considerations:
I think they are all resolvable, the most tricky is (2), since we probably need to add a new tag type and use this for things that are "almost ready to bake" and not verboten. I probably don't have cycles right now for extended PGV hacking to make (3) happen, but happy to open an issue.
|
|
Actually, just chatted to @rodaine. We can do (3) by using |
PiotrSikora
left a comment
There was a problem hiding this comment.
IMHO, changes in this PR won't be necessary once we have a proper fix in PGV (similarly to how we don't test for other PGV-enforced constraints in the code), but if you want to commit this, then that's fine with me, though it's more of "a fix for the case found by the fuzzer" than "a fix for the problem".
- Some use of
[#not-implemented-hide:]is for v1 deprecated code.
But that's an example of misusing [#not-implemented-hide:] 😜
|
Thanks, will merge and I will work on your suggested PGV based solution in the next few days. |
…ins. (envoyproxy#6022) Previously, it was possible to have two filter chains that didn't have proto equivalent FilterChainMatches, yet had semantically equivalent matchers. E.g. when one filter chain has a not-yet-implemented field. This led to a situation where the first filter chain might register for SDS (and corresponding initialization callbacks), then the second equivalent filter chain would replace it, freeing up the callback target. When SDS initialized, the stale callback would be invoked, resulting in heap-user-after-free. Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=13221. Risk level: Low Testing: Corpus entry and unit test added. Signed-off-by: Harvey Tuch <htuch@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>
This is an alternative to envoyproxy#6022. Fixes envoyproxy#6905. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
…ins.
Previously, it was possible to have two filter chains that didn't have proto equivalent
FilterChainMatches, yet had semantically equivalent matchers. E.g. when one filter chain has a
not-yet-implemented field. This led to a situation where the first filter chain might register for
SDS (and corresponding initialization callbacks), then the second equivalent filter chain would
replace it, freeing up the callback target. When SDS initialized, the stale callback would be
invoked, resulting in heap-user-after-free.
Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=13221.
Risk level: Low
Testing: Corpus entry and unit test added.
Signed-off-by: Harvey Tuch htuch@google.com