[PATCH v11 11/12] samples/landlock: Add network demo
Mickaël Salaün
mic at digikod.net
Mon Jul 3 17:09:26 UTC 2023
On 03/07/2023 14:50, Konstantin Meskhidze (A) wrote:
>
>
> 6/22/2023 1:18 PM, Mickaël Salaün пишет:
>>
>> On 22/06/2023 10:00, Konstantin Meskhidze (A) wrote:
>>>
>>>
>>> 6/19/2023 9:19 PM, Mickaël Salaün пишет:
>>>>
>>>> On 19/06/2023 16:24, Konstantin Meskhidze (A) wrote:
>>>>>
>>>>>
>>>>> 6/13/2023 11:38 PM, Mickaël Salaün пишет:
>>>>>>
>>>>>> On 13/06/2023 12:54, Konstantin Meskhidze (A) wrote:
>>>>>>>
>>>>>>>
>>>>>>> 6/6/2023 6:17 PM, Günther Noack пишет:
>>>>>>>> Hi Konstantin!
>>>>>>>>
>>>>>>>> Apologies if some of this was discussed before, in this case,
>>>>>>>> Mickaël's review overrules my opinions from the sidelines ;)
>>>>>>>>
>>>>>>>> On Tue, May 16, 2023 at 12:13:38AM +0800, Konstantin Meskhidze wrote:
>>>>>>>>> This commit adds network demo. It's possible to allow a sandboxer to
>>>>>>>>> bind/connect to a list of particular ports restricting network
>>>>>>>>> actions to the rest of ports.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze at huawei.com>
>>>>>>>>
>>>>>>>>
>>>>>>>>> diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
>>>>>>>>> index e2056c8b902c..b0250edb6ccb 100644
>>>>>>>>> --- a/samples/landlock/sandboxer.c
>>>>>>>>> +++ b/samples/landlock/sandboxer.c
>>>>>>>>
>>>>>>>> ...
>>>>>>>>
>>>>>>>>> +static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
>>>>>>>>> + const __u64 allowed_access)
>>>>>>>>> +{
>>>>>>>>> + int num_ports, i, ret = 1;
>>>>>>>>
>>>>>>>> I thought the convention was normally to set ret = 0 initially and to
>>>>>>>> override it in case of error, rather than the other way around?
>>>>>>
>>>>>> Which convention? In this case, by default the return code is an error.
>>>>>>
>>>>>>
>>>>>>>>
>>>>>>> Well, I just followed Mickaёl's way of logic here. >
>>>>>>>
>>>>>>>>> + char *env_port_name;
>>>>>>>>> + struct landlock_net_service_attr net_service = {
>>>>>>>>> + .allowed_access = allowed_access,
>>>>>>>>> + .port = 0,
>>>>>>>>> + };
>>>>>>>>> +
>>>>>>>>> + env_port_name = getenv(env_var);
>>>>>>>>> + if (!env_port_name)
>>>>>>>>> + return 0;
>>>>>>>>> + env_port_name = strdup(env_port_name);
>>>>>>>>> + unsetenv(env_var);
>>>>>>>>> + num_ports = parse_port_num(env_port_name);
>>>>>>>>> +
>>>>>>>>> + if (num_ports == 1 && (strtok(env_port_name, ENV_PATH_TOKEN) == NULL)) {
>>>>>>>>> + ret = 0;
>>>>>>>>> + goto out_free_name;
>>>>>>>>> + }
>>>>>>>>
>>>>>>>> I don't understand why parse_port_num and strtok are necessary in this
>>>>>>>> program. The man-page for strsep(3) describes it as a replacement to
>>>>>>>> strtok(3) (in the HISTORY section), and it has a very short example
>>>>>>>> for how it is used.
>>>>>>>>
>>>>>>>> Wouldn't it work like this as well?
>>>>>>>>
>>>>>>>> while ((strport = strsep(&env_port_name, ":"))) {
>>>>>>>> net_service.port = atoi(strport);
>>>>>>>> /* etc */
>>>>>>>> }
>>>>>>>
>>>>>>> Thanks for a tip. I think it's a better solution here. Now this
>>>>>>> commit is in Mickaёl's -next branch. I could send a one-commit patch later.
>>>>>>> Mickaёl, what do you think?
>>>>>>
>>>>>> I removed this series from -next because there is some issues (see the
>>>>>> bot's emails), but anyway, this doesn't mean these patches don't need to
>>>>>> be changed, they do. The goal of -next is to test more widely a patch
>>>>>> series and get more feedbacks, especially from bots. When this series
>>>>>> will be fully ready (and fuzzed with syzkaller), I'll push it to Linus
>>>>>> Torvalds.
>>>>>>
>>>>>> I'll review the remaining tests and sample code this week, but you can
>>>>>> still take into account the documentation review.
>>>>>
>>>>> Hi, Mickaёl.
>>>>>
>>>>> I have a few quetions?
>>>>> - Are you going to fix warnings for bots, meanwhile I run syzcaller?
>>>>
>>>> No, you need to fix that with the next series (except the Signed-off-by
>>>> warnings).
>>>
>>> Hi, Mickaёl.
>>> As I understand its possible to check bots warnings just after you
>>> push the next V12 series again into your -next branch???
>>
>> Yes, we get bot warnings on the -next tree, but the command that
>> generate it should be reproducible.
>
> Stephen Rothwell sent a few warnings he got with powerpc
> pseries_le_defconfig. Do I need to fix it in V12 patch? How can I handle
> it cause no warnings in current .config?
Yes, this need to be fixed in the next series. Could you point to the
message?
I'm almost done with the test, I revamped code and I'll send that tomorrow.
>>
>>
>>>
>>>>
>>>> What is your status on syzkaller? Do you need some help? I can write the
>>>> tests if it's too much.
>>>>
>>> Sorry. To be honest I'm busy with another project. I dont know how
>>> much time it will take for me to set up and run syzkaller. I need your
>>> help here please, how you do this, some roadmap.
>>
>> Ok, no worries, I have it set up so I'll take care of it and keep you in
>> the loop with your GitHub account.
>>
> Thank you!!
>>
>>>>
>>>>> - I will fix documentation and sandbox demo and sent patch v12?
>>>>
>>>> Yes please. Let me a few days to send more reviews.
>>>>
>>> Ok. Sure.
>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> + for (i = 0; i < num_ports; i++) {
>>>>>>>>> + net_service.port = atoi(strsep(&env_port_name, ENV_PATH_TOKEN));
>>>>>>>>
>>>>>>>> Naming of ENV_PATH_TOKEN:
>>>>>>>> This usage is not related to paths, maybe rename the variable?
>>>>>>>> It's also technically not the token, but the delimiter.
>>>>>>>>
>>>>>>> What do you think of ENV_PORT_TOKEN or ENV_PORT_DELIMITER???
>>>>>>
>>>>>> You can rename ENV_PATH_TOKEN to ENV_DELIMITER for the FS and network parts.
>>>>>>
>>>>> Ok. Got it.
>>>>>>
>>>>>>>
>>>>>>>>> + if (landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE,
>>>>>>>>> + &net_service, 0)) {
>>>>>>>>> + fprintf(stderr,
>>>>>>>>> + "Failed to update the ruleset with port \"%lld\": %s\n",
>>>>>>>>> + net_service.port, strerror(errno));
>>>>>>>>> + goto out_free_name;
>>>>>>>>> + }
>>>>>>>>> + }
>>>>>>>>> + ret = 0;
>>>>>>>>> +
>>>>>>>>> +out_free_name:
>>>>>>>>> + free(env_port_name);
>>>>>>>>> + return ret;
>>>>>>>>> +}
>>>>>>>>
>>>>>>>>
>>>>>>>>> fprintf(stderr,
>>>>>>>>> "Launch a command in a restricted environment.\n\n");
>>>>>>>>> - fprintf(stderr, "Environment variables containing paths, "
>>>>>>>>> - "each separated by a colon:\n");
>>>>>>>>> + fprintf(stderr,
>>>>>>>>> + "Environment variables containing paths and ports "
>>>>>>>>> + "each separated by a colon:\n");
>>>>>>>>> fprintf(stderr,
>>>>>>>>> "* %s: list of paths allowed to be used in a read-only way.\n",
>>>>>>>>> ENV_FS_RO_NAME);
>>>>>>>>> fprintf(stderr,
>>>>>>>>> - "* %s: list of paths allowed to be used in a read-write way.\n",
>>>>>>>>> + "* %s: list of paths allowed to be used in a read-write way.\n\n",
>>>>>>>>> ENV_FS_RW_NAME);
>>>>>>>>> + fprintf(stderr,
>>>>>>>>> + "Environment variables containing ports are optional "
>>>>>>>>> + "and could be skipped.\n");
>>>>>>>>
>>>>>>>> As it is, I believe the program does something different when I'm
>>>>>>>> setting these to the empty string (ENV_TCP_BIND_NAME=""), compared to
>>>>>>>> when I'm unsetting them?
>>>>>>>>
>>>>>>>> I think the case where we want to forbid all handle-able networking is
>>>>>>>> a legit and very common use case - it could be clearer in the
>>>>>>>> documentation how this is done with the tool. (And maybe the interface
>>>>>>>> could be something more explicit than setting the environment variable
>>>>>>>> to empty?)
>>>>>>
>>>>>> I'd like to keep it simple, and it should be seen as an example code,
>>>>>> not a full-feature sandboxer, but still a consistent and useful one.
>>>>>> What would you suggest?
>>>>>>
>>>>>> This sandboxer tool relies on environment variables for its
>>>>>> configuration. This is definitely not a good fit for all use cases, but
>>>>>> I think it is simple and flexible enough. One use case might be to
>>>>>> export a set of environment variables and simply call this tool. I'd
>>>>>> prefer to not deal with argument parsing, but maybe that was too
>>>>>> simplistic? We might want to revisit this approach but probably not with
>>>>>> this series.
>>>>>>
>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> + /* Removes bind access attribute if not supported by a user. */
>>>>>>>>> + env_port_name = getenv(ENV_TCP_BIND_NAME);
>>>>>>>>> + if (!env_port_name) {
>>>>>>>>> + ruleset_attr.handled_access_net &=
>>>>>>>>> + ~LANDLOCK_ACCESS_NET_BIND_TCP;
>>>>>>>>> + }
>>>>>>>>> + /* Removes connect access attribute if not supported by a user. */
>>>>>>>>> + env_port_name = getenv(ENV_TCP_CONNECT_NAME);
>>>>>>>>> + if (!env_port_name) {
>>>>>>>>> + ruleset_attr.handled_access_net &=
>>>>>>>>> + ~LANDLOCK_ACCESS_NET_CONNECT_TCP;
>>>>>>>>> + }
>>>>>>>>
>>>>>>>> This is the code where the program does not restrict network usage,
>>>>>>>> if the corresponding environment variable is not set.
>>>>>>>
>>>>>>> Yep. Right.
>>>>>>>>
>>>>>>>> It's slightly inconsistent with what this tool does for filesystem
>>>>>>>> paths. - If you don't specify any file paths, it will still restrict
>>>>>>>> file operations there, independent of whether that env variable was
>>>>>>>> set or not. (Apologies if it was discussed before.)
>>>>>>>
>>>>>>> Mickaёl wanted to make network ports optional here.
>>>>>>> Please check:
>>>>>>>
>>>>>>> https://lore.kernel.org/linux-security-module/179ac2ee-37ff-92da-c381-c2c716725045@digikod.net/
>>>>>>
>>>>>> Right, the rationale is for compatibility with the previous version of
>>>>>> this tool. We should not break compatibility when possible. A comment
>>>>>> should explain the rationale though.
>>>>>>
>>>>>>>
>>>>>>> https://lore.kernel.org/linux-security-module/fe3bc928-14f8-5e2b-359e-9a87d6cf5b01@digikod.net/
>>>>>>>>
>>>>>>>> —Günther
>>>>>>>>
>>>>>> .
>>>> .
>> .
More information about the Linux-security-module-archive
mailing list