Fix potential use after free in MacOS / iOS impl #54
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@lopopolo, does this implementation look sane? Could you run it in the leak sanitizer, please? |
|
Implementation looks a lot easier to reason about. thank you. I still get the leaks in LeakSan though: |
|
hmm I also get these leaks in 0.1.42 on my machine too. Is this a bug upstream in |
|
Could you test the |
|
At 9454971, running SUMMARY: LeakSanitizer: 7908 byte(s) leaked in 244 allocation(s). It's a big dump, I'm not sure if you want the whole thing. Here's an interesting snippet: Full log: leaksan.log. |
lopopolo
requested changes
Aug 15, 2022
src/tz_macos.rs
Outdated
|
|
||
| // Get system time zone, and its name. | ||
| let tz = Dropping::new(CFTimeZoneCopySystem())?; | ||
| let name = Dropping::new(CFTimeZoneGetName(tz.0))?; |
There was a problem hiding this comment.
this is UB since Dropping will free an un-owned pointer.
See: https://developer.apple.com/documentation/corefoundation/1543519-cftimezonegetname
A string containing the geopolitical region name that identifies tz. Ownership follows the The Get Rule.
The Get Rule
If you receive an object from any Core Foundation function other than a creation or copy function—such as a Get function—you do not own it and cannot be certain of the object’s life span. If you want to ensure that such an object is not disposed of while you are using it, you must claim ownership (with the CFRetain function). You are then responsible for relinquishing ownership when you have finished with it.
There was a problem hiding this comment.
Ah, thanks, fixed in the next commit. I guess that this could cause a leak, when the reference counter wraps around because it was decremented once too often.
Kijewski
added a commit
to Kijewski/iana-time-zone
that referenced
this pull request
Aug 15, 2022
Kijewski
added a commit
to Kijewski/iana-time-zone
that referenced
this pull request
Aug 15, 2022
Kijewski
force-pushed
the
pr-macos-use-after-free
branch
from
6c6ee1c
to
8b38556
Compare
lopopolo
reviewed
Aug 15, 2022
| buf.as_mut_ptr(), | ||
| buf.len() as isize, | ||
| &mut buf_bytes, | ||
| ) != range.length |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Ahh I see.
https://developer.apple.com/documentation/corefoundation/1543006-cfstringgetbytes
Return Value
The number of characters converted.
|
This is what I ended up with, which based on my reading of the core foundation docs is the simplest set of APIs that requires the least memory management in Rust: #[inline]
unsafe fn get_timezone() -> Option<String> {
// Get system time zone, and its name.
let tz = Dropping::new(CFTimeZoneCopySystem())?;
let name = CFTimeZoneGetName(tz.0);
if name.is_null() {
return None;
}
let cstr = CFStringGetCStringPtr(name, kCFStringEncodingUTF8);
if cstr.is_null() {
return None;
}
let cstr = std::ffi::CStr::from_ptr(cstr);
let s = cstr.to_str().ok()?;
Some(s.to_owned())
} |
|
the leak sanitizer bits are I think a red herring. All of the leaks are deep in the objective-c runtime. |
Kijewski
force-pushed
the
pr-macos-use-after-free
branch
from
8b38556
to
d68a25d
Compare
|
@Kijewski I also invited you to be an owner on crates.io. You can publish this as the next release if you like, else let me know and I will do it. |
|
@astraw, thank you! I accepted both invitation. @lopopolo, my implementation is the same now as yours, except it does the character set conversion if the name is not in UTF-8. Do you think we can simply omit the second part? Other implementations like abseil don't have the CFStringGetCStringPtr shortcut at all: time_zone_lookup.cc. |
lopopolo
approved these changes
Aug 15, 2022
| } | ||
|
|
||
| // Otherwise convert the name to UTF-8. | ||
| let mut buf = [0; MAX_LEN]; |
There was a problem hiding this comment.
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:
I think the core-foundation crate is likely to be more correct given it doesn't make any assumptions about the CFString's encoding.
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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.
| buf.as_mut_ptr(), | ||
| buf.len() as isize, | ||
| &mut buf_bytes, | ||
| ) != range.length |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
do we want the excessively long case to be a panic?
| return None; | ||
| } | ||
|
|
||
| // If the name is encoded in UTF-8, copy it directly. |
There was a problem hiding this comment.
thanks for adding these comments outlining the phases of this function. 🎉
Closed
astraw
changed the title
|
While I have not dug in deeply myself, this looks good to me. Shall I merge and publish a release? @Kijewski you are also welcome to do so. |
|
Sounds good to me @astraw 👍 |
Per Ryan Lopopolo's review: > This bit makes me a bit scared with the `Dropping` type and the `CStr` being dropped while a borrowed `&str` is taken from name. Is the `.map(|name| name.to_owned())` a use after free? > > To be sure, I'd probably restructure all of these combinators to use short circuit return to make sure the `Dropping` and `CStr` wrappers are dropped in the right order. <strawlab#50 (comment)>
Though it is unlikely that the time zone is stored in an encoding other than UTF-8, it's not much work add to support for this edge case.
Kijewski
force-pushed
the
pr-macos-use-after-free
branch
from
d68a25d
to
6bdb9ed
Compare
|
Thank you both very much! Released as v0.1.45. |
|
If there's a UAF here, it'd be much appreciated if you filed a rustsec advisory so folks will know to update: https://github.com/RustSec/advisory-db/blob/main/CONTRIBUTING.md Thanks! |
|
@alex, I yanked the affected versions. Do you think I should still file an advisory? I guess the answer is probably "yes, of course". |
|
Yes, advisories are useful: they help folks who are already pinned in their |
|
Filed as rustsec/advisory-db#1366. Thank you for the suggestion, @alex! Better safe than sorry. |
|
Published here: https://rustsec.org/advisories/RUSTSEC-2022-0049.html |
|
Thanks for this @Kijewski and thanks for the review work @lopopolo. One thought I had upon reading about impl<T> Drop for Dropping<T> {
#[inline]
fn drop(&mut self) {
if !self.0.is_null() {
unsafe { CFRelease(self.0 as CFTypeRef) };
}
}
}or potentially this is not needed as we have already asserted on creation of the |
|
Thank you for reviewing the code, too, @esheppa! I take it as an invariant here that no It is entirely possible that the exactly same byte code would be generated with the code you've posted, since the compiler should know that it just checked that the pointer isn't null and the method is inlined. I guess I should investigate this for the next time. If the compiler is smart enough, then we should let it evaluate the invariants. |
|
Only thought here is that if the timezone is changed while this code is executing, potentially the pointer could be invalidated? I'm not sure whether it would actually return |
|
To fix the bug I investigated a bit, and found Apple actually released the source code for The |
Makes sense! Just noticed one other potential thing - could we just remove |
|
The pointer returned by |
|
Looks like I'm too spoiled by rust function signatures to have a good understanding of this, but your explanation is excellent! It looks like we are having to maintain essentially parts of |
|
I've created: servo/core-foundation-rs#521 |
|
Thank you! I'll review/merge that PR after work if no one beats me to it.
בתאריך יום ג׳, 16 באוג׳ 2022, 8:53, מאת René Kijewski <
***@***.***>:
… Filed as rustsec/advisory-db#1366
<rustsec/advisory-db#1366>. Thank you for the
suggestion, @alex <https://github.com/alex>! Better safe than sorry.
—
Reply to this email directly, view it on GitHub
<#54 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAGBGLFJZ7HCUMLAJ2H4TVZOFKXANCNFSM56QU2H4Q>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Per Ryan Lopopolo's review:
#50 (comment)