linux-kernel - Re: [PATCH v20 02/20] ipe: add policy parser

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: <cbf1caa0-835b-4d1d-aed5-9741eb10cf8b@linux.microsoft.com>
Date: Wed, 14 Aug 2024 11:23:39 -0700
From: Fan Wu <wufan@...ux.microsoft.com>
To: Paul Moore <paul@...l-moore.com>, "Serge E. Hallyn" <serge@...lyn.com>
Cc: corbet@....net, zohar@...ux.ibm.com, jmorris@...ei.org, tytso@....edu,
 ebiggers@...nel.org, axboe@...nel.dk, agk@...hat.com, snitzer@...nel.org,
 mpatocka@...hat.com, eparis@...hat.com, linux-doc@...r.kernel.org,
 linux-integrity@...r.kernel.org, linux-security-module@...r.kernel.org,
 fsverity@...ts.linux.dev, linux-block@...r.kernel.org,
 dm-devel@...ts.linux.dev, audit@...r.kernel.org,
 linux-kernel@...r.kernel.org, Deven Bowers <deven.desai@...ux.microsoft.com>
Subject: Re: [PATCH v20 02/20] ipe: add policy parser



On 8/13/2024 6:53 PM, Paul Moore wrote:
> On Tue, Aug 13, 2024 at 1:54 PM Fan Wu <wufan@...ux.microsoft.com> wrote:
>> On 8/10/2024 8:50 AM, Serge E. Hallyn wrote:
>>> On Fri, Aug 02, 2024 at 11:08:16PM -0700, Fan Wu wrote:
>>>> From: Deven Bowers <deven.desai@...ux.microsoft.com>
>>>>
>>>> IPE's interpretation of the what the user trusts is accomplished through
>>>
>>> nit: "of what the user trusts" (drop the extra 'the')
>>>
>>>> its policy. IPE's design is to not provide support for a single trust
>>>> provider, but to support multiple providers to enable the end-user to
>>>> choose the best one to seek their needs.
>>>>
>>>> This requires the policy to be rather flexible and modular so that
>>>> integrity providers, like fs-verity, dm-verity, or some other system,
>>>> can plug into the policy with minimal code changes.
>>>>
>>>> Signed-off-by: Deven Bowers <deven.desai@...ux.microsoft.com>
>>>> Signed-off-by: Fan Wu <wufan@...ux.microsoft.com>
>>>
>>> This all looks fine.  Just one comment below.
>>>
>> Thank you for reviewing this!
>>
>>>
>>>> +/**
>>>> + * parse_rule() - parse a policy rule line.
>>>> + * @line: Supplies rule line to be parsed.
>>>> + * @p: Supplies the partial parsed policy.
>>>> + *
>>>> + * Return:
>>>> + * * 0              - Success
>>>> + * * %-ENOMEM       - Out of memory (OOM)
>>>> + * * %-EBADMSG      - Policy syntax error
>>>> + */
>>>> +static int parse_rule(char *line, struct ipe_parsed_policy *p)
>>>> +{
>>>> +    enum ipe_action_type action = IPE_ACTION_INVALID;
>>>> +    enum ipe_op_type op = IPE_OP_INVALID;
>>>> +    bool is_default_rule = false;
>>>> +    struct ipe_rule *r = NULL;
>>>> +    bool first_token = true;
>>>> +    bool op_parsed = false;
>>>> +    int rc = 0;
>>>> +    char *t;
>>>> +
>>>> +    r = kzalloc(sizeof(*r), GFP_KERNEL);
>>>> +    if (!r)
>>>> +            return -ENOMEM;
>>>> +
>>>> +    INIT_LIST_HEAD(&r->next);
>>>> +    INIT_LIST_HEAD(&r->props);
>>>> +
>>>> +    while (t = strsep(&line, IPE_POLICY_DELIM), line) {
>>>
>>> If line is passed in as NULL, t will be NULL on the first test.  Then
>>> you'll break out and call parse_action(NULL), which calls
>>> match_token(NULL, ...), which I do not think is safe.
>>>
>>> I realize the current caller won't pass in NULL, but it seems worth
>>> checking for here in case some future caller is added by someone
>>> who's unaware.
>>>
>>> Or, maybe add 'line must not be null' to the function description.
>>
>> Yes, I agree that adding a NULL check would be better. I will include it
>> in the next version.
> 
> We're still waiting to hear back from the device-mapper devs, but if
> this is the only change required to the patchset I can add a NULL
> check when I merge the patchset as it seems silly to resend the entire
> patchset for this.  Fan, do you want to share the code snippet with
> the NULL check so Serge can take a look?
> 

Sure, here is the diff.

diff --git a/security/ipe/policy_parser.c b/security/ipe/policy_parser.c
index 32064262348a..0926b442e32a 100644
--- a/security/ipe/policy_parser.c
+++ b/security/ipe/policy_parser.c
@@ -309,6 +309,9 @@ static int parse_rule(char *line, struct 
ipe_parsed_policy *p)
         int rc = 0;
         char *t;

+       if (IS_ERR_OR_NULL(line))
+               return -EBADMSG;
+
         r = kzalloc(sizeof(*r), GFP_KERNEL);
         if (!r)
                 return -ENOMEM;

-Fan