Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 12 Jul 2022 17:53:57 -0700
From:   Alexei Starovoitov <alexei.starovoitov@gmail.com>
To:     Roberto Sassu <roberto.sassu@huawei.com>
Cc:     ast@kernel.org, daniel@iogearbox.net, john.fastabend@gmail.com,
        andrii@kernel.org, martin.lau@linux.dev, song@kernel.org,
        yhs@fb.com, kpsingh@kernel.org, dhowells@redhat.com,
        jarkko@kernel.org, shuah@kernel.org, bpf@vger.kernel.org,
        keyrings@vger.kernel.org, linux-security-module@vger.kernel.org,
        linux-kselftest@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 0/7] bpf: Add bpf_verify_pkcs7_signature() helper
Message-ID: <20220713005357.z74s36yet3veysno@macbook-pro-3.dhcp.thefacebook.com>
References: <20220712184128.999301-1-roberto.sassu@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220712184128.999301-1-roberto.sassu@huawei.com>
Precedence: bulk

On Tue, Jul 12, 2022 at 08:41:21PM +0200, Roberto Sassu wrote:
> One of the desirable features in security is the ability to restrict import
> of data to a given system based on data authenticity. If data import can be
> restricted, it would be possible to enforce a system-wide policy based on
> the signing keys the system owner trusts.
> 
> This feature is widely used in the kernel. For example, if the restriction
> is enabled, kernel modules can be plugged in only if they are signed with a
> key whose public part is in the primary or secondary keyring.
> 
> For eBPF, it can be useful as well. For example, it might be useful to
> authenticate data an eBPF program makes security decisions on.
> 
> After a discussion in the eBPF mailing list, it was decided that the stated
> goal should be accomplished by introducing a new helper:
> bpf_verify_pkcs7_signature(), dedicated to verify PKCS#7 signatures.
> 
> Other than the data and the signature, the helper also receives two
> parameters for the keyring, which can be provided as alternatives: one is a
> key pointer returned by the new bpf_lookup_user_key() helper, called with a
> key serial possibly decided by the user; another is a pre-determined ID
> among values defined in include/linux/verification.h.
> 
> While the first keyring-related parameter provides great flexibility, it
> seems suboptimal in terms of security guarantees, as even if the eBPF
> program is assumed to be trusted, the serial used to obtain the key pointer
> might come from untrusted user space not choosing one that the system
> administrator approves to enforce a mandatory policy.
> 
> The second keyring-related parameter instead provides much stronger
> guarantees, especially if the pre-determined ID is not passed by user space
> but is hardcoded in the eBPF program, and that program is signed. In this
> case, bpf_verify_pkcs7_signature() will always perform signature
> verification with a key that the system administrator approves, i.e. the
> primary, secondary or platform keyring.
> 
> bpf_lookup_user_key() comes with the corresponding release helper
> bpf_key_put(), to decrement the reference count of the key found with the
> former helper. The eBPF verifier has been enhanced to ensure that the
> release helper is always called whenever the acquire helper is called, or
> otherwise refuses to load the program.
> 
> bpf_lookup_user_key() also accepts lookup-specific flags KEY_LOOKUP_CREATE
> and KEY_LOOKUP_PARTIAL. Although these are most likely not useful for the
> bpf_verify_pkcs7_signature(), newly defined flags could be.
> 
> bpf_lookup_user_key() does not request a particular permission to
> lookup_user_key(), as it cannot determine it by itself. Also, it should not
> get it from the user, as the user could pass an arbitrary value and use the
> key for a different purpose. Instead, bpf_lookup_user_key() requests
> KEY_DEFER_PERM_CHECK, and defers the permission check to the helper that
> actually uses the key, in this patch set to bpf_verify_pkcs7_signature().
> 
> Since key_task_permission() is called by the PKCS#7 code during signature
> verification, the only additional function bpf_verify_pkcs7_signature() has
> to call is key_validate(). With that, the permission check can be
> considered complete and equivalent, as it was done by bpf_lookup_user_key()
> with the appropriate permission (in this case KEY_NEED_SEARCH).
> 
> All helpers can be called only from sleepable programs, because of memory
> allocation (with lookup flag KEY_LOOKUP_CREATE) and crypto operations. For
> example, the lsm.s/bpf attach point is suitable,
> fexit/array_map_update_elem is not.
> 
> The correctness of implementation of the new helpers and of their usage is
> checked with the introduced tests.
> 
> The patch set is organized as follows.
> 
> Patch 1 exports bpf_dynptr_get_size(), to obtain the real size of data
> carried by a dynamic pointer. Patch 2 makes available for new eBPF helpers
> some key-related definitions. Patch 3 introduces the bpf_lookup_user_key()
> and bpf_key_put() helpers. Patch 4 introduces the
> bpf_verify_pkcs7_signature(). Finally, patches 5-7 introduce the tests.
> 
> Changelog
> 
> v6:
>  - Switch back to key lookup helpers + signature verification (until v5),
>    and defer permission check from bpf_lookup_user_key() to
>    bpf_verify_pkcs7_signature()
>  - Add additional key lookup test to illustrate the usage of the
>    KEY_LOOKUP_CREATE flag and validate the flags (suggested by Daniel)
>  - Make description of flags of bpf_lookup_user_key() more user-friendly
>    (suggested by Daniel)
>  - Fix validation of flags parameter in bpf_lookup_user_key() (reported by
>    Daniel)
>  - Rename bpf_verify_pkcs7_signature() keyring-related parameters to
>    user_keyring and system_keyring to make their purpose more clear
>  - Accept keyring-related parameters of bpf_verify_pkcs7_signature() as
>    alternatives (suggested by KP)
>  - Replace unsigned long type with u64 in helper declaration (suggested by
>    Daniel)
>  - Extend the bpf_verify_pkcs7_signature() test by calling the helper
>    without data, by ensuring that the helper enforces the keyring-related
>    parameters as alternatives, by ensuring that the helper rejects
>    inaccessible and expired keyrings, and by checking all system keyrings
>  - Move bpf_lookup_user_key() and bpf_key_put() usage tests to
>    ref_tracking.c (suggested by John)
>  - Call bpf_lookup_user_key() and bpf_key_put() only in sleepable programs

Judging by amount of back and forth in api design and still outstanding
questions whether it's something that will work long term we probably
should not be baking these helpers into uapi.
Let's extend verifier support for ARG_PTR_TO_DYNPTR in kfunc and make
them all as kfuncs.
This way we can change them later.