Stream: wg-secure-code

Topic: crate security


Tony Arcieri (Nov 26 2018 at 17:10, on Zulip):

fun case study https://github.com/dominictarr/event-stream/issues/116

briansmith (Nov 26 2018 at 18:45, on Zulip):

@Tony Arcieri Does that problem exist because Node doesn't have a Cargo.lock-like mechanism, or just because of the culture of not reviewing dependencies when updating the lock file?

Zach Reizner (Nov 26 2018 at 18:49, on Zulip):

npm does have a locking mechanism. Additionally, it's very easy to accidentally do a cargo update in Rust by simply adding dependencies in your Cargo.toml and then using cargo build.

briansmith (Nov 26 2018 at 18:53, on Zulip):

@Zach Reizner cargo build shouldn't do cargo update itself for this reason. There has to be a time after the lock file is updated before the build starts to verify that, you know, you're not pwning your system.

Zach Reizner (Nov 26 2018 at 18:56, on Zulip):

@briansmith Perhaps I'm misunderstanding what's actually going on. If one adds a dependency to Cargo.toml and then runs cargo build, the Cargo.lock is updated before building the crate.

Tony Arcieri (Nov 26 2018 at 18:56, on Zulip):

@briansmith I'd go with the latter, that nobody reviews dependencies before updating them, but even if that were to happen in this particular case, the payload was hidden in minified JS so it wouldn't have been easy to see

Zach Reizner (Nov 26 2018 at 18:58, on Zulip):

It looks shockingly similar to the theoretical hack outlined here: https://hackernoon.com/im-harvesting-credit-card-numbers-and-passwords-from-your-site-here-s-how-9a8cb347c5b5

Tony Arcieri (Nov 26 2018 at 19:01, on Zulip):

https://github.com/bitpay/copay/issues/9346 <--- target appears to be cryptocurrency wallets

Tony Arcieri (Nov 26 2018 at 19:01, on Zulip):

also re: npm and lockfiles, it has one

Tony Arcieri (Nov 26 2018 at 19:03, on Zulip):

the attack only affects people who update, but it sounds like the payload wound up in a version which was up for awhile

Tony Arcieri (Nov 29 2018 at 16:47, on Zulip):

"fun thread" https://users.rust-lang.org/t/how-does-crates-io-differ-from-npm/22658

Tony Arcieri (Nov 29 2018 at 16:48, on Zulip):

I'm surprised how passionate some people are about not sandboxing build.rs

Tony Arcieri (Nov 29 2018 at 16:48, on Zulip):

So even a perfect sandbox is still a total dick move towards end users and will spread malware far and wide

Tony Arcieri (Nov 29 2018 at 16:48, on Zulip):

:confused:

briansmith (Nov 29 2018 at 20:22, on Zulip):

I think it would be good to see a concrete proposal for sandboxing that includes a clear threat model. Perhaps starting with the Bazel design as a reference. Then we can see how useful it would be.

Tony Arcieri (Dec 18 2018 at 17:22, on Zulip):

TIL: https://github.com/dpc/crev/tree/master/cargo-crev

Tony Arcieri (Dec 18 2018 at 17:23, on Zulip):

just hopped in their gitter channel

Shnatsel (Dec 18 2018 at 19:46, on Zulip):

It's a lot more sane than most proposed solutions to the trust problem. I can see this approach being viable for e.g. a company that wants to audit third-party code they use.

nikomatsakis (Dec 18 2018 at 20:02, on Zulip):

seems to raise the question of whether you trust the reviewers :)

nikomatsakis (Dec 18 2018 at 20:02, on Zulip):

still, interesting concept

Shnatsel (Dec 18 2018 at 20:05, on Zulip):

web of trust concept hasn't really worked out for PGP, but that's inconclusive because PGP is held back by its UI, or rather lack thereof

Shnatsel (Dec 18 2018 at 20:05, on Zulip):

Still, for a commercial company I can see this being viable

Tony Arcieri (Dec 18 2018 at 20:43, on Zulip):

@nikomatsakis haha, I just asked that

Tony Arcieri (Dec 18 2018 at 20:43, on Zulip):

any thoughts about sybil attacks on a tool like this? e.g. someone maintaining a bot army which generates false reviews for the purposes of making a crate which contains a malicious payload appear reviewed

Tony Arcieri (Dec 18 2018 at 20:43, on Zulip):

(in their Gitter, guess I'll see!)

Shnatsel (Dec 18 2018 at 22:58, on Zulip):

Oh, there is no way around people making lots of fake reviews on malicious crates. No amount of identity management would prevent it. I can still sybil it to hell and back.
What they're building here is not a new concept, it's a basic web of trust. All the research in the past 30 years done on web of trust concept and in the past 15 years on PGP applies here.
In such a system the amount of reviews should not be a factor at all. The only factor is the count of people whom you trust.

Shnatsel (Dec 18 2018 at 22:59, on Zulip):

And it would work well only if you have a small number of trusted people who review incoming code, like a commercial company's security department or some such. Then you can use it to determine if it's okay to use or not, in the sense that it's been reviewed by your security department.

Shnatsel (Dec 18 2018 at 23:04, on Zulip):

You know, if I wanted to take over systems running Rust, I'd just make a PR against the stdlib optimizing or refactoring some important function, with a non-obvious bug. That's the cheapest option right now, and is already proven to work well.

Tony Arcieri (Dec 18 2018 at 23:04, on Zulip):

@Shnatsel yeah asked and got "It's WoT" back and had this to say:

Tony Arcieri (Dec 18 2018 at 23:05, on Zulip):

alrighty. I am generally skeptical of WoT efforts, but I think the problems they have might be solvable with a sufficiently good user experience. so good luck!

Shnatsel (Dec 18 2018 at 23:05, on Zulip):

But assuming I wanted to inject malicious code into a crate and then claim it's all good... yeah, I still probably wouldn't bother with messing with this trust concept because it's very easy to sneak fatal bugs through code review, humans just ain't good at dealing with unsafe code.
And there is no penalty for trying.

Tony Arcieri (Dec 18 2018 at 23:05, on Zulip):

regarding the "All the research in the past 30 years done on web of trust concept and in the past 15 years on PGP applies here.", well... I've been to SOUPS, and the research into that does not paint a good picture

Tony Arcieri (Dec 18 2018 at 23:06, on Zulip):

I'd still like to hope it can be solved with "sufficiently good UX"

Tony Arcieri (Dec 18 2018 at 23:07, on Zulip):

which is definitely easier in a greenfield system than something like PGP :wink:

Tony Arcieri (Dec 18 2018 at 23:10, on Zulip):

that said, I'll definitely be trying it out

Tony Arcieri (Dec 18 2018 at 23:11, on Zulip):

@Shnatsel as it were, I just saw an elliptic curve-based ZKP system containing a flaw that seemed obvious to me get professionally reviewed by several well-respected cryptographers and cryptography services groups...

Tony Arcieri (Dec 18 2018 at 23:12, on Zulip):

...it took something like 4 audits before one of the reviewers (QuarksLab) wrote up the issue I was concerned with

Tony Arcieri (Dec 18 2018 at 23:12, on Zulip):

(they do good work, we used them at Square)

Shnatsel (Dec 18 2018 at 23:13, on Zulip):

What's SOUPS?

Tony Arcieri (Dec 18 2018 at 23:15, on Zulip):

SOUPS is a security-oriented UX research conference

Tony Arcieri (Dec 18 2018 at 23:15, on Zulip):

it's sort of like the UX research side of PETS

Shnatsel (Dec 18 2018 at 23:16, on Zulip):

Woah, I'm glad that exists

Shnatsel (Dec 18 2018 at 23:17, on Zulip):

WoT will never work as a public system because first, it's not that hard to write underhanded code that does malicious things while passing review (see underhanded Rust competition), and second, it's not that hard to establish yourself as a trustworthy guy in the community and make people treat your signature as trusted.

Shnatsel (Dec 18 2018 at 23:18, on Zulip):

WoT kind of works in a company with a sorta-trusted security department, but the point about underhanded code still stands

Shnatsel (Dec 18 2018 at 23:20, on Zulip):

formal proofs of certain properties about the code might help there, but even those hinge on certain assumptions that can be violated, and I guess I could sneak a vulnerability past that as well if I really wanted. But a pull request with an exploit against stdlib is still cheaper.

Tony Arcieri (Dec 18 2018 at 23:20, on Zulip):

in terms of actual mechanisms built on a WoT model, there aren't a whole lot of success stories

Tony Arcieri (Dec 18 2018 at 23:21, on Zulip):

PGP in particular is quite confusing with its multitude of similarly named trust levels

Tony Arcieri (Dec 18 2018 at 23:24, on Zulip):

https://www.phildev.net/pgp/gpgtrust.html

Tony Arcieri (Dec 18 2018 at 23:24, on Zulip):

"marginally", "fully", and "ultimately" heh

Tony Arcieri (Dec 18 2018 at 23:25, on Zulip):

...valid keys, meaning one of the following:
- You have signed it personally
- It has been signed by one fully trusted key
- It has been signed by three marginally trusted keys

Shnatsel (Dec 18 2018 at 23:27, on Zulip):

It was always confusing and signing required command line and was always a hassle. Oh and it's not immediately obvious to people that signing in PGP only verifies that this public key belongs to this person but not that you can trust signatures made by this public key... I could go on and on about all the ways PGP is dysfunctional.

Tony Arcieri (Dec 19 2018 at 18:16, on Zulip):

interesting https://twitter.com/FiloSottile/status/1075429069888598016

Tony Arcieri (Dec 19 2018 at 18:17, on Zulip):

I guess Filippo is writing a custom Trillian personality for "binary transparency"-like purposes

Tony Arcieri (Dec 19 2018 at 18:19, on Zulip):

I guess the crates.io index sort of gives you something similar... unfortunately git is awful

Tony Arcieri (May 21 2019 at 22:09, on Zulip):

crate dependency permissions coming up again: https://internals.rust-lang.org/t/cargo-permissions-to-detect-tampered-dependecies/10236

Tony Arcieri (May 21 2019 at 22:10, on Zulip):

my list of the previous topics along the same lines grows longer and longer

DevQps (May 22 2019 at 10:00, on Zulip):

So I was thinking about this as well and I had some kind of idea:

I can imagine people should yank crates when a security vulnerability is present, or a bad bug. Old versions of a library would not be yanked (unless they are so deprecated that they cannot work with other crates anymore).

What about creating some kind of platform, let's call it amiyanked for now, that iterates through the tree of dependencies to check for yanked crates. So if a specific crate depends on a yanked dependency it is marked "should be updated/yanked". After all, that crate might also be vulnerable or might depend on a bugged crate. If we do this iteratively we get a list of crates that should be yanked / updated as well.

Since crates also specify the email of each author we could even use their email addresses to send them automatic notifications when one of their dependencies has been yanked. (Not sure if that is against any privacy rules, but it would be nice).

This way we could maybe keep the eco system at least aware of threats and potentially more safe as well? I guess if many people use crate A that depends on vulnerable crate B, people are likely to but a bit more pressure or effort into having crate A update its dependency to crate B, whereas now people still have to manually find that out themselves.

What do you think?

Tony Arcieri (May 22 2019 at 15:33, on Zulip):

@DevQps I had proposed something similar as a way that Cargo-proper could have a minimum viable RustSec integration

Tony Arcieri (May 22 2019 at 15:49, on Zulip):

https://internals.rust-lang.org/t/pre-rfc-reviving-security-advisories-in-crates-io-rfc-pr-1752/9017/14?u=bascule

Tony Arcieri (May 22 2019 at 15:50, on Zulip):

the core idea is giving users some feedback about yanked crates. I suggested doing it during cargo build and that put some people off

Tony Arcieri (May 22 2019 at 15:50, on Zulip):

it seems like something where it could be a periodic nag, like once a day or something

DevQps (May 24 2019 at 13:55, on Zulip):

I think that would be quite a good idea. Developers need some kind of nudge or hint that they should yank their crates and update them as well I guess.

Gerardo Di Giacomo (Jun 15 2019 at 21:40, on Zulip):

the core idea is giving users some feedback about yanked crates. I suggested doing it during cargo build and that put some people off

doesn't npm do it now?

Tony Arcieri (Jun 17 2019 at 21:36, on Zulip):

do what now? have integrated security audit functionality?

Tony Arcieri (Jun 17 2019 at 21:36, on Zulip):

the important part is doing it for cargo build and not just cargo update

defunct (Jun 18 2019 at 15:51, on Zulip):

not to suggest any sort gatekeeping here but it does seem that some sort of committee for security related confidence scoring for crates might be beneficial to prevent uninformed users from making bad design decisions, something that also takes into consideration downstream deps, previous vuln history, clearly poorly written crates. it would be nice to prevent senseless dep explosion that increase attack surface in general. obviously would need a relatively high level of automation for the entire process to be sustainable. - (didn't read all of the backlog, apologies if this has been brought up before)

Tony Arcieri (Jun 18 2019 at 16:12, on Zulip):

@defunct yeah there's been a lot of talk about ideas along those lines

defunct (Jun 18 2019 at 16:22, on Zulip):

@Tony Arcieri ah thanks, any chance there a WIP documentation / RFC proposal for potential requirements and implementation for the topic or just discussion for now?

Tony Arcieri (Jun 18 2019 at 16:25, on Zulip):

some ideas along those lines here https://blog.rust-lang.org/2017/05/05/libz-blitz.html

Shnatsel (Jun 20 2019 at 09:41, on Zulip):

And this thread is also solved by cargo-crev: https://github.com/dpc/crev

Shnatsel (Jun 20 2019 at 09:45, on Zulip):

I am really tempted to start using that and file a "this is dangerous" for libflate. Reasons being:
https://github.com/sile/libflate/issues/16
https://github.com/sile/libflate/issues/29
https://github.com/sile/libflate/issues/31
And there is probably more where that came from.
I was horrified when it showed up on crates-audit as a dependency of reqwest, but fortunately it's just a dev dependency.

Shnatsel (Jun 20 2019 at 13:37, on Zulip):

Oh hey look, I've found another memory safety bug: https://github.com/sile/libflate/issues/33
This one might be actually exploitable.

Tony Arcieri (Jun 20 2019 at 13:42, on Zulip):

oof

Stuart Small (Jun 20 2019 at 15:36, on Zulip):

I am really tempted to start using that and file a "this is dangerous" for libflate. Reasons being:
https://github.com/sile/libflate/issues/16
https://github.com/sile/libflate/issues/29
https://github.com/sile/libflate/issues/31
And there is probably more where that came from.
I was horrified when it showed up on crates-audit as a dependency of reqwest, but fortunately it's just a dev dependency.

In issue 16 why do you set detect_odr_violation=0? I haven't run into that in rust, what can trigger it?

Shnatsel (Jun 20 2019 at 15:43, on Zulip):

I have no idea what it is, seems to be some kind of check specific to C++. I used to get it randomly on seemingly benign code, so I've disabled it.

Shnatsel (Jun 20 2019 at 15:47, on Zulip):

...which does not sound reassuring

Stuart Small (Jun 20 2019 at 15:49, on Zulip):

I've heard of a few other rust specific asan rust specific false positives so it doesn't bother me too much. Specifically around zero sized types. I haven't hit them personally though

Shnatsel (Jun 20 2019 at 16:06, on Zulip):

Alternatives to libflate are:
1. inflate, which I've already cleansed of unsafe blocks save one, and found a bug in that one, so I'm not really worried about it. But it's even slower.
2. miniz_oxide, which has withstood my fuzzing attempts admirably, including differential fuzzing with libdiffuzz. It's much faster. I've opened the code now and... it's not reassuring.

Shnatsel (Jun 20 2019 at 16:11, on Zulip):

https://github.com/Frommi/miniz_oxide/blob/master/miniz_oxide/src/deflate/core.rs#L314 - this makes an assumption that is only true for allocations made with stdlib containers
https://github.com/Frommi/miniz_oxide/blob/master/miniz_oxide/src/deflate/core.rs#L280 - same here but not explicitly noted. Could be exploitable if writing to user-supplied buffer
"check if this is safe" TODOs:
https://github.com/Frommi/miniz_oxide/blob/master/miniz_oxide/src/deflate/core.rs#L370
https://github.com/Frommi/miniz_oxide/blob/master/miniz_oxide/src/deflate/core.rs#L572

Shnatsel (Jun 20 2019 at 16:18, on Zulip):

This function is very weird:
https://github.com/Frommi/miniz_oxide/blob/master/miniz_oxide/src/deflate/core.rs#L276
It doesn't use pos argument in any meaningful way, it seems superfluous. Also I don't understand what #[cfg(all(target_endian = "little", test))] does, I thought #[cfg(test)] should only be used for modules?
Does anyone understand what's happening in this code?

Dylan MacKenzie (Jun 20 2019 at 16:47, on Zulip):

The #[cfg(test)] means that function is only called in code that's also #[cfg(test)] (so the test module below)

Dylan MacKenzie (Jun 20 2019 at 16:49, on Zulip):

but I don't know what's going on with pos, presumably they meant to write a u16 to slice[pos..=pos+1] but forgot to add the offset?

Dylan MacKenzie (Jun 20 2019 at 16:50, on Zulip):

AFAICT it's only called with pos = 0, so luckily for them nothing breaks.

Dylan MacKenzie (Jun 20 2019 at 16:51, on Zulip):

Based on the surrounding code, they forgot to call offset(pos)

Shnatsel (Jun 20 2019 at 17:02, on Zulip):

Thanks! I'll file an issue on github.
But this is still not reassuring. And flate2 is by far the most popular gzip/zlib crate.

Shnatsel (Jun 20 2019 at 17:13, on Zulip):

Okay, opened a PR: https://github.com/Frommi/miniz_oxide/pull/45

Shnatsel (Jun 20 2019 at 17:16, on Zulip):

Also not reassuring - looks like they have some kind of underlying allocator corruption going on, or are mixing regions allocated from C and from Rust: https://github.com/Frommi/miniz_oxide/issues/14
This has been open for ages and is tagged help wanted, so looks like miniz_oxide devs aren't going to fix it. I don't think I'm competent enough either. Any takers?

Shnatsel (Jun 22 2019 at 14:29, on Zulip):

Oh, here's an interesting pull request for miniz_oxide: https://github.com/Frommi/miniz_oxide/pull/36
It fixes some segfaults because of "type confusion", but I'm not sure if that is exploitable in a context other than DoS. It's not merged, so the bug still exists.
Is anyone interested in evaluating the impact? This crate has 180,000 downloads per month, so if this is exploitable this would be a high-profile vulnerability by Rust standards.

Alex Gaynor (Jun 22 2019 at 14:32, on Zulip):

Depends what types you can confuse. If you can confuse something where you've got a user-controlled int at the same offset as a pointer that's a potentially very powerful exploit primitive

Shnatsel (Jun 23 2019 at 05:40, on Zulip):

I had a bout of insomnia, so I've poked miniz_oxide some more. Lo and behold, a buffer overflow on write
https://github.com/Frommi/miniz_oxide/pull/47
Now to figure out if this is actually exploitable

Shnatsel (Jun 23 2019 at 19:49, on Zulip):

Ooooh hey look, another vulnerability in smallvec: https://github.com/servo/rust-smallvec/issues/148

Shnatsel (Jun 23 2019 at 19:49, on Zulip):

Without an advisory, too

Alex Gaynor (Jun 23 2019 at 21:47, on Zulip):

Any reason not to just file an advisory yourself?

Shnatsel (Jun 23 2019 at 21:49, on Zulip):

I want to educate SmallVec maintainers. Also, I'm too lazy to go in and try to track down when was the vulnerability introduced to understand which versions are affected

Shnatsel (Jun 23 2019 at 21:50, on Zulip):

Also, speaking of type confusion in miniz_oxide: it's known to cause a double free, so probably exploitable. Still not fixed. https://github.com/Frommi/miniz_oxide/pull/36 - PR actually not ready for merging

Shnatsel (Jun 26 2019 at 01:14, on Zulip):

Threat estimation question: calling drop() on uninitialized memory could lead to arbitrary code execution in the worst case, right?

Shnatsel (Jun 26 2019 at 01:14, on Zulip):

I'm observing it on interesting non-Copy types like BufReader

Alex Gaynor (Jun 26 2019 at 01:16, on Zulip):

On an arbitrary type? Sure, it easily potentially triggers UAF, also can be basically any other sort of memory corruption.

Shnatsel (Jun 26 2019 at 01:25, on Zulip):

Thanks, UAF is a good way to put it. This is also in libflate crate, for reference. Filing an issue now.

Shnatsel (Jun 27 2019 at 13:50, on Zulip):

Okay, I am done auditing libflate. I have PRs outstanding for removing most unsafe blocks, and I've reported vulnerabilities in the rest of them to the maintainer. So now we just wait for me or the maintainer or myself to come up with fixes.

Tony Arcieri (Jul 01 2019 at 16:50, on Zulip):

nice, great work there @Shnatsel !

Tony Arcieri (Jul 01 2019 at 16:50, on Zulip):

as it were, we're using libflate right now :weary:

Tony Arcieri (Jul 01 2019 at 17:08, on Zulip):

so does smallvec do anything that heapless::Vec doesn't?

Tony Arcieri (Jul 01 2019 at 17:22, on Zulip):

this looks interesting: https://blog.trailofbits.com/2019/07/01/siderophile-expose-your-crates-unsafety/

Tony Arcieri (Aug 29 2019 at 21:50, on Zulip):

interesting https://twitter.com/FiloSottile/status/1075429069888598016

so this shipped!

Tony Arcieri (Aug 29 2019 at 21:50, on Zulip):

https://twitter.com/FiloSottile/status/1167156608545280005

Tony Arcieri (Aug 29 2019 at 21:50, on Zulip):

looks really interesting

Tony Arcieri (Aug 29 2019 at 21:51, on Zulip):

I've been thinking a lot about a similar "binary transparency"-style reproducible build system

Tony Arcieri (Aug 29 2019 at 21:51, on Zulip):

although I am less excited about Trillian lately than I used to be

Tony Arcieri (Aug 29 2019 at 21:51, on Zulip):

I have a pretty crazy idea I need to flesh out better :wink:

Tony Arcieri (Aug 29 2019 at 21:53, on Zulip):

there are a ton of really good ideas in the Go Checksum Database though

Shnatsel (Aug 30 2019 at 16:03, on Zulip):

This looks neat. Perhaps bring it up on Reddit?

jakubadamw (Aug 30 2019 at 16:27, on Zulip):

(deleted)

Last update: Nov 11 2019 at 22:00UTC