Opened 2 months ago

Closed 5 days ago

#27740 closed defect (fixed)

rust protover_all_supported() returns rust-allocated string in *missing_out

Reported by: cyberpunks Owned by: ahf
Priority: Very High Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version: Tor: 0.3.3.1-alpha
Severity: Normal Keywords: rust, protover, memory-safety, 035-must, fast-fix, 033-backport, 034-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: catalyst Sponsor:

Description


Child Tickets

TicketStatusOwnerSummaryComponent
#27803closedrust protover_all_supported() double-free of *missing_outCore Tor/Tor
#27804closedahfrust protover_all_supported() NULL pointer dereferenceCore Tor/Tor

Change History (20)

comment:1 Changed 2 months ago by dgoulet

Milestone: Tor: unspecified

comment:2 in reply to:  description Changed 2 months ago by cyberpunks

This needs the keywords rust, protover, memory-safety. You can't tor_free() a string allocated by rust code, which is what the C code will immediately try to do.

comment:3 Changed 2 months ago by teor

Keywords: rust protover memory-safety 035-must fast-fix added
Milestone: Tor: unspecifiedTor: 0.3.5.x-final

Thanks for this bug report.

As far as I understand it, it may be ok to allocate in Rust and deallocate in C, as long as they use the same allocator. But, this behaviour is not guaranteed to be safe in future Rust releases:
https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandardsRust.md#n365

But even if allocating in Rust and freeing in C was safe, this function is also memory unsafe because:

  • *missing_out is allocated in Rust, deallocated in Rust (when the function returns), used in C, and then freed in C
  • when missing_out is NULL, Rust still assigns to it

I'll open child tickets for these issues.

comment:4 Changed 2 months ago by teor

Keywords: 033-backport 034-backport added

comment:5 in reply to:  3 ; Changed 2 months ago by cyberpunks

Replying to teor:

As far as I understand it, it may be ok to allocate in Rust and deallocate in C, as long as they use the same allocator.

Oh right. Of course currently, they don't.

deallocated in Rust (when the function returns),

It actually isn't.

  • when missing_out is NULL, Rust still assigns to it

You're right. Nice catch!

Last edited 2 months ago by cyberpunks (previous) (diff)

comment:6 Changed 2 months ago by cyberpunks

Rust uses jemalloc to allocate memory by default. No CI has been failing even though test_protover_all_supported calls tor_free() on this jemalloc-allocated pointer though. Is it missing hardening that could catch similar issues in the future?

comment:7 in reply to:  5 Changed 2 months ago by teor

Replying to cyberpunks:

Replying to teor:

As far as I understand it, it may be ok to allocate in Rust and deallocate in C, as long as they use the same allocator.

Oh right. Of course currently, they don't.

We have allocate_and_copy_string() in Rust, which uses tor_malloc():
https://github.com/torproject/tor/blob/master/src/rust/tor_allocate/tor_allocate.rs#L30

We need to update our RustCodingStandards.md to mention allocate_and_copy_string():
https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandardsRust.md#n374

I'll open another ticket.

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

Replying to cyberpunks:

Rust uses jemalloc to allocate memory by default. No CI has been failing even though test_protover_all_supported calls tor_free() on this jemalloc-allocated pointer though. Is it missing hardening that could catch similar issues in the future?

ASAN would probably help, but it doesn't work with jemalloc, see #27272.

comment:10 Changed 7 weeks ago by nickm

Priority: MediumVery High

Mark all 035-must tickets as "very high"

comment:11 Changed 6 weeks ago by nickm

Owner: set to ahf
Status: newassigned

Assign a couple of quick and easy rust tickets

comment:12 Changed 5 weeks ago by teor

Status: assignedneeds_review

I think this has a patch, so it needs review?

comment:13 Changed 4 weeks ago by dgoulet

Reviewer: nickm

comment:14 Changed 4 weeks ago by nickm

Merged, with a conflict resolved in 289a7dbac32a981897e12a3c250f0b6c67eec809. (Does that look okay?)

comment:15 in reply to:  14 Changed 4 weeks ago by cyberpunks

Replying to nickm:

(Does that look okay?)

No, that merge completely undid the whole diff in 42558df7c8affeec33e66d987ccf4d632a9d5466 that fixed this bug. It's right back to creating a Rust-allocated CString and then calling CString::into_raw() which as the commit summary notes is not safe.

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

comment:16 Changed 3 weeks ago by nickm

I've tried to restore the fix in bug27740_035_fix; PR at https://github.com/torproject/tor/pull/458 . How does that look?

comment:17 Changed 11 days ago by nickm

Reviewer: nickm

comment:18 Changed 8 days ago by asn

Reviewer: catalyst

comment:19 Changed 5 days ago by catalyst

Status: needs_reviewmerge_ready

Looks good to me!

comment:20 Changed 5 days ago by nickm

Resolution: fixed
Status: merge_readyclosed

Thanks; merged it!

Note: See TracTickets for help on using tickets.