Opened 2 months ago

Closed 4 weeks ago

#27206 closed defect (fixed)

rust protover_all_supported() takes forever on some inputs

Reported by: cyberpunks Owned by:
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: rust, protover, 033-backport, 034-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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

Child Tickets

Change History (14)

comment:1 Changed 2 months ago by cyberpunks

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

comment:2 Changed 2 months ago by nickm

Milestone: Tor: 0.3.5.x-final
Status: newneeds_review

comment:3 Changed 2 months ago by teor

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

And we added a 216 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.

comment:4 Changed 7 weeks ago by chelseakomlo

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?

Last edited 7 weeks ago by chelseakomlo (previous) (diff)

comment:5 Changed 7 weeks ago by chelseakomlo

Status: needs_reviewneeds_information

comment:6 in reply to:  4 Changed 7 weeks ago by cyberpunks

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.

comment:7 in reply to:  5 Changed 6 weeks ago by cyberpunks

Does this still being needs_information?

comment:8 Changed 6 weeks ago by teor

Status: needs_informationneeds_review

comment:9 in reply to:  3 ; Changed 5 weeks ago by 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.

comment:10 in reply to:  9 ; Changed 4 weeks ago by teor

Status: needs_reviewneeds_revision

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

comment:11 in reply to:  10 ; Changed 4 weeks ago by 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.

Last edited 4 weeks ago by cyberpunks (previous) (diff)

comment:12 in reply to:  11 Changed 4 weeks ago by teor

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, but we can do it now, and backport.

comment:13 Changed 4 weeks ago by teor

Status: needs_revisionmerge_ready

Ok, in #27739 and #27741, 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.

comment:14 Changed 4 weeks ago by nickm

Resolution: fixed
Status: merge_readyclosed

merged to 0.3.3 and forward

Note: See TracTickets for help on using tickets.