Skip to content

Commit

Permalink
Check input in rustls_error to avoid UB (#195)
Browse files Browse the repository at this point in the history
The input parameter for rustls_error was `rustls_result`. However, in
Rust it's undefined behavior for an enum to hold an invalid value. That
meant that if C passed an invalid value to rustls_error, UB would
result.

This changes the input parameter to be a uint, and relies on a macro
from the num_enum crate to check the value of that input parameter. If
the input is invalid, we emit the error for "InvalidParameter".
  • Loading branch information
jsha authored Nov 9, 2021
1 parent 42a1557 commit f4e370b
Show file tree
Hide file tree
Showing 4 changed files with 108 additions and 83 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ If you are importing this as a library from other Rust code, you should import `
expected.
- `rustls_version` returns a `rustls_str` that points to a static string in
memory, and the function no longer accepts a character buffer or length.
- `rustls_error` now takes a `unsigned int` instead of rustls_result directly.
This is necessary to avoid undefined behavior if an invalid enum value is
passed.
- Some errors starting with RUSTLS_RESULT_CERT_ have been removed, and
some renamed.
- rustls_client_config_builder_set_protocols is now rustls_client_config_builder_set_alpn_protocols.
Expand Down
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ libc = "0.2"
sct = "0.7"
rustls-pemfile = "0.2.1"
log = "0.4.14"
num_enum = "0.5.4"

[dev_dependencies]
cbindgen = "*"
Expand Down
28 changes: 24 additions & 4 deletions src/error.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use std::{cmp::min, fmt::Display, slice};
use std::{cmp::min, convert::TryFrom, fmt::Display, slice};

use crate::ffi_panic_boundary;
use libc::{c_char, size_t};
use libc::{c_char, c_uint, size_t};
use num_enum::TryFromPrimitive;
use rustls::Error;

/// A return value for a function that may return either success (0) or a
Expand All @@ -18,7 +19,7 @@ impl rustls_result {
/// UTF-8 encoded, and not NUL-terminated.
#[no_mangle]
pub extern "C" fn rustls_error(
result: rustls_result,
result: c_uint,
buf: *mut c_char,
len: size_t,
out_n: *mut size_t,
Expand All @@ -35,6 +36,7 @@ impl rustls_result {
}
slice::from_raw_parts_mut(buf as *mut u8, len as usize)
};
let result: rustls_result = rustls_result::try_from(result).unwrap_or(rustls_result::InvalidParameter);
let error_str = result.to_string();
let len: usize = min(write_buf.len() - 1, error_str.len());
write_buf[..len].copy_from_slice(&error_str.as_bytes()[..len]);
Expand All @@ -60,8 +62,26 @@ impl rustls_result {
}
}

#[test]
fn test_rustls_error() {
let mut buf = [0 as c_char; 512];
let mut n = 0;
rustls_result::rustls_error(0, &mut buf as *mut _, buf.len(), &mut n);
let output: String = String::from_utf8(buf[0..n].iter().map(|b| *b as u8).collect()).unwrap();
assert_eq!(&output, "a parameter had an invalid value");

rustls_result::rustls_error(7000, &mut buf as *mut _, buf.len(), &mut n);
let output: String = String::from_utf8(buf[0..n].iter().map(|b| *b as u8).collect()).unwrap();
assert_eq!(&output, "OK");

rustls_result::rustls_error(7101, &mut buf as *mut _, buf.len(), &mut n);
let output: String = String::from_utf8(buf[0..n].iter().map(|b| *b as u8).collect()).unwrap();
assert_eq!(&output, "peer sent no certificates");
}

#[allow(dead_code)]
#[repr(C)]
#[repr(u32)]
#[derive(TryFromPrimitive)]
pub enum rustls_result {
Ok = 7000,
Io = 7001,
Expand Down
Loading

0 comments on commit f4e370b

Please sign in to comment.