Skip to content

Apply depth limit to unknown groups#756

Merged
stepancheg merged 7 commits into
stepancheg:masterfrom
esrauchg:master
Mar 9, 2025
Merged

Apply depth limit to unknown groups#756
stepancheg merged 7 commits into
stepancheg:masterfrom
esrauchg:master

Conversation

@esrauchg

@esrauchg esrauchg commented Mar 8, 2025

Copy link
Copy Markdown
Contributor

This avoids the denial of service issue as described on:
#749
https://rustsec.org/advisories/RUSTSEC-2024-0437

@esrauchg esrauchg mentioned this pull request Mar 8, 2025
@esrauchg esrauchg changed the title Apply depth limit to unknown groupv Apply depth limit to unknown groups Mar 8, 2025
@stepancheg

Copy link
Copy Markdown
Owner

Should not be hard to fix CI #757

@stepancheg

Copy link
Copy Markdown
Owner

Can you rebase and resubmit please?

@esrauchg

esrauchg commented Mar 8, 2025

Copy link
Copy Markdown
Contributor Author

Done; CI is passing now.

Please take another look, thanks!

Comment thread protobuf/src/coded_input_stream/mod.rs Outdated
@esrauchg

esrauchg commented Mar 9, 2025

Copy link
Copy Markdown
Contributor Author

Fixed the tests, just let me know if there's anything else you'd like changed. Thanks!

@stepancheg stepancheg merged commit f06992f into stepancheg:master Mar 9, 2025
stepancheg pushed a commit that referenced this pull request Mar 9, 2025
* Fix issue where a deeply nested unknown group could cause arbitrarily recursion depth.

* Add drop(os) to fix tests

* Check err message on recursion limit exceeded.

* Run formatter

* Fix test with .unwrap_err()
@stepancheg

Copy link
Copy Markdown
Owner

Thank you!

Version v3.7.2 should be published tomorrow or so.

@abernix

abernix commented Mar 10, 2025

Copy link
Copy Markdown

Thanks for the quick action on this. I know folks — myself included — appreciate it. 🙇

@esrauchg Do you think your fix here could be applied to https://github.com/stepancheg/rust-protobuf/tree/v2.28.0? (It doesn't appear as if it would apply cleanly as-is.) And @stepancheg would you entertain a 2.x release for this if the contribution could be made? (The README suggests critical fixes may be accepted, but want to make sure that's still true.)

Reason-being: My hunch/observation is that some high-download packages (like prometheus and thus transitively opentelemetry-prometheus) are still dependent on protobuf 2.x and may not be able to swiftly update to 3.x.

While I realize that's not a predicament you want to be in and those packages should figure out a path to 3.x (I'm just a messenger here, but I can try to assist!), I suspect the availability of a 2.x would more quickly resolve the impact of the vulnerability across the ecosystem. I know it would for my team, who are currently looking at the "dependency of a dependency" gridlock without much of an option on how to proceed. This 2.x backport would unblock that.

@esrauchg

Copy link
Copy Markdown
Contributor Author

The exact same patch couldn't be applied because things files shifted bit, but it appears the same fix can be done on the 2.28 branch the same issue could be mitigated there by a is.incr_recursion()? / .decr_recursion(); before/after this line:
https://github.com/stepancheg/rust-protobuf/blob/v2.28.0/protobuf/src/rt.rs#L806

(Whether the 2.x branch is considered EOL for those kind of patches, I wouldn't know)

@pjenvey

pjenvey commented Mar 11, 2025

Copy link
Copy Markdown

I've attempted the same fix against 2.28.0 here:

  • test_shallow_nested_unknown_groups passes w/ the additional skip_group !is.eof()? change
  • test_deeply_nested_unknown_groups fails to trigger a skip_group failure, seemingly due to read_unknown not recursively calling skip_group as done on master

This may suggest the unchecked recursion doesn't apply to the skip_group case (and thus the tests don't really apply) but I'm flying blind here

@stepancheg

stepancheg commented Mar 11, 2025

Copy link
Copy Markdown
Owner

@abernix

those packages should figure out a path to 3.x

They should be migrating to protobuf version 4, which is currently in beta This version of protobuf crate is on the way to archive.

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.

4 participants