Opened 2 months ago

Closed 3 weeks ago

#27741 closed defect (fixed)

too many arguments in rust protover_compute_vote()

Reported by: cyberpunks Owned by: nickm
Priority: Very High Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version: Tor: 0.3.3.6
Severity: Normal Keywords: 035-must, protover, memory-safety, 033-backport, 034-backport
Cc: Actual Points:
Parent ID: #27739 Points:
Reviewer: teor Sponsor:

Description

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.

Child Tickets

Change History (21)

comment:1 Changed 2 months ago by teor

Keywords: 035-must protover memory-safety added
Milestone: 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.

comment:2 Changed 2 months ago by teor

Parent ID: #27206

comment:3 Changed 2 months ago by teor

Keywords: 033-backport 034-backport added

comment:4 Changed 2 months ago by teor

Parent ID: #27206#27739

comment:5 Changed 2 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:6 Changed 2 months ago by nickm

Status: acceptedneeds_review

How do you like my branch bug27741_033, with PR at https://github.com/torproject/tor/pull/346 ?

comment:7 in reply to:  6 ; Changed 2 months ago by cyberpunks

Replying to nickm:

https://github.com/torproject/tor/pull/346 ?

You probably shouldn't remove the trailing comma after the last argument (threshold: c_int,), pretty sure rustfmt would add it back.

comment:8 in reply to:  1 ; Changed 2 months ago by cyberpunks

If #27273 were fixed and there were rust + asan CI runs, would asan have probably caught this?

comment:9 in reply to:  8 ; Changed 2 months ago by teor

Replying to cyberpunks:

If #27273 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?), if the values in the uninitialised register weren't always zero, or if the architecture poisons uninitialised registers
  • fuzzing C against Rust (#27229), if the values in the uninitialised register weren't always zero, or if the architecture poisons uninitialised registers

comment:10 in reply to:  9 ; Changed 2 months ago by cyberpunks

Replying to teor:

  • C to Rust unit tests (maybe depends on #25386?), 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), 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.

comment:11 in reply to:  10 Changed 2 months ago by teor

Replying to cyberpunks:

Replying to teor:

  • C to Rust unit tests (maybe depends on #25386?), 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), 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.

comment:12 Changed 2 months ago by dgoulet

Reviewer: teor

comment:13 Changed 7 weeks ago by nickm

Priority: MediumVery High

Mark all 035-must tickets as "very high"

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

rustfmt actually says the bug27741_033 branch 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 after continue,.

-            Ok(n)  => n,
-            Err(_) => continue
+            Ok(n) => n,
+            Err(_) => continue,

comment:15 Changed 5 weeks ago by teor

Status: needs_reviewneeds_revision

Let's revise for rustfmt

comment:16 in reply to:  14 ; Changed 4 weeks ago by teor

Status: needs_revisionneeds_review

Replying to cyberpunks:

rustfmt actually says the bug27741_033 branch 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_035 on https://github.com/teor2345/tor.git

GitHub is having issues, so I can't open a pull request:
https://status.github.com/messages

comment:17 in reply to:  16 Changed 4 weeks ago by teor

The 0.3.3 pull request is:
https://github.com/torproject/tor/pull/346

Replying to teor:

Replying to cyberpunks:

rustfmt actually says the bug27741_033 branch 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_035 on https://github.com/teor2345/tor.git

GitHub 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

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

Also, this ticket is missing the rust tag.

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

comment:19 in reply to:  18 Changed 3 weeks ago by teor

Status: needs_reviewneeds_revision

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 rust tag.

Fixed!

nickm, if you're happy with the branches as-is, please merge:

If you decide to copy the rustfmt changes from 0.3.5 to 0.3.3, I can do a quick review.

comment:20 Changed 3 weeks ago by teor

Also, 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.

comment:21 Changed 3 weeks ago by nickm

Resolution: fixed
Status: needs_revisionclosed

I don't think it's necessary to backport the rustfmt changes. Merged as suggested above.

Note: See TracTickets for help on using tickets.