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

Fix potential use after free in MacOS / iOS impl #54

Merged
merged 3 commits into from Aug 16, 2022
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
@@ -1,7 +1,7 @@
[package]
name = "iana-time-zone"
description = "get the IANA time zone for the current system"
version = "0.1.44"
version = "0.1.45"
authors = ["Andrew Straw <strawman@astraw.com>"]
repository = "https://github.com/strawlab/iana-time-zone"
license = "MIT OR Apache-2.0"
Expand Down
71 changes: 55 additions & 16 deletions src/tz_macos.rs
@@ -1,22 +1,61 @@
use core_foundation_sys::base::{CFRelease, CFTypeRef};
use core_foundation_sys::string::{kCFStringEncodingUTF8, CFStringGetCStringPtr};
use core_foundation_sys::base::{Boolean, CFRange, CFRelease, CFTypeRef};
use core_foundation_sys::string::{
kCFStringEncodingUTF8, CFStringGetBytes, CFStringGetCStringPtr, CFStringGetLength,
};
use core_foundation_sys::timezone::{CFTimeZoneCopySystem, CFTimeZoneGetName};

pub(crate) fn get_timezone_inner() -> Result<String, crate::GetTimezoneError> {
unsafe {
Dropping::new(CFTimeZoneCopySystem())
.and_then(|tz| Dropping::new(CFTimeZoneGetName(tz.0)))
.and_then(|name| {
let name = CFStringGetCStringPtr(name.0, kCFStringEncodingUTF8);
if name.is_null() {
None
} else {
Some(name)
}
})
.and_then(|name| std::ffi::CStr::from_ptr(name).to_str().ok())
.map(|name| name.to_owned())
.ok_or(crate::GetTimezoneError::OsError)
unsafe { get_timezone().ok_or(crate::GetTimezoneError::OsError) }
}

#[inline]
unsafe fn get_timezone() -> Option<String> {
// The longest name in the IANA time zone database is 25 ASCII characters long.
const MAX_LEN: usize = 32;

// Get system time zone, and borrow its name.
let tz = Dropping::new(CFTimeZoneCopySystem())?;
let name = CFTimeZoneGetName(tz.0);
if name.is_null() {
return None;
}

// If the name is encoded in UTF-8, copy it directly.
Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for adding these comments outlining the phases of this function. 🎉

let cstr = CFStringGetCStringPtr(name, kCFStringEncodingUTF8);
if !cstr.is_null() {
let cstr = std::ffi::CStr::from_ptr(cstr);
if let Ok(name) = cstr.to_str() {
return Some(name.to_owned());
}
}

// Otherwise convert the name to UTF-8.
let mut buf = [0; MAX_LEN];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I love that we're avoiding the heap allocation here but would like to make sure our assumptions are valid. What do you think about adding an assert that MAX_LEN is sufficient? If the assert triggers, we at least get a shot at getting a bug report.

core-foundation has a similar assert for the vector they allocate and abseil grabs the length to resize a vector:

https://github.com/servo/core-foundation-rs/blob/e64eab3ecad0071aff2f08aa4254f797a85475d1/core-foundation/src/string.rs#L137-L141

https://github.com/servo/core-foundation-rs/blob/e64eab3ecad0071aff2f08aa4254f797a85475d1/core-foundation/src/string.rs#L57-L68

I think the core-foundation crate is likely to be more correct given it doesn't make any assumptions about the CFString's encoding.

https://github.com/abseil/abseil-cpp/blob/59cba2b50dd689010f338c63db95ca164f195798/absl/time/internal/cctz/src/time_zone_lookup.cc#L142-L143

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh I see now, the check below for buf_bytes being in range checks for this. All good.

let mut buf_bytes = 0;
let range = CFRange {
location: 0,
length: CFStringGetLength(name),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: this length is "character length" not byte length.

https://developer.apple.com/documentation/corefoundation/1542853-cfstringgetlength

Return Value

The number (in terms of UTF-16 code pairs) of characters stored in theString.

};
if CFStringGetBytes(
name,
range,
kCFStringEncodingUTF8,
b'\0',
false as Boolean,
buf.as_mut_ptr(),
buf.len() as isize,
&mut buf_bytes,
) != range.length
Copy link
Collaborator

Choose a reason for hiding this comment

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

This return value is the number of encoding-aware characters, not the length of the byte string.

the length stored in the out parameter buf_bytes is the number of bytes copied.

https://developer.apple.com/documentation/corefoundation/1543006-cfstringgetbytes

Copy link
Collaborator Author

@Kijewski Kijewski Aug 15, 2022

Choose a reason for hiding this comment

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

Yes, I check that all characters in the original encoding were consumed, not more or less. But I use buf_bytes to build the UTF-8 string. (At least that's what I want to do? Am I wrong in here?)

core-foundation even asserts that the length matches: https://docs.rs/core-foundation/0.9.3/src/core_foundation/string.rs.html#82

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh I see.

https://developer.apple.com/documentation/corefoundation/1543006-cfstringgetbytes

Return Value

The number of characters converted.

{
// Could not convert the name.
None
} else if !(1..MAX_LEN as isize).contains(&buf_bytes) {
// The name should not be empty, or excessively long.
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we want the excessively long case to be a panic?

None
} else {
// Convert the name to a `String`.
let name = core::str::from_utf8(&buf[..buf_bytes as usize]).ok()?;
Some(name.to_owned())
}
}

Expand Down