Stream: wg-secure-code

Topic: async and HTTP client rant


Shnatsel (Jan 07 2020 at 00:18, on Zulip):

I have a draft of a new post on my personal block, I'd appreciate pre-reading and feedback: https://medium.com/@shnatsel/smoke-testing-rust-http-clients-b8f2ee5db4e6

Shnatsel (Jan 07 2020 at 01:05, on Zulip):

Well, I've just learned that https://crates.io/crates/attohttpc is a thing, I guess I have still more testing and writing to do

Shnatsel (Jan 07 2020 at 03:07, on Zulip):

Please disregard that, looks like I will be rewriting that

Tony Arcieri (Jan 07 2020 at 03:22, on Zulip):

I'm definitely interested in minimalist security-oriented HTTP clients

Tony Arcieri (Jan 07 2020 at 03:22, on Zulip):

particularly ones which use rustls by default

Wesley Moore (Jan 07 2020 at 03:26, on Zulip):

Interesting post Shnatsel. In, "Here are two probably-exploitable bugs that are still unpatched in the latest version of libcurl: 1, 2." the second link is in a unit test so might not apply.

Shnatsel (Jan 07 2020 at 05:43, on Zulip):

Good catch, thanks!

Shnatsel (Jan 07 2020 at 05:45, on Zulip):

I'm re-running the test on attohttpc now, will put in the results and rewrite conclusion

Shnatsel (Jan 07 2020 at 05:45, on Zulip):

Also turns out https://crates.io/crates/http_req is a thing, even has some downloads unlike the other 2 sync clients

Shnatsel (Jan 07 2020 at 19:25, on Zulip):

Also I think I need a better title for this post

Tony Arcieri (Jan 07 2020 at 19:29, on Zulip):

huh, another one

Tony Arcieri (Jan 07 2020 at 19:29, on Zulip):

I wrote one of these but it barely works and I'd like to abandon it

Tony Arcieri (Jan 07 2020 at 19:29, on Zulip):

heh

Tony Arcieri (Jan 07 2020 at 19:29, on Zulip):

https://crates.io/crates/harp

Shnatsel (Jan 07 2020 at 20:12, on Zulip):

There's also https://crates.io/crates/cabot

RalfJ (Jan 07 2020 at 21:33, on Zulip):

@Shnatsel ureq got a bad crev rating the first time around, has the situation gotten better since then?

Shnatsel (Jan 07 2020 at 21:34, on Zulip):

Yes, I believe most points from that review are addressed

Shnatsel (Jan 07 2020 at 21:34, on Zulip):

I am rewriting my article to be even more nihilistic now because I've found that none of the sync crates allow specifying a timeout for the entire request, so they're all trivial to DoS

RalfJ (Jan 07 2020 at 21:37, on Zulip):

the version you linked up there has been a fun read, good job :)

RalfJ (Jan 07 2020 at 21:37, on Zulip):

though even more nihilistic might be hard to swallow^^

RalfJ (Jan 07 2020 at 21:38, on Zulip):

(for my pet projects I used attohttpc instead of ureq based on that crev comment, maybe I should switch. but then, they're just toys, and I am not sure if I want to play type golf again until I figured out how to get a Read trait out of the response^^)

Shnatsel (Jan 07 2020 at 21:39, on Zulip):

I have run attohttpc through the same test but haven't looked at the results yet. My gripe with attohttpc is that it doesn't support rustls, only openssl or native-tls

Shnatsel (Jan 07 2020 at 21:39, on Zulip):

plus it pulls in the same hand-rolled hashmap advertised as HTTP types, but I guess I can live with that

Shnatsel (Jan 07 2020 at 21:42, on Zulip):

@RalfJ btw you will probably appreciate this gem:
https://github.com/actix/actix-net/blob/7dddeab2a8c4fdcd0c7de6aa4303aca8faffcd53/actix-service/src/cell.rs#L40
and this:
https://github.com/actix/actix-net/blob/7dddeab2a8c4fdcd0c7de6aa4303aca8faffcd53/actix-service/src/cell.rs#L35

RalfJ (Jan 07 2020 at 21:43, on Zulip):

the allow(clippy::mut_from_ref) is great^^

RalfJ (Jan 07 2020 at 21:44, on Zulip):

though this does have interior mutability, so if used correctly this could avoid UB

RalfJ (Jan 07 2020 at 21:47, on Zulip):

wait... get_mut makes no sense, does it? &mut Rc<T> doesnt mean we can safely get mutable access to the interior.

Shnatsel (Jan 07 2020 at 21:47, on Zulip):

Ah, so the &T to &mut T transmute in here is actually okay because they are doing this to an UnsafeCell?

Shnatsel (Jan 07 2020 at 21:48, on Zulip):

Of course it makes no sense, they're trying to use a hand-rolled Cell as a RefCell

Shnatsel (Jan 07 2020 at 21:48, on Zulip):

which it isn't

RalfJ (Jan 07 2020 at 21:48, on Zulip):

it's not doing such a transmute, is it? it's calling UnsafeCell::get

RalfJ (Jan 07 2020 at 21:48, on Zulip):

which is the one oaky way to go from &UnsafeCell<T> to &mut T if you can assure uniqueness

RalfJ (Jan 07 2020 at 21:49, on Zulip):

but, making get_mut safe just seems entirely wrong, this is like Rc::get_mut but without the check that the refcount is 1...

Shnatsel (Jan 07 2020 at 21:49, on Zulip):

Yep, the entire reason why std::Cell doesn't have get_mut() is because you need RefCell to guarantee uniqueness

RalfJ (Jan 07 2020 at 21:50, on Zulip):

not sure what you mean? https://doc.rust-lang.org/nightly/std/cell/struct.Cell.html#method.get_mut

Shnatsel (Jan 07 2020 at 21:50, on Zulip):

hmm, let me rethink that

RalfJ (Jan 07 2020 at 21:50, on Zulip):

the thing it doesnt have is RefCell's borrow_mut

RalfJ (Jan 07 2020 at 21:51, on Zulip):

but the thing is, actix' Cell is an Rc, so the &mut doesnt mean anything

RalfJ (Jan 07 2020 at 21:52, on Zulip):

that's why std:::Cell:get_mut makes sense, but this one doesn't

Shnatsel (Jan 07 2020 at 22:46, on Zulip):

oh cool, https://crates.io/crates/cabot is actually async and uses async-std under the hood. I'll have to try that

Tony Arcieri (Jan 07 2020 at 22:51, on Zulip):

I don't get the point of that

Tony Arcieri (Jan 07 2020 at 22:51, on Zulip):

if that's what you want, why not surf?

Tony Arcieri (Jan 07 2020 at 22:52, on Zulip):

does it have significantly fewer dependencies?

Shnatsel (Jan 07 2020 at 23:28, on Zulip):

surf has every async dependency under the sun, except async-std

Tony Arcieri (Jan 08 2020 at 03:37, on Zulip):

oh weird, I guess it's only a dev-dependency?

Shnatsel (Jan 15 2020 at 22:14, on Zulip):

Here's the final draft of the article, i.e. this is what's going to be on reddit tomorrow if you don't stop me:
https://medium.com/@shnatsel/smoke-testing-rust-http-clients-b8f2ee5db4e6
Proofreading is _very_ welcome.

Yerkebulan Tulibergenov (Jan 16 2020 at 06:35, on Zulip):

I loved it! You wrote sever instead of server in one place, purpoise instead of purpose in another. Since you mention Go in the end, would you be able to run the same experiment with it for fun?

Shnatsel (Jan 16 2020 at 13:35, on Zulip):

Thanks! I am rather tired of doing this after performing and analyzing 10+ runs, so I'll leave trying Go as an exercise to the reader.

Shnatsel (Jan 16 2020 at 20:02, on Zulip):

Finalized and posted: https://medium.com/@shnatsel/smoke-testing-rust-http-clients-b8f2ee5db4e6
Thanks for all the help!

Shnatsel (Jan 16 2020 at 22:45, on Zulip):

Wow, I'm getting some high praise on Reddit! https://www.reddit.com/r/rust/comments/epoloy/ive_smoketested_rust_http_clients_heres_what_i/

Shnatsel (Jan 18 2020 at 16:13, on Zulip):

Hmm, this is having some profound consequences: https://words.steveklabnik.com/a-sad-day-for-rust
I've already solicited feedback elsewhere, but I liked to note here that I'm very much open to input on how I could have done better.

Stuart Small (Jan 18 2020 at 16:24, on Zulip):

I think the tone was harsh but at least it focused on the code. The harshness was compounded by the follow up from others.

Stuart Small (Jan 18 2020 at 16:24, on Zulip):

For me that was the real problem. The follow up became really personal and turned into direct attacks

Stuart Small (Jan 18 2020 at 16:28, on Zulip):

Doesn't mean there isn't room to improve

centril (Jan 18 2020 at 16:31, on Zulip):

@Shnatsel I think your post was fair, accurate, and necessary. I don't think you are to blame.

centril (Jan 18 2020 at 16:31, on Zulip):

and I'm sorry for the abuse leveled at you.

Stuart Small (Jan 18 2020 at 16:36, on Zulip):

Maybe it would be a good time to talk about guidelines on how to present posts like this? We've all had issues where other developers have trouble accepting security related bug reports especially when there isn't a clear POC to come along with it. Hopefully in the future there will be others filing the bugs and writing these posts too. And guidance goes a long way then

Stuart Small (Jan 18 2020 at 16:37, on Zulip):

I'm glad you brought this up too. Retrospection is such a good and healthy thing, for all of us

Shnatsel (Jan 18 2020 at 17:02, on Zulip):

@centril there is very little abuse levelled at me, actually. I got called a zealot, what, once? For an article with 25,000 views that's nothing.
I am my own worst critic.

Shnatsel (Jan 18 2020 at 17:04, on Zulip):

There's bound to be some abuse and negativity at these exposure levels. It's a simple matter of probability. I have elaborated on it more here: https://www.reddit.com/r/rust/comments/eq11t3/a_sad_day_for_rust/feo2eh4/

centril (Jan 18 2020 at 17:05, on Zulip):

Yeah I agree wrt. "probability"

Shnatsel (Jan 18 2020 at 17:09, on Zulip):

@Stuart Small I absolutely agree that we need clear guidelines on what the demarcation and contracts between safe and unsafe Rust should be. But it's already described in detail in the very first chapter of the Nomicon: https://doc.rust-lang.org/nomicon/safe-unsafe-meaning.html

Stuart Small (Jan 18 2020 at 22:00, on Zulip):

I was thinking more of guidelines for the community in approaching security issues than use of unsafe guidelines. The author had already made it pretty clear where he stands on unsafe use (and a few other software engineering best practices) and I don’t think more documentation of best practices would change that. I think we should look more at how we can meet maintainers where they are.

I think we’ve all run into situations where we’ve reported a vulnerability to a maintainer, company or other engineer in our org and gotten a negative reaction. Sometimes dismissal, denial or flat out hostility. It isn’t surprising. I think most engineers believe they write high quality secure code and unsurprisingly get defense when someone shows up saying otherwise. When faced with a bug report there are usually pretty clear repro steps it is easier to accept the mistake. Often security vulnerabilities don’t have those clear steps or they come with a lot of conditionals. Without POC exploit code, or POC code that relies on a situation the engineer sees as realistic, they might push back or ignore the patch.

Also there will always be libraries that don’t match what we view as proper security posture. Either they don’t care (ESR’s add a segfault handlers to silence fuzzer reports), they have other interests (willing to accept the risk of deadlocks for higher throughput ie the hyper example), not viewing the ability to trigger UB through misuse of library APIs as a library bug, or something else. It is okay if other libraries have a different threat model than we do, but when that happens we should think about how to communicate this difference to the community in a respectful way.

Part of the objection I saw several people brought up was the heavy editorialize of code quality when reviewing the libraries. Whether the criticism were right or wrong it rubbed people the wrong way. We often find ourselves diving into some of the darker, dustier parts of codebases that a lot of people can ignore and this can lead to a lot of cynicism about the code. Having that come through in writing, especially a long form piece like this, can be really hard to avoid.

To be fair, when I first read the post when you posted it in here I didn’t see a problem with the writing. I thought it was an interesting and well written article. I still don’t think it was the heart of what went wrong but it was a contributing factor.

I think out of this we can look at improving on a couple different fronts:

  1. If clear bugs are found how they can be communicated without causing a stir? I’ll be honest I’m not even possible on best practices on communicated security issues in open source. Most of my career has been working within corporate walled gardens and working out on the open is a strange feeling. Inside a corporation I am a lot less worried about other’s feelings than public posts on the internet. How can we communicate opinions on quality in a way that doesn’t put people on the defensive? When should we reach out to a maintainer in private? When should we elevate an issue to a RUSTSEC filing?

While part of this is problem is how bug reports are communicated, educating the community is another huge part. The lines between soundness error, UB and RCE started getting blurring in some of those comment sections because of some uninformed commenters. Some educational blog posts on fundamentals to point to could go a long way in stopping mobs forming on reddit. We take a lot of this for granted but others will find it useful.

  1. If there is a crate is not designed with security in mind how can this be communicated to the community without generating hostility?

This is one I’ve been thinking about a lot. The unsafe issues with actix have been known for a while but haven’t been communicated in the healthiest way. It became a bit of a meme on reddit which helped more users find out about it but also lead to this problem. How can we communicate something like this to the community without repeat of what happened? This will involve talking about how users evaluate dependencies. Are they checking cargo crev? What about cargo gieger? Are a series of “closed will not fix” UB issues in the tracker enough to scare people away? Do users care if their dependencies heavily use unsafe? I don’t know the answer here but have a couple ideas.

Stuart Small (Jan 18 2020 at 22:00, on Zulip):

Bit of a monologue. Sorry I dropped off earlier. I literally got a page right after I sent the earlier message. Today has been weird.

Shnatsel (Jan 19 2020 at 00:08, on Zulip):

Thanks for sharing your thoughts. I did not have the time to carefully read and fully understand this yet, but I will do so later.

Tony Arcieri (Jan 19 2020 at 15:57, on Zulip):

this has been bumming me out. mostly because I feel the responses to it have been so bad. Steve's post is probably the best one...

Tony Arcieri (Jan 19 2020 at 15:58, on Zulip):

"unsafety" seems like a red herring people are diving for enthusiastically, overlooking the real problem IMO was the mob mentality, plastering every GitHub comment with emoji, etc

Tony Arcieri (Jan 19 2020 at 15:59, on Zulip):

but everyone is rushing to have a hot take where they identify their particular axe to grind as The Thing That Went Wrong

Alex Gaynor (Jan 19 2020 at 16:11, on Zulip):

Yeah, the dogpiling seems like the real issue. And it's sadly incredibly common, basically anytime a specific github issue is on reddit/Orange Website/goes viral on twitter/etc. it seems to happen.

Github has better tools now than it did 5 years ago, but I'm not sure tooling can be a sufficient solution

Alex Gaynor (Jan 19 2020 at 16:11, on Zulip):

I think there's an interesting question of "are there things that can be done in our unsafety work to discourage dogpiling", but I don't have an answer to that.

Stuart Small (Jan 19 2020 at 16:12, on Zulip):

I totally agree. If I wanted to point to one thing it was reddit culture and moderation. I heard one idea of banning posting links to GitHub issues and I really like that idea. It's small, enforceable and would have direct impact.

I didn't want to bring any of that because it is outside the scope of this group. I think we do have things we can do to help though

Alex Gaynor (Jan 19 2020 at 16:16, on Zulip):

That seems like a really big hammer. It's very useful to be able to share a link to an issue in response to a question someone has!

It seems like what you want, which isn't possible ATM, is links to github issues from reddit to have a mandatory backoff time before you can comment via that link. You want the roadblock to dogpiling.

Alex Gaynor (Jan 19 2020 at 16:16, on Zulip):

s/roadblock/speedbump/

Tony Arcieri (Jan 19 2020 at 16:31, on Zulip):

I think a simple thing GitHub could do is remove the non-affirming emoji

Tony Arcieri (Jan 19 2020 at 16:32, on Zulip):

because when people come from upboat/downboat-driven sites like Reddit or HN, they use the emoji as if they were on Reddit or HN...

Alex Gaynor (Jan 19 2020 at 16:32, on Zulip):

That's another really big hammer, there's lots of valid use cases for those! At work we do lots of voting on things like "which snacks should we buy for the office" with github emoji. (This makes me so happy)

Tony Arcieri (Jan 19 2020 at 16:33, on Zulip):

haha, ok

Alex Gaynor (Jan 19 2020 at 16:35, on Zulip):

Maybe our snack voting is less important :-)

Stuart Small (Jan 19 2020 at 18:43, on Zulip):

That isn't really what I want. It is an idea that I saw thrown out that makes sense but I don't know what is best on the moderation front. There is a working group for that and it'll be best to leave that to them.

Stuart Small (Jan 19 2020 at 18:45, on Zulip):

For me to boils down to one question, what can we do to help prevent mobs forming over security issues?

Tony Arcieri (Jan 21 2020 at 21:41, on Zulip):

FWIW, it seems like this may all have a happy ending after all: the new maintainer of Actix has reached out to us about fixing the unsoundness and filing RustSec issues for it

Thom Chiovoloni (Jan 22 2020 at 00:52, on Zulip):

Happy is... relative. The old maintainer being driven out of open source makes this bittersweet at best

Tony Arcieri (Jan 22 2020 at 01:10, on Zulip):

I'm just glad something good could come of it

Last update: Jan 28 2020 at 00:35UTC