Security review notes for followup

[randomdross]

Hi @blt, as requested I'm including security review notes here. (I'm not particularly Rust-savvy at the moment, so some of this may not be interesting in reality.) PTAL and assess if there's anything worth following up on.

---

Various “unsafe” usages in Rust code: https://github.com/postmates/cernan/search?q=unsafe&unscoped_q=unsafe

• If any of this involves handling untrusted data, and there is a bug, presumably that can enable memory corruption & thus RCE
  • Note from John Koenig: "Lua scripts were only in use while Postmates still relied on Cernan to ingest / transform Postmates owned logs. As we are currently ingesting logs via fluentd, my expectation is that this is a non-issue at lest for us internally."
• For example, here:

  cernan/src/protocols/prometheus.rs

  Line 2988 in b770703

  parse_descriptor_proto()

• What is parse_descriptor_proto() parsing?
  • Note from John Koenig: "This is a generated protobuf definition for prometheus's binary protocol.
    AFAIK, this is unused in the code base and is a candidate for deprecation. The Prometheus source currently only supports Prom's text protocol compressed with gzip."
• Or:

  cernan/src/config.rs

  Line 821 in e63c6e6

  fl.max_read_bytes = tbl.get("max_read_bytes")

• “Someday a static analysis system will flag this as unsafe. Welcome.”
• OK =) Well, at least this doesn’t appear to be in an unsafe block, so maybe OK?
  “By default cernan will put its on-disk queues into TMPDIR. While this is acceptable for testing and development this is not desirable for production deployments.”
• Is this essentially insecure by default? If it’s insecure with the default setting that should probably at minimum be called out explicitly in the wiki.
  https://github.com/postmates/cernan/wiki/Configuration#Scripts-Directory
• Same issue as above. If something can create /tmp/cernan-scripts then it seems cernan will run untrusted scripts
  For SourcesNative: “The payload must be length prefixed, the length being an unsigned, network-ordered 32 bit integer.”
• What if it’s not?
• (Ref: https://github.com/postmates/cernan/wiki/SourcesNative)
  Similar with SourcesStatsd -- how is malformed input handled? If we can hit any of the code marked unsafe then things get interesting & there is potential for memory corruption / RCE.
  https://github.com/postmates/cernan/wiki/SinksElasticSearch
• secure :: whether to attempt HTTPS or not with the elasticsearch host [default: false]
  • How about changing the default to True. =)
    How does cernan authenticate to any of the sinks (or sources for that matter)?

---

Sorry if the formatting makes that a bit hard to digest. Let me know if you have any questions on any of this.

[blt]

* If any of this involves handling untrusted data, and there is a bug, presumably that can enable memory corruption & thus RCE

Is this referring to a specific bit of code?

* What is parse_descriptor_proto() parsing?

Couldn't tell you. This is part of the generated protobuf code. It'd be reasonable to regen this and see if the unsafe bits have disappeared. It'd also be willing to open up an issue for deprecation in the next release, removal in the release after.

* “Someday a static analysis system will flag this as unsafe. Welcome.”

Yeah, welcome to the fun edge cases in Rust allocation. If an allocation fails the entire program panics. We should document a limit to this value and check the user's input, emit a warning if the value is too large.

* Is this essentially insecure by default?  If it’s insecure with the default setting that should probably at minimum be called out explicitly in the wiki.

Depends on what cernan is shipping of course, but, yeah. I guess it is. The wiki should be amended. I'm open to suggestions for alternative behavior. My main thinking when I wrote this was that it's easy to screw up the operation of cernan by fiddling with those files. We never implemented checksums or recovery for queue files, on account of there not being a call for them at the time of implementation.

Weak area, generally.

Oh, you're talking about scripts here. Well, both are probably unsafe. If you craft a special purpose queue file for, say, the kafka sink you can get cernan to ship whatever you want. Same deal for scripts.

* Similar with SourcesStatsd -- how is malformed input handled?  If we can hit any of the code marked unsafe then things get interesting & there is potential for memory corruption / RCE.

IIRC all of these have been fuzzed. Cernan should be broken up into sub-crates and fuzz targets implemented for the sources so we can get this audit cycle going for each release. My recollection is that malformed inputs are detected, logged in self-telemetry and dropped.

* How about changing the default to True.  =)

Ahm fer it.

How does cernan authenticate to any of the sinks (or sources for that matter)?

None of the sources auth, the few sinks that have auth credentials are, iirc, not wired up to authenticate. IP whitelisting and/or presence in a blessed subnet was Good Enough. Obvious issues there.

[randomdross]

Is this referring to a specific bit of code?
Nope just general commentary.

It'd also be willing to open up an issue for deprecation in the next release, removal in the release after.
I opened: #462

We should document a limit to this value and check the user's input, emit a warning if the value is too large.
I opened: #463

Is this essentially insecure by default? If it’s insecure with the default setting that should probably at minimum be called out explicitly in the wiki.
https://github.com/postmates/cernan/wiki/Configuration#Scripts-Directory
I filed #464 to track.

How about changing the default to True. =)
I filed #465 to track.

Re auth, I filed #466 to track.

I think we're now tracking all the outstanding issues and it should be safe to close this master review bug.

[blt]

Rad, sounds good. Closed.