Re: [PATCH] PR28204, debuginfod IMA

["Frank Ch. Eigler" <fche@redhat.com>]

Hi -


Thanks for the review.

> [...]
> BTW. The diff doesn't show the newly added binary files. So the patch
> cannot be applied. Please use git send-email or git format-patch for
> that.

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.


> >     debuginfod: PR28204 - RPM IMA per-file signature verification
> >     
> >     Recent versions of Fedora/RHEL include per-file cryptographic
> >     signatures in RPMs, not just an overall RPM signature.  This work
> >     extends debuginfod client & server to extract, transfer, and verify
> >     those signatures.  These allow clients to assure users that the
> >     downloaded files have not been corrupted since their original
> >     packaging.  Downloads that fail the test are rejected.
> 
> It is not just corruption, since it is a cryptographic signature, it
> is also a proof that the files are what the distro actually packaged
> and distributes.

Yes, that seems another way of saying the same thing.


> [...]
> >     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.

> As pointed out in another bug (#30978) if you want to do checking
> for (accidental) corruption of files you can also use the
> .gnu_debuglink CRC.
>
> Or maybe add this is a followup to this patch, adding an ima:checking
> and crc:checking marker (or maybe a generic checking marker that might
> do both)?

A .gnu_debuglink checksum that is part of the executable can to some
extent protect the debuginfo that it is a checksum of, but cannot
protect the executable, or the source files.


> >     NB: debuginfod section queries are excluded from signature
> >     verification at this time, and function as though ima:ignore were in
> >     effect.
> 
> imho this is odd. You either enforce the data produced by the server
> is certified, or it isn't. I understand that doing that for the
> section data is difficult because you effectively have to download and
> check the whole file. But it feels wrong to claim to enforce the
> signatures and then not do it.

Yes, it is odd.  Unfortunately, it is not possible to enforce crypto
signatures from distros upon unsigned slices.  A couple of possible
solutions:

- accept it as is with documentation

- disable section queries from enforcing-mode servers (which could
  then nuke gdbindex capability for e.g. future fedora/gdb users)

- have debuginfod servers operate their own crypto signing engine,
  sign sections and everything, distribute keys somehow (DNS?
  distributed with elfutils?), let clients fetch & trust those
  certificates; however does note prove distro provenance 


> >     IMA signatures are verified against a set of signing certificates.
> >     These are normally published by distributions.  A selection of such
> >     certificates is included with the debuginfod client, but some system
> >     directories are also searched.  See $DEBUGINFOD_IMA_CERT_PATH.  These
> >     certificates are assumed trusted.
> 
> 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.

> 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.

> If we ship certificates we also need some mechanism to invalidate
> them if they get compromised.

We can "git rm" them from our repo, and the next elfutils update would
make them go away.

> [...]
> > +AM_CONDITIONAL([ENABLE_IMA_VERIFICATION],[test "$enable_ima_verification" = "xrpmimaevmcrypto"])
> 
> So currently it is enabled if you have the right libraries installed,
> otherwise it is disabled. It might be nice if it can be explicitly
> enabled/disabled. And make configure fail if --enable-ima-verification
> is given, but the libraries aren't there.

Can do.


> [...]
> > +typedef enum {ignore, permissive, enforcing, undefined} ima_policy_t;
> > +#ifdef ENABLE_IMA_VERIFICATION
> > +  #include <imaevm.h>
> > +  #include <openssl/pem.h>
> > +  #include <openssl/evp.h>
> > +  #include <openssl/x509v3.h>
> > +  #include <arpa/inet.h>
> 
> Why the arpa/init.h ?

Not sure, probably harmless.


> > +  static inline unsigned char hex2dec(char c)
> > +  {
> > +    if (c >= '0' && c <= '9') return (c - '0');
> > +    if (c >= 'a' && c <= 'f') return (c - 'a') + 10;
> > +    if (c >= 'A' && c <= 'F') return (c - 'A') + 10;
> > +    return 0;
> > +  }
> > +
> 
> OK. Since this doesn't signal error (except by returning 0, which is a
> valid value) it should probably be guarded by some kind of input check
> before being called.

Well, this need not signal an error at all.  If the hex encoding of
the signature is corrupt (and get 0s or whatever from this function),
then it will fail crypto verification, and that's all the error we
need.


> [...]
> It would be good to add some comments for extract_skid, I am not sure
> I understand how this works.

(Ditto.)

> Do we need name[PATH_MAX]? That could be really big.

Yeah, just simpler than doing asprintf etc. everywhere, and doing all
the C free cleanups, I guess.


> BTW. This and other code doesn't follow standard GNU style
> indentation and use lines > 76 chars. Not a fan.

Yes, I'd like to rerun a batch reformatter on the code at some point.


> > +static int
> > +debuginfod_validate_imasig (debuginfod_client *c, const char* tmp_path, int fd)
> > +{
> > +  (void) c;
> > +  (void) tmp_path;
> > +  (void) fd;
> > +  int rc = ENOSYS;
> 
> Why not define c, tmp_path and fd under ENABLE_IMA_VERIFICATION?

Not sure what you mean.  This is not a definition, it is a "use", to
eschew gcc warnings.


> [...]
> > +    // Extract the HEX IMA-signature from the header
> > +    char* sig_buf = NULL;
> > +    char* hdr_ima_sig = strcasestr(c->winning_headers, "x-debuginfod-imasignature");
> > +    if (!hdr_ima_sig || 1 != sscanf(hdr_ima_sig + strlen("x-debuginfod-imasignature:"), "%ms", &sig_buf))
> > +    {
> > +      rc = -ENODATA;
> > +      goto exit_validate;
> > +    }
> 
> Should that be strcasestr(c->winning_headers, "x-debuginfod-imasignature:"); ?
> Including the ':'. If only for consistency?

Yeah.

> Can sig_buf contain whitespace around the (hex)string?

Not sure whether this would come up with web servers.  If it were to
become a problem, one could sscanf(" %ms") instead, where that leading
space accepts and skips leading whitespace, if any.


> > +    // Convert the hex signature to bin
> > +    size_t bin_sig_len = strlen(sig_buf)/2;
> > +    unsigned char bin_sig[MAX_SIGNATURE_SIZE/2];
> > +    for (size_t b = 0; b < bin_sig_len; b++)
> > +      bin_sig[b] = (hex2dec(sig_buf[2*b]) << 4) | hex2dec(sig_buf[2*b+1]);
> 
> What happens if strlen(sig_buf) is odd? shouldn't you check the
> characters are actually hex?

Not necessary, as above.


> > +    // Compute the binary digest of the cached file (with file descriptor fd)
> > +    ctx = EVP_MD_CTX_new();
> > +    int hash_algo = imaevm_hash_algo_from_sig(bin_sig + 1);
> 
> Why bin_sig + 1?

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)


> > +    const EVP_MD *md = EVP_get_digestbyname(imaevm_hash_algo_by_id(hash_algo));
> > +    if (!ctx || !md || !EVP_DigestInit(ctx, md))
> > +    {
> > +      rc = -EBADMSG;
> > +      goto exit_validate;
> > +    }
> 
> I guess technically ctx being NULL means ENOMEM?

Yeah or some other error.


> > +    long data_len;
> > +    char* hdr_data_len = strcasestr(c->winning_headers, "x-debuginfod-size");
> > +    if (!hdr_data_len || 1 != sscanf(hdr_data_len + strlen("x-debuginfod-size:") , "%ld", &data_len))

> Same as above, include the ':' in the strcasestr?

Sure.


> > +      if (-1 == (n = pread(fd, file_data, DATA_SIZE, k)))

> Nice, but where is DATA_SIZE defined?

/usr/include/imaevm.h:#define  DATA_SIZE        4096


> > +    /* Iterate over the directories in DEBUGINFOD_IMA_CERT_PATH looking
> > +     * for the first verification certificate which matches keyid
> > +     */
> > +    uint32_t keyid = ntohl(((struct signature_v2_hdr *)(bin_sig + 1))->keyid); // The signature's keyid
> 
> Eep. Could this have a comment and/or pointer to docs? +1?
> cast to struct? ntohl?

Yeah.  ntohl endianness conversion is not unusual in binary transport
protocols.


> [...]
> > +    for(cert_dir_path = strtok(cert_paths, ":"); cert_dir_path != NULL; cert_dir_path = strtok(NULL, ":"))
> 
> strtok isn't thread-safe, I think you need to use strtok_r. Otherwise
> strtok(NULL, ":") has no context.

Right.


> > +        if(entry->d_type != DT_REG || 0 != fnmatch("*.@(der|pem|crt|cer)", entry->d_name, FNM_EXTMATCH)) continue;
> 
> OK. Note that some alternative libc implementations don't support
> FNM_EXTMATCH (but we already use it in other places).

(Or, I suppose it could be written as a bunch of individual fnmatches.)


> [...]
> > +          char tmp_file[FILENAME_MAX] = "debuginfod_tempcert_XXXXXX";
> 
> Why is tmp_file FILENAME_MAX big? Can it just be strlen (debuginfod_tempcert_XXXXXX) + 1?

Yeah, that does not need to be so big.  Too bad I can't find a libc
function to prefix it with $TMPDIR automagically.  tmpfile(3) is not
good enough because we need its name here.

> > +          init_public_keys(tmp_file);
> 
> 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.


> [...]
> > +    else
> > +    {
> > +      // By default we are permissive, so since the signature isn't invalid we
> > +      // give it the benefit of the doubt
> > +      if (vfd >= 0) dprintf (vfd, "warning: invalid or missing signature (%d)\n", result);
> > +    }
> > +  }
> 
> The warning message seems wrong. If I follow the code logic this
> signature cannot be invalid in the last else block.

Right, not "invalid", just missing.


- FChE