Hi Simon,
On 7/13/2021 7:48 AM, THOBY Simon wrote:
Since you had stated that this is your first kernel contribution, I
wanted to give you some ideas on how to organize the code changes, how
to write patch description, etc. I hope that helps.
This patch was built on top of linux-5.14.0-rc1, and I successfully tested the patch
in the following configurations:
- disabled: no visible impact (as expected)
- with ima_allowed_hashes=sha256,sha512:
- alone: blocks both execution and xattr writes
(tested with `emvctl ima_hash -a md5 <my binary>`)
- with ima_appraise=log: audit logs but still permits access
- with ima_appraise=fix: audit logs but still permits access, however it
doesn't fix the hash (so maybe I should do something about it, because
right now it's basically the same as ima_appraise=log w.r.t. hash
algorithms ?)
- with ima_accept_any_hash: permits access, no warnings whatsoever
Do you want ideas of other configurations that I could test?
As Mimi had stated, you should split the code changes into smaller
patches and include a cover letter for the patch set.
In the cover letter you can describe the problem you are addressing,
provide the motivation for the change and a more detailed description of
the feature.
And, in the individual patches describe the code changes specific to
that patch.
Please take a look at the cover letter and patches in the patch set
Tushar Sugandhi had authored for "measuring kernel integrity critical
data using IMA" (Links given below):
https://lore.kernel.org/linux-integrity/20210108040708.8389-1-tusharsu@xxxxxxxxxxxxxxxxxxx/
https://patchwork.kernel.org/project/linux-integrity/patch/20210108040708.8389-2-tusharsu@xxxxxxxxxxxxxxxxxxx/
Split the code changes into smaller patches with each patch building on
top of the previous patch(es). You can look at Tushar's patch set to get
an idea on how to organize the patches in a logical way.
I would also like to point out that I'm more than open to suggestions for
changing the names of the parameters (`ima_allowed_hashes` and
`ima_accept_any_hash`) and the "cause" in the audit log (currently
forbidden-hash-algorithm"), because as you know, "naming things is hard".
IMA: add an option to restrict the accepted hash algorithms
Adds two command-line parameters to limit the hash algorithms
allowed for the security.ima xattr. This gives users the
ability to ensure their systems will not accept weak hashes,
The patch description should first state what the problem is that you
are fixing. For example, the patch to "generalize keyring specific
measurement constructs" states:
IMA functions such as ima_match_keyring(), process_buffer_measurement(),
ima_match_policy() etc. handle data specific to keyrings. Currently,
these constructs are not generic to handle any func specific data.
This makes it harder to extend them without code duplication.
Then, describe the change to address the issue. The patch description
should be written in imperative style (as if you are issuing commands to
do certain actions). For example, to address the above issue in the
current code that makes it hard to extend without code duplication, what
action is to be performed:
Refactor the keyring specific measurement constructs to be generic and
reusable in other measurement scenarios.
With respect to the actual code change, please look into the comment
Mimi had given earlier (copied below from her email) on minimizing boot
command line options:
Boot command line options should be minimized as much as possible.
Perhaps without defining new kernel boot paramaters there are some
additional checks that could be added. For example, on a FIPS enabled
system, prevent writing non FIPS allowed file signatures, limit file
signature algorithms to those enabled on the system, define new policy
rules to limit the permitted hash algorithms.
thanks,
-lakshmi
and potentially increase users trust in their IMA configuration,
because they can ensure only strong collision-resistant hashes
are employed and files generated otherwise will not be accepted.
The main point is to safeguard users from mislabelling their files
when using userland utilities to update their files, e.g. evmctl
(`evmctl ima_hash` defaults to sha1). Another possible target
would be people that have deployed IMA years ago, possibly using
algorithms that was then deemed sufficiently collision-resistant,
but that proved to be weak with the passage of time (note that this
could also happen in the future with algorithms considered safe today).
This patch also provides a migration path for users.
The first parameter supplies a list of allowed algorithms to
the kernel, restricting appraisal to files hashed with "strong"
hash algorithms (by default IMA will keep accepting any hash
algorithm, as enabling such a feature by default would both be
backward-incompatible and very probably break real systems).
The second parameter is an escape hatch that adds a weaker form
of backward-compatibility: when activated, IMA apparaisal will
keep working on files hashed with an algorithm not present in
the list, but updates to these files or new writes to the security.ima
xattr will enforce the selected hash algorithms. This may be useful
to perform an online migration from one algorithm to another.
Signed-off-by: Simon Thoby <simon.thoby@xxxxxxxxxx>
---
.../admin-guide/kernel-parameters.txt | 42 +++++++++
security/integrity/ima/ima.h | 6 +-
security/integrity/ima/ima_appraise.c | 22 +++++
security/integrity/ima/ima_main.c | 88 ++++++++++++++++++-
4 files changed, 156 insertions(+), 2 deletions(-)
