Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

repo: Assorted CVE fixes (IMPORTANT!) #32300

Merged
merged 5 commits into from
Feb 9, 2024
Merged

Conversation

phlax
Copy link
Member

@phlax phlax commented Feb 9, 2024

Fix crash from AWS NLB healthchecks when proxy protocol is enabled
Fix: CVE-2024-23327

Cache RE object in uri template matcher.
Fix CVE-2024-23323

Fix crash when idle and request per try timeouts occur within backoff interval
Fix CVE-2024-23322

Fix crash when proxy protocol receives an address type that isn't supported by the operating system
Fix CVE-2024-23325

Proxy protocol: sanitise non utf8 chars in TLVs
Fix CVE-2024-23324

jacobneiltaylor and others added 5 commits February 9, 2024 14:30
Fix: [CVE-2024-23327](GHSA-4h5x-x9vh-m29j)

Signed-off-by: Jacob Neil Taylor <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Ryan Northey <[email protected]>
Fix [CVE-2024-23323](GHSA-x278-4w4x-r7ch)

Signed-off-by: Yan Avlasov <[email protected]>
Signed-off-by: Ryan Northey <[email protected]>
… interval

Fix [CVE-2024-23322](GHSA-6p83-mfmh-qv38)

Signed-off-by: Yan Avlasov <[email protected]>
Signed-off-by: Ryan Northey <[email protected]>

Signed-off-by: yanavlasov <[email protected]>
supported by the operating system

Fix [CVE-2024-23325](GHSA-5m7c-mrwr-pm26)

Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Ryan Northey <[email protected]>
Fix [CVE-2024-23324](GHSA-gq3v-vvhj-96j6)

Signed-off-by: Kateryna Nezdolii <[email protected]>
Signed-off-by: Ryan Northey <[email protected]>
Copy link

CC @envoyproxy/coverage-shephards: FYI only for changes made to (test/per_file_coverage.sh).
envoyproxy/coverage-shephards assignee is @RyanTheOptimist

🐱

Caused by: #32300 was opened by phlax.

see: more, trace.

@phlax phlax enabled auto-merge (rebase) February 9, 2024 14:48
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update the PR description to link to the CVEs that this addresses?

@phlax
Copy link
Member Author

phlax commented Feb 9, 2024

Can we update the PR description to link to the CVEs that this addresses?

i think we have been here before 8/

this PR will be rebased, the commits contain the linked CVEs

this is what needs checking @RyanTheOptimist if you could look through that would be appreciated

@RyanTheOptimist
Copy link
Contributor

Can we update the PR description to link to the CVEs that this addresses?

i think we have been here before 8/

this PR will be rebased, the commits contain the linked CVEs

this is what needs checking @RyanTheOptimist if you could look through that would be appreciated

yes, i agree that the code changes look good. It would still be good to copy the links to the CVEs into the PR description so that when we see the commit in git history it has the relevant context.

@phlax
Copy link
Member Author

phlax commented Feb 9, 2024

i already have to copy this information to each commit on each branch - ie 20+ and also to the release summaries

given that the information is there and needs to be checked ill leave any further copypasta as an exercise for the reader

@RyanTheOptimist
Copy link
Contributor

RyanTheOptimist commented Feb 9, 2024

I do not believe that "Assorted CVE fixes (IMPORTANT!)" meeds the requirements of a PR description from CONTRIBUTING.md. Feel free to get a second opinion from another maintainer.

* Your PR description should have details on what the PR does. If it fixes an existing issue it
  should end with "Fixes #XXX".

@phlax
Copy link
Member Author

phlax commented Feb 9, 2024

comment updated

@RyanTheOptimist RyanTheOptimist enabled auto-merge (squash) February 9, 2024 15:27
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revised the PR description.

@phlax phlax disabled auto-merge February 9, 2024 15:29
@phlax phlax enabled auto-merge (rebase) February 9, 2024 15:29
@phlax phlax merged commit 29989f6 into envoyproxy:main Feb 9, 2024
53 of 54 checks passed
@fedya-at-db
Copy link

I believe the commit to sanitize the proxy protocol values seems wrong. The sanitizeUtf8String function used modifies the bytes in a non-recoverable way, but the actual value isn't always meant to be a valid utf8 string. I believe the proxy protocol spec allows for this.

Materially, Google's private service connect (PSC) feature encodes a TLV value as a binary encoded int64. With this commit there's no way to recover the original value in most cases. Example value bytes that should be a valid PSC id
00 22 6b cf 0a 00 00 02 (decodes to 9688686178336770)

I think either using set_bytes within ProtobufWkt::Value (which doesn't seem to exist today) or escaping the bad bytes in a reverseable way would be necessary.

@ggreenway
Copy link
Contributor

I think this happened because Envoy is using a protobuf string for both moving data around internally, and sending protobufs flattened over the network. I agree this is an unintended change in behavior. I'm not sure what the best way to fix this is. Can you open an issue for this @fedya-at-db ?

@fedya-at-db
Copy link

Thanks, I filed an issue here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants