Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MapGuard dereferences to a dangling pointer #45

Closed
Qwaz opened this issue Dec 10, 2020 · 5 comments
Closed

MapGuard dereferences to a dangling pointer #45

Qwaz opened this issue Dec 10, 2020 · 5 comments

Comments

@Qwaz
Copy link

@Qwaz Qwaz commented Dec 10, 2020

Hello fellow Rustacean,
we (Rust group @sslab-gatech) are scanning Rust code on crates.io for potential memory safety and soundness bugs and found an issue in this crate which allows safe Rust code to exhibit an undefined behavior.

Issue Description

arc-swap/src/access.rs

Lines 279 to 293 in b5ec44c

impl<A, T, F, R> Access<R> for Map<A, T, F>
where
A: Access<T>,
F: Fn(&T) -> &R,
{
type Guard = MapGuard<A::Guard, R>;
fn load(&self) -> Self::Guard {
let guard = self.access.load();
let value: *const _ = (self.projection)(&guard);
MapGuard {
_guard: guard,
value,
}
}
}

arc-swap/src/access.rs

Lines 230 to 242 in b5ec44c

impl<G, T> Deref for MapGuard<G, T> {
type Target = T;
fn deref(&self) -> &T {
// Why this is safe:
// * The pointer is originally converted from a reference. It's not null, it's aligned,
// it's the right type, etc.
// * The pointee couldn't have gone away ‒ the guard keeps the original reference alive, so
// must the new one still be alive too. Moving the guard is fine, we assume the RefCnt is
// Pin (because it's Arc or Rc or something like that ‒ when that one moves, the data it
// points to stay at the same place).
unsafe { &*self.value }
}
}

As noted in the comment, unsafe code in MapGuard expects the underlying guard type to dereferences to the same value even when the guard object is moved. However, Map uses Access as a trait bound which does not guarantee this property. As a result, Map accesses a dangling pointer when it is used with an Access implementation that does not dereferences to the same value when moved.

arc-swap/src/access.rs

Lines 295 to 322 in b5ec44c

#[doc(hidden)]
#[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)]
pub struct ConstantDeref<T>(T);
impl<T> Deref for ConstantDeref<T> {
type Target = T;
fn deref(&self) -> &T {
&self.0
}
}
/// Access to an constant.
///
/// This wraps a constant value to provide [`Access`] to it. It is constant in the sense that,
/// unlike [`ArcSwapAny`] and [`Map`], the loaded value will always stay the same (there's no
/// remote `store`).
///
/// The purpose is mostly testing and plugging a parameter that works generically from code that
/// doesn't need the updating functionality.
#[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)]
pub struct Constant<T>(pub T);
impl<T: Clone> Access<T> for Constant<T> {
type Guard = ConstantDeref<T>;
fn load(&self) -> Self::Guard {
ConstantDeref(self.0.clone())
}
}

Constant seems to be the only type in this crate that implements Access in this way, but there can be other user types that implements Access on their own.

Reproduction

Below is an example program that segfaults, written only with safe APIs of arc-swap.

Show Detail

#![forbid(unsafe_code)]

use arc_swap::access::Map;
use arc_swap::access::{Access, Constant};

static CORRECT_ADDR: &str = "I'm pointing to the correct location!";

#[derive(Clone)]
struct MemoryChecker {
    // should be always CORRECT_ADDR
    message: &'static str,
}

impl MemoryChecker {
    pub fn new() -> Self {
        MemoryChecker {
            message: CORRECT_ADDR,
        }
    }

    pub fn validate(&self) {
        println!(
            "Pointing to {:?}, len {}",
            self.message as *const _,
            self.message.len()
        );
        println!("Message: {}", self.message);
    }
}

fn overwrite() {
    let a = 123;
    let b = 456;
    println!("Overwriting stack content {} {}", a, b);
}

fn main() {
    let constant = Constant(MemoryChecker::new());
    constant.0.validate();

    // Create a map with identity mapping
    let map = Map::new(constant, |checker: &MemoryChecker| checker);

    // After calling this, `value` pointer of `MapGuard` points to a dangling stack address
    let loaded = map.load();

    overwrite();

    loaded.validate();
}

Output:

Pointing to 0x55dcc9269000, len 37
Message: I'm pointing to the correct location!
Overwriting stack content 123 456
Pointing to 0x55dcc9267530, len 140726361573556

Terminated with signal 11 (SIGSEGV)

Tested Environment

  • Crate: arc-swap
  • Version: 1.0.0
  • OS: Ubuntu 20.04.1 LTS
  • Rustc version: rustc 1.48.0 (7eac88abb 2020-11-16)

@vorner
Copy link
Owner

@vorner vorner commented Dec 10, 2020

Hello

It's a great discovery, though disturbing this has slipped through. I'll have to think how to plug this problem in a way that doesn't disrupt users too much, but I'll have a look at it today or tomorrow.

Thank you for finding it.

vorner added a commit that referenced this issue Dec 10, 2020
The assumption that the address of access's guarded reference stays the
same is not true.

Costs:
* The Map is now slower and adds an allocation.
* It no longer can be Clone (but non-trivial guards weren't anyway) and
  it can stop being Sync/Send if the closure is not.

Fixes #45.

Technically, it is a breaking change, but the plan is not to raise major
version, because:

* Even rust std gives exception to break compatibility for soundness
  hole fixes.
* It is not that likely people's code would break.
* Even if it breaks, they are much more likely to go to the fixed
  version then if the version got bumped and that's what they should be
  doing ASAP due to the potential UB.
@vorner vorner closed this as completed in dfeb84b Dec 11, 2020
@vorner
Copy link
Owner

@vorner vorner commented Dec 11, 2020

Fix released as 1.1.0 (by eliminating all unsafe code in that file).

It is technically a breaking change, but the chance of actually breaking code is low and the chance people will migrate from the broken version are higher this way, and even rustc makes an exception for soundness issues in the stability guarantees. It seems less bad to release as part of the 1. version.

vorner added a commit that referenced this issue Dec 11, 2020
Backport to the 0.4 version, releasing as 0.4.8.

The assumption that the address of access's guarded reference stays the
same is not true.

Costs:
* The Map is now slower and adds an allocation.
* It can stop being Copy (but non-trivial guards weren't anyway) and
  it can stop being Sync/Send if the closure is not.
* The taken closure needs to be Clone.

Fixes #45.

Technically, it is a breaking change, but the plan is not to raise major
version, because:

* Even rust std gives exception to break compatibility for soundness
  hole fixes.
* It is not that likely people's code would break.
* Even if it breaks, they are much more likely to go to the fixed
  version then if the version got bumped and that's what they should be
  doing ASAP due to the potential UB.
@vorner
Copy link
Owner

@vorner vorner commented Dec 11, 2020

Added a backport, released as 0.4.8, for all the reverse dependencies that didn't migrate to the 1. version yet.

@Qwaz
Copy link
Author

@Qwaz Qwaz commented Dec 12, 2020

Thank you for the quick fix!

@abergmann
Copy link

@abergmann abergmann commented Dec 28, 2020

CVE-2020-35711 was assigned to this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants