Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

OOM vulnerability in protobuf 2.0.6: RUSTSEC-2019-0003 #7760

Closed
jsirois opened this issue May 18, 2019 · 5 comments · Fixed by #7770
Closed

OOM vulnerability in protobuf 2.0.6: RUSTSEC-2019-0003 #7760

jsirois opened this issue May 18, 2019 · 5 comments · Fixed by #7770
Assignees
@jsirois

Comments

@jsirois
Copy link
Member

jsirois commented May 18, 2019 •
edited

Nightly CRON picked this up:

./build-support/bin/ci.sh -a
...
    Scanning /home/travis/build/pantsbuild/pants/src/rust/engine/Cargo.lock for vulnerabilities (318 crate dependencies)
error: Vulnerable crates found!
ID:	 RUSTSEC-2019-0003
Crate:	 protobuf
Version: 2.0.6
Date:	 2018-06-08
URL:	 https://github.com/stepancheg/rust-protobuf/issues/411
Title:	 Out of Memory in stream::read_raw_bytes_into()
Solution: upgrade to: 
error: 1 vulnerability found!

The vulnerability is here: https://github.com/RustSec/advisory-db/blob/0854d2baeea4ccbe3cb5189f6633d98fa773b388/crates/protobuf/RUSTSEC-2019-0003.toml#L1-L15

The fix is pointed to by stepancheg/rust-protobuf#411 and contained in stepancheg/rust-protobuf@66a22c8 which will eventually release with protobuf 3.0.0.
@jsirois jsirois self-assigned this May 18, 2019
jsirois pushed a commit to jsirois/rust-protobuf that referenced this issue May 18, 2019
@stepancheg @jsirois
Fix OOM on incorrect input.
0751204 
This is a cherry-pick of 66a22c8
against the v2.0 branch that omits the protobuf-fuzz changes since
protobuf-fuzz doesn't exist on the v2.0 branch.

Part of fixing pantsbuild/pants#7760.
This was referenced May 18, 2019
Closed
Merged
@jsirois
Copy link
Member Author

jsirois commented May 18, 2019 •
edited

The plan is to:

  1. Land Fix OOM on incorrect input. rust-protobuf#2.
  2. Upgrade to cargo-audit > 0.6.1 to pick up --ignore support added in Add an --ignore option rustsec/rustsec#53 and add an --ignore for RUSTSEC-2019-0003 since we know we'll be safe using Fix OOM on incorrect input. rust-protobuf#2
  3. Use Fix OOM on incorrect input. rust-protobuf#2 via git url in Pants

@stuhood
Copy link
Sponsor Member

stuhood commented May 19, 2019 •
edited

Thank you for investigating!

Given that we never open a receiving socket with protobuf, I wonder if a shorter path for this case might be to contribute a --please-ignore-this-vuln=RUSTSEC-2019-0003 flag to cargo-audit and then wait for stepancheg/rust-protobuf#411 ?

@jsirois
Copy link
Member Author

jsirois commented May 19, 2019

Agreed on adding --ignore for cargo audit! I took up rustsec/rustsec#53 last night - still working on a test.

Given that we never open a receiving socket with protobuf, ...

FWICT the affected stream read function is also used on the client side to decode messages. In our generated bazel_protos code in particular I find 67 of uses of ::protobuf::rt::read_unknown_or_skip_group which indirectly uses the patched code via:

  1. read_unknown_or_skip_group -> read_unknown:
    https://github.com/stepancheg/rust-protobuf/blob/e8ebb8802926539c97335861856847d7f3e9b851/protobuf/src/rt.rs#L818-L829
  2. read_unknown -> read_raw_bytes:
    https://github.com/stepancheg/rust-protobuf/blob/e8ebb8802926539c97335861856847d7f3e9b851/protobuf/src/stream.rs#L598-L610
  3. read_raw_bytes -> read_raw_bytes_into:
    https://github.com/stepancheg/rust-protobuf/blob/e8ebb8802926539c97335861856847d7f3e9b851/protobuf/src/stream.rs#L637-L640
  4. read_raw_bytes_into:
    https://github.com/stepancheg/rust-protobuf/blob/e8ebb8802926539c97335861856847d7f3e9b851/protobuf/src/stream.rs#L625-L629

And here e8ebb8802926539c97335861856847d7f3e9b851 is v2.0.6 which we use:

$ git show -q v2.0.6
commit e8ebb8802926539c97335861856847d7f3e9b851 (tag: v2.0.6)
Author: Stepan Koltsov <stepan.koltsov@gmail.com>
Date:   Sun Jan 13 15:04:09 2019 -0800

    Bump version

@jsirois
Copy link
Member Author

jsirois commented May 19, 2019

The --ignore PR: rustsec/rustsec#75

jsirois added a commit to jsirois/pants that referenced this issue May 19, 2019
@jsirois
Ignore RUSTSEC-2019-0003.
80b0a0e 
We have a fix in-flight in
pantsbuild/rust-protobuf#2 that will still need
this `--ignore` even when we're consuming it. Adding the `--ignore` now
silences nightly CRON audit noise in the meantime and going forward
until we can upgrade to a public official release of protobuf with the
`RUSTSEC-2019-0003` fix.

Part of fixing pantsbuild#7760
@jsirois jsirois mentioned this issue May 19, 2019
Merged
@stuhood
Copy link
Sponsor Member

stuhood commented May 20, 2019

Awesome!

jsirois added a commit that referenced this issue May 20, 2019
@jsirois
Ignore RUSTSEC-2019-0003. (#7766)
91a88ba 
We have a fix in-flight in
pantsbuild/rust-protobuf#2 that will still need
this `--ignore` even when we're consuming it. Adding the `--ignore` now
silences nightly CRON audit noise in the meantime and going forward
until we can upgrade to a public official release of protobuf with the
`RUSTSEC-2019-0003` fix.

Part of fixing #7760
jsirois pushed a commit to jsirois/rust-protobuf that referenced this issue May 20, 2019
@stepancheg @jsirois
Fix OOM on incorrect input
9d6f9a6 
This is a cherry-pick of 66a22c8
against the v2.0.4 tag that omits the protobuf-fuzz changes since
protobuf-fuzz doesn't exist in v2.0.4.

Part of fixing pantsbuild/pants#7760.
jsirois added a commit to jsirois/pants that referenced this issue May 20, 2019
@jsirois
Use patched protobuf with RUSTSEC-2019-0003 fix.
9e93f6e 
A crates index patch was needed here to ensure both our crates and
transitive dependent crates saw the same rust-protobuf. Without this we
hit many errors like:
```
   Compiling bazel_protos v0.0.1 (/home/jsirois/dev/pantsbuild/jsirois-pants/src/rust/engine/process_execution/bazel_protos)
error[E0277]: the trait bound `gen::bytestream::ReadRequest: protobuf::core::Message` is not satisfied
  --> process_execution/bazel_protos/src/gen/bytestream_grpc.rs:23:42
   |
23 |     req_mar: ::grpcio::Marshaller { ser: ::grpcio::pb_ser, de: ::grpcio::pb_de },
   |                                          ^^^^^^^^^^^^^^^^ the trait `protobuf::core::Message` is not implemented for `gen::bytestream::ReadRequest`
   |
   = note: required by `grpcio::codec::pb_codec::ser`
```

Fixes pantsbuild#7760
This was referenced May 20, 2019
Merged
Closed
jsirois added a commit that referenced this issue May 20, 2019
@jsirois
Use patched protobuf with RUSTSEC-2019-0003 fix. (#7770)
d4d8f4c 
A crates index patch was needed here to ensure both our crates and
transitive dependent crates saw the same rust-protobuf. Without this we
hit many errors like:
```
   Compiling bazel_protos v0.0.1 (/home/jsirois/dev/pantsbuild/jsirois-pants/src/rust/engine/process_execution/bazel_protos)
error[E0277]: the trait bound `gen::bytestream::ReadRequest: protobuf::core::Message` is not satisfied
  --> process_execution/bazel_protos/src/gen/bytestream_grpc.rs:23:42
   |
23 |     req_mar: ::grpcio::Marshaller { ser: ::grpcio::pb_ser, de: ::grpcio::pb_de },
   |                                          ^^^^^^^^^^^^^^^^ the trait `protobuf::core::Message` is not implemented for `gen::bytestream::ReadRequest`
   |
   = note: required by `grpcio::codec::pb_codec::ser`
```

Fixes #7760
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jsirois jsirois

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use patched protobuf with RUSTSEC-2019-0003 fix. jsirois/pants
2 participants
@stuhood @jsirois