[MIR] mini-iso-tools

Bug #2008742 reported by Dan Bungert
14
This bug affects 1 person
Affects Status Importance Assigned to Milestone
mini-iso-tools (Ubuntu)
In Progress
Undecided
Unassigned

Bug Description

mini-iso-tools is a new archive package that is an extension to casper
capabilities. It provides a ncurses TUI for selecting an ISO, and
scripts to integrate with casper. These casper hooks download and
chain-boot the selected ISO. This is functionality we intend to offer
in a new ubuntu-mini-iso deliverable.

[Availability]
- The package mini-iso-tools is already in Ubuntu universe.
- The package mini-iso-tools builds for the architecture it is
  designed to work on (amd64), along with other architectures that we
  aspire for it to work on but which require further install media
  work.
- It currently builds and works for architectures: amd64
- Link to package https://launchpad.net/ubuntu/+source/mini-iso-tools

[Rationale]
- The package mini-iso-tools is required in Ubuntu main:
  - Install media is expected to be built from main only
  - The package provides menu functionality to list relevant install
    ISOs discovered by simplestreams, enabling a installer starting
    point from UEFI that won't go out of date, or convenience USB keys
    that can be burned once and stay relevant for years
- The package mini-iso-tools is required in Ubuntu main no later than
  March-21-2023 due to the desire to build install media without
  needing universe.

[Security]
- No CVEs/security issues in this software in the past - software is
  new
- no `suid` or `sgid` binaries
- no executables in `/sbin` and `/usr/sbin`
- Package does not install services, timers or recurring jobs
- Packages does not open privileged ports (ports < 1024)
- Packages is an extension to casper

[Quality assurance - function/usage]
- The package has a similar use story to casper, in the sense that it
  should not be installed on end user systems, but doing so isn't
  likely to hurt anything other than some cruft in the initrd
- The initrd portions do not run unless casper runs; the entry point
  is a casper hook. Casper doesn't run unless the system building
  the initrd has configured /etc/initramfs-tools/conf.d/casperize.conf
  to do so, and that is not a default setting.

[Quality assurance - maintenance]
- The package is too new to demonstrate a history of maintenance

[Quality assurance - testing]
- The package runs a test suite on build time, if it fails it makes
  the build fail.
  https://github.com/dbungert/mini-iso-tools/actions/runs/4285593082/jobs/7464080468#step:3:905
- The package has no autopkgtest currently, I would like to do so as time
  permits.

[Quality assurance - packaging]
- debian/watch is not present because it is a native package
- debian/control defines a correct Maintainer field
- This package does not yield massive lintian Warnings, Errors
- `lintian --pedantic` output is empty
- Lintian overrides are not present
- This package does not rely on obsolete or about to be demoted
  packages.
- This package has no python2 or GTK2 dependencies
- The package will not be installed by default
- Packaging and build is easy and has a 3-line debian/rules file:
  https://github.com/canonical/mini-iso-tools/blob/main/debian/rules

[UI standards]
- Application is end-user facing but has no translation present.
  Single page UI where the user chooses an ISO to chain-boot to.
- A desktop file is not relevant

[Dependencies]
- No further depends or recommends dependencies that are not yet in
  main

[Standards compliance]
- This package correctly follows FHS and Debian Policy

[Maintenance/Owner]
- Owning Team will be Foundations
- Team is already subscribed to the package
- This does not use static builds
- This does not use vendored code
- This package is not rust based
- The package was test rebuilt in PPA or sbuild recently
  https://launchpad.net/~dbungert/+archive/ubuntu/proposed-all-the-things/+build/25625585

[Background information]
- The Package description explains the package well, but I wrote the
  description so
- Upstream Name is Canonical
- Link to upstream project https://github.com/canonical/mini-iso-tools

Tags: sec-1803
Dan Bungert (dbungert)
description: updated
Dan Bungert (dbungert)
description: updated
Changed in mini-iso-tools (Ubuntu):
assignee: nobody → Ioanna Alifieraki (joalif)
Revision history for this message
Łukasz Zemczak (sil2100) wrote :

Since this is not a user-system facing package, only used to build the mini iso images, I'll proceed with promoting this package to main to enable daily builds of this flavor.

Revision history for this message
Łukasz Zemczak (sil2100) wrote :

$ change-override -s lunar-proposed -c main -S mini-iso-tools
Override component to main
mini-iso-tools 0.2.1 in lunar: universe/misc -> main
mini-iso-tools 0.2.1 in lunar amd64: universe/misc/optional/100% -> main
mini-iso-tools 0.2.1 in lunar arm64: universe/misc/optional/100% -> main
mini-iso-tools 0.2.1 in lunar armhf: universe/misc/optional/100% -> main
mini-iso-tools 0.2.1 in lunar ppc64el: universe/misc/optional/100% -> main
mini-iso-tools 0.2.1 in lunar s390x: universe/misc/optional/100% -> main
Override [y|N]? y
6 publications overridden.
$ change-override -s lunar -c main -S mini-iso-tools
Override component to main
mini-iso-tools 0.2.0 in lunar: universe/misc -> main
mini-iso-tools 0.2.0 in lunar amd64: universe/misc/optional/100% -> main
mini-iso-tools 0.2.0 in lunar arm64: universe/misc/optional/100% -> main
mini-iso-tools 0.2.0 in lunar armhf: universe/misc/optional/100% -> main
mini-iso-tools 0.2.0 in lunar ppc64el: universe/misc/optional/100% -> main
mini-iso-tools 0.2.0 in lunar riscv64: universe/misc/optional/100% -> main
mini-iso-tools 0.2.0 in lunar s390x: universe/misc/optional/100% -> main
Override [y|N]? y
7 publications overridden.

Please give me a sign if the MIR review for this package reveals some issues that need to be addressed ASAP.

Revision history for this message
Ioanna Alifieraki (joalif) wrote :

@Lukasz I just finished with the MIR review (next comment).
All seem good apart from autopkgtest missing.
IMO it also needs a security review, so I'll assign security team to the bug
in case they find anything extra to be addressed.

Revision history for this message
Ioanna Alifieraki (joalif) wrote :
Download full text (3.3 KiB)

Review for Package: mini-iso-tools

[Summary]
The package looks ok, only autopkg tests missing.
In the security section I marked that it parses json from an untrusted
source, I am not sure about it, but since the package is
messing with installation I think a sec review is a good idea.

MIR team ACK under the constraint to resolve the below listed
required TODOs and as much as possible having a look at the
recommended TODOs.

This does need a security review, so I'll assign ubuntu-security
List of specific binary packages to be promoted to main: mini-iso-tools

Notes:
Required TODOs:
1. Please add autopkg tests.

- The package already has a team subscriber.

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

[Dependencies]
OK:
- no other Dependencies to MIR due to this
  - checked with `check-mir`
  - all dependencies can be found in `seeded-in-ubuntu` (already in main)
  - none of the (potentially auto-generated) dependencies (Depends
    and Recommends) that are present after build are not in main
- no -dev/-debug/-doc packages that need exclusion
- No dependencies in main that are only superficially tested requiring
  more tests now.

Problems: None

[Embedded sources and static linking]
OK:
- no embedded source present
- no static linking
- does not have unexpected Built-Using entries
- not a go package, no extra constraints to consider in that regard
- not a rust package, no extra constraints to consider in that regard
- Does not include vendored code

Problems: None

[Security]
OK:
- history of CVEs does not look concerning
- does not run a daemon as root
- does not use webkit1,2
- does not use lib*v8 directly
- does not open a port/socket
- 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)
- does not deal with security attestation (secure boot, tpm, signatures)
- does not deal with cryptography (en-/decryption, certificates, signing, ...)

Problems:
- does not parse data formats (files [images, video, audio,
  xml, json, asn.1], network packets, structures, ...) from
  an untrusted source.

[Common blockers]
OK:
TODO: - does not FTBFS currently
- does have a test suite that runs at build time
  - test suite fails will fail the build upon error.
- no new python2 dependency

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

[Packaging red flags]
OK:
- Ubuntu does not carry a delta
- symbols tracking not applicable for this kind of code.
- d/watch is not present but also not needed (e.g. native)
- Upstream update history is good
- 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
- It is not on the lto-disabled list

Problems: None

[Upstream red flags]
OK:
- no Errors/warnings during the build
- no incautious use of malloc/sprintf (as far as we can check it)
- no use of sudo, gksu, pkexec, or LD_LIBRARY_PATH (usage is OK inside
  tests)
- no use of user nobody
- no...

Read more...

Changed in mini-iso-tools (Ubuntu):
assignee: Ioanna Alifieraki (joalif) → Ubuntu Security Team (ubuntu-security)
Steve Beattie (sbeattie)
tags: added: sec-1803
Revision history for this message
Camila Camargo de Matos (ccdm94) wrote (last edit ):
Download full text (9.7 KiB)

I reviewed mini-iso-tools 0.2.1 as checked into lunar. This shouldn't be considered a full audit but rather a quick gauge of maintainability.

mini-iso-tools is a package that provides a ncurses TUI for selecting an iso during boot and that allows integration with casper to download and chain-boot the selected iso.

- CVE History:
  - The package currently has no CVEs associated with it as it is a fairly new package.
  - Upstream is Canonical.
- Build-Depends? debhelper-compat (= 13), gcovr, kexec-tools, libcmocka-dev, libjson-c-dev, libncurses-dev, meson, ninja-build, pkg-config
- Dependencies seem to not have a huge number of CVEs related to them, and the ones that have a few are mostly patched.
- 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? No
- unit tests / autopkgtests? Tests can be run locally easily and there are no autopkgtests.
- cron jobs? No
- Build logs:
  - No relevant error warnings or lintian failures

- Processes spawned?
  - a lot of bash scripts work together with the main script in order to provide the menu functionality. One of them is the 'iso-menu-session' script which recovers JSON data to be parsed and then presented through the ncurses menu.
  - The '30mini-iso-menu' script is added to '/usr/share/initramfs-tools/scripts/casper-premount/' during install. The owner is 'root:root' and permissions are set to '644'.
- Memory management?
  - a lot of memory management follows the standard C way of programming. Memory management is always a tricky thing to do in C, and is always something that ends up introducing many vulnerabilities, so we must be careful when expanding on this code to avoid these types of issues. It is helpful in this situation that upstream is Canonical itself.
  - main.c: 'args_free' is called after 'args_create' (by the end of 'main'), however, if any intermediary check in the main function fails, this memory actually not freed since 'main' returns first. Therefore, we can have possible memory leaks. Some leaks are detected by valgrind in a normal execution for the program.
  - json.c: uses 'strcmp' with no checks for size before the comparison, and 'json.c' will be processing data recovered from external sources. This does not seem to be a problem at this time and with the current code we have, however, this is not one of the best programming practices when it comes to security.
  - from cppcheck/coverity: "common.c:33:29: error: va_list 'ap' was opened but not closed by va_end(). [va_end_missing]". However, documentation mentions that 'va_end' should be called or else the stack can be corrupted.
  - calling the 'iso-chooser-menu' with empty files or files that do not have the expected contents as input, results in a crash with a segmentation fault, and this seems like a problem that could be easily avoided and not result in a crash. The way the program is meant/expected to be run kind of "guarantees" that this kind of thing won't happen, but it is always best to have checks in place in case we run across unexpected situations.
  - in function 'choices_extend_...

Read more...

Changed in mini-iso-tools (Ubuntu):
status: New → In Progress
assignee: Ubuntu Security Team (ubuntu-security) → nobody
Revision history for this message
Dan Bungert (dbungert) wrote :

Thanks for the review.

> The cppcheck/coverity result involving the 'va_start()' call with no corresponding 'va_end()' call in common.c be corrected in the code.

Thanks, I will fix this. cppcheck looks trivial to incorporate for future use. Is coverity available somehow if I wanted to run that in the future?

> Is there a reason why the 'iso-menu-session' saves the recovered JSON files in '/tmp'? Could we consider possibly changing the script to have this data be saved in a better location?

Changes here should not be difficult - what would the ideal solution look like to you? Is the concern world visibility on that directory? Maybe a `mktemp -d` with a umask of 077 instead? Something under /run instead of /tmp?

> Is there a reason why we wouldn't be able to use GPG signatures in order to guarantee that the contents of the recovered JSON files are trustyworthy? Could we possibly consider using GPG signatures to check that the actual content of the recovered JSON files are trusted, instead of only relying in TLS and hardcoded values only, should there be nothing stopping us from doing so?

Nothing prevents this, so this can be implemented. I will do so.

Revision history for this message
Camila Camargo de Matos (ccdm94) wrote (last edit ):

Thanks for the answers, Dan, and for considering the suggested changes.

To now answer your questions:

- Regarding the Coverity availability for future use, I think it is possible to use a free version of Coverity, available through https://scan.coverity.com/ (Coverity offers free scanning for open source projects via integrations with GitHub/GitLab).

- Regarding the second issue (saving files in the '/tmp' directory), the concern is indeed the fact that the '/tmp' directory has a high visibility and is commonly used to assist with certain attacks. In this case, we are creating a directory with a predictable name which will have files in it that also will have predictable names, so that might be dangerous. When saving to '/tmp' it is always ideal to generate things with sufficiently strong permissions (umask 077, as you mentioned, for example) and random names, however, I saw that creating the directory or the files with random names might involve more of a complicated change to the code, so maybe adding it to '/run' instead of '/tmp' would be best in this case. The important thing would be to also properly set permissions to the temporary directory where the final JSONs are uploaded to in order to avoid situations where a user could simple add a possibly malicious file into the directory for processing.

Let me know if there are any more questions regarding the review or even the answers provided above!
Thanks!

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.