Update for oss-fuzz Issue 22106 by twghu · Pull Request #16405 · envoyproxy/envoy

twghu · 2021-05-10T07:38:08Z

Signed-off-by: Tim Walsh temporal.differential@gmail.com

envoy:codec_impl_fuzz_test: Crash in Envoy::Http::HttpStream::directionalAction

Commit Message: Update for oss-fuzz Issue 22106
Additional Description: Note that #4709 covers a number of oss-fuzz error This is part of the ongoing effort started for #4709.
Risk Level: Low
Testing: test/common/http
Docs Changes: None
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Tim Walsh <temporal.differential@gmail.com>

repokitteh-read-only · 2021-05-10T07:38:11Z

Hi @twghu, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #16405 was opened by twghu.

see: more, trace.

twghu · 2021-05-10T07:39:18Z

Originally it seemed like this case was not reproducable. Tested against clang envoy master branch.

* docs: record distroless images Signed-off-by: Long Dai <long0dai@foxmail.com>

Signed-off-by: Tim Walsh <temporal.differential@gmail.com>

antoniovicente

Thanks for the fuzz fixes. I think there's something odd in stat_merger_fuzz_test.cc; @jmarantz, could you take a look?

antoniovicente · 2021-05-11T00:33:01Z

test/common/stats/stat_merger_fuzz_test.cc

    // segments, which trigger some inconsistent handling as described in that
    // bug.
    uint32_t num_bytes = (1 + data[index]) & 0x7;
+    if (index == 0 && num_bytes == 0) {


There seems to be an inconsistency between the line above and the comment before it. The code above generate num_bytes between 0 and 7, not 1 and 8.

https://github.com/envoyproxy/envoy/pull/10965/files#r629769284

I think you are right; the code should be changed to reflect the comment. However it's not material to the problems in this fuzz test.

There's an outstanding PR that will fix it, however: #16239

@jessicayuen will you be able to push that forward?

I think the fix offered here is too aggressive; it doesn't just skip over the troublesome cases but completely stops the test on the passed-in corpus. It could just 'continue'.

Change to initial byte count has resolved crash and removed need for additional testing of num_bytes.

antoniovicente · 2021-05-11T00:33:39Z

test/common/stats/stat_merger_fuzz_test.cc

    // bug.
    uint32_t num_bytes = (1 + data[index]) & 0x7;
+    if (index == 0 && num_bytes == 0) {
+      return;


is num_bytes == 0 ok if index != 0 ?

That was the specific fail case, it does seem logical that if the control byte yields a length of 0 then the specic segment is suspect regardless.

Change to initial byte count has resolved crash and removed need for additional testing of num_bytes.

…cache_impl.{h,cc} (envoyproxy#16424) grid: Rename alternate_protocols_cache.{h,cc} to alternate_protocols_cache_impl.{h,cc} Risk Level: Low Testing: N/A - rename only Docs Changes: N/A Release Notes: N/A Platform Specific Features: N/A Signed-off-by: Ryan Hamilton <rch@google.com>

Signed-off-by: Ryan Northey <ryan@synca.io>

Signed-off-by: Tim Walsh <temporal.differential@gmail.com>

Fixes: CVE-2021-29492 Signed-off-by: Yan Avlasov <yavlasov@google.com>

jmarantz

lgtm from my perspective. Antonio do you want to take another look?

jmarantz · 2021-05-12T12:07:54Z

/retest

repokitteh-read-only · 2021-05-12T12:07:57Z

Retrying Azure Pipelines:

🐱

Caused by: a #16405 (comment) was created by @jmarantz.

see: more, trace.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

* build(deps): bump protobuf in /examples/grpc-bridge/client Bumps [protobuf](https://github.com/protocolbuffers/protobuf) from 3.15.8 to 3.16.0. - [Release notes](https://github.com/protocolbuffers/protobuf/releases) - [Changelog](https://github.com/protocolbuffers/protobuf/blob/master/generate_changelog.py) - [Commits](protocolbuffers/protobuf@v3.15.8...v3.16.0) Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Ryan Northey <ryan@synca.io> * build(deps): bump attrs from 20.3.0 to 21.2.0 in /ci/flaky_test Bumps [attrs](https://github.com/python-attrs/attrs) from 20.3.0 to 21.2.0. - [Release notes](https://github.com/python-attrs/attrs/releases) - [Changelog](https://github.com/python-attrs/attrs/blob/main/CHANGELOG.rst) - [Commits](python-attrs/attrs@20.3.0...21.2.0) Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Ryan Northey <ryan@synca.io> * build(deps): bump attrs from 20.3.0 to 21.2.0 in /tools/testing Bumps [attrs](https://github.com/python-attrs/attrs) from 20.3.0 to 21.2.0. - [Release notes](https://github.com/python-attrs/attrs/releases) - [Changelog](https://github.com/python-attrs/attrs/blob/main/CHANGELOG.rst) - [Commits](python-attrs/attrs@20.3.0...21.2.0) Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Ryan Northey <ryan@synca.io> * build(deps): bump flake8 from 3.9.1 to 3.9.2 in /tools/code_format Bumps [flake8](https://gitlab.com/pycqa/flake8) from 3.9.1 to 3.9.2. - [Release notes](https://gitlab.com/pycqa/flake8/tags) - [Commits](https://gitlab.com/pycqa/flake8/compare/3.9.1...3.9.2) Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Ryan Northey <ryan@synca.io> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

The python3-pip package is needed on both Ubuntu and Fedora. Signed-off-by: James Peach <jpeach@apache.org>

Conform to default C:\msys64 path name. Signed-off-by: Long Dai <long0dai@foxmail.com>

* build(deps): bump sphinx-tabs from 2.1.0 to 3.0.0 in /docs Bumps [sphinx-tabs](https://github.com/executablebooks/sphinx-tabs) from 2.1.0 to 3.0.0. - [Release notes](https://github.com/executablebooks/sphinx-tabs/releases) - [Changelog](https://github.com/executablebooks/sphinx-tabs/blob/master/CHANGELOG.md) - [Commits](executablebooks/sphinx-tabs@v2.1.0...v3.0.0) Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Ryan Northey <ryan@synca.io> * build(deps): bump sphinx from 3.5.4 to 4.0.1 in /docs Bumps [sphinx](https://github.com/sphinx-doc/sphinx) from 3.5.4 to 4.0.1. - [Release notes](https://github.com/sphinx-doc/sphinx/releases) - [Changelog](https://github.com/sphinx-doc/sphinx/blob/4.x/CHANGES) - [Commits](sphinx-doc/sphinx@v3.5.4...v4.0.1) Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Ryan Northey <ryan@synca.io> * update-intersphinx Signed-off-by: Ryan Northey <ryan@synca.io> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Signed-off-by: Ryan Northey <ryan@synca.io>

…roxy#16454) Signed-off-by: Mike Schore <mike.schore@gmail.com>

Signed-off-by: Snow Pettersen <snowp@lyft.com>

…#16451) Signed-off-by: Piotr Sikora <piotrsikora@google.com>

Signed-off-by: Vladimir Moskva <vladmos@google.com>

In order to support calls to Server::Instance::flushStats while also having a flush timer activated we need to handle a flush being requested while in the process of doing a periodic flush. This PR avoids triggering a flush in these cases under the assumption that it is not needed since we're already flushing. Signed-off-by: Snow Pettersen <snowp@lyft.com>

Signed-off-by: Ryan Northey <ryan@synca.io>

This PR is part of envoyproxy#16049 to support general tracing. Please check envoyproxy#16049 get more details. Commit Message: remove trace drivers' dependency on HttpTracerImpl Additional Description: Now all tracers (zipkin, skywalking, etc.) will depend on HttpTracerImpl, making it difficult for Tracers to be reused by other protocols (Dubbo, Thrift, etc.). The purpose of this PR is to change this dependency. Risk Level: Low (No new logic). Testing: Add. Docs Changes: N/A Release Notes: N/A Signed-off-by: wbpcode <comems@msn.com>

canonical must include the hashed payload for most services. The prior behavior of using UNSIGNED-PAYLOAD is an exception to the rule, which select services like s3 support, since hashing the payload may be impractical if the payload is very large. A new filter option is introduced, so that the filter may be explicitly configured to use the UNSIGNED-PAYLOAD string literal as specified in the S3 signing docs: https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html fixes envoyproxy#13904 Additional Description: The original implementation was seemingly very specific to S3 and was subsequently amended to extend the same niche singing behaviors for ES and Glacier. This changes the filter's default behavior to match the general SigV4 guidelines while providing a configuration option to enable the specialized UNSIGNED-PAYLOAD behavior. https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html Risk Level: Medium? Deployments using the filter will now buffer requests by default, which could result in 413 responses for requests with bodies exceeding the buffer limit. These users can mitigate buffering by enabling the `unsigned_payload` option. Testing: I tested locally with a filter config. I anticipate updating the automated tests based on feedback from maintainers. Docs Changes: Added Signed-off-by: Jonathan Stewmon <jstewmon@gmail.com>

) Signed-off-by: Piotr Sikora <piotrsikora@google.com>

Signed-off-by: Tim Walsh <temporal.differential@gmail.com>

To resolve CI issues on Update for oss-fuzz Issue 30088, calculation of initial byte count Update for oss-fuzz Issue 22106

repokitteh-read-only · 2021-05-21T14:06:18Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #16405 was synchronize by twghu.

see: more, trace.

phlax · 2021-05-21T14:08:06Z

@twgu - not sure why this happens sometimes with merging - it can be fixed with a rebase and force-push

twghu · 2021-05-21T14:15:36Z

Uggh This has got too messy.
I merged main branch and pushed
then I attempted to rebase "work" branch.

The only commits needed (can be picked?) are the latest I added 22/5

Maybe a new PR is best option and close this one.

phlax · 2021-05-21T14:17:56Z

up to you - but what will work assuming you have main envoy set as upstream remote

git fetch upstream
git rebase upstream/main
git push -f

you also might need to fix dco - but that could be an artefact of merge mash

cpakulski · 2021-05-21T14:23:33Z

Yeah, I encountered the same situation in the past and closed the PR and opened a new one.
But you will loose code reviews....

To go a round missing DCOs I push my changes first without merging upstream/main. It executes all checks.
After that I merge upstream/main and push with --no-verify.

twghu · 2021-05-21T15:06:45Z

Added PR #16616 for OSSFuzz 22106
Added PR #16636 for OSSFuzz 30088

htuch · 2021-05-23T12:57:05Z

Suggest a force push to reset things here, thanks. Prefer not to do this generally, but it's the best option given the branch state.

twghu · 2021-05-24T08:42:21Z

New PR's added, same code, same corpus entries. Added these PR's in case a cleaner push to upstream main is preferrred.

phlax · 2021-05-24T09:51:19Z

closing this in favour of the 2 breakout prs

Update for oss-fuzz Issue 22106

7dd9689

Signed-off-by: Tim Walsh <temporal.differential@gmail.com>

twghu mentioned this pull request May 10, 2021

Fix bugs discovered by Envoy's fuzzers on bugs.chromium.org #4709

Open

daixiang0 and others added 2 commits May 10, 2021 11:47

docs: record distroless images (envoyproxy#16359)

9375301

* docs: record distroless images Signed-off-by: Long Dai <long0dai@foxmail.com>

Update for oss-fuzz Issue 30088

950dc9e

Signed-off-by: Tim Walsh <temporal.differential@gmail.com>

antoniovicente assigned jmarantz May 11, 2021

antoniovicente reviewed May 11, 2021

View reviewed changes

RyanTheOptimist and others added 4 commits May 10, 2021 20:47

docs: Simplify api build (/cont) (envoyproxy#16417)

5afbb4d

Signed-off-by: Ryan Northey <ryan@synca.io>

Update for oss-fuzz Issue 30088, calculation of initial byte count

3f3e684

Signed-off-by: Tim Walsh <temporal.differential@gmail.com>

Implement handling of escaped slash characters in URL path

5333b92

Fixes: CVE-2021-29492 Signed-off-by: Yan Avlasov <yavlasov@google.com>

jmarantz previously approved these changes May 12, 2021

View reviewed changes

alyssawilk and others added 15 commits May 12, 2021 08:46

docs: follow up to envoyproxy#15926 (envoyproxy#16421)

06c8d41

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Document that python3-pip is a build dependency. (envoyproxy#16409)

b57f187

The python3-pip package is needed on both Ubuntu and Fedora. Signed-off-by: James Peach <jpeach@apache.org>

bazel: unify msys (envoyproxy#16394)

955f15d

Conform to default C:\msys64 path name. Signed-off-by: Long Dai <long0dai@foxmail.com>

docs: Use repo role for repo links (envoyproxy#16455)

38cec23

Signed-off-by: Ryan Northey <ryan@synca.io>

bugfix: test fails to build on MacOS due to unused parameters (envoyp…

55a23b2

…roxy#16454) Signed-off-by: Mike Schore <mike.schore@gmail.com>

matching: disable hcm integration by default (envoyproxy#16387)

aaebda2

Signed-off-by: Snow Pettersen <snowp@lyft.com>

bazel: add --config=docker-asan and --config=remote-tsan. (envoyproxy…

f97112c

…#16451) Signed-off-by: Piotr Sikora <piotrsikora@google.com>

Update rules_apple (envoyproxy#16404)

ce071f6

Signed-off-by: Vladimir Moskva <vladmos@google.com>

docs: Fix bootstrap docs version link (envoyproxy#16457)

e7ddd61

Signed-off-by: Ryan Northey <ryan@synca.io>

wasm: fix V8 build with --config={docker,remote}-msan. (envoyproxy#16452

3f57381

) Signed-off-by: Piotr Sikora <piotrsikora@google.com>

twghu added 3 commits May 22, 2021 00:03

Update for oss-fuzz Issue 30088

240e999

Signed-off-by: Tim Walsh <temporal.differential@gmail.com>

Update for oss-fuzz Issue 30088, calculation of initial byte count

e0c65db

Signed-off-by: Tim Walsh <temporal.differential@gmail.com>

Merge branch 'issue-4709' of github.com:twghu/envoy into issue-4709

0748634

To resolve CI issues on Update for oss-fuzz Issue 30088, calculation of initial byte count Update for oss-fuzz Issue 22106

twghu dismissed jmarantz’s stale review via 0748634 May 21, 2021 14:05

twghu requested review from Shikugawa, alyssawilk, asraa, cpakulski, dio, ggreenway, htuch, lizan, mattklein123 and snowp as code owners May 21, 2021 14:05

repokitteh-read-only bot added api deps Approval required for changes to Envoy's external dependencies and removed waiting labels May 21, 2021

repokitteh-read-only bot assigned htuch May 21, 2021

This was referenced May 24, 2021

[fuzz]: fix oss fuzz 30088, envoy:stat_merger_fuzz_test: ASSERT: name = #16636

Merged

[fuzz]: fix oss fuzz 22106, envoy:codec_impl_fuzz_test: Crash in #16616

Merged

phlax closed this May 24, 2021

Conversation

twghu commented May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented May 10, 2021

Uh oh!

twghu commented May 10, 2021

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

antoniovicente May 11, 2021

Choose a reason for hiding this comment

Uh oh!

jmarantz May 11, 2021

Choose a reason for hiding this comment

Uh oh!

twghu May 11, 2021

Choose a reason for hiding this comment

Uh oh!

antoniovicente May 11, 2021

Choose a reason for hiding this comment

Uh oh!

twghu May 11, 2021

Choose a reason for hiding this comment

Uh oh!

twghu May 11, 2021

Choose a reason for hiding this comment

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

jmarantz commented May 12, 2021

Uh oh!

repokitteh-read-only bot commented May 12, 2021

Uh oh!

repokitteh-read-only bot commented May 21, 2021

Uh oh!

phlax commented May 21, 2021

Uh oh!

twghu commented May 21, 2021

Uh oh!

phlax commented May 21, 2021

Uh oh!

cpakulski commented May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

twghu commented May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch commented May 23, 2021

Uh oh!

twghu commented May 24, 2021

Uh oh!

phlax commented May 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

twghu commented May 10, 2021 •

edited

Loading

cpakulski commented May 21, 2021 •

edited

Loading

twghu commented May 21, 2021 •

edited

Loading