open-power / secvarctl Goto Github PK

When test cases fail in the CI environment, it might be useful to be able to download and run in a debugger -- this is particularly useful if the errors seem to only occur in CI.

Furthermore, this may be useful for supplying prebuilt binaries for testing purposes, if for example you want to propose a fix via PR, and can then point the reporter of the issue to the compiled artifact.

NOTE: This should probably not be an automatic workflow. It's probably a good idea to require manual intervention so that the code can at least be briefly inspected for malicious intent before distributing a binary.

This is part of the eventual greater refactor to try to share code between the backends as best as possible. Functions should have clear names describing what they do, and ideally do as little as possible to avoid massive monolith functions with excessive levels of indentation. Duplicate functions should be merged into some common code.

This also refers to an eventual rework of generic.c, perhaps a common/ source directory.

[root@ltcrain80-lp2 home]# secvarctl read -e PK.esl.sigsegv
	ESL SIG LIST SIZE: 857
	GUID is : a159c0a5e494a74a87b5ab155c2bf072
	Signature type is: X509
Segmentation fault (core dumped)

and with earlier secvarctl not seen the SIGSEGV - (probable git head at e3658f2ce5d0089e72eb243e8deacaa2ddd577a4)

[root@ltcrain80-lp2 home]# /home/secvarctl/secvarctl -m guest read -e PK.esl.sigsegv
	ESL SIG LIST SIZE: 857
	GUID is : a159c0a5e494a74a87b5ab155c2bf072
	Signature type is: X509
	Found 0 ESL's

RESULT: SUCCESS

The primary motivation for this is to no longer maintain two build systems, that currently do not have the same feature set. I propose that we improve the CMake build to match the functionality of the Makefile build, then remove the Makefile build.

Off the top of my head, this would require:

• Running unit test cases
• Ensure debug/release builds are correctly split (potentially have ASAN as an independent option, enabled by default for tests?)
• Static analysis shortcuts like make cppcheck, make format.

Here is a probably non-exhaustive opinionated list of the pros and cons of each build system, subject to change:

CMake

Pros

• Very easy to integrate into rpmbuild workflows, probably other packagers as well
• Fairly easy to add options and other targets, without needing to write a bunch of boilerplate logic
• Largely takes care of a lot of compiler flags, dependency linking, etc
• Plenty of documentation, and is a fairly standard choice in the ecosystem*
• Unlikely to cause issues when building from a weird environment
  • See con in Makefile, for more clarity
• IDEs/etc probably have better tooling/integration

• = Makefiles are standard, but there is no standard for custom Makefile build systems, and can lead to a lot of jank. Autotools and CMake avoid a lot of the jank of custom Makefiles.

Cons

• Somewhat arcane to initially set up
• More difficult to implement or run simple shortcuts like cppcheck or format
  • Note: these could be served by a script instead that runs all the static analysis and/or
• Requires slightly more effort to run a build (make vs cmake --build foo)

Makefile

Pros

• Quicker to use (just type make)
• Much easier to integrate shortcuts like make format
• Low level control over the build -- easier access to compiler flags, linking, custom intermediate steps, etc
• Does not depend on an additional build tool (make is ubiquitous)

Cons

• Everything has to be written from scratch, much more error-prone
• Integration and building from other directories has to be handled manually and carefully
  • To clarify, running make clean as root (stupid, I'm aware. Container problem.) deleted my /bin and /lib directories because some variable wasn't set correctly, therefore appending /bin to "" results in /bin.
• Harder to get contributions for, as it would require someone to understand your Makefile system
• No default integrations with packaging, will have to manually handle install/uninstall targets
• Heavy opinion here: documentation is acceptable, but unclear on some harder-to-grasp concepts that higher level build systems handle for you.
  • Anything with wildcards can be a massive pain if not done in a particularly idiomatic way, but there is no document that I've found that defines good idioms.

There is a lot of work needed for the guest secure boot support. This work will be done in the guest-devel branch. All other branches appear to be obsolete and should be deleted after this work is merged.

Current known items needed to be fixed:

• rewrite build system
• re-implement cmake build (for packaging, mostly)
• reformat entire tree in accordance to style guide
• reorganize source directories in a more sensible way
• remove excessive prints to stdout
• reconcile crypto backend differences with libstb-secvar
  • move probably all crypto code into libstb-secvar
• re-enable and improve CI

Post 2.0:

• properly factor out common code between the backends
• rework tests probably

Failing scenarios:

[root@ltcrain80-lp2 secvarctl]# /usr/bin/secvarctl -m guest generate c:a -k ./test/guest/testdata/goldenkeys/PK/PK.key -c ./test/guest/testdata/goldenkeys/PK/PK.crt -n PK -i ./test/guest/testdata/goldenkeys/PK/PK.crt -o pk.auth -a %d234324
RESULT: SUCCESS
[root@ltcrain80-lp2 secvarctl]# /usr/bin/secvarctl -m guest generate c:a -k ./test/guest/testdata/goldenkeys/PK/PK.key -c ./test/guest/testdata/goldenkeys/PK/PK.crt -n PK -i ./test/guest/testdata/goldenkeys/PK/PK.crt -o pk.auth -a string1
RESULT: SUCCESS

secvarctl tool throws the following warning and error while read/write operations.

ERROR: failed to read whole contents of /sys/firmware/secvar/format in one go
WARNING!! Could not extract data from /sys/firmware/secvar/format , assuming platform does not support secure variables
WARNING: Unsupported backend detected, assuming ibm,edk2-compat-v1 backend
Read/write may not work as expected

Size and content of the sysfs file /sys/firmware/secvar/format

# cat /sys/firmware/secvar/format
ibm,edk2-compat-v1
# ls -l /sys/firmware/secvar/format
-r--r--r-- 1 root root 65536 Jun 22 09:37 /sys/firmware/secvar/format

Current working is any update of secure variable completely replaces the previous value. For example, if the KEK currently contains the ESL's X, Y and Z then using secvarctl or writing to the sysfs locations to update the KEK with a new auth file containing the ESL's A and B will set the updated KEK to just contain A and B . So, appending and removing specific entries is currently not supported.
This is the request for feature/enhancement to support append/remove of specific entries from the secure variables.
Thank you.

It'd be really nice to know if a commit passes the tests before I merge it. I'm sure all our committers make only the finest pull requests and test them locally first, of course. But I for one have made a 'trivial' change to my patches and pushed, only to later find out my trivial change was not so trivial and broke something.

I think because the tests work on x86 we could run them under GH Actions without any difficulty.

While reading a certificate using '-c' option seen SIGSEGV

/usr/bin/secvarctl read -c /home/secvarctl/test/guest/testdata/x509certs/grubdb_by_KEK.crt
Segmentation fault (core dumped)

from gdb:
Core was generated by `/usr/bin/secvarctl read -c /home/secvarctl/test/guest/testdata/x509certs/grubdb'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x0000000105f6f620 in guest_read_command ()
Missing separate debuginfos, use: dnf debuginfo-install secvarctl-1.0.0-1.el9.ppc64le
(gdb) bt
#0 0x0000000105f6f620 in guest_read_command ()
#1 0x0000000105f53620 in main ()

This issue not seen in RC2/RC1.

There are a significant number of changes to the skiboot edk2-compat code that was supposed to be a direct copy of the code from skiboot. This goes against the idea that secvarctl is using the exact same code that firmware uses. While it is far too late to go back and fix deployed machines using the skiboot code, we can at least attempt to amend the inconsistency between the two.

There are a few options of differing levels of work:

1. Do nothing, carry changes in secvarctl as is
2. Reorganize the skiboot/edk2-compat code to function better as a standalone library
3. Shove the skiboot/edk2-compat code into libstb-secvar, and migrate skiboot to use libstb-secvar

Option 1

Obviously the easiest solution is to ignore the wild differences between skiboot and secvarctl's versions of the edk2 backend. We can continue to document the changes we make. This is personally least preferable to me, as that leads to a large section of code that we will constantly have to dance around, potentially avoid in static analysis, formatting, etc, BUT we can't largely ignore as we do libstb-secvar, being a separate project.

A sub option here is that secvarctl becomes authoritative, and we move it from external/ to backends/host, and thus stops being considered "external" code. We would then break any consideration of it being the same code as in skiboot, however.

Option 2

Most of the changes made to the edk2-compant backend were largely to work around specific skiboot-isms. This option would see separating out some of the code from headers, files, etc and a migration to using the crypto abstractions, so that the files can work without needing anything from skiboot. Then, no changes should need to be carried in secvarctl for the skiboot/edk2-compat.

Ideally, this updated version would then be sent upstream to skiboot, so that the code between the two can finally be consistent, despite being quite a bit late.

Option 3

This is probably the most invasive change, but the final option is to complete the original intent of libstb-secvar: have it be a single library that handles secure boot key management for all POWER secure boot platforms. This would see upstream skiboot to change to use libstb-secvar instead (either as a new backend, or just straight up amend the old edk2-compat backend). Then any edk2-compat specific code would be moved to libstb-secvar, and there would only need to be one source of external code.

While reading the grubdb: it has 6 ESL’s but secvarctl read only one and thrown error.

...
                85:42:F6:AF:EE:9C:10:2D:47:18:5D:B8:09:66:09:CF:72:00:6B:F7

ERROR: invalid ESL size (184844848)
        Found 1 ESL's

RESULT: SUCCESS

with the internal secvarctl (probable git head at e3658f2ce5d0089e72eb243e8deacaa2ddd577a4) it was showing all 6 ESLs

...
	Found 6 ESL's

RESULT: SUCCESS

Tried to compile on the system where openssl-3.0.1-41 installed and seeing the following

# make OPENSSL=1
make -C ../ secvarctl-cov
make[1]: Entering directory '/home/secvarctl-main'
gcc   -MMD -O2 -std=gnu99 -I./ -Iinclude/ -Iexternal/skiboot/ -Iexternal/skiboot/include -Wall -Werror -DSECVAR_CRYPTO_WRITE_FUNC -DSECVAR_CRYPTO_OPENSSL -c  --coverage external/skiboot/libstb/secvar/crypto/crypto-openssl.c -o external/skiboot/libstb/secvar/crypto/crypto-openssl.cov.o
external/skiboot/libstb/secvar/crypto/crypto-openssl.c: In function 'crypto_x509_get_pk_bit_len':
external/skiboot/libstb/secvar/crypto/crypto-openssl.c:518:9: error: 'EVP_PKEY_get1_RSA' is deprecated: Since OpenSSL 3.0 [-Werror=deprecated-declarations]
  518 |         rsa = EVP_PKEY_get1_RSA(pub);
      |         ^~~
In file included from /usr/include/openssl/x509.h:29,
                 from external/skiboot/libstb/secvar/crypto/crypto.h:10,
                 from external/skiboot/libstb/secvar/crypto/crypto-openssl.c:8:
/usr/include/openssl/evp.h:1348:16: note: declared here
 1348 | struct rsa_st *EVP_PKEY_get1_RSA(EVP_PKEY *pkey);
      |                ^~~~~~~~~~~~~~~~~
external/skiboot/libstb/secvar/crypto/crypto-openssl.c:524:9: error: 'RSA_bits' is deprecated: Since OpenSSL 3.0 [-Werror=deprecated-declarations]
  524 |         length = RSA_bits(rsa);
      |         ^~~~~~
In file included from /usr/include/openssl/x509.h:36,
                 from external/skiboot/libstb/secvar/crypto/crypto.h:10,
                 from external/skiboot/libstb/secvar/crypto/crypto-openssl.c:8:
/usr/include/openssl/rsa.h:203:27: note: declared here
  203 | OSSL_DEPRECATEDIN_3_0 int RSA_bits(const RSA *rsa);
      |                           ^~~~~~~~
external/skiboot/libstb/secvar/crypto/crypto-openssl.c:525:9: error: 'RSA_free' is deprecated: Since OpenSSL 3.0 [-Werror=deprecated-declarations]
  525 |         RSA_free(rsa);
      |         ^~~~~~~~
In file included from /usr/include/openssl/x509.h:36,
                 from external/skiboot/libstb/secvar/crypto/crypto.h:10,
                 from external/skiboot/libstb/secvar/crypto/crypto-openssl.c:8:
/usr/include/openssl/rsa.h:293:28: note: declared here
  293 | OSSL_DEPRECATEDIN_3_0 void RSA_free(RSA *r);
      |                            ^~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:78: external/skiboot/libstb/secvar/crypto/crypto-openssl.cov.o] Error 1
make[1]: Leaving directory '/home/secvarctl-main'
make: *** [Makefile:35: ../secvarctl-cov] Error 2

Thanks!!

This is primarily a tool for running on a POWER box, so we really should actually test that it compiles for and (mostly) runs on POWER. We can't test that it works on native hardware, but we can at least confirm it compiles and runs.

Side bonus: we may want to be testing both endians, as that might be important if something like libstb-secvar code runs in a BE environment, even though we will likely not be running in a BE userspace.

We recently had a bug come in because file locations changed which caused make install to fail (#58 ).

This is a request to add a test in build_test.yml to ensure make and cmake installations work.

In multiple places in the code (namely guest_svc_read.c:read_path()), we load in data from a variable, then proceed to do some level of validation which mostly consists of checking that the buffer is an expected size before doing any parsing. As asked in PR #75 , the "expected" size of a buffer should be defined, documented, and probably abstracted out so that other places of the code can call a function to read in a particular variable and do this same validation.

From my understanding, the following conditions should be checked and handled on the buffer size:

• size == 0 → error case, likely invalid file or some other I/O error. variables should not be completely empty
• 0 < size < HEADER_LEN → error case, variable must contain at least the 16-byte header
• size == HEADER_LEN → valid case, variable exists but contains no data
• size > HEADER_LEN → valid case, data exists and can be parsed

Data read from a variable contains a header of 16 bytes, consisting of a 1-byte version number, then 15 bytes of timestamp, truncating the last pad so that the header remains aligned.

Cleaning this up will probably looking some like:

• factor out header parsing into another function, perhaps variable_from_path()
• remove references to TIMESTAMP_LEN and TIMESTAMP_LEN - 1 when referring to this header
• actually parse and inspect the version number?
• clean up any duplicate code paths that differentiated reading from a variable vs reading from a local file

Since the large merge/overhaul, documentation has gotten fairly messy and probably out of date. These should be reviewed and potentially entirely rewritten to match the current state.

build.log

Looks like the path of the file to install is bad

There is no need for this anymore, and it is annoying for symbols to go missing with no warning.