From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (gnu.wildebeest.org [45.83.234.184]) by sourceware.org (Postfix) with ESMTPS id 237B93858D35 for ; Tue, 31 Oct 2023 13:20:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 237B93858D35 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=klomp.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=klomp.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 237B93858D35 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=45.83.234.184 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698758412; cv=none; b=Kq+ntZhx2HuBqHPPBE4MBKW9ZHkszizXjSc2ZXsK5lM/OOzQrvzNVPPtxG/OIEMeWrMm8dj2/GQWAQIh8ZITyIuzkpk7vmupD0jjxxreAs7yLlzsnvpzGi68wJOhID55TjE5DtuY3ysiFufjkjLZTPPVoaaGKoBHIVGfjVXfFqU= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698758412; c=relaxed/simple; bh=CzH+ht8UtSq4/BzGkkiBnYOmgHlf9aooDVclGv2BKxU=; h=Message-ID:Subject:From:To:Date:MIME-Version; b=p1zTWd/jWJoa/pm3HOf0Cxm9c+5D1+HhRBSw7cFevsGBDCtJUmJIMfYIVect6yKu9sVJF9pzil5JiXXPneISha/QrgQR6uV8tx93qjqvt1tXvchpN5xh0/76pCSgrQcH/DNucl5rG4MW5msBVFVWJxNWIliqN/YaKY2Hnlsr3dI= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from r6.localdomain (82-217-174-174.cable.dynamic.v4.ziggo.nl [82.217.174.174]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id 75944302FDC1; Tue, 31 Oct 2023 14:20:08 +0100 (CET) Received: by r6.localdomain (Postfix, from userid 1000) id A32A2340328; Tue, 31 Oct 2023 14:20:07 +0100 (CET) Message-ID: <2148d29762c2046d5d7ce88df51ef91eb2113046.camel@klomp.org> Subject: Re: [PATCH] PR28204, debuginfod IMA From: Mark Wielaard To: "Frank Ch. Eigler" Cc: elfutils-devel@sourceware.org Date: Tue, 31 Oct 2023 14:20:07 +0100 In-Reply-To: <20231027191555.GD22548@redhat.com> References: <20231023193347.GB2863@gnu.wildebeest.org> <20231024132743.GC9683@redhat.com> <20231024210345.GE2863@gnu.wildebeest.org> <20231027191555.GD22548@redhat.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) MIME-Version: 1.0 X-Spam-Status: No, score=-3026.8 required=5.0 tests=BAYES_00,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,RCVD_IN_BARRACUDACENTRAL,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi Frank, On Fri, 2023-10-27 at 15:15 -0400, Frank Ch. Eigler wrote: > > > I would not expect the emailed patch to apply, esp. with all the othe= r > > > work done in the intermediate months, which is why the code is also i= n > > > the git branch. The binary files do not seem effectively reviewable > > > anyway. > >=20 > > It would be really convenient though. And modern git format-patch does > > includes base tree information which allows tools to stich commits at > > the right place.=20 >=20 > (I would be surprised if many-month-old patches could just be > automatically "stitched".) >=20 > > It would also help with patchwork and pre-commit CI. > > https://git-scm.com/docs/git-format-patch#_base_tree_information >=20 > Considering how easily the trybots can process the actual code - and > have done so before posting the patch for review - we can consider > some CI well done already. After approval but before merge, it would > undergo another round of trybotting. With such workflow, patchwork > does not need to concern itself with additional pre-commit CI/CD. My point is really that posting with git format-patch or send-email makes it possible for someone to simply use git am, b4 or git pw to try out a patch. If the patch doesn't apply then that will be the first review issue. > > > > [...] > > > > > The default is ima:permissive mode, which allows signatures t= o > > > > > function like a checksum to detect accidental corruption, but= accepts > > > > > operation in a mix of signed and unsigned packages & servers. > > > >=20 > > > > I still think "permissive" is confusing. Since it is a term also us= ed > > > > by e.g. selinux, but doesn't work that way. And it doesn't seem > > > > connected with the threat-model that enforcing protects against. > > >=20 > > > The connection is the following: > > > "enforcing" mode protects against accidental or deliberate (MITM) cor= ruption. > > > "permissive" mode protects against accidental corruption. > > >=20 > > > > Since it is a different concept maybe it shouldn't be part of this > > > > patch. It is a form of integrity checking, but doesn't protect (or > > > > warns) about integrity failures.=20 > > >=20 > > > It does protect and warn against integrity failures of the form of= =20 > > > incorrect signatures. >=20 > > My issue is that I don't really understand "permissive". Originally I > > assumed it was like selinux permissive mode, it does do the checks, > > but if they fail we just warn and continue. That seems a clear concept. >=20 > The proposed documentation explains it thusly: >=20 > ima:enforcing Every downloaded file requires a valid signature. >=20 > ima:permissive Every downloaded file accompanied by a signature > must be valid, but downloads without signatures are accepted. >=20 > ima:ignore Skips verification altogether. >=20 > You're right that it is not an exact match for the selinux concept. > But if one's not hunting around for a precise analogy, and just reads > the single sentence description, it tries to be clear. >=20 > > [...] if there is a signature, but we don't have the corresponding > > certificate to check it against, should it still fail, or is it > > more like a none-signed file and we can be "permissive" and accept > > it? Maybe I don't have enough imagination. >=20 > I see your point. One could make an argument either way, coming from > fuzziness with the concept of an "invalid signature". One could > clarify with a rewording to "known-invalid". Then "permissive" > becomes permit everything except known-invalid files. Missing > certificates would not qualify as known-invalid, merely unknown. > Would you like me to draft up a sentence or two description of that > concept for the man page? >=20 > The intended benefit of permissive mode as a default is to give > elfutils users as much reassurance possible, without requiring manual > configuration changes or manual downloads. See also the certificate > distribution topic below - it's really toward the same goal. I think my issue is really that it is unclear what "reassurances" we are making. What is the threat-model? Permissive says to me that although checks are done, they don't block receiving files. Maybe calling it ima:known-valid ? ima:checking (if you reject unknown- valid)? I don't have an issue with ima:enforcing, that seems to have clear semantics. The threat-model is clear, you only want to get files that are signed by a specific set of keys/certificates you trust. No middle- person acting as an intermediary between the distributor and user can circumvent that. > > [...] > > > Yes, it is odd. Unfortunately, it is not possible to enforce crypto > > > signatures from distros upon unsigned slices. A couple of possible > > > solutions: > > > [...] > > > - disable section queries from enforcing-mode servers (which could > > > then nuke gdbindex capability for e.g. future fedora/gdb users) > > [...] > >=20 > > I think only option 2 makes sense given the enforcing threat-model. > >=20 > > Optionally we could do the section part locally, download the whole > > file, check the ima signature, then provide the application with just > > the section data. >=20 > Yeah. That is what I was thinking, just not expressing properly. OK. But again in enforcing mode this is simple and clear, the semantics for "permissive" not so much imho. > > > > Including default system directories seems fine. But I don't think > > > > elfutils should ship certificates itself. That is the job of the > > > > distro or user. > > >=20 > > > The user or the distro the user is running may not be the same one > > > that the binaries the user is debugging comes from. By shipping > > > Fedora/RHEL/CentOS certificates, we allow a Ubuntu person to debug a > > > RHEL container, and trust debuginfod content for it. > >=20 > > But it should be the distro/user who makes that choice. We cannot > > decide for others who they trust as provider of the files they > > download. >=20 > They already make the decision whom they download debuginfo from. > That's literally what setting $DEBUGINFOD_URLS is. The scenario > you're describing would be trusting a server enough to supply content, > trusting our code to fetch & check that content, but not trusting us > to redistribute public certificates for the servers. The user shouldn't trust a middle-person. Unless we are signing those files we shouldn't distribute the certificates are being trustable imho. > > > > We aren't in a position to make sure the certificates are valid > > > > and/or can be trusted. > > >=20 > > > Why not? We can document where we got them - I believe they are all > > > public somewhere or other already. > >=20 > > We certainly should document that and provide pointers to where > > distros publish their certificates. But we shouldn't install them by > > default. The distro/user can make their own choice of using them, just > > like they decide whether or not the have default DEBUGINFOD_URLS. >=20 > An elfutils-carrying distro can already decide what to do with out > certificates by including or excluding them from their package. They > govern what's installed by default, not we. By including the certs in > elfutils, we are making it easy for a packager to pass these on, if > they wish. So you propose we setup a curating process to decide which certificates to include? If we do then I would suggest we create a separate package for that (or just a separate tar ball or repository). But I really don't think we are the right party to do that. > Another way of looking at it is to remind ourselves of the goal of > this permissive/cert-distribution default mentality: to provide > maximum possible assurance possible out of the box. See above, what is this "maximum possible assurance" that "permissive" provides? And how does that interact with the enforcing policy? > > > > [...] > > > > It would be good to add some comments for extract_skid, I am not su= re > > > > I understand how this works. > > >=20 > > > (Ditto.) > >=20 > > I do understand hex2dec, but I don't understand what extract_skid > > does. Maybe add an explanation what a certificates subject key id is > > and why we need it. >=20 > (I meant I'm not sure how this works either. :-) It's based on code > from imaevm.) That concerns me a little. > > [...] > > > Not sure, but this is how libimaevm.c similar code does it. I assume > > > the first byte of the signature is something else. > > > (https://git.code.sf.net/p/linux-ima/ima-evm-utils) > >=20 > > ewww. Does this pass ubsan (--enable-sanitize-undefined)? =20 >=20 > Haven't tried but it passes valgrind. valgrind doesn't check for undefined behavior or type alignment requirements. > > The issue is that this seems to access structure values at a > > possibly unaligned address. >=20 > Interesting. >=20 >=20 > > > > What does init_public_keys do? Is it thread-safe? > > >=20 > > > Good catch. It initialized a global inside libimaevm.c. It does not > > > appear thread-safe. Will wrap this in a pthread-once or somesuch. >=20 > > libimaevm.c seems not thread-safe in general. You might have to put > > a big lock around the whole signature extraction/checking block > > which uses those library functions. >=20 > OK, will take a look at that. What other global-state conflicting > things did you notice? A quick look at the code shows that various functions can read/write a static public_keys variable linked list, including (indirectly) ima_verify_signature. So that can cause data-races. One other issue I noticed is that it seems to be distributed under GPLv2-only. Which seems to mean that anything based on it should also be distributed under GPLv2-only, which would include libdebuginfod. Is there code we can rely on that is GPLv2+ and LGPLv3+ compatible? > > Another possible issue might be the initialization of openssl in the > > static constructure. How does that interact with how libcurl inits > > openssl? >=20 > openssl's initialization is fine & thread-safe in practice, despite > the documentation's warnings. OK. But even if it is thread-safe, you also need to make sure it inits the same. This for example worries me a little: https://github.com/curl/curl/pull/12153 Cheers, Mark