[MIR] [mir] yaml-cpp

Hi,
FYI The current version 0.5.2-4ubuntu2 should be affected by
- https://www.cvedetails.com/cve/CVE-2017-11692/
- https://www.cvedetails.com/cve/CVE-2017-5950/
right?

On security review these might become a requirement to be fixed, so FYI ahead in case you want to fix them.

Oh, bah! I had a section detailing those CVEs but obviously overwrote it at some point. Damn browser editors.

Yeah, those CVEs affect the current Ubuntu package, and are unfixed upstream.

As far as I can tell they're only DoS risks - the first is an assert() hit on a particular yaml construct, second is stack exhaustion via unbounded recursion on specially crafted input.

*Mir* doesn't use this library to parse untrusted input, so we don't much care about them, but other users might.

 - the packaged version is 3 1/2 years old. The packaged
   version failed to build with boost 1.67, now fixed in
   Ubuntu. Upstream has 0.6.2. I guess we want to have
   the new version in Ubuntu, and track newer versions.
 - please add symbols files or use another means of symbols
   checking for this library.
 - could require some autopkg tests (but not required).

 - finally the security team should review that package.
   maybe only once the 0.6.2 version is packaged.

Please forgive my humor. 😁

that's now blocking some transitions. Please address this issue

Urgh. Some of the rdepends of yaml-cpp are not built with c++11 support, and so FTBFS against the new yaml-cpp.

I'll see if I can fix that tomorrow.

yaml-cpp 0.6.2-1ubuntu1 uploaded, with a symbols file (and proposed on salsa, too https://salsa.debian.org/debian/yaml-cpp/merge_requests/2 ). I'll upload rebuilds of the rdepends, too.

This should be ready to review.

looks ok. reassigning to the security team for a review.

please don't forgot the no-change uploads for the transition.

Upstream seems remarkably unresponsive.

I've had a fairly low impression of YAML the specification after reading https://arp242.net/weblog/yaml_probably_not_so_great_after_all.html#its-pretty-complex

What brought us to this point? Were alternatives considered and discarded for good reasons?

The json spec is drastically smaller and less surprising. (It's not perfect.)

We already have several json parsers in main:
- json-c
- json-glib
- libfastjson (only bionic and newer)
- libjsoncpp (only in main in xenial and newer)

Thanks

We considered json, yaml, and toml as the configuration format, as well as just an ad-hoc configuration for the single feature which (currently) requires configuration.

We choose yaml mainly because it seems to be the consensus configuration format for Canonical projects.

So, it would be entirely possible to switch to json or toml and write a small converter program (which would go to Universe) to handle the conversion of existing configuration files.

It would be a bit of work, but not an outrageous amount of work. How much does the security team want to not have yaml-cpp in main?

@seth

json-c / json-glib / libfastjson are C, rather than CPP.
libjsoncpp may be suitable.

But

json, by default is unreadable garbage. Whilst yaml is actually readable. I do understand that it is syntactic sugar / nice to have. But that also makes all the difference. And indeed yaml is fairly standard. We use it in upstart netplan.io cloud-init juju snapcraft etc. So displeasure of yaml as a format is a bit too late and out of scope for security review of an yaml-cpp or any other yaml library suitable to be used from C++.

libyaml-dev is in main, but i'm not sure how nice it is to use that one from C++, hence my guess is the preference is to libyaml-cpp-dev in main.

Yeah, when surveying the choices for yaml libraries we looked at the C++ libraries (and I forgot that Mir was still in main, so didn't consider the library's component, just it's availability and maintenance in Ubuntu/Debian).

It would probably not be an unreasonable amount of work to write a small internal C++ wrapper around libyaml-dev, if the security team feels strongly about it.

xnox, raof, many thanks for your replies earlier.

I've read through yaml-cpp and can see the benefits: it sticks to C++ things and is remarkably readable. There's a lot of tests.

But there's six CVEs that have been completely ignored. While at least some of the CVEs wouldn't affect Mir's use (no one is going to feed Mir a config file with a few thousand '{' or '[' characters) once it's in main we'd need to assess issues from perspective of all consumers.

CVE-2017-11692 is extremely poor error handling.

https://github.com/jbeder/yaml-cpp/issues/519 CVE-2017-11692
https://github.com/jbeder/yaml-cpp/issues/459 CVE-2017-5950
https://github.com/jbeder/yaml-cpp/issues/655 CVE-2018-20573
https://github.com/jbeder/yaml-cpp/issues/654 CVE-2018-20574
https://github.com/jbeder/yaml-cpp/issues/660 CVE-2019-6285
https://github.com/jbeder/yaml-cpp/issues/657 CVE-2019-6292

FORTIFY_SOURCE is missing from the build logs.

I have to wonder if this package has seen sufficient real-world use.

Would the Mir team be in a position to work with upstream on addressing these issues? If we accept yaml-cpp into main it'd be nice to have these issues addressed before 20.04 LTS.

Thanks

Huh, I see people have started a bunch more whacking on yaml-cpp since the start of this MIR. Great!

The Mir team certainly have the skills required to submit PRs for these, and failing anything else we can distro-patch them in. If fixing these bugs is the price of security-team signoff, I think we can pay that (but let me ping Saviq first ☺).

I reviewed yaml-cpp version 0.6.2-4fakesync1 as packaged in
So, security team ACK on promoting yaml-cpp to main is granted provided
sarnold@hunt:~/ubuntu/security/audits/yaml-cpp/disco/audits$ cat bug.txt
I reviewed yaml-cpp version 0.6.2-4fakesync1 as packaged in
disco-proposed. This shouldn't be considered a full security audit but
rather a quick gauge of maintainability.

- There are six CVEs found since 2017 and as far as I can tell none have
  been addressed since they were discovered. The library appears to be
  entirely unsuitable for handling untrusted input. (And even for trusted
  input, crashing rather than returning an error message is really poor
  user experience.)

  If we're going to have this in main, then we need to work with upstream
  to provide the missing reliability.

- Build-Depends: cmake, debhelper
- Does no cryptography
- Does no networking
- Does not daemonize
- No pre/post inst/rm scripts
- No init scripts
- No systemd unit files
- No dbus service files
- No setuid files
- No executables in PATH
- No sudo fragments
- No udev rules
- Decent-sized test suite run during build
- No cron jobs
- Some CMake warnings, large number of warnings from test suite, nothing
  too bad

- Does not spawn subprocesses
- Older c++ style memory management
- util/parse.cpp can take a filename in argv[1]
- Probably insufficient logging for real use, but logging looked safe
- No environment variable use
- No privileged functions
- No cryptography
- No networking
- No privleged portions of code
- No temp files
- No webkit
- cppcheck results only in test suite
- No policykit

The code is clean and simple, but perhaps too simple -- the six open
CVEs show insufficient handling for unexpected inputs. This library is
currently unsafe for use on untrusted inputs, and will probably give a
poor user experience for innocent typos.

So, security team ACK on promoting yaml-cpp to main is granted provided
that the requesting team:

- Promises to work with upstream developers to handle the six currently
  open CVEs. Obviously I can't expect anyone to promise that upstream will
  be receptive, but the responses to github issues appears like help would
  be accepted positively.

  If upstream doesn't respond, we'll need to either carry a delta or work
  with Debian to carry a delta.

- Address the lack of FORTIFY_SOURCE in build log. I didn't investigate
  how it came to lack FORTIFY_SOURCE, I just didn't see it in the logs
  where I expected to see it.

Thanks

As the Debian maintainer for yaml-cpp, I would be more than happy to work with the Mir team to keep yaml-cpp in sync with Ubuntu.

Thanks!

Hey all,

Sorry for the late reply, I confirm that we (~mir-team) will help maintain this package between Debian and Ubuntu. I've subscribed us to https://launchpad.net/ubuntu/+source/yaml-cpp bugs to that effect.

There's quite a lot of duplication in the CVEs where dubious input causes stack overflow. There's one underlying cause which already had a fix under review (but no tests).

I've create PRs to upstream as follows:

https://github.com/jbeder/yaml-cpp/pull/806 - fixes CVE-2017-11692

https://github.com/jbeder/yaml-cpp/pull/807 - fixes CVE-2017-5950, CVE-2018-20573, CVE-2018-20574, CVE-2019-6285 and (already marked as a dup) CVE-2019-6292.

Will await upstream reaction before creating any patches.

CVE-2017-11692 is now fixed upstream by:

https://github.com/jbeder/yaml-cpp/commit/c9460110e072df84b7dee3eb651f2ec5df75fb18

(My PR above got declined, but inspired a better fix.)