rust nsString bindings fail to null-terminate DataFlags::OWNED strings

RESOLVED FIXED in Firefox 58

Status

()

defect
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: Nika, Assigned: Nika)

Tracking

({sec-audit})

unspecified
mozilla58
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57 wontfix, firefox58 fixed)

Details

(Whiteboard: [adv-main58-][post-critsmash-triage])

Attachments

(1 attachment)

The rust nsstring bindings tried to take advantage that TERMINATED was a data flag on strings. Namely strings created by using `from(String)` or `from(Vec<u{8,16}>)` had the ownership of their backing buffers transferred to the nsstring bindings, and did not add a null terminator.

Not adding a null terminator was fine for our dependent string variants (such as nsString<'a>::from(&str)) as we don't depend on dependent strings in gecko having null terminators, but unfortunately we do have the constraint that all OWNED strings are null terminated. 

This is taken advantage of through methods like nsString::get() which return a raw pointer to the start of the input string, presuming that they are null-terminated.

This could be triggered right now through something like this:

// C++
struct SharedStruct {
  nsCString mName;
};

SharedStuct a;
some_rust_func(&a);
printf("str is %s\n", a.mName.get());

// Rust
#[repr(C)]
struct SharedStruct {
  name: nsCString<'static>,
}

#[no_mangle]
pub extern "C" some_rust_func(a: *mut SharedStruct) {
  let name = "whatever".to_string();
  a.name = name.into();
}

In this case mName will not be null-terminated, but get() is type-system enforced, so it won't be caught.

This gets worse after the patch I am working on where I add a `Assign(substring_type&& aStr)` overload, as in that patch a OWNED string buffer without null-termination would be able to travel around the code base. I doubt that we have any exploitable cases of this right now, but it would be much easier to get one once it can travel.

I think that the way to fix this would be to ensure that all OWNED strings are null-terminated in rust, as nsCString<'static> ought to act the same way as C++ strings.

Potentially in the future if we decide to allow for OWNED non-terminated strings we will be able to make the rust nsstring bindings more efficient again.

====

Unfortunately, after this, we still have a footgun in the bindings. Namely this will still cause a problem:

#[no_mangle]
pub extern "C" some_rust_func(a: *mut SharedStruct) {
  a.name = "whatever".into();
}

Namely, as we use a single ns[C]String type in the rust bindings, and simply use nsString<'static> to represent one which can live forever, we have no way to detect whether the string we need to create is allowed to be DEPENDENT (as when I designed the API I didn't realize that some types had that constraint - I thought that it was fully dynamic).

In this case it would be fine, except that we don't need DEPENDENT strings to be null terminated (and it would be a bad idea to require that). I suppose the solution here would be to add a nsDependent[C]String type to the rust side (dunno what to call it - nsDependent[C]String? ns[C]StringRef? ns[C]Str? :-/), and remove the lifetime parmaeter on ns[C]String.

====

I don't think that stylo will have triggered any of these problems. If I remember correctly, stylo uses ns[C]StringRepr in its bindgen-ed struct declarations, as that way bindgen can handle destructors better. This means that the assignment operator wouldn't be used, and `assign()` would be called instead. Calling `assign` is safe right now, so we're probably OK.

This is also OK for our rust_url bindings, as they use nsA[C]String, which also can only be `.assign`-ed, so it won't end up being a problem.

Unfortunately, this will probably still require changes to large amounts of rust code to add in the new rust string type and create the split.
Sounds like this isn't an issue for current code, so I'm marking this audit.
Keywords: sec-audit
(In reply to Andrew McCreight [:mccr8] from comment #1)
> Sounds like this isn't an issue for current code, so I'm marking this audit.

This is true, I did a look through our current codebase and we currently don't use any of the problematic APIs in any of our rust code, including the stylo bindings, so this isn't a problem unless someone adds code which uses the nsstring APIs in a problematic way.
Group: core-security → dom-core-security
I figure we should make this change to split the borrowed and owning string
variants apart before we start spawning more consumers of the `nsstring` crate.

I'm fairly confident that we have no security bugs caused by this issue, as if
we did they wouild be broken by these changes.
Attachment #8906779 - Flags: review?(nfroyd)
Comment on attachment 8906779 [details] [diff] [review]
Introduce a distinction between nsCStr<'a> and nsCString

Review of attachment 8906779 [details] [diff] [review]:
-----------------------------------------------------------------

I think all of this makes sense, just one comment on the naming below that we should work out.

::: xpcom/rust/nsstring/src/lib.rs
@@ +11,5 @@
> +//! These conversions will attempt to re-use the passed-in buffer, appending a
> +//! null.
> +//!
> +//! Use `ns[C]Str` (`nsDependent[C]String` in C++) as an intermediate between
> +//! borrowed rust data structures (such as `&str` and `&[u16]`) and `&{mut,}

Is the `ns[C]Str` name supposed to imply correspondence between it and Rust's `str` type?   I am inclined to actually spell out `nsDependent[C]String` so it's a little more obvious.  Maybe it is my C++-centric view showing, but it's hard to tell what the difference between `ns[C]String` and `ns[C]Str` is at first glance.
Attachment #8906779 - Flags: review?(nfroyd) → review+
This is Rust code, so better to adopt Rust idioms for naming than Gecko-specific C++ idioms, IMO.
(In reply to Anthony Ramine [:nox] from comment #5)
> This is Rust code, so better to adopt Rust idioms for naming than
> Gecko-specific C++ idioms, IMO.

Since we are dealing with essentially FFI types here, it makes sense to me to name things with more emphasis on the conventions on the FFI side, rather than on the Rust side.  I can imagine situations where it makes sense to shift the emphasis more towards the Rust side, but what's not clear to me here is whether the Rust idiom shines clearly through in the naming here.
(In reply to Nathan Froyd [:froydnj] from comment #6)
> (In reply to Anthony Ramine [:nox] from comment #5)
> > This is Rust code, so better to adopt Rust idioms for naming than
> > Gecko-specific C++ idioms, IMO.
> 
> Since we are dealing with essentially FFI types here, it makes sense to me
> to name things with more emphasis on the conventions on the FFI side, rather
> than on the Rust side.  I can imagine situations where it makes sense to
> shift the emphasis more towards the Rust side, but what's not clear to me
> here is whether the Rust idiom shines clearly through in the naming here.

My reasoning behind the use of the name `nsCStr` rather than `nsDependentCString` was that I wanted to make the type easier to use in rust. In rust code it's very safe to use (unlike nsDependentCString in C++ code).

For example, suppose you have a C++ function which takes an `const nsACString&` in C++ (*const nsACString in rust), and you're calling it from rust:
```
// I want you to use a non-owning nsCString to call the function, so this:
RandomGeckoFunction(&nsCStr::from(&rustString));
// Is shorter to write than
RandomGeckoFunction(&nsDependentCString::from(&rustString));

// And I worry that people will instead write the final form:
RandomGeckoFunction(&nsCString::from(&rustString));
// Which will perform an extra allocation and copy unnecessarily. 
// IMO we should try to make the most performant version of the code 
// the easiest to write.
``` 

(NOTE: You can't use .into() in these contexts, as there's no way to infer that we need to convert a `&str` to a `nsCStr` and then `Deref` it into a `&nsACString` which then needs to be coerced into a `*const nsACString`).

Potentially some of this could be mitigated with a helper macro like `nsastring!(...)` which just performs whatever conversion and/or allocation is required to make the conversion using `ns[C]StringLike`.

ni? to get a final decision from nathan as to whether to use nsDependent[C]String or ns[C]Str.
Flags: needinfo?(nfroyd)
(In reply to Michael Layzell [:mystor] from comment #7)
> ni? to get a final decision from nathan as to whether to use
> nsDependent[C]String or ns[C]Str.

The other reason why I didn't want to call it nsDependent[C]String is that unlike `nsA[C]String` and `ns[C]String` this type shouldn't ever appear in a FFI function signature. `ns[C]Str` is exclusively meant to be used within rust, and then dereferenced into a borrowed `&nsA[C]String` before passing into C++ code. It might be possible to use it as a `nsDependent[C]String` in a shared struct right now, but that isn't being checked and I didn't have it in mind.
(In reply to Michael Layzell [:mystor] from comment #8)
> (In reply to Michael Layzell [:mystor] from comment #7)
> > ni? to get a final decision from nathan as to whether to use
> > nsDependent[C]String or ns[C]Str.
> 
> The other reason why I didn't want to call it nsDependent[C]String is that
> unlike `nsA[C]String` and `ns[C]String` this type shouldn't ever appear in a
> FFI function signature. `ns[C]Str` is exclusively meant to be used within
> rust, and then dereferenced into a borrowed `&nsA[C]String` before passing
> into C++ code.

I am not entirely sure this property is obvious, and people will do all manner of things if there's no lint-like check telling them they shouldn't.

We'll try `ns*Str` for a name and see how it goes.
Flags: needinfo?(nfroyd)
https://hg.mozilla.org/mozilla-central/rev/b4c0c90d8078

Calling this wontfix for older releases based on comment 2, though it does seem worth re-running that analysis to make sure that statement still holds up for 57.
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(michael)
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Blocks: 1403170
(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)
> https://hg.mozilla.org/mozilla-central/rev/b4c0c90d8078
> 
> Calling this wontfix for older releases based on comment 2, though it does
> seem worth re-running that analysis to make sure that statement still holds
> up for 57.

I looked manually through usage of nsstring in servo/servo and saw no examples of code which would've caused problems, so I think we're good for 57.
Flags: needinfo?(michael)
Group: dom-core-security → core-security-release
Whiteboard: [adv-main58-]
Flags: qe-verify-
Whiteboard: [adv-main58-] → [adv-main58-][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.