Skip to content

Check input in rustls_error to avoid UB #195

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

Merged
merged 1 commit into from
Nov 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the extra dependency for this is overkill. In this case it seems fine to write a custom TryFrom impl based on the repr type.


[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