linux-kernel - Re: [PATCH v2 4/5] LSM: Define SELinux function to measure security state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9478ddca-8298-5170-836d-8cbc7a070df2@linux.microsoft.com>
Date:   Thu, 16 Jul 2020 12:13:15 -0700
From:   Lakshmi Ramasubramanian <nramas@...ux.microsoft.com>
To:     Stephen Smalley <stephen.smalley.work@...il.com>
Cc:     Mimi Zohar <zohar@...ux.ibm.com>,
        Casey Schaufler <casey@...aufler-ca.com>,
        James Morris <jmorris@...ei.org>,
        linux-integrity@...r.kernel.org,
        SElinux list <selinux@...r.kernel.org>,
        LSM List <linux-security-module@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 4/5] LSM: Define SELinux function to measure security
 state

On 7/16/20 11:54 AM, Stephen Smalley wrote:

>> The data for selinux-state in the above measurement is:
>> enabled=1;enforcing=0;checkreqprot=1;network_peer_controls=1;open_perms=1;extended_socket_class=1;always_check_network=0;cgroup_seclabel=1;nnp_nosuid_transition=1;genfs_seclabel_symlinks=0;
>>
>> The data for selinux-policy-hash in the above measurement is
>> the SHA256 hash of the SELinux policy.
> 
> Can you show an example of how to verify that the above measurement
> matches a given state and policy, e.g. the sha256sum commands and
> inputs to reproduce the same from an expected state and policy?
Sure - I'll provide an example.

>> +/* Pre-allocated buffer used for measuring state */
>> +static char *selinux_state_string;
>> +static size_t selinux_state_string_len;
>> +static char *selinux_state_string_fmt =
>> +       "%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;";
>> +
>> +void __init selinux_init_measurement(void)
>> +{
>> +       selinux_state_string_len =
>> +       snprintf(NULL, 0, selinux_state_string_fmt,
>> +       "enabled", 0,
>> +       "enforcing", 0,
>> +       "checkreqprot", 0,
>> +       selinux_policycap_names[POLICYDB_CAPABILITY_NETPEER], 0,
>> +       selinux_policycap_names[POLICYDB_CAPABILITY_OPENPERM], 0,
>> +       selinux_policycap_names[POLICYDB_CAPABILITY_EXTSOCKCLASS], 0,
>> +       selinux_policycap_names[POLICYDB_CAPABILITY_ALWAYSNETWORK], 0,
>> +       selinux_policycap_names[POLICYDB_CAPABILITY_CGROUPSECLABEL], 0,
>> +       selinux_policycap_names[POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION], 0,
>> +       selinux_policycap_names[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS],
>> +       0);
> 
> I was thinking you'd dynamically construct the format string with a
> for loop from 0 to POLICYDB_CAPABILITY_MAX
> and likewise for the values so that we wouldn't have to patch this
> code every time we add a new one.
That's a good point - will do.

> 
>> +
>> +       if (selinux_state_string_len < 0)
>> +               return;
> 
> How can this happen legitimately (i.e. as a result of something other
> than a kernel bug)?
Since snprintf can return an error I wanted to handle that. But I agree 
this should not happen for the input data to snprintf used here.

> 
>> +
>> +       ++selinux_state_string_len;
>> +
>> +       selinux_state_string = kzalloc(selinux_state_string_len, GFP_KERNEL);
>> +       if (!selinux_state_string)
>> +               selinux_state_string_len = 0;
>> +}
> 
> Not sure about this error handling approach (silent, proceeding as if
> the length was zero and then later failing with ENOMEM on every
> attempt?). I'd be more inclined to panic/BUG here but I know Linus
> doesn't like that.
I am not sure if failing (kernel panic/BUG) to "measure" LSM data under 
memory pressure conditions is the right thing. But I am open to treating 
this error as a fatal error. Please let me know.

> 
>> +       if (ret)
>> +               pr_err("%s: error %d\n", __func__, ret);
> 
> This doesn't seem terribly useful as an error message; I'd be inclined
> to drop it.
> 
Will do.

thanks,
  -lakshmi