sbkeysync fails to return non-zero on error

[dann frazier]

[Impact]
sbkeysync may exit with exitcode 0 even if it failed to update keys. The secureboot-db service will report no error in this case. This can lead a user to believe they have protected themselves against known insecure bootloaders when they have not.

An example of when this can happen - and where I noticed it - is if you have a system w/ limited variable store space and you try to import a new DBX update file. This is the case today if you pull in the latest DBX for boothole on an OVMF VM w/ a 2M NV variable store (we've since added 4M images - see bug 1885662).

[Test Case]

Boot a secureboot VM w/ 2MB flash, e.g.:
$ cat > user-data.yaml << EOF
#cloud-config
password: ubuntu
chpasswd: { expire: False }
ssh_pwauth: True
EOF

$ cloud-localds seed.img user-data.yaml
$ wget https://cloud-images.ubuntu.com/focal/current/focal-server-cloudimg-amd64.img
$ virt-install --name test --boot loader=/usr/share/OVMF/OVMF_CODE.secboot.fd,loader_ro=yes,loader_type=pflash --import --disk path=test.img --disk path=test-seed.img --ram 4096 --vcpus 4 --os-type linux --os-variant ubuntu18.04 --graphics none --console pty,target_type=serial --network network:default --feature smm=on

Then, from within the guest:
$ wget https://uefi.org/sites/default/files/resources/dbxupdate_x64.bin
$ sudo cp dbxupdate_x64.bin /usr/share/secureboot/updates/dbx
$ sudo service secureboot-db stop
$ sudo service secureboot-db start
$ sudo systemctl status secureboot-db.service
<...>
 /usr/share/secureboot/updates --verbose (code=exited, status=0/SUCCESS)
   Main PID: 2271 (code=exited, status=0/SUCCESS)

Aug 25 16:41:07 ubuntu sbkeysync[2271]: Error syncing keystore file /usr/share/secureboot/updates/dbx/dbxupdate_x64.bin
<...>

[Fix]
https://git.kernel.org/pub/scm/linux/kernel/git/jejb/sbsigntools.git/commit/?id=f12484869c9590682ac3253d583bf59b890bb826

[Regression Potential]
It's possible that causing a command to fail that previously did not will lead to other issues. For example, if someone has a 'set -e' shell script that restarts the secureboot-db service, and then does other things, those other things would no longer happen after the secureboot-db servic restart begins to fail.

[Launchpad Janitor (janitor)]

This bug was fixed in the package sbsigntool - 0.9.2-2ubuntu3

---------------
sbsigntool (0.9.2-2ubuntu3) groovy; urgency=medium

  * sbkeysync: exit non-zero upon key insertion failure. (LP: #1892797)

 -- dann frazier <email address hidden> Mon, 24 Aug 2020 18:35:41 -0600

[Steve Langasek (vorlon)]

I agree in principle with having the sbkeysync tool return non-zero when it fails to update. However, as implemented, my understanding is that this will also cause the systemd unit to go into a failed state, and to leave the entire system boot in a 'degraded' state, and I don't think at all that this is something we want - because there will be various cases where, based on the realities of the system firmware, we will be unable to apply the secureboot db updates, and I do not think we should have such systems show in degraded state in perpetuity.

I particularly don't think such a behavior change is appropriate for an SRU.

[dann frazier (dannf)]

Fair enough, I suppose I should think more about a way to let a user know that the db update failed - and the consequences of it - that do not involve failing a systemd unit. No objections to rejected the SRUs.

[Dimitri John Ledkov (xnox)]

sbsigntool (0.9.2-2ubuntu3) groovy; urgency=medium

  * sbkeysync: exit non-zero upon key insertion failure. (LP: #1892797)

 -- dann frazier <email address hidden> Mon, 24 Aug 2020 18:35:41 -0600

Please revert this!

[Dimitri John Ledkov (xnox)]

If you want this in, then one must adjust secureboot-db package service unit to ignore the error from sbkeysync, and/or declare the relevant error codes as normal.

This behaviour has been discussed on the grub_distros keybase channel, without any objections raised.

And no, seeing that package update / sbkeysync succeeded once, is not good enough. As one has to verify that on every boot. Becuase dbx variable store can be reverted/reset between each boot back to stock defaults. Thus a single success from sbkeysync, can only give a false sense of security.

[Steve Langasek (vorlon)]

Dann, I see that you've uploaded secureboot-db to change the systemd unit. I think secureboot-db also needs to declare a Breaks: on versions of secureboot-db that precede this change.

[Steve Langasek (vorlon)]

Sorry, I mean sbsigntool needs to declare a Breaks: on versions of secureboot-db that precede this change.

[dann frazier (dannf)]

Sure thing, I'll prepare one.

[dann frazier (dannf)]

sbsigntool w/ "Breaks: secureboot-db (<< 1.7~)" now uploaded to groovy.

[Brian Murray (brian-murray)]

An upload of sbsigntool to focal-proposed has been rejected from the upload queue for the following reason: "This is missing the Breaks that went into Groovy.".

[Robie Basak (racb)]

There's still an sbsigntool upload sitting in Bionic unapproved. Is this also now to be superseded by a new upload? With both the Bionic and Focal tasks marked Won't Fix, are there any plans remaining to proceed with SRUs?

[dann frazier (dannf)]

@rcb: No plans, please reject the bionic upload as well.