rust protover_all_supported() takes forever on some inputs

It calls ProtoSet::retain() to manually check whether each individual version is supported. Potentially checking over 4 billion of them.

Trac:
Username: cyberpunks

changed milestone to %Tor: 0.3.5.x-final

added 033-backport 034-backport component::core tor/tor milestone::Tor: 0.3.5.x-final priority::medium protover reporter::cyberpunks resolution::fixed rust severity::normal status::closed type::defect labels

See branch rust-allsupported1 at ​​​​​​https://gitgud.io/onionk/tor.git

Trac:
Username: cyberpunks

Trac:
Milestone: N/A to Tor: 0.3.5.x-final
Status: new to needs_review

Oh, I thought we fixed all these DoS bugs in #24031 (moved) and #25250 (moved).

And we added a 2^16^ limit for protocol expansion before rejecting the descriptor, vote, or consensus: https://github.com/torproject/tor/blob/master/src/rust/protover/protover.rs#L29 https://github.com/torproject/tor/blob/master/src/core/or/protover.c#L451

So I don't think this is security-low, just slightly inefficient.

Thanks for the patch, we can definitely be more efficient here.

Can you give more info about the reasoning for the test case changes in commit 4288d755? Does this change pass the tests as they were originally written, or was the reasoning that these test cases are not representative of what should be tested?

Trac:
Status: needs_review to needs_information

Replying to chelseakomlo:

Can you give more info about the reasoning for the test case changes

The old version of the tests didn't hit enough codepaths, since the protocol name was entirely unknown it would bail out early and never reach the part where it had to call .retain(). So now they test that it doesn't hang nearly forever.

Trac:
Username: cyberpunks

Does this still being needs_information?

Trac:
Username: cyberpunks

Trac:
Status: needs_information to needs_review

Replying to teor:

So I don't think this is security-low, just slightly inefficient.

It creates a temporary Vec containing up to 4 billion integers, actually, in order to call .retain() on it. The updated tests try to use massive amounts of RAM without this change.

Trac:
Username: cyberpunks

Replying to cyberpunks:

Replying to teor:

So I don't think this is security-low, just slightly inefficient.

It creates a temporary Vec containing up to 4 billion integers, actually, in order to call .retain() on it. The updated tests try to use massive amounts of RAM without this change.

Ah, thanks! That was the information I needed to know.

If this change fixes (enough) rust inefficiencies, we should be able to re-enable this test as well: https://github.com/torproject/tor/blob/991bec67ee41fd7f3c12e9194d96491b51bedd50/src/test/test_protover.c#L327

Trac:
Status: needs_review to needs_revision

Replying to teor:

If this change fixes (enough) rust inefficiencies, we should be able to re-enable this test as well:

Did nickm disable that test on rust builds for efficiency reasons? It would only allocate 256KiB.

It looks like it's disabled because the assert fails.

Trac:
Username: cyberpunks

Replying to cyberpunks:

Replying to teor:

If this change fixes (enough) rust inefficiencies, we should be able to re-enable this test as well:

Did nickm disable that test on rust builds for efficiency reasons? It would only allocate 256KiB.

It looks like it's disabled because the assert fails.

We need to fix the Rust code to behave the same as the C code. We should have done that back in #25517 (moved), but we can do it now, and backport.

Ok, in #27739 (moved) and #27741 (moved), let's fix the rust code, remove the extra argument, re-enable the test, and backport to 0.3.3.

We can merge this branch, and then work on the other changes in a separate branch.

Trac:
Status: needs_revision to merge_ready

merged to 0.3.3 and forward

Trac:
Resolution: N/A to fixed
Status: merge_ready to closed

closed

mentioned in issue #27739 (moved)

mentioned in issue #27741 (moved)

moved to tpo/core/tor#27206 (closed)