Re: [PATCH] PR28204, debuginfod IMA

[Mark Wielaard <mark@klomp.org>]

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 other
> > > work done in the intermediate months, which is why the code is also in
> > > the git branch.  The binary files do not seem effectively reviewable
> > > anyway.
> > 
> > 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. 
> 
> (I would be surprised if many-month-old patches could just be
> automatically "stitched".)
> 
> > It would also help with patchwork and pre-commit CI.
> > https://git-scm.com/docs/git-format-patch#_base_tree_information
> 
> 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 to
> > > > >     function like a checksum to detect accidental corruption, but accepts
> > > > >     operation in a mix of signed and unsigned packages & servers.
> > > > 
> > > > I still think "permissive" is confusing. Since it is a term also used
> > > > by e.g. selinux, but doesn't work that way. And it doesn't seem
> > > > connected with the threat-model that enforcing protects against.
> > > 
> > > The connection is the following:
> > > "enforcing" mode protects against accidental or deliberate (MITM) corruption.
> > > "permissive" mode protects against accidental corruption.
> > > 
> > > > 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. 
> > > 
> > > It does protect and warn against integrity failures of the form of 
> > > incorrect signatures.
> 
> > 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.
> 
> The proposed documentation explains it thusly:
> 
>   ima:enforcing Every downloaded file requires a valid signature.
> 
>   ima:permissive  Every downloaded file accompanied by a signature
>   must be valid, but downloads without signatures are accepted.
> 
>   ima:ignore Skips verification altogether.
> 
> 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.
> 
> > [...]  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.
> 
> 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?
> 
> 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)
> > [...]
> > 
> > I think only option 2 makes sense given the enforcing threat-model.
> > 
> > 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.
> 
> 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.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.
> > > 
> > > Why not?  We can document where we got them - I believe they are all
> > > public somewhere or other already.
> > 
> > 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.
> 
> 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 sure
> > > > I understand how this works.
> > > 
> > > (Ditto.)
> > 
> > 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.
> 
> (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)
> > 
> > ewww. Does this pass ubsan (--enable-sanitize-undefined)?  
> 
> 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.
> 
> Interesting.
> 
> 
> > > > What does init_public_keys do? Is it thread-safe?
> > > 
> > > Good catch.  It initialized a global inside libimaevm.c.  It does not
> > > appear thread-safe.  Will wrap this in a pthread-once or somesuch.
> 
> > 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.
> 
> 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?
> 
> 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