Unsound uses of Unsafe in API

[axon-q]

Right now the actix-web code contains 100+ uses of unsafe. Presumably this is in order to achieve the best possible performance in hot parts of the code.

However, web servers often face the public Internet, so security is extremely important for web server implementations. This issue is especially critical for organizations that intend to use the software in large-scale production environments. One of the main reasons to choose a Rust-based HTTP implementation is the guaranteed memory safety that safe Rust provides. Unfortunately this guarantee is eroded for every use of unsafe in the codebase. Performance isn't worth much if it comes at the cost of critical security vulnerabilities due to unsafe memory access. It's also nice to know for certain that your web server won't segfault in production.

I propose that we leave this open as a tracking issue to track design and implementation issues concerning the use of unsafe code. Some of the items that should be explored:

• Is it possible to remove any of the current uses of unsafe without significantly impacting performance?
• Is it appropriate to remove some uses of unsafe even if there's a performance impact?
• Is there a long-term plan to reduce or eliminate the use of unsafe code?
• Security analysis, testing, and fuzzing of the codebase
• Profiling and performance analysis to assess the impact of converting unsafe to safe code

[DoumanAsh]

@axon-q First of all if your concern is unsafe code then you cannot use even Rust's standard library which is the biggest collection of unsafe code.

Is it possible to remove any of the current uses of unsafe without significantly impacting performance?

Most likely no, but it should be investigated on per case basis, not overall

Is it appropriate to remove some uses of unsafe even if there's a performance impact?

No, I don't believe it is right approach to problem.
Just because code is unsafe doesn't mean it is going to to segfault or cause memory corruption.
Note that Rust has strong guarantees about UB and it is really difficult to write code that would unexpectedly cause problems (applicable for most unsafe code aside from raw pointers)

P.s. Of course if code is not correct it should be corrected

Is there a long-term plan to reduce or eliminate the use of unsafe code?

AFAIK there is no long-term plan, but nothing stops you from eliminating it on per case basis(as long as it doens't impact performance significantly)

But overall goal should not be to remove 100% of unsafe code

[fafhrd91]

unsafe code is used mostly for two reasons:

• performance. and i dont think it is possible to remove those unsafes without impact. we need to evaluate every use
• api ergonomics. i think we can remove some of the usafes

in any case we have to evaluate every use case for unsafe. i used unsafe for the reason, in most cases i couldnt come up with safe solution. also you need to take into account size of the codebase, at the moment actix-web is ~27k loc

[axon-q]

@DoumanAsh The point is to reduce risk and the attack surface. Of course the entire stack, including std, rustc, LLVM and the kernel can potentially have vulnerabilities. The Rust foundations are considered very solid, however, due to their maturity and the amount of use and scrutiny they receive. The question is how much risk actix-web adds on top of that, and how to minimize it. This is something that everyone considering using it in production has to evaluate.

I agree that unsafe isn't necessarily bad, and that some kinds of unsafe code are better than others. But from the perspective of risk, the less unsafe the better.

[axon-q]

@fafhrd91 Makes sense! Good to know that it might be possible to work toward removing some of the non-performance-critical usages. Also, some uses of unsafe are more problematic than others: having a bit of unsafe sealed inside an internal component is not so bad, but unsafe handling of things like memory buffers and parsing that are directly affected by untrusted input is a red flag from a security perspective.

[memoryruins]

If anyone would like to try fuzzing, these will come in handy:

• cargo-fuzz — currently supports libfuzzer
• hongfuzz-rs — helped discover issues in crates such as hyper and h2
• afl.rs — discovered issues in rustc and serde
• Rust Fuzz Trophy Case — lists issues found in crates and the fuzzers used

[seanmonstar]

After noticing this issue, and then noticing #301, I figured I'd do a more in-depth analysis of the unsafe usages here. I've so far found ~15 cases where a user can "safely" trigger memory bugs. I first offered to send this in a private email, but it was preferred I just post this here.

I haven't done a complete review. I first looked in public API files, and so have not analyzed client/*, server/*, or pipeline.rs, all of which do happen to contain a large amount of unsafe code as well.

The two main classes of issues I have found are:

1. Unsafe Send, allowing memory unsafety to occur if the affected code every moves to another thread.
2. Unchecked multiple mutable references, allowing invalidating references to memory accidentally.

While a couple unsafe usages can be fine, I'd strongly recommend to remove as much as possible. Consider:

• The performance in many cases isn't actually improved because of it.
• When performance is improved, usually by just a tiny bit, it's not worth it when it's so easy to violate memory safety. Otherwise why would we have Rust, when we could have this fun property in C/C++.
• Many C/C++ libraries have felt that they can manage the unsafety, and yet we constantly see CVS notices from those same libraries. Unsafety is extremely tricky.
• I suspect most all of these can be made safe with no performance cost, just be restructuring some things.

---

The issues I've found so far:

Actix core

• ContextImpl unsafe impl Send
  • ContextImpl contains a vector of Box<Future> (note, not Box<Future + Send>).
  • ContextImpl::spawn has F: Future bounds, so a user can safely insert !Send futures into the items list. If that ContextImpl ever crosses to another thread, it can easily trigger memory unsafety (double-free or use-after-free).
• mutable alias of &mut Actor in ContextImpl
  • A user can call Context::create(new_actor) to create a new Context. After grabbing a &T of something on their Actor (&mut self), they can mem::replace(context, new_context). When the returned old_context drops, it will have invalidated the memory of &T.
• ContextImpl::actor() returns an aliased &mut Actor
  • Someone implementing a Context using ContextImpl could safely get 2 mutable references to the Actor, and then mutating one can invalidate references into the 2nd.
• Writer contains UnsafeWrite, which is Send, but holds just an Arc<UnsafeCell>
  • Calling Writer::new(some_io, ctx) will spawn a clone of the UnsafeWriter to be costantly flushing its buffer on the io
  • Writer can then be sent to another thread, where a user can "safely" write into it's buffer.
  • If the Actor in the first thread accesses &self.buffer to pass to std::io::Write::write just before the second thread causes the buffer to re-allocate to grow, the Write will try to copy freed memory.

Actix-web

• HttpRequest unsafely accesses mutable inner fields without synchronization
  • HttpRequest internally has an Rc
  • InnerHttpMessage has get_mut() method which just transmutes from &self to &mut self.
  • HttpRequest can be cloned (both impl Clone, and methods like change_state, drop_state, etc).
  • This allows having 2 HttpRequests, and while taking a reference to some inner data, like let header = req1.headers().get("connection"), and then invalidate the memory it points to, by doing req2.headers_mut() = HeaderMap::new().
• HttpRequest::connection_info transmutes references from the internal HeaderMap to &'static.
  • A call to req.connection_info() save static references from the current HeaderMap and Uri
  • Afterwards, a user can mutate the headers, like req.headers_mut().insert("x-forward-for", "woopsies").
  • A second call to req.connection_info() will access cached "static" strs that have had their memory freed.
  • It seems that filters/predicates/middleware actually fill this in automatically, so all it would take is mutating some headers to invalidate those references.
• HttpRequest::query() and match_info() store references into the Uri path as &'static strs in the Extensions, just like connection_info.
  • Its possible to generate these unsafe static refernces with req.query()
  • Then, the extensions can be replaced via req.extensions_mut().
  • The extensions can be put in a different HttpRequest.
  • req.query() will return the cached but likely freed "static" references.
• HttpContext mutable alias of self, along with HttpContext::actor(new_actor) will free the old actor, which can be done inside any Actor method

  • impl Actor for Woops {
        fn start(&mut self, ctx: &mut HttpContext<Woops>) {
            let bomb = &self.some_str[10..];
            ctx.actor(Woops::new())
            println!("hey look at this free memory: {}", bomb);
        }
    }

• Scope takes an unsafe reference to the HttpRequest::uri before passed &mut HttpRequest to each Predicate
  • A user can implement a safe Predicate that could accidentally free the Uri.
  • There is a constructor, HttpRequest::new. A user could clone each of the pieces, modifying a little, and then inserting the new HttpRequest in Predicate::check(&mut req), via *req = my_sub_request.
  • The next step in the loop would then check the path for the next filter, accessing freed memory.
• WebsocketContext mutable alias has same problem as HttpContext
  • A user can replace the ctx with a new one, thanks to constructors, and free the Actor that represents &mut self
  • A user can also do it more easily with just ctx.actor(Woopsies::new()
• Identity middleware has unsafe impl for Box<Identity> (not Box<Identity + Send>)
  • A user can very easily accidentally use !Send types when implementing Identity, such as using Rc
  • That type is then stored in the extensions, and if they get sent to another thread, can lead to memory unsafety
• The Session middleware has the exact same problem.
• ExtractorConfig has an Rc<UnsafeCell>
  • It implements Clone, and DerefMut, without synchronization, meaning a user can have multiple mutable references to the same Config

[fafhrd91]

@seanmonstar thanks for analysis. i will think about this.

[fafhrd91]

@seanmonstar thanks again for pointing.

• using tokio threadpool was bad idea in first place. actix does not use threadpool anymore, so all Send related problems should be fixed.
• Extensions is fixed too request.extensions requires Send #315
• uri will be fixed later with remove http::Uri from public api #178
• HttpContext/WebsocketContext should be fixed

last is header_mut(), i need to think how to fix it

[fafhrd91]

fixed most of the problems. let's open new ticket for each new case.
closing this one

[seanmonstar]

I'm glad that some of the issues could be fixed, that's great!

However, I think it's early to close this issue, as "most of the problems" are not actually fixed. Some of the holes have been filled in, but the underlying unsafety still exists, and several of the "fixes" actually still leave a public API that is unsound.

1. It does seem like most of the unsoundness due to unsafe impl Send has been removed (though, I was a little shocked to read one of the patches add a brand new unsafe impl Send, instead of removing all of them. I haven't yet analyzed the new one.).
2. Some of the things were fixed, like transmuting &'a str into &'static str. However, several others still remain. The Contexts still have ways to trigger memory bugs. HttpRequest still has was for a user to trigger memory bugs. The URI issue is not fixed.

The biggest problem is there several different versions of code like this:

let alias: &mut Foo = unsafe { &mut *(self as *const Foo as *mut Foo) };
self.inner.do_something_mut(alias)

This is a very unsafe thing to do! True, it's possible for the do_something_mut method to not trigger memory unsafey, but doing the above has completely disable the compiler from helping you notice when you do! And, this pattern is actually the root of most of the unsound APIs that are available. I'd beg that instead of trying to fill in more holes, the root of the unsoundness be fixed instead.

For that reason, I'm not spelling out exactly how to trigger the memory bugs that still exist (though I have exact steps), since it's not my goal to have the ones I find fixed. It's my goal to have people using and writing safe software. Instead, I can write up some steps on how to fix the roots.

---

• The HttpRequest continues to have just an Rc<InnerHttpMessage>, and yet thanks to HttpRequest::as_mut() simply transmuting to a &mut InnerHttpMessage, references can still be invalidated. The as_mut method should add a check that there is only 1 reference, and panic otherwise.

  fn as_mut(&mut self) -> &mut InnerHttpMessage {
      if Rc::strong_count(self) > 1 || Rc::weak_count(self) > 0 {
          panic!("as_mut access when other references exist")
      }
      // now take a mutable reference
  }

• The Scope takes an unsafe reference to the path, and then passes a mutable reference to each predicate (memory bugs are still possible with the fixes so far). It should not do this. Instead, it should only grab a temporary reference when it is needed.

  // This should be removed
  let path = unsafe { &*(&req.uri().path()[prefix..] as *const [u8]) }

  Instead, this should be done:

  thing_using_path_prefix(&req.uri().path()[prefix..])

• The various Contexts, ContextImpl, Context, HttpContext, WebsocketContext, all do the mutable alias example I mentioned above:

  let ctx: &mut Context = unsafe { &mut *(self as *const Context as *mut Context) };
  self.inner.poll(ctx)

  This still has several unsound holes, and they could all be fixed by not doing the above. Since the Context is passed a &mut Context to all Actor and Handler methods, the structure should be changed. The Context should stop holding the actual Actor, so that a mutable alias isn't required to pass the Context to an actor method.

---

Besides the unsoundness that I've been able to identify, there may exist other instances that I haven't noticed yet. Instead of fixing the individual issues, fixing the root makes any unnoticed issues go away too. I'd expect this issue to remain open so users of Actix can track how to know that the code they rely on is safe, until everything has been resolved.

1. The fixes that have been applied so far are only for the unreleased 0.7 version. How can a user of 0.6 be sure their code is sound, without having to migrate through breaking changes to get 0.7 (and hope any other libraries that they use that also depend on Actix upgrade to 0.7 quickly)?
2. How does a user feel confident that these kinds of unsoundness don't happen again? With the root cause remaining in the library, it's far too easy to forget and introduce a new unsound API. That's what happened the first time, the compiler errors were turned off, assuming that things were safe, and perhaps the very first usage of it was. But since the unsafety gets hidden away in a small function somewhere, while the invariant that keeps it safe is not actually local to that function, new instances can easily pop up.

(An example of number 2 is how server::TransferEncoding holds a *mut BytesMut. This is unsafe, but it's unsafety has been wrapped up in a couple of debug assertions. While analyzing the code, it doesn't appear that memory bugs are triggered, there is absolutely no need to have this unsafety, but allows for future code to accidently violate some invariants. Instead of keeping a *mut BytesMut inside the TransferEncoding, a &mut BytesMut could be passed to TransferEncoding::encode, like encoder.encode(&mut self.buffer, bytes).)

I believe Actix users would want these to be addressed. If not by reopening this issue, then by opening a new one with that objective.

[hayd]

Also, the issue's headline is:

Right now the actix-web code contains 100+ uses of unsafe.

This is still the case. Being able to track this is useful...

[fafhrd91]

@seanmonstar i can not see how to trigger memory unsafety with req.uri().path() and predicates. could you elaborate?

[gnzlbg]

First of all if your concern is unsafe code then you cannot use even Rust's standard library which is the biggest collection of unsafe code.

Each use of unsafe in the std library contains a comment "proving"/explaining why the unsafe operations performed are safe. The explanations typically list the invariants that, when violated, could lead to undefined behavior, and the code typically asserts these, such that the program will abort or unwind instead of running into undefined behavior.

OTOH unsafe inside actix-web appears to be used nitty-willy, without any explanations of why each unsafe operation is actually safe. That's a completely different bar.

i used unsafe for the reason, in most cases i couldnt come up with safe solution.

Do you mean that you couldn't come up with a safe solution in safe Rust? Because unsafe is a tool to write safe code that the compiler cannot prove correctly, not a tool to write code that is not safe (EDIT: that is, that has undefined behavior).