Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RFC: Fapi preview fw and ima logging cel #2170

Merged
merged 7 commits into from
Oct 31, 2022
Merged

RFC: Fapi preview fw and ima logging cel #2170

merged 7 commits into from
Oct 31, 2022

Conversation

JuergenReppSIT
Copy link
Member

@JuergenReppSIT JuergenReppSIT commented Oct 7, 2021 •
edited
Loading

FAPI: Add event logging for firmware and IMA events.

Add serialization and deserialization for firmware events:

  • Serialization and deserialization of firmware events specified in the
    TCG PC Client Platform Firmware Profile was added.
  • Also for the firmare legacy format (sha1) the serialization and deserialization
    was added.

Add serialization and deserialization for IMA events:

Integrate firmware and IMA eventlog:

  • The IMA event logging was integrated.
  • The PC-Client firmware logging was integrated.

CEL Events:

  • The cel_version event was added as first event for the event log of PCR 0.
  • The firmware_end event was added behind the last firmware event.
  • Serialization and deserialization of CEL events was added.

Event logging:

  • For invalid digests an error will only produced for quote verification.
    In other cases only a warning will be displayed.
  • rename "type" to "content_type".
  • rename "sub_event" to "content".

Tests for event logging for firmware and IMA events:

  • An Unit test for several binary examples was added.
  • Tests in fapi-jons.c were adapted.
  • Copied functions were removed from unit test fapi-json.c
  • An Integration test for The PC-Client firmware logging was added.
  • An Integration test for The PC-Client firmware logging in the
    legacy format was added.

Signed-off-by: Juergen Repp juergen.repp@sit.fraunhofer.de

@lgtm-com
Copy link

lgtm-com bot commented Oct 7, 2021

This pull request introduces 2 alerts when merging e77b7f9 into 5902fba - view on LGTM.com

new alerts:

  • 1 for Suspicious add with sizeof
  • 1 for Comparison result is always the same

@JuergenReppSIT JuergenReppSIT force-pushed the fapi-preview-fw-and-ima-logging-cel branch from e77b7f9 to 9e13b2f Compare October 7, 2021 11:47
@lgtm-com
Copy link

lgtm-com bot commented Oct 7, 2021

This pull request introduces 2 alerts when merging 9e13b2f into 5902fba - view on LGTM.com

new alerts:

  • 1 for Suspicious add with sizeof
  • 1 for Comparison result is always the same

@JuergenReppSIT JuergenReppSIT force-pushed the fapi-preview-fw-and-ima-logging-cel branch from 9e13b2f to 9ef82b6 Compare October 7, 2021 16:44
@lgtm-com
Copy link

lgtm-com bot commented Oct 7, 2021

This pull request introduces 2 alerts when merging 9ef82b6 into 5902fba - view on LGTM.com

new alerts:

  • 1 for Suspicious add with sizeof
  • 1 for Comparison result is always the same

@JuergenReppSIT JuergenReppSIT force-pushed the fapi-preview-fw-and-ima-logging-cel branch from 9ef82b6 to ddb1768 Compare October 7, 2021 21:06
@lgtm-com
Copy link

lgtm-com bot commented Oct 7, 2021

This pull request introduces 2 alerts when merging ddb1768 into 5902fba - view on LGTM.com

new alerts:

  • 1 for Suspicious add with sizeof
  • 1 for Comparison result is always the same

@JuergenReppSIT JuergenReppSIT force-pushed the fapi-preview-fw-and-ima-logging-cel branch from ddb1768 to 476bd6e Compare October 11, 2021 09:07
@lgtm-com
Copy link

lgtm-com bot commented Oct 11, 2021

This pull request introduces 2 alerts when merging 476bd6e into 5902fba - view on LGTM.com

new alerts:

  • 1 for Suspicious add with sizeof
  • 1 for Comparison result is always the same

@JuergenReppSIT JuergenReppSIT force-pushed the fapi-preview-fw-and-ima-logging-cel branch from 476bd6e to dc12cb6 Compare October 11, 2021 13:49
@lgtm-com
Copy link

lgtm-com bot commented Oct 11, 2021

This pull request introduces 2 alerts when merging dc12cb6 into 5902fba - view on LGTM.com

new alerts:

  • 1 for Suspicious add with sizeof
  • 1 for Comparison result is always the same

@JuergenReppSIT JuergenReppSIT force-pushed the fapi-preview-fw-and-ima-logging-cel branch from dc12cb6 to 1bb8dec Compare October 11, 2021 14:40
@lgtm-com
Copy link

lgtm-com bot commented Oct 11, 2021

This pull request introduces 2 alerts when merging 1bb8dec into 5902fba - view on LGTM.com

new alerts:

  • 1 for Suspicious add with sizeof
  • 1 for Comparison result is always the same

@JuergenReppSIT JuergenReppSIT force-pushed the fapi-preview-fw-and-ima-logging-cel branch from 1bb8dec to 2fa0d3f Compare October 11, 2021 15:08
@lgtm-com
Copy link

lgtm-com bot commented Oct 11, 2021

This pull request introduces 2 alerts when merging 2fa0d3f into 5902fba - view on LGTM.com

new alerts:

  • 1 for Suspicious add with sizeof
  • 1 for Comparison result is always the same

@JuergenReppSIT JuergenReppSIT force-pushed the fapi-preview-fw-and-ima-logging-cel branch from 2fa0d3f to 81fe356 Compare October 11, 2021 15:31
@lgtm-com
Copy link

lgtm-com bot commented Oct 11, 2021

This pull request introduces 2 alerts when merging 81fe356 into 5902fba - view on LGTM.com

new alerts:

  • 1 for Suspicious add with sizeof
  • 1 for Comparison result is always the same

@lgtm-com
Copy link

lgtm-com bot commented Oct 13, 2021

This pull request introduces 2 alerts when merging ba755a8 into 5902fba - view on LGTM.com

new alerts:

  • 1 for Suspicious add with sizeof
  • 1 for Comparison result is always the same

@JuergenReppSIT JuergenReppSIT force-pushed the fapi-preview-fw-and-ima-logging-cel branch from ba755a8 to 62d0a28 Compare October 13, 2021 11:18
@JuergenReppSIT JuergenReppSIT force-pushed the fapi-preview-fw-and-ima-logging-cel branch 2 times, most recently from e4fcee5 to dc4d1a6 Compare November 16, 2021 13:24
@AndreasFuchsTPM AndreasFuchsTPM added this to the Version 3.3 milestone Nov 30, 2021
@JuergenReppSIT JuergenReppSIT force-pushed the fapi-preview-fw-and-ima-logging-cel branch 5 times, most recently from d2a9657 to 61cc8cd Compare January 27, 2022 15:12
@JuergenReppSIT JuergenReppSIT force-pushed the fapi-preview-fw-and-ima-logging-cel branch from 61cc8cd to 0d72974 Compare March 1, 2022 18:36
@JuergenReppSIT JuergenReppSIT changed the title Fapi preview fw and ima logging cel RFC: Fapi preview fw and ima logging cel Mar 10, 2022
@JuergenReppSIT JuergenReppSIT force-pushed the fapi-preview-fw-and-ima-logging-cel branch from 0d72974 to 7cb07d8 Compare April 5, 2022 17:02
@AndreasFuchsTPM
Copy link
Member

@williamcroberts @JuergenReppSIT Let's have the discussion here:
Should we roll a new major release (4.0) because we change the CEL format or do we consider the old format was still beta anyways and not much used and just roll 3.3 after this one ?

@JuergenReppSIT
Copy link
Member Author

@williamcroberts @JuergenReppSIT Let's have the discussion here: Should we roll a new major release (4.0) because we change the CEL format or do we consider the old format was still beta anyways and not much used and just roll 3.3 after this one ?

@williamcroberts @AndreasFuchsTPM I think the new CEL format can be part of 3.3 because the old format was beta without IMA and without system events. An old log file could be converted to the new format with a simple sed script if really needed.

@JuergenReppSIT JuergenReppSIT force-pushed the fapi-preview-fw-and-ima-logging-cel branch from bfe6f4b to f550fb0 Compare September 14, 2022 11:05
@williamcroberts
Copy link
Member

@williamcroberts @JuergenReppSIT Let's have the discussion here: Should we roll a new major release (4.0) because we change the CEL format or do we consider the old format was still beta anyways and not much used and just roll 3.3 after this one ?

@williamcroberts @AndreasFuchsTPM I think the new CEL format can be part of 3.3 because the old format was beta without IMA and without system events. An old log file could be converted to the new format with a simple sed script if really needed.

Yeah this was what @JuergenReppSIT and I discussed before and we landed on this approach. If we can prevent a major release I am for it.

@JuergenReppSIT JuergenReppSIT force-pushed the fapi-preview-fw-and-ima-logging-cel branch from f550fb0 to 02840dc Compare September 20, 2022 15:04
@JuergenReppSIT
Copy link
Member Author

@williamcroberts @AndreasFuchsTPM AFL fuzzing has now been running for more than 5 days on 20 CPUs without finding more errors. So could the PR now be merged for 3.3?
@williamcroberts I could not get into the maintainer sync today. Was it canceled?

@JuergenReppSIT JuergenReppSIT force-pushed the fapi-preview-fw-and-ima-logging-cel branch from 02840dc to 7b6c203 Compare September 28, 2022 08:34
@JuergenReppSIT JuergenReppSIT force-pushed the fapi-preview-fw-and-ima-logging-cel branch from 7b6c203 to aeccd71 Compare October 8, 2022 14:09
@williamcroberts
Copy link
Member

williamcroberts commented Oct 26, 2022 •
edited by JuergenReppSIT
Loading

@JuergenReppSIT

  • Make sure all added, non-generated, files have the BSD-2-Clause SPDX identifier
  • for the bin files, could we make a build rule that generates them from b64 encoded to avoid sticking a bunch of binary blobs in the repo?
  • Fix the conflict in Makefile.am

Then I think we can merge this, right?

@JuergenReppSIT
configure: Allow warning address-of-packed-member
61ad975 
The desearialization of bios events need packed structures. Therefore this
warning has to be allowed.

Signed-off-by: Juergen Repp <juergen.repp@sit.fraunhofer.de>
@JuergenReppSIT
FAPI: Change initial value of recnum for PCR event lists.
673f91c 
The initial value is changed from 1 to 0.

Signed-off-by: Juergen Repp <juergen.repp@sit.fraunhofer.de>
@JuergenReppSIT JuergenReppSIT force-pushed the fapi-preview-fw-and-ima-logging-cel branch from aeccd71 to ca9f13b Compare October 26, 2022 19:04
@JuergenReppSIT
Copy link
Member Author

@JuergenReppSIT

* [x]  Make sure all added, non-generated, files have the BSD-2-Clause SPDX identifier

I have added a new commit to the PR which adds the SPDX identifier to all files without one or a wrong identifier.

* [ ]  for the bin files, could we make a build rule that generates them from b64 encoded to avoid sticking a bunch of binary blobs in the repo?

Is this really necessary? The tpm-tools also store binaries under test/integration/fixtures

* [x]  Fix the conflict in Makefile.am

Then I think we can merge this, right?
I would say yes.

@williamcroberts
Copy link
Member 

* [ ]  for the bin files, could we make a build rule that generates them from b64 encoded to avoid sticking a bunch of binary blobs in the repo?

Is this really necessary? The tpm-tools also store binaries under test/integration/fixtures

We should never be stuffing binary files into git. If we have to frequently update the files it can/may consume a lot of space in the .git db. I guess in this scenario it's unlikely, but I think its best to avoid any issue. We can then just have the Makefile-test.am include them as a dependency and build them using b64decode.

@JuergenReppSIT
Copy link
Member Author

We should never be stuffing binary files into git. If we have to frequently update the files it can/may consume a lot of space in the .git db. I guess in this scenario it's unlikely, but I think its best to avoid any issue. We can then just have the Makefile-test.am include them as a dependency and build them using b64decode.

OK I will add it.

@williamcroberts
Copy link
Member

We should never be stuffing binary files into git. If we have to frequently update the files it can/may consume a lot of space in the .git db. I guess in this scenario it's unlikely, but I think its best to avoid any issue. We can then just have the Makefile-test.am include them as a dependency and build them using b64decode.

OK I will add it.

Thanks, I know it's a pain.

@JuergenReppSIT
Copy link
Member Author

Thanks, I know it's a pain.

I think next Friday will be the day of pain for me ;-)

@JuergenReppSIT JuergenReppSIT force-pushed the fapi-preview-fw-and-ima-logging-cel branch 2 times, most recently from f7ffc3c to 7382004 Compare October 28, 2022 19:52
JuergenReppSIT and others added 2 commits October 28, 2022 23:46
@JuergenReppSIT
FAPI: Add event logging for firmware and IMA events.
58fcf90 
 Add serialization and deserialization for firmware events:

 * Serialization and deserialization of firmware events specified in the
   TCG PC Client Platform Firmware Profile was added.
 * Also for the firmare legacy format (sha1) the serialization and deserialization
   was added.

 Add serialization and deserialization for IMA events:

 * Serialization of IMA events as described on: https://sourceforge.net/p/linux-ima/wiki/Home/
   was added. The templates ima, img-sig, ima-ng, and ima-sig are supported.
 * Tests in fapi-jons.c were adapted.

 Integrate firmware and IMA eventlog:

   has to be explicitly activated with a configure switch.
 * The IMA event logging was integrated.
 * The PC-Client firmware logging was integrated.

CEL Events:

* The cel_version event was added as first event for the event log of PCR 0.
* The firmware_end event was added behind the last firmware event.
* Serialization and deserialization of CEL events was added.

Event logging:

* For invalid digests an error will only produced for quote verification.
  In other cases only a warning will be displayed.
* rename "type" to "content_type".
* rename "sub_event" to "content".

Tests for event logging for firmware and IMA events:

* An Unit test for several binary examples was added.
* Tests in fapi-jons.c were adapted.
* Copied functions were removed from unit test fapi-json.c
* An Integration test for The PC-Client firmware logging was added.
* An Integration test for The PC-Client firmware logging in the
  legacy format was added.

Parameters for configure were added with the following defaults:
* --with-imameasuerments=/sys/kernel/security/ima/binary_runtime_measurements
* --with-sysmeasurements=/sys/kernel/security/tpm0/binary_bios_measurement

Signed-off-by: Juergen Repp <juergen.repp@sit.fraunhofer.de>
Signed-off-by: Juergen Repp <juergen_repp@web.de>
@JuergenReppSIT
fapi: start systemd oneshot service
a981e26 
If systemd is available a systemd oneshot service to change the group
of the IMA and the TPM system logging files
/sys/kernel/security/ima/binary_runtime_measurements
/sys/kernel/security/tpm0/binary_bios_measurements
from root to tss will be enabled and started.

Signed-off-by: Juergen Repp <juergen_repp@web.de>
@JuergenReppSIT JuergenReppSIT force-pushed the fapi-preview-fw-and-ima-logging-cel branch from 7382004 to 8eac55f Compare October 28, 2022 21:53
@JuergenReppSIT
Copy link
Member Author

@williamcroberts I have added a commit where the fapi binary test files are created from base64 encoded files.

@codecov
Copy link

codecov bot commented Oct 29, 2022

Codecov Report

Merging #2170 (7382004) into master (6c88eea) will decrease coverage by 0.51%.
The diff coverage is 70.23%.

❗ Current head 7382004 differs from pull request most recent head 88bcdbe. Consider uploading reports for the commit 88bcdbe to get more accurate results

@@            Coverage Diff             @@
##           master    #2170      +/-   ##
==========================================
- Coverage   83.62%   83.11%   -0.52%     
==========================================
  Files         351      355       +4     
  Lines       37959    39643    +1684     
==========================================
+ Hits        31745    32949    +1204     
- Misses       6214     6694     +480
Impacted Files Coverage Δ
src/tss2-esys/api/Esys_Commit.c 93.93% <ø> (ø)
src/tss2-esys/api/Esys_EC_Ephemeral.c 93.58% <ø> (ø)
src/tss2-esys/api/Esys_GetCapability.c 96.15% <ø> (ø)
src/tss2-esys/api/Esys_GetTestResult.c 93.58% <ø> (ø)
src/tss2-esys/api/Esys_PCR_Allocate.c 96.10% <ø> (ø)
src/tss2-esys/api/Esys_PCR_Read.c 92.85% <ø> (ø)
src/tss2-esys/esys_iutil.c 91.33% <ø> (ø)
src/tss2-esys/esys_mu.c 91.53% <0.00%> (+2.88%) ⬆️
src/tss2-fapi/fapi_crypto.c 78.80% <ø> (+1.16%) ⬆️
src/tss2-fapi/tpm_json_deserialize.c 85.92% <ø> (ø)
... and 43 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@JuergenReppSIT
FAPI: Store binary test data base64 encoded.
3655b30 
* To avoid storing binary data in git the binary test files are stored
  bas64 encoded. Rules to decode the files if needed were added.
* The tests had to be adapted to use test data which has now been
  generated in the build directory.

Signed-off-by: Juergen Repp <juergen_repp@web.de>
@JuergenReppSIT
FAPI: Enable usage of AFL Fuzzer for IMA and system events.
8af8882 
* Two unit tests which can be used for fuzzing were added.
* Scripts to start AFL fuzzin were added:
  afl-fuzzing/fuzz-system.sh
  afl-fuzzing/fuzz-ima.sh
* The tests can be started if afl++ is installed.
* The tests are not integrated into the CI because of the long
  run time
* If crashes are detected the unit tests can be used for debugging
  with the crash file in findings-system/crashes or finding-ima/crashes:
    ./test/unit/fapi-{ima,sysem}-fuzzing <crash-file>

Signed-off-by: Juergen Repp <juergen_repp@web.de>
@JuergenReppSIT
SPDX License Identifier added.
b9041aa 
* The licence identifier war added to file without one.
* BSD-2 was replaced by BSD-2-Clause.

Signed-off-by: Juergen Repp <juergen_repp@web.de>
@JuergenReppSIT JuergenReppSIT force-pushed the fapi-preview-fw-and-ima-logging-cel branch from 88bcdbe to b9041aa Compare October 29, 2022 07:25
@williamcroberts williamcroberts merged commit 213d0cf into tpm2-software:master Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndreasFuchsTPM AndreasFuchsTPM AndreasFuchsTPM left review comments

@williamcroberts williamcroberts williamcroberts approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Version 4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@JuergenReppSIT @AndreasFuchsTPM @williamcroberts