Unverified Commit c81d3498 authored by Steven Fackler's avatar Steven Fackler Committed by GitHub
Browse files

Merge pull request #1660 from sfackler/oss3-error-strs

Fix use after free in 3.x error reporting
parents 047fb44e 4d218d1f
Loading
Loading
Loading
Loading
+2 −4
Original line number Diff line number Diff line
@@ -82,15 +82,13 @@ jobs:
  windows-vcpkg:
    name: windows-vcpkg
    runs-on: windows-latest
    env:
      VCPKGRS_DYNAMIC: 1
    steps:
      - uses: actions/checkout@v2
      - uses: sfackler/actions/rustup@master
      - run: echo "::set-output name=version::$(rustc --version)"
        id: rust-version
      - run: echo "VCPKG_ROOT=$env:VCPKG_INSTALLATION_ROOT" | Out-File -FilePath $env:GITHUB_ENV -Append
      - run: vcpkg install openssl:x64-windows
      - run: vcpkg install openssl:x64-windows-static-md
      - uses: actions/cache@v1
        with:
          path: ~/.cargo/registry/index
@@ -107,7 +105,7 @@ jobs:
        with:
          path: target
          key: target-${{ github.job }}-${{ steps.rust-version.outputs.version }}-${{ hashFiles('Cargo.lock') }}
      - run: cargo run -p systest
      # - run: cargo run -p systest
      - run: cargo test -p openssl
      - run: cargo test -p openssl-errors

+21 −0
Original line number Diff line number Diff line
@@ -72,3 +72,24 @@ fn dynamic_data() {
    assert_eq!(error.line(), line!() - 11);
    assert_eq!(error.data(), Some("hello world"));
}

#[test]
fn deferred_error_render() {
    openssl_errors::put_error!(Test::BAR, Test::NO_MILK);

    let error = Error::get().unwrap();

    for _ in 0..100 {
        openssl_errors::put_error!(Test::FOO, Test::NO_BACON);
    }

    assert_eq!(error.function().unwrap(), "function bar");
    // Replace Windows `\` separators with `/`
    assert_eq!(
        error.file().replace('\\', "/"),
        "openssl-errors/tests/test.rs"
    );

    // clear out the stack for other tests on the same thread
    while Error::get().is_some() {}
}
+62 −18
Original line number Diff line number Diff line
@@ -92,9 +92,9 @@ impl From<ErrorStack> for fmt::Error {
#[derive(Clone)]
pub struct Error {
    code: c_ulong,
    file: *const c_char,
    file: ShimStr,
    line: c_int,
    func: *const c_char,
    func: Option<ShimStr>,
    data: Option<Cow<'static, str>>,
}

@@ -129,6 +129,15 @@ impl Error {
                    } else {
                        None
                    };

                    let file = ShimStr::new(file);

                    let func = if func.is_null() {
                        None
                    } else {
                        Some(ShimStr::new(func))
                    };

                    Some(Error {
                        code,
                        file,
@@ -174,7 +183,11 @@ impl Error {
    fn put_error(&self) {
        unsafe {
            ffi::ERR_new();
            ffi::ERR_set_debug(self.file, self.line, self.func);
            ffi::ERR_set_debug(
                self.file.as_ptr(),
                self.line,
                self.func.as_ref().map_or(ptr::null(), |s| s.as_ptr()),
            );
            ffi::ERR_set_error(
                ffi::ERR_GET_LIB(self.code),
                ffi::ERR_GET_REASON(self.code),
@@ -190,7 +203,7 @@ impl Error {
                ffi::ERR_GET_LIB(self.code),
                ffi::ERR_GET_FUNC(self.code),
                ffi::ERR_GET_REASON(self.code),
                self.file,
                self.file.as_ptr(),
                self.line,
            );
        }
@@ -214,14 +227,8 @@ impl Error {
    }

    /// Returns the name of the function reporting the error.
    pub fn function(&self) -> Option<&'static str> {
        unsafe {
            if self.func.is_null() {
                return None;
            }
            let bytes = CStr::from_ptr(self.func).to_bytes();
            Some(str::from_utf8(bytes).unwrap())
        }
    pub fn function(&self) -> Option<RetStr<'_>> {
        self.func.as_ref().map(|s| s.as_str())
    }

    /// Returns the reason for the error.
@@ -237,12 +244,8 @@ impl Error {
    }

    /// Returns the name of the source file which encountered the error.
    pub fn file(&self) -> &'static str {
        unsafe {
            assert!(!self.file.is_null());
            let bytes = CStr::from_ptr(self.file as *const _).to_bytes();
            str::from_utf8(bytes).unwrap()
        }
    pub fn file(&self) -> RetStr<'_> {
        self.file.as_str()
    }

    /// Returns the line in the source file which encountered the error.
@@ -308,7 +311,27 @@ impl error::Error for Error {}

cfg_if! {
    if #[cfg(ossl300)] {
        use std::ffi::{CString};
        use ffi::ERR_get_error_all;

        type RetStr<'a> = &'a str;

        #[derive(Clone)]
        struct ShimStr(CString);

        impl ShimStr {
            unsafe fn new(s: *const c_char) -> Self {
                ShimStr(CStr::from_ptr(s).to_owned())
            }

            fn as_ptr(&self) -> *const c_char {
                self.0.as_ptr()
            }

            fn as_str(&self) -> &str {
                self.0.to_str().unwrap()
            }
        }
    } else {
        #[allow(bad_style)]
        unsafe extern "C" fn ERR_get_error_all(
@@ -322,5 +345,26 @@ cfg_if! {
            *func = ffi::ERR_func_error_string(code);
            code
        }

        type RetStr<'a> = &'static str;

        #[derive(Clone)]
        struct ShimStr(*const c_char);

        impl ShimStr {
            unsafe fn new(s: *const c_char) -> Self {
                ShimStr(s)
            }

            fn as_ptr(&self) -> *const c_char {
                self.0
            }

            fn as_str(&self) -> &'static str {
                unsafe {
                    CStr::from_ptr(self.0).to_str().unwrap()
                }
            }
        }
    }
}
+1 −2
Original line number Diff line number Diff line
@@ -845,11 +845,10 @@ impl X509Extension {

    /// Adds an alias for an extension
    ///
    /// This corresponds to [`X509V3_EXT_add_alias`]
    ///
    /// # Safety
    ///
    /// This method modifies global state without locking and therefore is not thread safe
    #[corresponds(X509V3_EXT_add_alias)]
    pub unsafe fn add_alias(to: Nid, from: Nid) -> Result<(), ErrorStack> {
        ffi::init();
        cvt(ffi::X509V3_EXT_add_alias(to.as_raw(), from.as_raw())).map(|_| ())