[MIR] flashrom + libftdi

[Star Labs]

[Summary]
Further review will be needed. The Package does not have a test suite that runs as autopkgtest.

[Availability]
Currently in universe.

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

[Rationale]
fwupd depends on libflashrom1 for its flashrom plugin, something that's required to update Coreboot firmware.

[Security]
No CVE's, but due to the nature of the package security should review.

[Quality Assurance]
Package builds and runs easily

[Dependencies]
N/A

[Standards Compliance]
Complies with FHS, though the organization of files in the source package could be organized better.

[Common blockers]
flashrom does NOT have a test suite that runs at build time.
flashrom does NOT have a test suite that runs as autopkgtest.

[Maintenance]
Actively maintained - https://github.com/flashrom/flashrom
Packaging - https://salsa.debian.org/myczko-guest/flashrom.git

[Christian Ehrhardt  (paelzer)]

@Matt - Foundations is the owner of fwupd - do you agree to own this package as well then?
If yes please subscribe the Team and set the status back to "new" for a full review.
If no - we need to have a discussion about the alternatives.

[Matthieu Clemenceau (mclemenceau)]

@Christian, yes it makes sense for Foundations to own this package.
I'll subscribe the team and move set the status back to new.

[Carl-Daniel Hailfinger (hailfinger)]

flashrom does have built-in self tests and also supports external tests,
but apparently that is not well-known.

If your build VM supports flash chip host controller emulation and flash
chip emulation, you can run flashrom with root privileges against the
emulated hardware.

If you want to run only unprivileged code, the man page flashrom.8 has
you covered with multiple pages of explanations and option descriptions
and even working examples:

dummy programmer
              The dummy programmer operates on a buffer in memory only.
It provides a safe and fast way to test various aspects of flashrom and
is mainly used in development and while debugging.  It is able to
emulate some chips to a certain degree (basic identify/read/erase/write
operations work).
[...]

That feature is enabled by default if flashrom is built with the classic
Makefile. AFAIK the Meson build doesn't have feature parity yet, so that
feature might be missing (no promises).

Regards,
Carl-Daniel

Am 04.02.21 um 16:17 schrieb William Wilson:
> ** Description changed:
>
> + [Summary]
> + There are a few issues before we can MIR this package. The package does not have a test suite that runs at build time, nor does it have a test suite that runs as autopkgtest.
> +
> [Availability]
> Currently in universe.
> +
> + [Duplication]
> + There is no other package in main providing the same functionality.
>
> [Rationale]
> fwupd depends on libflashrom1 for its flashrom plugin, something that's required to update Coreboot firmware.
>
> [Security]
> - All known issues have bee resolved.
> + All known issues have been resolved.
>
> [Quality Assurance]
> + Package builds and runs easily
>
> [Dependencies]
> N/A
>
> [Standards Compliance]
> + Complies with FHS, though the organization of files in the source package could be organized better.
> +
> + [Common blockers]
> + flashrom does NOT have a test suite that runs at build time.
> + flashrom does NOT have a test suite that runs as autopkgtest.
>
> [Maintenance]
> Actively maintained - https://github.com/flashrom/flashrom
> Packaging - https://salsa.debian.org/myczko-guest/flashrom.git
>
> ** Description changed:
>
> [Summary]
> There are a few issues before we can MIR this package. The package does not have a test suite that runs at build time, nor does it have a test suite that runs as autopkgtest.
>
> [Availability]
> Currently in universe.
>
> [Duplication]
> There is no other package in main providing the same functionality.
>
> [Rationale]
> fwupd depends on libflashrom1 for its flashrom plugin, something that's required to update Coreboot firmware.
>
> [Security]
> - All known issues have been resolved.
> + No CVE's, but due to the nature of the package security should review.
>
> [Quality Assurance]
> Package builds and runs easily
>
> [Dependencies]
> N/A
>
> [Standards Compliance]
> Complies with FHS, though the organization of files in the source package could be organized better.
>
> [Common blockers]
> flashrom does NOT have a test suite that runs at build time.
> flashrom does NOT have a test suite that runs as autopkgtest.
>
> [Maintenance]
> Actively maintaine...

[William Wilson (jawn-smith)]

I do see now that there are built in tests, so I will update the description. There is no debian/tests file so I will leave that part in. Thanks for the input!

[Carl-Daniel Hailfinger (hailfinger)]

[Adding flashrom mailing list to CC. Context: The Ubuntu bug is about
moving flashrom from universe to main.
See https://bugs.launchpad.net/ubuntu/+source/flashrom/+bug/1912371/ for
details.]

If anyone is interested, I can dig up my test scripts which I used to
test flashrom, but I do not have any autopkgtest experience. Someone(TM)
would have to convert that to autopkgtest format, or we can simply ship
selftest scripts to be called from autopkgtest.
The amount of testing depends on how much test coverage you want. We can
do self-tests with internally emulated hardware or externally emulated
hardware or real hardware (the last variant is probably not desirable in
a build system).
Besides that, flashrom always performs a few self-tests (mostly internal
data structure consistency) during startup. The tests have repeatedly
caught bugs before a merge when people added new features, but forgot to
update all relevant data structures.

[Christian Ehrhardt  (paelzer)]

[Summary]
MIR Team Ack under the constraint to implement the all Required (and as much
Recommended as possible) of the lists below.

This does need a security review, so I'll assign ubuntu-security

List of specific binary packages to be promoted to main: libflashrom1

Required TODOs:
- src:libftdi1 is required as well, adding a task and assigning mclemenceau to
  decide if he wants to own that as well.
- Future owners (=Foundation) needs to check the interaction with fwupd
  if that is safe to not trigger any of the flashrom warnings to brick laptops.
  => https://flashrom.org/Laptops
  Once you have taken an explicit look at that please state here that it was
  functionally reviewed and considered safe.
- get the existing self-tests in play at build time. We can't set up VMs for
  flash emulation in autopkgtests - but the build time tests that exist should
  be added to the meson build. Or dh_auto_test runs partially on makefiles?
  => https://bugs.launchpad.net/ubuntu/+source/flashrom/+bug/1912371/comments/3

Recommended TODOs:
- this is no 1.0 so maybe one should acknowledge that stability. Please consider
  dropping the commandline syntax change warning fromt he man page as well as
  think about adding a .symbols file to auto-detect API breakage.

[Duplication]
There is no other package in main providing the same functionality.
There is ftdi-eeprom and xc3sprog, but both cover smaller subsets of use-cases
and both are not in main. But ftdi-eeprom will come back below.

[Dependencies]
OK:
- There is a -dev which will be auto-promoted, but it brings in no further
  dependencies on top of what we need anyway.

Problems:
- other Dependencies to MIR due to this. "flashrom" as well as "libflashrom1"
  depend on src:libftdi1 this will have to be owned/prepared/reviewed as well
  to be able to complete this promotion of src:flashrom.

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

[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
- 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
- does process arbitrary web content (depending on where you get your rom files)
  For both issues it is more the permission level of such roms that makes them
  security relevant. If one can sneak in odd code into the early boot that is
  potentially later undetectable. Also while gladly not a daemon, executing
  this will run at the highest possible permissions as those are needed to
  program the hardware.

[Common blockers]
OK:
- does not FTBFS currently
- The package has an intended team bug subscriber
- no translation present, but none needed for this case (user visible)?
- not a python/go package, no extra constraints to consider in that regard

Problems:
- does not have a test suite that runs at build time
- does not have a test suite that runs as autopkgtest
Here as outlined by Carl-Daniel it might need some dev effort to leverage the
existing self-tests...

[Matthieu Clemenceau (mclemenceau)]

@paelzer, Sorry for dropping this, I saw the ping this morning on ubuntu-meeting,
I'll put this in front of the team for review. I missed the notification.

Thanks for the reminder.

[William Wilson (jawn-smith)]

[Summary]
This package is safe to include in main.
This does need a security review, so I'll assign ubuntu-security
List of specific binary packages to be promoted to main:
  * ftdi-eeprom
  * libftdi1-2
  * libftdi1-dev
  * libftdi1-doc
  * libftdipp1-3
  * libftdipp1-dev
  * python3-ftdi1

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

[Dependencies]
OK: All binary dependencies not in main are built by this package

[Embedded sources and static linking]
OK: none

[Security]
OK:
- no CVEs
- does not run a daemon
- does not use webkit1,2
- does not use lib*v8 directly
- does not open a port
- does not use centralized online accounts
- does not integrate arbitrary javascript into the desktop
- does not deal with system authentication (eg, pam, etc)
Problems:
- Needs review due to the nature of the package
- does parse data formats
- The FTDI devices can be used for many security relevant purposes.
    For example, flashrom makes use of FTDI devices in some cases to flash chips.
    This happens at the highest possible security levels.

[Common blockers]
OK:
- Does not FTBFS
- Added foundations-bugs as a bug subscriber
- no translation needed
- not a python or go package
- has test suites that run at build time and as autopkgtest

[Packaging red flags]
OK:
Upstream update history is slow, but not unreasonably so. A new version was released last July

[Upstream red flags]
OK:

[William Wilson (jawn-smith)]

I have reviewed flashrom along with fwupd and I don't see anything that looks like it will cause problems. The flashrom documentation recommends using vendors' flashing utilities when possible for laptops, and there is a link to that page of the documentation in the manpage.

I have attached a debdiff to this comment that addresses several other concerns about this MIR. The attached debdiff
  1) Adds the build time unit tests to the meson build
  2) Adds a simple test script that operates on an emulated
     chip without sudo privileges. This runs as an autopkgtest
  3) Removes the warning about command line syntax change
     from the manpage
  4) Adds a comment on the stability of this program to the manpage
  5) Adds a .symbols file for libflashrom1

[Alex Murray (alexmurray)]

I am trying to do the security review of flashrom for this MIR but currently it FTBFS with the attached debdiff in comment 9 - the build log is attached. Can this please be resolved (along with removing the 'WILLIAM we're doing tests!' part of the output :)

[Alex Murray (alexmurray)]

These failures of the mocked tests have been reported upstream at https://github.com/flashrom/flashrom/issues/186 by others but no response - however I am pretty sure this is due to the use of LTO in impish - cmocka is known to fail with LTO due to the use of --wrap which doesn't play nicely with LTO - so in this case, the original functions get called, not the wrapped ones - https://bugzilla.redhat.com/show_bug.cgi?id=1693831

I can't find an easy way to only disable LTO for these tests so perhaps just best to set DEB_BUILD_MAINT_OPTIONS = optimize=-lto in debian/rules for now.

As such, I will proceed with the security audit based on this for now.

[Stefan Tauner (stefanct)]

On Wed, 19 May 2021 06:52:55 -0000
Alex Murray <email address hidden> wrote:

> These failures of the mocked tests have been reported upstream at
> https://github.com/flashrom/flashrom/issues/186 by others but no
> response

Hi there,

I was the flashrom maintainer till a few years ago. While I was using
the github repo+tracker, there is currently ~nobody taking care of that.
A more promising way to reach the right people is to write an email to
<email address hidden>
--
Kind regards/Mit freundlichen Grüßen, Stefan Tauner

[Carl-Daniel Hailfinger (hailfinger)]

Hi,

I thought we had deactivated the github bugtracker because flashrom
development does not happen at github.
The github mirror of flashrom is there purely for people who are
unwilling or unable (due to corporate restrictions etc.) to download the
source code from our self-hosted git trees. I'll try to get the github
tracker deactivated to avoid giving people wrong impressions.

As Stefan wrote, you can reach the flashrom developers at
<email address hidden> and you will also get attention if you send email
there.

Regards,
Carl-Daniel

Am 19.05.21 um 09:33 schrieb Stefan Tauner:
> On Wed, 19 May 2021 06:52:55 -0000
> Alex Murray <email address hidden> wrote:
>
>> These failures of the mocked tests have been reported upstream at
>> https://github.com/flashrom/flashrom/issues/186 by others but no
>> response
> Hi there,
>
> I was the flashrom maintainer till a few years ago. While I was using
> the github repo+tracker, there is currently ~nobody taking care of that.
> A more promising way to reach the right people is to write an email to
> <email address hidden>

[William Wilson (jawn-smith)]

This debdiff corrects the accidentally left in debug statement and adds `optimize=-lto` to DEB_BUILD_MAINT_OPTIONS. Tests are succeeding again.

[Matthias Klose (doko)]

yes, disabling lto explicitly for the build seems reasonable

[Alex Murray (alexmurray)]

I reviewed flashrom 1.2-5ubuntu1 as per the debdiff attached to this bug as
applied to 1.2-5 in impish. This shouldn't be considered a full audit but
rather a quick gauge of maintainability.

flashrom is a tool used for reading and flashing BIOS/ROM/firmware onto the
various flash chips within a machine - this can include the UEFI BIOS or
optionROMs plus other devices like NICs etc.

- No CVE History
- Relevant Build-Depends:
  - libpci-dev, libusb-1.0-0-dev, libftdi1-dev
- No pre/post inst/rm scripts
- No init scripts
- No systemd units
- No dbus services
- No setuid binaries
- 1 binary in PATH
  - /usr/sbin/flashrom
- No sudo fragments
- No polkit files
- 1 udev rules
  - Grants read/write access to the various specific USB devices for users
    in the plugdev group
- Includes unit tests run during build and autopkgtests to do more
  system-level testing via dummy devices
- No cron jobs
- Build logs:
  - No significant warnings during the build other than the following
    lintian issues:
E: libflashrom1: symbols-file-contains-current-version-with-debian-revision on symbol LIBFLASHROM_1.0@LIBFLASHROM_1.0 and 23 others
W: flashrom: appstream-metadata-missing-modalias-provide lib/udev/rules.d/60-flashrom.rules
W: flashrom source: illegal-runtime-test-name emulated_programmer.sh in line 1

- Processes spawned
  - Uses popen() to call dmidecode() to read various hard-coded
    identifiers - as these are hard-coded there is no chance for these to
    be used for command injection so whilst this is a bit ugly it is safe.
- Memory management
  - Being written in C there is a lot of dynamic memory management via
    malloc, realloc and free etc - but in general these seem quite
    defensive, with return values being checked and no instances that I
    could see with obvious chance for integer overflow when calculating
    sizes to allocate etc.
- File IO
  - Opens and reads from a number of hard-coded paths or from paths as
    specified via command-line arguments
  - Can dump out to a user-specified path as well
- Logging
  - Lots of printf() style logging but again this looks pretty defensive
- No environment variable usage
- Uses various privileged ioctl() calls so likely would need to be run as
  root in these cases but again I don't see any obvious chance to abuse
  this
- No use of cryptography / random number sources
- No use of temp files
- Use of networking
  - Supports connecting to a remote serial programmer over TCP/IP - this is
    treated as trusted like a real device
- No use of WebKit
- No use of PolicyKit

- No significant cppcheck/coverity results

flashrom appears to be reasonably defensively written and does not have a
history of security issues, whilst the upstream project seems relatively
healthy so should likely be responsive and supporting for any potential
future security issues.

Security team ACK for promoting flashrom to main assuming this includes the
changes from comment:14 above - can the lintian issues highlighted earlier
please be investigated?

[Alex Murray (alexmurray)]

I reviewed libftdi1 1.5-5build1 as checked into impish. This shouldn't be
considered a full audit but rather a quick gauge of maintainability.

libftdi1 is a library and CLI tool (ftdi-eeprom) for reading/writing to
FTDI chips used in many USB-Serial converters and other USB peripheral
controllers.

- No CVE History
- Relevant Build-Depends
  - libusb-1.0, libconfuse2
- pre/post inst/rm scripts
  - python3-ftdi contains prerm/postinst scripts auto-generated by
    dh_python3 to precompile its python code
- No init scripts
- No systemd units
- No dbus services
- No setuid binaries
- binaries in PATH
  - ftdi-eeprom provides:

    -rwxr-xr-x root/root 30952 2020-12-08 19:01 ./usr/bin/ftdi_eeprom

    which is a CLI tool to read/write to the EEPROM of FTDI chips

  - libftdi1-dev provides:

    -rwxr-xr-x root/root 1276 2020-12-08 19:01 ./usr/bin/libftdi1-config

    which is a CLI tool similar to pkg-config, used to get CFLAGS/LDFLAGS
    for applications that want to link against libftdi
- No sudo fragments
- No polkit files
- No udev rules
- No cron jobs
- unit tests / autopkgtests
  - unit tests are run during the build - this does a very test to just
    init the library, and also to test the code which calculates baud rates
  - autopkgtest runs the same tests as the unit test but recompiles it from
    scratch
  - these tests are only very basic so it would be useful for more tests to
    be added to ensure other parts of libftdi1 are tested to avoid any
    regressions on future changes
- No significant warnings/errors etc seen in the build logs

- No processes spawned
- Memory management appears to be careful and defensive, return values are
  checked on allocation and bounds are respected on memcpy(), memmove() is
  used where appropriate etc
- File IO
  - libftdi does no direct file IO - libusb is used to interface with USB
    devices, whilst ftdi-eeprom uses a configuration file which is handled
    by libconfuse2 - the path to this is specified on the command-line
- No logging in libftdi other than printing errors when failing to parsing
  configuration options in ftdi-eeprom - these are safe from format string
  vulns
- No environment variable usage
- No use of privileged functions
- No use of cryptography / random number sources etc
- No use of temp files
- No use of networking
- No use of WebKit
- No use of PolicyKit

- No significant cppcheck results
- No significant Coverity results (all false-positives or just minor issues
  in test/example code, not in the actual library/application code)

libfdti1 appears well written and quite defensive, plus seems well
maintained upstream and does not have any history of security
issues. Whilst there is some small unit tests already present it would be
useful if these could be expanded to test more of the functionality of the
library etc to try and ensure any future regressions that may be introduced
via security updates etc can be caught in testing before being released.

Security team ACK for promoting libftdi1 to main.

[William Wilson (jawn-smith)]

The attached patch cleans up the lintian warnings and errors in flashrom.

[Alex Murray (alexmurray)]

@jawn-smith: was the removal of the udev rules intentional?

[William Wilson (jawn-smith)]

Yes, they were included in the package twice. Once in debian/flashrom.udev and once in util/z60_flashrom.rules. The debian/flashrom.udev was causing a lintian warning, so I removed that and added a section to meson.build that installs z60_flashrom.rules to the correct location in a way that doesn't cause a lintian warning. However, it seems that change to meson.build did not get included in that last patch somehow. I will investigate and add another patch.

[William Wilson (jawn-smith)]

That should fix the udev rules issue.

[Christian Ehrhardt  (paelzer)]

We clarified that Doko on 2021-03-11 really meant to MIR-ack the check on libftdi.
Thereby this is now actually fully ready and waiting for an upload of fwupd to pull it in.
Updating the state accordingly.

[William Wilson (jawn-smith)]

This patch enables fwupd to build with the flashrom plugin enabled on Ubuntu.

[Steve Langasek (vorlon)]

Override component to main
flashrom 1.2-5 in impish: universe/electronics -> main
flashrom 1.2-5 in impish amd64: universe/electronics/extra/100% -> main
flashrom 1.2-5 in impish arm64: universe/electronics/extra/100% -> main
flashrom 1.2-5 in impish armhf: universe/electronics/extra/100% -> main
flashrom 1.2-5 in impish ppc64el: universe/electronics/extra/100% -> main
flashrom 1.2-5 in impish riscv64: universe/electronics/extra/100% -> main
flashrom 1.2-5 in impish s390x: universe/electronics/extra/100% -> main
libflashrom-dev 1.2-5 in impish amd64: universe/libdevel/optional/100% -> main
libflashrom-dev 1.2-5 in impish arm64: universe/libdevel/optional/100% -> main
libflashrom-dev 1.2-5 in impish armhf: universe/libdevel/optional/100% -> main
libflashrom-dev 1.2-5 in impish ppc64el: universe/libdevel/optional/100% -> main
libflashrom-dev 1.2-5 in impish riscv64: universe/libdevel/optional/100% -> main
libflashrom-dev 1.2-5 in impish s390x: universe/libdevel/optional/100% -> main
libflashrom1 1.2-5 in impish amd64: universe/libs/optional/100% -> main
libflashrom1 1.2-5 in impish arm64: universe/libs/optional/100% -> main
libflashrom1 1.2-5 in impish armhf: universe/libs/optional/100% -> main
libflashrom1 1.2-5 in impish ppc64el: universe/libs/optional/100% -> main
libflashrom1 1.2-5 in impish riscv64: universe/libs/optional/100% -> main
libflashrom1 1.2-5 in impish s390x: universe/libs/optional/100% -> main
libftdi1 1.5-5build1 in impish: universe/misc -> main
ftdi-eeprom 1.5-5build1 in impish amd64: universe/utils/optional/100% -> main
ftdi-eeprom 1.5-5build1 in impish arm64: universe/utils/optional/100% -> main
ftdi-eeprom 1.5-5build1 in impish armhf: universe/utils/optional/100% -> main
ftdi-eeprom 1.5-5build1 in impish i386: universe/utils/optional/100% -> main
ftdi-eeprom 1.5-5build1 in impish ppc64el: universe/utils/optional/100% -> main
ftdi-eeprom 1.5-5build1 in impish riscv64: universe/utils/optional/100% -> main
ftdi-eeprom 1.5-5build1 in impish s390x: universe/utils/optional/100% -> main
libftdi1-2 1.5-5build1 in impish amd64: universe/libs/optional/100% -> main
libftdi1-2 1.5-5build1 in impish arm64: universe/libs/optional/100% -> main
libftdi1-2 1.5-5build1 in impish armhf: universe/libs/optional/100% -> main
libftdi1-2 1.5-5build1 in impish i386: universe/libs/optional/100% -> main
libftdi1-2 1.5-5build1 in impish ppc64el: universe/libs/optional/100% -> main
libftdi1-2 1.5-5build1 in impish riscv64: universe/libs/optional/100% -> main
libftdi1-2 1.5-5build1 in impish s390x: universe/libs/optional/100% -> main
libftdi1-dev 1.5-5build1 in impish amd64: universe/libdevel/optional/100% -> main
libftdi1-dev 1.5-5build1 in impish arm64: universe/libdevel/optional/100% -> main
libftdi1-dev 1.5-5build1 in impish armhf: universe/libdevel/optional/100% -> main
libftdi1-dev 1.5-5build1 in impish i386: universe/libdevel/optional/100% -> main
libftdi1-dev 1.5-5build1 in impish ppc64el: universe/libdevel/optional/100% -> main
libftdi1-dev 1.5-5build1 in impish riscv64: universe/libdevel/optional/100% -> main
libftdi1-dev 1.5-5build1 in impish s390x: universe/libdevel/optional/100% -> main
libftdi1-do...

[Launchpad Janitor (janitor)]

This bug was fixed in the package fwupd - 1.5.11-0ubuntu2

---------------
fwupd (1.5.11-0ubuntu2) impish; urgency=medium

  * Compile with the flashrom plugin in Ubuntu now that flashrom is
    in main (LP: #1912371)

 -- William 'jawn-smith' Wilson <email address hidden> Tue, 29 Jun 2021 10:08:47 -0500