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 1F10A3858D37 for ; Mon, 23 Oct 2023 19:33:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1F10A3858D37 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 1F10A3858D37 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=1698089633; cv=none; b=Y4BAGk6fh3ZODHrPsJEaOE0oijdQHRKh5wOJWTTsKyULz66KECll9Bl5hMAkfXdIwp6SySW9nOaKxAYVRgwDr9tBtikOyJyZF/apz7d33lyZot5hw54ZhYId1L9EgJSZ+wCmUK2EeURaSluKoGD3S3T6J4RI75VRdi8oJu+2uKQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698089633; c=relaxed/simple; bh=aKHcU6TWzSyLL/+s9nA7gPiJ42I7zgrWk1uJJ+cdqTM=; h=Date:From:To:Subject:Message-ID:MIME-Version; b=adJciOmfEYyq4Ch/xeTQ9HSn/qjQ/bCvN4b48yZpyC1Y1WUyZA/3T1jn6mL+y2Q2kSQZt/Kjf3A01uFDISVbASWqKPfVpCckSBPTJIgIlEroraJGvsyWpVE0qE9T3KbGClkezEYSxTkwb73AbnjuQuFacXbVeBVWsT45ep0Mi7Y= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by gnu.wildebeest.org (Postfix, from userid 1000) id 57A7A302FDC0; Mon, 23 Oct 2023 21:33:47 +0200 (CEST) Date: Mon, 23 Oct 2023 21:33:47 +0200 From: Mark Wielaard To: "Frank Ch. Eigler" Cc: elfutils-devel@sourceware.org Subject: Re: [PATCH] PR28204, debuginfod IMA Message-ID: <20231023193347.GB2863@gnu.wildebeest.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-Spam-Status: No, score=-3034.4 required=5.0 tests=BAYES_00,GIT_PATCH_0,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham 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 Thu, Sep 07, 2023 at 08:55:10AM -0400, Frank Ch. Eigler via Elfutils-devel wrote: > Here's a squashed/rebased version of the big IMA patch. I also > tweaked a few documentation oriented bits, and removed the > "ima:default" tag. Thanks. Sorry the reviews take so long. But it is a big concept and new code. Getting a good feeling for the concept and the code is hard. 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. > commit 4e45a08aee42958298a3fad6043cbf96243d13a5 (HEAD -> users/fche/try-bz28204, origin/users/fche/try-bz28204) > Author: Ryan Goldberg > Date: Mon Aug 14 13:51:00 2023 -0400 > > 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. > Clients may select a desired level of enforcement for sets of URLs in > the DEBUGINFOD_URLS by inserting special markers ahead of them: > > ima:ignore pay no attention to absence or presence of signatures > ima:permissive require signatures to be correct, but accept absent signatures > ima:enforcing require every file to be correctly signed > > 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. 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. 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)? > 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. > 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. We aren't in a position to make sure the certificates are valid and/or can be trusted. If we ship certificates we also need some mechanism to invalidate them if they get compromised. > > Signed-off-by: Ryan Goldberg > Signed-off-by: Frank Ch. Eigler > > diff --git a/ChangeLog b/ChangeLog > index 6aed95b6974e..b3b1a8ebc93a 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,3 +1,9 @@ > +2023-08-14 Ryan Goldberg > + > + * configure.ac (ENABLE_IMA_VERIFICATION): Look for librpm, libimaevm and libcrypto > + * (DEBUGINFOD_IMA_CERT_PATH): Default path for ima certificate containing > + dirs. See also profile.*.in. > + > 2023-03-27 Di Chen > > * NEWS: Support readelf -Ds for using dynamic segment to Please feel free to move ChangeLog entries into the commit message. That might make rebases simpler. > diff --git a/config/ChangeLog b/config/ChangeLog > index ce1f74f621aa..30fc3deea09e 100644 > --- a/config/ChangeLog > +++ b/config/ChangeLog > @@ -1,3 +1,10 @@ > +2023-08-14 Ryan Goldberg > + > + * profile.csh.in: Set DEBUGINFOD_IMA_CERT_PATH directly. > + * profile.sh.in: Set DEBUGINFOD_IMA_CERT_PATH directly. > + * elfutils.spec.in: Add BuildRequires rpm-devel, > + ima-evm-utils-devel, openssl-devel, rpm-sign. > + > 2023-02-21 Mark Wielaard > > * eu.am (USE_AFTER_FREE3_WARNING): Define. > diff --git a/config/elfutils.spec.in b/config/elfutils.spec.in > index 9277c08f7c82..2e962bb40d07 100644 > --- a/config/elfutils.spec.in > +++ b/config/elfutils.spec.in > @@ -43,6 +43,12 @@ BuildRequires: curl > # For run-debuginfod-response-headers.sh test case > BuildRequires: socat > > +# For debuginfod rpm IMA verification > +BuildRequires: rpm-devel > +BuildRequires: ima-evm-utils-devel > +BuildRequires: openssl-devel > +BuildRequires: rpm-sign > + > %define _gnu %{nil} > %define _programprefix eu- OK. > diff --git a/config/profile.csh.in b/config/profile.csh.in > index d962d969c05b..2a2ecacb3c80 100644 > --- a/config/profile.csh.in > +++ b/config/profile.csh.in > @@ -4,13 +4,17 @@ > # See also [man debuginfod-client-config] for other environment variables > # such as $DEBUGINFOD_MAXSIZE, $DEBUGINFOD_MAXTIME, $DEBUGINFOD_PROGRESS. > > +set prefix="@prefix@" > if (! $?DEBUGINFOD_URLS) then > - set prefix="@prefix@" > set DEBUGINFOD_URLS=`sh -c 'cat /dev/null "$0"/*.urls 2>/dev/null; :' "@sysconfdir@/debuginfod" | tr '\n' ' '` > if ( "$DEBUGINFOD_URLS" != "" ) then > setenv DEBUGINFOD_URLS "$DEBUGINFOD_URLS" > + if (! $?DEBUGINFOD_IMA_CERT_PATH) then > + set DEBUGINFOD_IMA_CERT_PATH="@sysconfdir@/debuginfod/ima-certs/:@DEBUGINFOD_IMA_CERT_PATH@" > + setenv DEBUGINFOD_IMA_CERT_PATH "$DEBUGINFOD_IMA_CERT_PATH" > + endif > else > unset DEBUGINFOD_URLS > endif > - unset prefix > endif > +unset prefix OK. > diff --git a/config/profile.sh.in b/config/profile.sh.in > index 3f4397dcb44d..adc06a7ed939 100644 > --- a/config/profile.sh.in > +++ b/config/profile.sh.in > @@ -4,9 +4,14 @@ > # See also [man debuginfod-client-config] for other environment variables > # such as $DEBUGINFOD_MAXSIZE, $DEBUGINFOD_MAXTIME, $DEBUGINFOD_PROGRESS. > > +prefix="@prefix@" > if [ -z "$DEBUGINFOD_URLS" ]; then > - prefix="@prefix@" > DEBUGINFOD_URLS=$(cat /dev/null "@sysconfdir@/debuginfod"/*.urls 2>/dev/null | tr '\n' ' ') > [ -n "$DEBUGINFOD_URLS" ] && export DEBUGINFOD_URLS || unset DEBUGINFOD_URLS > - unset prefix > + > + if [ -z "$DEBUGINFOD_IMA_CERT_PATH" ]; then > + DEBUGINFOD_IMA_CERT_PATH="@sysconfdir@/debuginfod/ima-certs/:@DEBUGINFOD_IMA_CERT_PATH@" > + export DEBUGINFOD_IMA_CERT_PATH > + fi > fi > +unset prefix > \ No newline at end of file OK. Although I like having a newline at the end of the file. > diff --git a/configure.ac b/configure.ac > index 4b67c84425fa..bedd99e3b8a4 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -665,6 +665,35 @@ case "$ac_cv_search__obstack_free" in > esac > AC_SUBST([obstack_LIBS]) > > +enable_ima_verification="x" > +AC_CHECK_LIB(rpm, headerGet, [ > + AC_CHECK_DECL(RPMSIGTAG_FILESIGNATURES, > + [ > + enable_ima_verification=$enable_ima_verification"rpm" > + AC_SUBST(rpm_LIBS, '-lrpm -lrpmio') > + ], > + [], [#include ]) > +]) > + > +AC_CHECK_LIB(imaevm, imaevm_hash_algo_from_sig, [ > + enable_ima_verification=$enable_ima_verification"imaevm" > + AC_SUBST(imaevm_LIBS, '-limaevm') > +]) > + > +AC_CHECK_LIB(crypto, EVP_MD_CTX_new, [ > + enable_ima_verification=$enable_ima_verification"crypto" > + AC_SUBST(crypto_LIBS, '-lcrypto') > +]) > + > +debuginfod_ima_verification_enabled="no" > +if test "$enable_ima_verification" = "xrpmimaevmcrypto"; then > + debuginfod_ima_verification_enabled="yes" > + default_ima_cert_path=`eval echo "/etc/keys/ima:/etc/pki/rpm-ima:$sysconfdir/debuginfod/ima-certs"` # expand $prefix too > + AC_DEFINE([ENABLE_IMA_VERIFICATION], [1], [Define if the required ima verification libraries are available]) > + AC_DEFINE_UNQUOTED(DEBUGINFOD_IMA_CERT_PATH_DEFAULT, "$default_ima_cert_path", [Default IMA certificate path]) > +fi > +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. > dnl The directories with content. > > dnl Documentation. > @@ -916,6 +945,7 @@ AC_MSG_NOTICE([ > libdebuginfod client support : ${enable_libdebuginfod} > Debuginfod server support : ${enable_debuginfod} > Default DEBUGINFOD_URLS : ${default_debuginfod_urls} > + Debuginfod RPM sig checking : ${debuginfod_ima_verification_enabled} Should probably also print the default_ima_cert_path? > EXTRA TEST FEATURES (used with make check) > have bunzip2 installed (required) : ${HAVE_BUNZIP2} > diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog > index 0e4810bba501..f4d98c2e93bc 100644 > --- a/debuginfod/ChangeLog > +++ b/debuginfod/ChangeLog > @@ -1,3 +1,17 @@ > +2023-08-14 Ryan Goldberg > + > + * debuginfod.cxx (handle_buildid_r_match): Added extraction of the > + per-file IMA signature for the queried file and store in http header. > + * (find_globbed_koji_filepath): New function. > + * (parse_opt): New flag --koji-sigcache. > + * debuginfod-client.c (debuginfod_query_server): Added policy for > + validating IMA signatures > + * (debuginfod_validate_imasig): New function. > + * debuginfod.h.in: Added DEBUGINFOD_IMA_CERT_PATH_ENV_VAR. > + * Makefile.am: Add linker flags for rpm and imaevm and crypto. Also add install/uninstall > + ima-certs/ to known location. > + * ima-certs/: New directory containing known ima verification certificates. > + > 2023-04-21 Frank Ch. Eigler > > * debuginfod.cxx (groom): Fix -r / -X logic. > diff --git a/debuginfod/Makefile.am b/debuginfod/Makefile.am > index 125be97bbfcc..fb9f1fbc9b0c 100644 > --- a/debuginfod/Makefile.am > +++ b/debuginfod/Makefile.am > @@ -70,7 +70,7 @@ bin_PROGRAMS += debuginfod-find > endif > > debuginfod_SOURCES = debuginfod.cxx > -debuginfod_LDADD = $(libdw) $(libelf) $(libeu) $(libdebuginfod) $(argp_LDADD) $(fts_LIBS) $(libmicrohttpd_LIBS) $(sqlite3_LIBS) $(libarchive_LIBS) -lpthread -ldl > +debuginfod_LDADD = $(libdw) $(libelf) $(libeu) $(libdebuginfod) $(argp_LDADD) $(fts_LIBS) $(libmicrohttpd_LIBS) $(sqlite3_LIBS) $(libarchive_LIBS) $(rpm_LIBS) -lpthread -ldl > > debuginfod_find_SOURCES = debuginfod-find.c > debuginfod_find_LDADD = $(libdw) $(libelf) $(libeu) $(libdebuginfod) $(argp_LDADD) $(fts_LIBS) > @@ -97,7 +97,7 @@ libdebuginfod_so_LIBS = libdebuginfod_pic.a > if DUMMY_LIBDEBUGINFOD > libdebuginfod_so_LDLIBS = > else > -libdebuginfod_so_LDLIBS = -lpthread $(libcurl_LIBS) $(fts_LIBS) $(libelf) > +libdebuginfod_so_LDLIBS = -lpthread $(libcurl_LIBS) $(fts_LIBS) $(libelf) $(imaevm_LIBS) $(crypto_LIBS) > endif > $(LIBDEBUGINFOD_SONAME): $(srcdir)/libdebuginfod.map $(libdebuginfod_so_LIBS) > $(AM_V_CCLD)$(LINK) $(dso_LDFLAGS) -o $@ \ > @@ -117,15 +117,16 @@ install: install-am libdebuginfod.so > $(DESTDIR)$(libdir)/libdebuginfod-$(PACKAGE_VERSION).so > ln -fs libdebuginfod-$(PACKAGE_VERSION).so $(DESTDIR)$(libdir)/$(LIBDEBUGINFOD_SONAME) > ln -fs libdebuginfod-$(PACKAGE_VERSION).so $(DESTDIR)$(libdir)/libdebuginfod.so > - > + cp -R $(top_srcdir)/debuginfod/ima-certs $(DESTDIR)$(sysconfdir)/debuginfod/ > uninstall: uninstall-am > rm -f $(DESTDIR)$(libdir)/libdebuginfod-$(PACKAGE_VERSION).so > rm -f $(DESTDIR)$(libdir)/$(LIBDEBUGINFOD_SONAME) > rm -f $(DESTDIR)$(libdir)/libdebuginfod.so > rmdir --ignore-fail-on-non-empty $(DESTDIR)$(includedir)/elfutils > + rm -rf $(DESTDIR)$(sysconfdir)/debuginfod/ima-certs > endif > > -EXTRA_DIST = libdebuginfod.map > +EXTRA_DIST = libdebuginfod.map ima-certs > MOSTLYCLEANFILES = $(am_libdebuginfod_pic_a_OBJECTS) $(LIBDEBUGINFOD_SONAME) > CLEANFILES += $(am_libdebuginfod_pic_a_OBJECTS) libdebuginfod.so I am not sure adding/installing that ima-certs directory is a good idea, see above. > diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c > index 6882cb190d3c..7163c8872dfe 100644 > --- a/debuginfod/debuginfod-client.c > +++ b/debuginfod/debuginfod-client.c > @@ -92,6 +92,7 @@ void debuginfod_end (debuginfod_client *c) { } > #include > #include > #include > +#include > > /* If fts.h is included before config.h, its indirect inclusions may not > give us the right LFS aliases of these functions, so map them manually. */ > @@ -114,6 +115,69 @@ void debuginfod_end (debuginfod_client *c) { } > > #include > > +typedef enum {ignore, permissive, enforcing, undefined} ima_policy_t; > +#ifdef ENABLE_IMA_VERIFICATION > + #include > + #include > + #include > + #include > + #include Why the arpa/init.h ? > + 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. > + static inline ima_policy_t ima_policy_str2enum(const char* ima_pol) > + { > + if (NULL == ima_pol) return undefined; > + if (0 == strcmp(ima_pol, "ignore")) return ignore; > + if (0 == strcmp(ima_pol, "permissive")) return permissive; > + if (0 == strcmp(ima_pol, "enforcing")) return enforcing; > + return undefined; > + } > + > + static inline const char* ima_policy_enum2str(ima_policy_t ima_pol) > + { > + switch (ima_pol) > + { > + case ignore: > + return "ignore"; > + case permissive: > + return "permissive"; > + case enforcing: > + return "enforcing"; > + case undefined: > + return ""; > + } > + return ""; > + } OK. Not sure it is needed to mark these inline. The compiler can probably figure that out. > + static uint32_t extract_skid(X509* x509) > + { > + if (!x509) return 0; > + uint32_t keyid = 0; > + // Attempt to get the skid from the certificate > + const ASN1_OCTET_STRING *skid_asn1_str = X509_get0_subject_key_id(x509); > + if (skid_asn1_str) > + { > + int skid_len = ASN1_STRING_length(skid_asn1_str); > + memcpy(&keyid, ASN1_STRING_get0_data(skid_asn1_str) + skid_len - sizeof(keyid), sizeof(keyid)); > + } > + else > + { > + // If it is not there fallback to trying to extract it from the > + // public key itself > + EVP_PKEY * pkey = X509_get0_pubkey(x509); > + char name[PATH_MAX]; > + calc_keyid_v2(&keyid, name, pkey); > + } > + return ntohl(keyid); > + } > +#endif > + > static pthread_once_t init_control = PTHREAD_ONCE_INIT; It would be good to add some comments for extract_skid, I am not sure I understand how this works. Do we need name[PATH_MAX]? That could be really big. BTW. This and other code doesn't follow standard GNU style indentation and use lines > 76 chars. Not a fan. > static void > @@ -851,6 +915,174 @@ cache_find_section (const char *scn_name, const char *target_cache_dir, > return rc; > } > > +/* Validate an IMA file signature. > + * returns 0 on signature validity, EINVAL on signature invalidity, ENOSYS on undefined imaevm machinery, > + * ENOKEY on key issues and -errno on error > + */ > +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? > + > + #ifdef ENABLE_IMA_VERIFICATION > + char *cert_paths = NULL; // need to copy because of strtok > + int vfd = c->verbose_fd; > + EVP_MD_CTX *ctx = NULL; > + if (!c || !c->winning_headers) > + { > + rc = -ENODATA; > + goto exit_validate; > + } > + // 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? Can sig_buf contain whitespace around the (hex)string? > + if (strlen(sig_buf) > MAX_SIGNATURE_SIZE) // reject if too long > + { > + rc = -EBADMSG; > + goto exit_validate; > + } > + // 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? > + // 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? > + 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? > + 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)) > + { > + rc = -ENODATA; > + goto exit_validate; > + } Same as above, include the ':' in the strcasestr? > + char file_data[DATA_SIZE]; > + ssize_t n; > + for(long k = 0; k < data_len; k += n) > + { > + if (-1 == (n = pread(fd, file_data, DATA_SIZE, k))) > + { > + rc = -errno; > + goto exit_validate; > + } > + > + if (!EVP_DigestUpdate(ctx, file_data, n)) > + { > + rc = -EBADMSG; > + goto exit_validate; > + } > + } Nice, but where is DATA_SIZE defined? > + uint8_t bin_dig[MAX_DIGEST_SIZE]; > + unsigned int bin_dig_len; > + if (!EVP_DigestFinal(ctx, bin_dig, &bin_dig_len)) > + { > + rc = -EBADMSG; > + goto exit_validate; > + } > + > + /* 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? > + imaevm_params.verbose = 0; > + cert_paths = strdup (getenv(DEBUGINFOD_IMA_CERT_PATH_ENV_VAR) ?: strdup(DEBUGINFOD_IMA_CERT_PATH_DEFAULT)); > + rc = ENOKEY; // This is updated iff a good cert is found > + if (!cert_paths) > + goto exit_validate; > + > + char* cert_dir_path; > + DIR *dp; > + struct dirent *entry; > + 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. > + { > + dp = opendir(cert_dir_path); > + if(!dp) continue; OK, directory cannot be opened, skip. > + while((entry = readdir(dp))) > + { > + // Only consider regular files with common x509 cert extensions > + 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). > + char certfile[PATH_MAX]; > + strncpy(certfile, cert_dir_path, PATH_MAX - 1); > + if(certfile[strlen(certfile)-1] != '/') certfile[strlen(certfile)] = '/'; > + strncat(certfile, entry->d_name, PATH_MAX - strlen(certfile) - 1); > + certfile[strlen(certfile)] = '\0'; Not a fan of using PATH_MAX arrays since they can be large. But this looks OK. > + FILE *cert_fp = fopen(certfile, "r"); > + if(!cert_fp) continue; > + X509 *x509 = NULL; > + bool cert_found = false; > + // Attempt to read the fp as DER and assuming the key matches that of the signature add this key to be used > + if(d2i_X509_fp(cert_fp, &x509) && keyid == extract_skid(x509)) > + { > + init_public_keys(certfile); > + if (vfd >= 0) dprintf(vfd, "Loaded certificate %s (keyid = %04x, encoding = der)\n", certfile, keyid); > + cert_found = true; > + } > + // Attempt to read the fp as PEM and assuming the key matches that of the signature add this key to be used > + // Note we fseek since this is the second time we read from the fp > + else if(0 == fseek(cert_fp, 0, SEEK_SET) && PEM_read_X509(cert_fp, &x509, NULL, NULL) && \ > + keyid == extract_skid(x509)) > + { > + /* init_public_keys requires the path to a DER format x509 cert, so when we encounter a PEM > + * format cert we first need to encode x509 to DER and write it to a new temp file. > + * We can then pass this to init_public_keys and remove it afterwards > + */ > + char tmp_file[FILENAME_MAX] = "debuginfod_tempcert_XXXXXX"; Why is tmp_file FILENAME_MAX big? Can it just be strlen (debuginfod_tempcert_XXXXXX) + 1? > + int tmp_fd = mkstemp(tmp_file); > + if(-1 == tmp_fd) break; > + FILE* der_cert_fp = fopen(tmp_file, "w"); > + i2d_X509_fp(der_cert_fp, x509); > + fclose(der_cert_fp); > + init_public_keys(tmp_file); > + if (vfd >= 0) dprintf(vfd, "Loaded certificate %s (keyid = %04x, encoding = pem)\n", certfile, keyid); > + unlink(tmp_file); > + close(tmp_fd); > + cert_found = true; > + } What does init_public_keys do? Is it thread-safe? > + fclose(cert_fp); > + if(x509) X509_free(x509); > + > + if(cert_found) > + { > + closedir(dp); > + int res = ima_verify_signature(tmp_path, bin_sig, bin_sig_len, bin_dig, bin_dig_len); > + if (c->verbose_fd >= 0) > + dprintf (c->verbose_fd, "Computed ima signature verification res=%d\n", res); > + > + rc = (res == 1) ? EINVAL : res; // We update rc such that res = 1 is mapped to EINVAL, res = 0 is considered valid, res = -1 is an error > + goto exit_validate; > + } > + } > + closedir(dp); > + } > + > + exit_validate: > + free (sig_buf); > + free (cert_paths); > + EVP_MD_CTX_free(ctx); > + #endif > + return rc; > +} OK. > /* Query each of the server URLs found in $DEBUGINFOD_URLS for the file > with the specified build-id and type (debuginfo, executable, source or > section). If type is source, then type_arg should be a filename. If > @@ -1206,12 +1438,29 @@ debuginfod_query_server (debuginfod_client *c, > /* Initialize the memory to zero */ > char *strtok_saveptr; > char **server_url_list = NULL; > - char *server_url = strtok_r(server_urls, url_delim, &strtok_saveptr); > + ima_policy_t* url_ima_policies = NULL; > + char* server_url; > /* Count number of URLs. */ > int num_urls = 0; > > - while (server_url != NULL) > + ima_policy_t verification_mode = permissive; // The default mode > + for(server_url = strtok_r(server_urls, url_delim, &strtok_saveptr); > + server_url != NULL; server_url = strtok_r(NULL, url_delim, &strtok_saveptr)) OK > { > + // When we encounted a (well-formed) token off the form ima:foo, we update the policy > + // under which results from that server will be ima verified > + if(startswith(server_url, "ima:")) > + { > +#ifdef ENABLE_IMA_VERIFICATION > + ima_policy_t m = ima_policy_str2enum(server_url + strlen("ima:")); > + if(m != undefined) verification_mode = m; > +#else > + if (vfd >= 0) > + dprintf(vfd, "IMA signature verification is not enabled, skipping %s\n", server_url); > +#endif > + continue; // Not a url, just a mode change so keep going > + } > + > /* PR 27983: If the url is already set to be used use, skip it */ > char *slashbuildid; > if (strlen(server_url) > 1 && server_url[strlen(server_url)-1] == '/') > @@ -1243,19 +1492,19 @@ debuginfod_query_server (debuginfod_client *c, > else > { > num_urls++; > - char ** realloc_ptr; > - realloc_ptr = reallocarray(server_url_list, num_urls, > - sizeof(char*)); > - if (realloc_ptr == NULL) > + if (NULL == (server_url_list = reallocarray(server_url_list, num_urls, sizeof(char*))) > +#ifdef ENABLE_IMA_VERIFICATION > + || NULL == (url_ima_policies = reallocarray(url_ima_policies, num_urls, sizeof(ima_policy_t))) > +#endif > + ) > { > free (tmp_url); > rc = -ENOMEM; > goto out1; > } > - server_url_list = realloc_ptr; > server_url_list[num_urls-1] = tmp_url; > + if(NULL != url_ima_policies) url_ima_policies[num_urls-1] = verification_mode; > } > - server_url = strtok_r(NULL, url_delim, &strtok_saveptr); > } > > int retry_limit = default_retry_limit; OK > @@ -1324,7 +1573,11 @@ debuginfod_query_server (debuginfod_client *c, > if ((server_url = server_url_list[i]) == NULL) > break; > if (vfd >= 0) > - dprintf (vfd, "init server %d %s\n", i, server_url); > +#ifdef ENABLE_IMA_VERIFICATION > + dprintf (vfd, "init server %d %s [IMA verification policy: %s]\n", i, server_url, ima_policy_enum2str(url_ima_policies[i])); > +#else > + dprintf (vfd, "init server %d %s\n", i, server_url); > +#endif > > data[i].fd = fd; > data[i].target_handle = &target_handle; OK > @@ -1769,7 +2022,32 @@ debuginfod_query_server (debuginfod_client *c, > > /* PR27571: make cache files casually unwriteable; dirs are already 0700 */ > (void) fchmod(fd, 0400); > - > + > + if(NULL != url_ima_policies && ignore != url_ima_policies[committed_to]) > + { > + int result = debuginfod_validate_imasig(c, target_cache_tmppath, fd); > + if(0 == result) > + { > + if (vfd >= 0) dprintf (vfd, "valid signature\n"); > + } > + else if(EINVAL == result || enforcing == url_ima_policies[committed_to]) > + { > + // All invalid signatures are rejected. > + // Additionally in enforcing mode any non-valid signature is rejected, so by reaching > + // this case we do so since we know it is not valid. Note - this not just invalid signatures > + // but also signatures that cannot be validated > + if (vfd >= 0) dprintf (vfd, "error: invalid or missing signature (%d)\n", result); > + rc = -EPERM; > + goto out2; > + } > + 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. But in general I think the "permissive" policy is weird. > /* rename tmp->real */ > rc = rename (target_cache_tmppath, target_cache_path); > if (rc < 0) > @@ -1790,6 +2068,7 @@ debuginfod_query_server (debuginfod_client *c, > for (int i = 0; i < num_urls; ++i) > free(server_url_list[i]); > free(server_url_list); > + free(url_ima_policies); > free (data); > free (server_urls); > > @@ -1823,6 +2102,7 @@ debuginfod_query_server (debuginfod_client *c, > for (int i = 0; i < num_urls; ++i) > free(server_url_list[i]); > free(server_url_list); > + free(url_ima_policies); > > out0: > free (server_urls); OK. Stopping here for now. Will go through debuginfod.cxx and tests tomorrow. Cheers, Mark