too many arguments in rust protover_compute_vote()
569b4e57e23d728969a12751afc6b45f32d0f093 added an argument allow_long_proto_names: bool in order to maintain compatibility with consensus methods older than 29.
The C code never added this 3rd argument and only calls it with 2, which can't be safe.
Trac:
Username: cyberpunks
Activity
changed milestone to %Tor: 0.3.5.x-final
There is no consensus method 29: https://gitweb.torproject.org/torspec.git/tree/dir-spec.txt#n2877 https://github.com/torproject/tor/blob/master/src/feature/dirauth/dirvote.h#L78
Instead, we decided to unconditionally reject relay descriptors, votes, and consensuses containing long protocol names.
It looks like we merged an old version of the Rust fix. It is possible that we updated the C fix to unconditionally reject bad documents, but never updated the Rust fix to match.
The C code never added this 3rd argument and only calls it with 2, which can't be safe.
In most calling conventions, Rust will read a register for the 3rd argument, but C hasn't initialised that register. Then the arbitrary (or uninitialised) value read from the register will be interpreted as a boolean.
This could cause a crash due to a register poison exception on some platforms. But on x86_64, I think will will just result in an arbitrary choice between validated and unvalidated.
We should fix this issue in 0.3.5, and backport.
Trac:
Milestone: N/A to Tor: 0.3.5.x-final
Keywords: N/A deleted, memory-safety, 035-must, protover addedTrac:
Parent: N/A to #27206 (moved)Trac:
Parent: #27206 (moved) to #27739 (moved)
How do you like my branch
bug27741_033, with PR at https://github.com/torproject/tor/pull/346 ?Trac:
Status: accepted to needs_review
Replying to nickm:
You probably shouldn't remove the trailing comma after the last argument (
threshold: c_int,), pretty sure rustfmt would add it back.Trac:
Username: cyberpunks-
If #27273 (moved) were fixed and there were rust + asan CI runs, would asan have probably caught this?
Trac:
Username: cyberpunks Replying to cyberpunks:
If #27273 (moved) were fixed and there were rust + asan CI runs, would asan have probably caught this?
Probably not:
- ASAN monitors RAM accesses, and the first few arguments are almost always passed in registers
- ASAN can only find out of bounds accesses past a certain number of bytes. 1 byte or 1 word might not be far enough.
Here are some things that probably would have caught the bug:
- using automated Rust to C function prototype generation (I'm sure we have a ticket for this, but I can't find it right now)
- C to Rust unit tests (maybe depends on #25386 (moved)?), if the values in the uninitialised register weren't always zero, or if the architecture poisons uninitialised registers
- fuzzing C against Rust (#27229 (moved)), if the values in the uninitialised register weren't always zero, or if the architecture poisons uninitialised registers
-
Replying to teor:
- C to Rust unit tests (maybe depends on #25386 (moved)?), if the values in the uninitialised register weren't always zero, or if the architecture poisons uninitialised registers
We already have C unit tests for this function and they run against the Rust implementation in CI, don't they?
- fuzzing C against Rust (#27229 (moved)), if the values in the uninitialised register weren't always zero, or if the architecture poisons uninitialised registers
Wouldn't the C fuzzing code need to know about the 3rd argument in order to fuzz it, and not knowing it's in the Rust version of the function signature is the problem?
But wait, MSan could probably catch this use of an uninitialized value.
Trac:
Username: cyberpunks Replying to cyberpunks:
Replying to teor:
- C to Rust unit tests (maybe depends on #25386 (moved)?), if the values in the uninitialised register weren't always zero, or if the architecture poisons uninitialised registers
We already have C unit tests for this function and they run against the Rust implementation in CI, don't they?
We do, so:
- the architectures we're running on don't poison uninitialised registers, and
- either the value in the uninitialised register is always the same, or the unit test results don't change based on that value.
- fuzzing C against Rust (#27229 (moved)), if the values in the uninitialised register weren't always zero, or if the architecture poisons uninitialised registers
Wouldn't the C fuzzing code need to know about the 3rd argument in order to fuzz it, and not knowing it's in the Rust version of the function signature is the problem?
No, not necessarily: if the value of the 3rd argument changes arbitrarily, a fuzzer would identify differences between the Rust and C implementation.
But wait, MSan could probably catch this use of an uninitialized value.
I think I tried MSan once. We would probably take a patch to add MSan as an option to configure.
-
rustfmt actually says the
bug27741_033branch should put the function arguments all in one line, since they fit within 100 characters now.+pub extern "C" fn protover_compute_vote(list: *const Stringlist, threshold: c_int) -> *mut c_char {Also that there shouldn't be a double space in the match statement
Ok(n) => n,and there should be a comma aftercontinue,.- Ok(n) => n, - Err(_) => continue + Ok(n) => n, + Err(_) => continue,Trac:
Username: cyberpunks Replying to cyberpunks:
rustfmt actually says the
bug27741_033branch should put the function arguments all in one line, since they fit within 100 characters now.We only applied rustfmt in 0.3.5, so I made a separate branch for 0.3.5 and later. See
bug27741_035on https://github.com/teor2345/tor.gitGitHub is having issues, so I can't open a pull request: https://status.github.com/messages
Trac:
Status: needs_revision to needs_reviewThe 0.3.3 pull request is: https://github.com/torproject/tor/pull/346
Replying to teor:
Replying to cyberpunks:
rustfmt actually says the
bug27741_033branch should put the function arguments all in one line, since they fit within 100 characters now.We only applied rustfmt in 0.3.5, so I made a separate branch for 0.3.5 and later. See
bug27741_035on https://github.com/teor2345/tor.gitGitHub is having issues, so I can't open a pull request: https://status.github.com/messages
The 0.3.5 pull request is: https://github.com/torproject/tor/pull/431
-
Replying to teor:
We only applied rustfmt in 0.3.5,
That's true. Though we might as well try to follow the rustfmt rules for the commits merged to maint-0.3.3 too, right? If we introduce new style differences between the branches that weren't there before, future fixes to adjacent code will have unnecessary merge conflicts.
Also, this ticket is missing the
rusttag.Trac:
Username: cyberpunks Replying to cyberpunks:
Replying to teor:
We only applied rustfmt in 0.3.5,
That's true. Though we might as well try to follow the rustfmt rules for the commits merged to maint-0.3.3 too, right? If we introduce new style differences between the branches that weren't there before, future fixes to adjacent code will have unnecessary merge conflicts.
It's nickm's code, so I'm going to let him decide if he wants to copy the rustfmt changes from 0.3.5 to 0.3.3.
Also, this ticket is missing the
rusttag.Fixed!
nickm, if you're happy with the branches as-is, please merge:
- https://github.com/torproject/tor/pull/346 to 0.3.3 and later
- https://github.com/torproject/tor/pull/431 to 0.3.5 and later
If you decide to copy the rustfmt changes from 0.3.5 to 0.3.3, I can do a quick review.
Trac:
Status: needs_review to needs_revisionAlso, appveyor failed to run on https://github.com/torproject/tor/pull/346 , so I triggered it again.
It was probably a consequence of the GitHub downtime. But we should watch out for that in future.
mentioned in issue #28081 (moved)
moved to tpo/core/tor#27741 (closed)
