[MIR] mdevctl, jq, libonig

Bug #1889248 reported by Christian Ehrhardt 
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
jq (Ubuntu)
Fix Released
Undecided
Unassigned
libonig (Ubuntu)
Fix Released
Undecided
Unassigned
mdevctl (Ubuntu)
Fix Released
Undecided
Unassigned

Bug Description

MDEVCTL

[Availability]
The package is in the Ubuntu universe, and builds arch-neutral
Just one binary package `mdevctl`

[Rationale]
This has two reasons:
1. it is a very nice tool to handle meidiated devices in general.
   It more and more becomes the one tool people refer to (other than fully
   manual working through sysfs)
2. libvirt with coming version 6.5 starts to use it and it will become a
   recommends.
   https://www.libvirt.org/news.html
   https://salsa.debian.org/libvirt-team/libvirt/-/merge_requests/55/diffs?commit_id=b8a07b4b19e3ba03b2d36b6def4c5e4527ec2f11

[Security]

No open bugs or CVEs (nor any in the past).

[Quality assurance]

Packaging wise it seems sage it doesn't ask debconf questions, has no
long-term outstanding bugs and upstream has a bug tracker.

OTOH it is just a pumped up shell script these days, there is no upstream
testsuite or anything like it.

I have personally used it to manage vGPUs of nvidia and intel and it worked
for both - due to the very HW specific nature of the tools use case
there can't be autopkgtests or other easy tests.

Package is maintained in Debian and Ubuntu by the server team.
No outstanding bugs:
https://tracker.debian.org/pkg/mdevctl

The HW the package deals with is not really exotic - while everyone wants
to use it (pro users) for nvidia - it does work with almost any i915 laptop
GPU of the last decade. Only you need to reboot/reconfig your laptop which makes
it a bit harder.

Lintian is happy with the package.

No obsolete or demoted dependencies.

[UI standards]

No end user UI
Just a few CLI bits used by admins and parsable output used by tools.

[Dependencies]

Fine except for `jq` which also need `libonig` (see below)

[Standards compliance]

The package meets the FHS and Debian Policy standards.

[Maintenance]

The Server team will subscribe for the package for maintenance

---

JQ

[Availability]
The package is in the Ubuntu universe, and builds for all architectures.
It has a commandline tool in pkg:jq and a lib/devel package libjq1/libjq-dev. For the current case only the commandline-tool will be promoted.

[Rationale]
Obviously one rational is the dependency from mdevctl (see above), but TBH `jq` is a quite common tool. You'll find in almost any howto dealing with json on the commandline.

[Security]

No open bugs or CVEs right now, but it has a CVE history.
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2015-8863
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2016-4074

Both fixed nowadays and about crafted JSON file leading to a crash.
The handling of these issues in the past LGTM, but security team will have a better insight to that.

[Quality assurance]

Packaging wise it seems sage it doesn't ask debconf questions, has no
long-term outstanding bugs in Ubuntu and upstream has a bug tracker.

It has build time tests that run and would break build.

Lintian is rather happy with the package and no special HW is needed to work with it

No obsolete or demoted dependencies.

[UI standards]

No end user UI
Just a few CLI bits used by admins and parsable output used by tools.

[Dependencies]

Fine except that is also needs `libonig` (see below)

[Standards compliance]

The package meets the FHS and Debian Policy standards.

[Maintenance]

The Server team will subscribe for the package for maintenance.
Although TBH while "needed as dep for a server package" this is clearly not a server package from it's own use case, so ownership might be re-discussed.

---

LIBONIG

[Availability]
The package is in the Ubuntu universe, and builds for all architectures.
It has a commandline tool in pkg:jq and a lib/devel package libjq1/libjq-dev. For the current case only the commandline-tool will be promoted.

[Rationale]
There are other regexp libraries, but JQ (above) can only use this one.

There was an old denied MIR in bug 675757, but the old issues identified back then (packaging issues and rare updates) both seem to be good now.
Also symbols trackign wasn't done back then, but is now.

https://github.com/kkos/oniguruma/tags
https://tracker.debian.org/pkg/libonig

[Security]

No open bugs or CVEs right now, but it has a CVE history most found in 2019.

[Quality assurance]

Packaging wise it seems sage it doesn't ask debconf questions, has no
long-term outstanding bugs in Ubuntu and upstream has a bug tracker.

It has build time tests that run under dh_auto_test.

No special HW is needed to work with it

No obsolete or demoted dependencies.

[UI standards]

No end user UI, just a lib to build against.

[Dependencies]

No further non-main dependencies.

[Standards compliance]

The package meets the FHS and Debian Policy standards.

[Maintenance]

The Server team will subscribe for the package for maintenance.
Although TBH while "needed as dep for a server package" this is clearly not a server package from it's own use case, so ownership might be re-discussed.

Related branches

description: updated
Changed in mdevctl (Ubuntu):
status: New → Incomplete
description: updated
summary: - [MIR] mdevctl
+ [MIR] mdevctl, jq, libonig
Changed in libonig (Ubuntu):
status: New → Incomplete
Changed in jq (Ubuntu):
status: New → Incomplete
description: updated
description: updated
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Ok, after discussing with the Team we think JQ on its own is worth for main an mdevctl as well.
So let us open these up for review ...

Changed in jq (Ubuntu):
status: Incomplete → New
Changed in libonig (Ubuntu):
status: Incomplete → New
Changed in mdevctl (Ubuntu):
status: Incomplete → New
Changed in libonig (Ubuntu):
assignee: nobody → Christian Ehrhardt  (paelzer)
Changed in jq (Ubuntu):
assignee: nobody → Christian Ehrhardt  (paelzer)
Changed in mdevctl (Ubuntu):
assignee: nobody → Matthias Klose (doko)
description: updated
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

-- For JQ --

[Summary]
MIR Team ack, but a security team review is required.

Recommended but optional:
- adding a autopkgtest would be useful to detect issues early on

[Duplication]
There is no other package in main providing the same functionality yet.

[Dependencies]
OK:
- no other Dependencies to MIR due to this (other than the already listed ones)
- There is a dev package that will be auto-promoted, but without further
  problematic dependencies.

[Embedded sources and static linking]
OK:
- no embedded source present
- no static linking

[Security]
OK:
- history of CVEs does not look concerning (wo cases, both seemed handled well)
- does not run a daemon as root
- does not use webkit1,2
- does not use lib*v8 directly
- does not open a port
- does not process arbitrary web content
- does not use centralized online accounts
- does not integrate arbitrary javascript into the desktop
- does not deal with system authentication (eg, pam), etc)

Problems:
- does not parse data formats

Since there were issues with the parsing in the past this is worth a security
review.

[Common blockers]
OK:
- does not FTBFS currently
- does have a test suite that runs at build time
  - test suite fails will fail the build upon error.
- The package has a team bug subscriber
- no translation present, but none needed for this case (user visible)?
- not a python package, no extra constraints to consider int hat regard

Problems:
- does not have a test suite that runs as autopkgtest
  currently unsure what to test on top of the build time tests, if there
  are bugs and lessons learned down the road those should be added.

[Packaging red flags]
OK:
- Ubuntu does not carry a delta
- symbols tracking is in place
- d/watch is present and looks ok
- Upstream update history is good (releases not too often, but fixes seem
  to be in time)
- Debian/Ubuntu update history is good
- the current release is packaged
- promoting this does not seem to cause issues for MOTUs that so far
  maintained the package
- no massive Lintian warnings
- d/rules is rather clean
- Does not have Built-Using

[Upstream red flags]
OK:
- no Errors/warnings during the build
- no incautious use of malloc/sprintf (as far as I can check it)
- no use of sudo, gksu, pkexec, or LD_LIBRARY_PATH
- no use of user nobody
- no use of setuid
- no important open bugs (crashers, etc) in Debian or Ubuntu
  - crash but minor https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=943558
- no dependency on webkit, qtwebkit, seed or libgoa-*
- no embedded source copies
- not part of the UI for extra checks

Changed in jq (Ubuntu):
assignee: Christian Ehrhardt  (paelzer) → Ubuntu Security Team (ubuntu-security)
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :
Download full text (3.7 KiB)

-- For LIBONIG --

[Summary]
Other than the dark shadow of "sigh another regex lib" no issue seems to be
present in the packaging. MIR Team Ack.

A security review is needed as there is CVE history. It seems to be already
monitored and fixed for the use cases around PHP so this might be a quick one.

Optional:
- Add an autopkgtest for onigurama to not only test on builds but also
  on dependency changes.

[Duplication]
Well this is a sore point in regex libraries already. These are already in main
- libre2
- pcre2
- pcre3
But it seems it is always very intense for projects to switch between any of
those, therefore we already have three in main. And it seems jq (as well as
php-mbstring) have no code to support alternative backends.
Therefore - yes there are other regex libs, but none for the use case required
here.

[Dependencies]
OK:
- no other Dependencies to MIR due to this
- -dev has no further deps that would be a problem

[Embedded sources and static linking]
OK:
- no embedded source present
- no static linking

[Security]
OK:
- history of CVEs does not look too concerning
  - there is quite a bunch of CVEs, in 2017 and 2019 a burst of them
    was identified. Amplified by the common use of libonig in some PHP
    environments which makes this more exposed than e.g. the use in
    commandline jq here.
    The issues were all handled in time and are fixed by now according to
    the Ubutnu CVE Tracker
    See [1] for the full list.
    But it is worth to say that due to its use in universe php modules
    this already seems to get enhance security coverage, see [2][3][4][5]
- does not run a daemon as root
  (not on its own, it is just a lib)
- does not use webkit1,2
- does not use lib*v8 directly
- does not open a port
- does not process arbitrary web content
- does not use centralized online accounts
- does not integrate arbitrary javascript into the desktop
- does not deal with system authentication (eg, pam), etc)

Problems:
- does parse data formats
- The above shows that there can and have been security issues with it, but also
  that it seems security already monitors and fixes these issues for its use in
  PHP. A review is needed, but might end quickly with "yeah we do that anyway"

[Common blockers]
OK:
- does not FTBFS currently
- does have a test suite that runs at build time
  - test suite fails will fail the build upon error.
- The package has a team bug subscriber
- no translation present, but none needed for this case (user visible)?
- not a python/go package, no extra constraints to consider int hat regard

Problems:
- does not have a test suite that runs as autopkgtest

[Packaging red flags]
OK:
- Ubuntu does not carry a delta
- symbols tracking is in place
- d/watch is present and looks ok
- Upstream update history is good
- Debian update history is good
- the current release is packaged
- promoting this does not seem to cause issues for MOTUs that so far
  maintained the package
- no massive Lintian warnings
- d/rules is rather clean
- Does not have Built-Using

[Upstream red flags]
OK:
- no Errors/warnings during the build
  only a few "-Wmaybe-uninitialized"
- no incautious use of malloc/sprintf (as far as I can check...

Read more...

Changed in libonig (Ubuntu):
assignee: Christian Ehrhardt  (paelzer) → Ubuntu Security Team (ubuntu-security)
Revision history for this message
Eduardo Barretto (ebarretto) wrote : security audit

I reviewed libonig 6.9.5-2 as checked into groovy. This shouldn't be
considered a full audit but rather a quick gauge of maintainability.

libonig (or Oniguruma) is a regular expression library. It supports
different encodings.

- CVE History:
  - libonig has been assigned 13 CVEs since 2017. All those CVEs were
    fixed by upstream in a timely manner.
  - As one would expect, most of the issues wer in the processing of
    regexes, what could cause DoS, arbitrary code execution or even
    information leakage.
  - Since the beginning of the year the project has been part of oss-fuzz
    and a lot of fixes have been applied.
- Build-Depends
  - debhelper-compat
- No pre/post inst/rm scripts
- No init scripts
- No systemd units
- No dbus services
- No setuid binaries
- No binaries in PATH
- No sudo fragments
- No polkit files
- No udev rules
- Unit tests / autopkgtests
  - There a 7 tests under test/ and 13 under sample/ that are ran during
    build time, as mentioned previously.
  - Under sample, it tests parsing regex from SQL, POSIX and others.
- No cron jobs
- Build logs:
  - regparse.c:413:24: warning: pfetch_prev may be used uninitialized in this function [-Wmaybe-uninitialized]
  - regparse.c:3576:53: warning: c may be used uninitialized in this function [-Wmaybe-uninitialized]
  - This might end up triggering FTBFS.

- No processes spawned
- Lots of memory management
  - Mostly uses xmalloc, xrealloc and other xFUNCTIONS.
- File IO
  - A file IO in src/st.c, seems ok.
  - Also in harnesses/base.c, lgtm.
- No logging
- No environment variable usage
- No use of privileged functions
- No use of cryptography / random number sources
- Use of temp files
  - only used in src/st.c, not much file IO.
- No use of networking
- No use of WebKit
- No use of PolicyKit

- Coverity results:
  - 17 high issues, most of them seem like real issues, we will be
    reporting it to upstream.
  - we don't think those issues should prevent this MIR moving forward.
- No significant cppcheck results, seem all false positives.
- A bunch of shellcheck results, but none in code that users probably
  make use of.

The code has matured a lot in the past months, but still it is a regex
library and as always regexes can be tricky, so issues might still come up.

Security team ACK for promoting libonig to main.

tags: added: security-review-done
Changed in libonig (Ubuntu):
assignee: Ubuntu Security Team (ubuntu-security) → nobody
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Thanks Eduardo, marking as "ready" by setting to "In Progress".
Once the other tasks reach that state we can start to pull it in.

Changed in libonig (Ubuntu):
status: New → In Progress
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

@Doko - any ETA on the MIR review part of mdevctl itself?

Revision history for this message
Matthias Klose (doko) wrote :

mdevctl looks ok, however it's missing copyright information for debian/*

Changed in mdevctl (Ubuntu):
status: New → Incomplete
assignee: Matthias Klose (doko) → nobody
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Thank you Doko, fixing that ...

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Copyright improved in https://salsa.debian.org/debian/mdevctl/-/commit/80d9705bfb95c0a1824ab6409761e369ba62d256

It will be part of the next upload, but on it's own that isn't worth a new upload (nor a sync into the final freeze of groovy).
Furthermore debian/* copyright is only me and thereby should be ok as-is even before the new upload happened and is synced.

Thereby mdevctl also is ok and set to "in progress" once jq completes the security review we can make the change to the dependencies to pull things in and promote.

Changed in mdevctl (Ubuntu):
status: Incomplete → In Progress
Revision history for this message
Steve Beattie (sbeattie) wrote :

I reviewed jq 1.6-2.1 as checked into hirsute. This shouldn't be
considered a full audit but rather a quick gauge of maintainability.

jq is a flexible command-line JSON processor. It ends up implementing
its own language for querying and manipulating JSON structures. As such,
there are times where it is used to parse and process untrusted input.

- The jq package has had a couple of CVEs, one for a one-byte heap
  overflow, and one for unbounded stack usage in some situations.
  Upstream has been reasonably responsive in addressing the issues in a
  timely matter.
- No build depends of concern. It does use bison/flex for its
  manipulation language parser and ruby for generating docs.
- No pre/post inst/rm scripts.
- No init scripts.
- No systemd units.
- No dbus services.
- No setuid binaries.
- The only binary provided is jq.
- No sudo fragments.
- No polkit files.
- No udev rules.
- No autopkgtests. jq provides a bunch of functional tests that are run
  during the build, while wrapped by valgrind to find memory errors.
- No cron jobs.
- Lintian clean. The build produces some warnings, mostly around
  the casting performed for the builtin language functions, as well
  as some implicit case/switch fall-through that look to be "clever"
  programming. (The generated lexer also had one signedness comparison
  warning)

- Does not spawn processes.
- Memory management is okay. In most cases, allocation wrappers are used
  that check for failures directly, and reference counting is used for
  higher level JSON objects.
- As a general purpose command line tool, files are either read from
  stdin or passed on the command line. It tries to be defensive in
  its handling of JSON input. It also supports module loading, but this
  is again specified via command line arguments.
- Error logging is handled through wrapper functions and avoids format
  string issues.
- jq uses environment variables for module loading paths and for
  specifying colorized output.
- No use of privileged functions.
- Does not appear to Use of cryptography / random number sources etc.
- Does not appear to use temp files.
- Does not use networking.
- No use of WebKit.
- No use of PolicyKit.

- Most issues coverity and cppcheck highlighted are either false
  positives or non-issues that come about from "clever" programming.
- Only shellcheck issues are in build scripts and tests.

Generally, jq is implemented with thought and care. It attempts to
be cautious in its handling of input. The only concern that I have
about supporting jq is that it is dense and complex code, and for
future issues, backporting fixes may be difficult where significant
code changes have occurred. The extensive test suite helps mitigate
this somewhat.

Security team ACK for promoting jq to main.

Changed in jq (Ubuntu):
status: New → In Progress
assignee: Ubuntu Security Team (ubuntu-security) → nobody
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Great, thank you all who was involved!

That completes the prep-processing for mdevctl.
I have planned a merge of libvirt anyway - I'll pull mdevctl to main from there.

Andy Whitcroft (apw)
Changed in jq (Ubuntu):
assignee: nobody → Andy Whitcroft (apw)
Changed in mdevctl (Ubuntu):
assignee: nobody → Andy Whitcroft (apw)
Changed in libonig (Ubuntu):
assignee: nobody → Andy Whitcroft (apw)
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Libvirt 7.0 in Hirsute is now pulling it in, setting to Fix Comitted

Changed in jq (Ubuntu):
status: In Progress → Fix Committed
Changed in libonig (Ubuntu):
status: In Progress → Fix Committed
Changed in mdevctl (Ubuntu):
status: In Progress → Fix Committed
Revision history for this message
Andy Whitcroft (apw) wrote :
Download full text (3.6 KiB)

All promoted as indicated in component missmatches:

$ change-override -c main -s hirsute -S mdevctl
Override component to main
mdevctl 0.78-1 in hirsute: universe/misc -> main
mdevctl 0.78-1 in hirsute amd64: universe/admin/optional/100% -> main
mdevctl 0.78-1 in hirsute arm64: universe/admin/optional/100% -> main
mdevctl 0.78-1 in hirsute armhf: universe/admin/optional/100% -> main
mdevctl 0.78-1 in hirsute i386: universe/admin/optional/100% -> main
mdevctl 0.78-1 in hirsute ppc64el: universe/admin/optional/100% -> main
mdevctl 0.78-1 in hirsute riscv64: universe/admin/optional/100% -> main
mdevctl 0.78-1 in hirsute s390x: universe/admin/optional/100% -> main
8 publications overridden.

$ change-override -c main -s hirsute -S jq
Override component to main
jq 1.6-2.1ubuntu1 in hirsute: universe/misc -> main
jq 1.6-2.1ubuntu1 in hirsute amd64: universe/utils/optional/100% -> main
jq 1.6-2.1ubuntu1 in hirsute arm64: universe/utils/optional/100% -> main
jq 1.6-2.1ubuntu1 in hirsute armhf: universe/utils/optional/100% -> main
jq 1.6-2.1ubuntu1 in hirsute i386: universe/utils/optional/100% -> main
jq 1.6-2.1ubuntu1 in hirsute ppc64el: universe/utils/optional/100% -> main
jq 1.6-2.1ubuntu1 in hirsute riscv64: universe/utils/optional/100% -> main
jq 1.6-2.1ubuntu1 in hirsute s390x: universe/utils/optional/100% -> main
libjq-dev 1.6-2.1ubuntu1 in hirsute amd64: universe/libdevel/optional/100% -> main
libjq-dev 1.6-2.1ubuntu1 in hirsute arm64: universe/libdevel/optional/100% -> main
libjq-dev 1.6-2.1ubuntu1 in hirsute armhf: universe/libdevel/optional/100% -> main
libjq-dev 1.6-2.1ubuntu1 in hirsute i386: universe/libdevel/optional/100% -> main
libjq-dev 1.6-2.1ubuntu1 in hirsute ppc64el: universe/libdevel/optional/100% -> main
libjq-dev 1.6-2.1ubuntu1 in hirsute riscv64: universe/libdevel/optional/100% -> main
libjq-dev 1.6-2.1ubuntu1 in hirsute s390x: universe/libdevel/optional/100% -> main
libjq1 1.6-2.1ubuntu1 in hirsute amd64: universe/utils/optional/100% -> main
libjq1 1.6-2.1ubuntu1 in hirsute arm64: universe/utils/optional/100% -> main
libjq1 1.6-2.1ubuntu1 in hirsute armhf: universe/utils/optional/100% -> main
libjq1 1.6-2.1ubuntu1 in hirsute i386: universe/utils/optional/100% -> main
libjq1 1.6-2.1ubuntu1 in hirsute ppc64el: universe/utils/optional/100% -> main
libjq1 1.6-2.1ubuntu1 in hirsute riscv64: universe/utils/optional/100% -> main
libjq1 1.6-2.1ubuntu1 in hirsute s390x: universe/utils/optional/100% -> main
22 publications overridden.

$ change-override -c main -s hirsute -S libonig
Override component to main
libonig 6.9.5-2 in hirsute: universe/libs -> main
libonig-dev 6.9.5-2 in hirsute amd64: universe/libdevel/optional/100% -> main
libonig-dev 6.9.5-2 in hirsute arm64: universe/libdevel/optional/100% -> main
libonig-dev 6.9.5-2 in hirsute armhf: universe/libdevel/optional/100% -> main
libonig-dev 6.9.5-2 in hirsute i386: universe/libdevel/optional/100% -> main
libonig-dev 6.9.5-2 in hirsute ppc64el: universe/libdevel/optional/100% -> main
libonig-dev 6.9.5-2 in hirsute riscv64: universe/libdevel/optional/100% -> main
libonig-dev 6.9.5-2 in hirsute s390x: universe/libdevel/optional/100% -> main
libonig5 6.9.5-2 in hirsute amd64: un...

Read more...

Changed in jq (Ubuntu):
assignee: Andy Whitcroft (apw) → nobody
Changed in libonig (Ubuntu):
assignee: Andy Whitcroft (apw) → nobody
Changed in mdevctl (Ubuntu):
assignee: Andy Whitcroft (apw) → nobody
Changed in jq (Ubuntu):
status: Fix Committed → Fix Released
Changed in libonig (Ubuntu):
status: Fix Committed → Fix Released
Changed in mdevctl (Ubuntu):
status: Fix Committed → Fix Released
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

FYI - mdevctl switched to rust, which to some extend makes everything a lot harder.
There are plenty of things around the rust ecosystem that need to be sorted out first.
As that is a whole new discussion it is tracked in a new bug 1942394

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.