* [PATCH] PR28204, debuginfod IMA
@ 2023-09-07 12:55 Frank Ch. Eigler
2023-10-23 19:33 ` Mark Wielaard
2023-10-24 15:25 ` Mark Wielaard
0 siblings, 2 replies; 11+ messages in thread
From: Frank Ch. Eigler @ 2023-09-07 12:55 UTC (permalink / raw)
To: elfutils-devel
Hi -
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.
commit 4e45a08aee42958298a3fad6043cbf96243d13a5 (HEAD -> users/fche/try-bz28204, origin/users/fche/try-bz28204)
Author: Ryan Goldberg <rgoldber@redhat.com>
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.
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.
NB: debuginfod section queries are excluded from signature
verification at this time, and function as though ima:ignore were in
effect.
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.
Signed-off-by: Ryan Goldberg <rgoldber@redhat.com>
Signed-off-by: Frank Ch. Eigler <fche@redhat.com>
diff --git a/ChangeLog b/ChangeLog
index 6aed95b6974e..b3b1a8ebc93a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2023-08-14 Ryan Goldberg <rgoldber@redhat.com>
+
+ * 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 <dichen@redhat.com>
* NEWS: Support readelf -Ds for using dynamic segment to
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 <rgoldber@redhat.com>
+
+ * 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 <mark@klomp.org>
* 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-
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
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
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 <rpm/rpmlib.h>])
+])
+
+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"])
+
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}
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 <rgoldber@redhat.com>
+
+ * 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 <fche@redhat.com>
* 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
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 <sys/stat.h>
#include <sys/utsname.h>
#include <curl/curl.h>
+#include <fnmatch.h>
/* 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 <pthread.h>
+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>
+ 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;
+ }
+
+ 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 "";
+ }
+
+ 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;
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;
+
+ #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;
+ }
+ 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]);
+
+ // 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);
+ 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;
+ }
+
+ 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;
+ }
+
+ 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;
+ }
+ }
+
+ 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
+
+ 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, ":"))
+ {
+ dp = opendir(cert_dir_path);
+ if(!dp) continue;
+ 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;
+ 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';
+
+ 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";
+ 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;
+ }
+ 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;
+}
+
/* 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))
{
+ // 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;
@@ -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;
@@ -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);
+ }
+ }
+
/* 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);
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index d72d2ad16960..8c3298586672 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -113,6 +113,13 @@ using namespace std;
#define MHD_RESULT int
#endif
+#ifdef ENABLE_IMA_VERIFICATION
+ #include <rpm/rpmlib.h>
+ #include <rpm/rpmfi.h>
+ #include <rpm/header.h>
+ #include <glob.h>
+#endif
+
#include <curl/curl.h>
#include <archive.h>
#include <archive_entry.h>
@@ -431,6 +438,8 @@ static const struct argp_option options[] =
{ "disable-source-scan", ARGP_KEY_DISABLE_SOURCE_SCAN, NULL, 0, "Do not scan dwarf source info.", 0 },
#define ARGP_SCAN_CHECKPOINT 0x100A
{ "scan-checkpoint", ARGP_SCAN_CHECKPOINT, "NUM", 0, "Number of files scanned before a WAL checkpoint.", 0 },
+#define ARGP_KEY_KOJI_SIGCACHE 0x100B
+ { "koji-sigcache", ARGP_KEY_KOJI_SIGCACHE, NULL, 0, "Do a koji specific mapping of rpm paths to get IMA signatures.", 0 },
{ NULL, 0, NULL, 0, NULL, 0 },
};
@@ -486,6 +495,7 @@ static bool scan_source_info = true;
static string tmpdir;
static bool passive_p = false;
static long scan_checkpoint = 256;
+static bool requires_koji_sigcache_mapping = false;
static void set_metric(const string& key, double value);
// static void inc_metric(const string& key);
@@ -692,6 +702,9 @@ parse_opt (int key, char *arg,
if (scan_checkpoint < 0)
argp_failure(state, 1, EINVAL, "scan checkpoint");
break;
+ case ARGP_KEY_KOJI_SIGCACHE:
+ requires_koji_sigcache_mapping = true;
+ break;
// case 'h': argp_state_help (state, stderr, ARGP_HELP_LONG|ARGP_HELP_EXIT_OK);
default: return ARGP_ERR_UNKNOWN;
}
@@ -1933,6 +1946,131 @@ handle_buildid_r_match (bool internal_req_p,
return 0;
}
+ // Extract the IMA per-file signature (if it exists)
+ string ima_sig = "";
+ #ifdef ENABLE_IMA_VERIFICATION
+ do
+ {
+ FD_t rpm_fd;
+ if(!(rpm_fd = Fopen(b_source0.c_str(), "r.ufdio")))
+ {
+ if (verbose) obatched(clog) << "There was an error while opening " << b_source0 << endl;
+ break; // Exit IMA extraction
+ }
+
+ Header rpm_hdr;
+ if(RPMRC_FAIL == rpmReadPackageFile(NULL, rpm_fd, b_source0.c_str(), &rpm_hdr))
+ {
+ if (verbose) obatched(clog) << "There was an error while reading the header of " << b_source0 << endl;
+ Fclose(rpm_fd);
+ break; // Exit IMA extraction
+ }
+
+ // Fill sig_tag_data with an alloc'd copy of the array of IMA signatures (if they exist)
+ struct rpmtd_s sig_tag_data;
+ rpmtdReset(&sig_tag_data);
+ do{ /* A do-while so we can break out of the koji sigcache checking on failure */
+ if(requires_koji_sigcache_mapping)
+ {
+ /* NB: Koji builds result in a directory structure like the following
+ - PACKAGE/VERSION/RELEASE
+ - ARCH1
+ - foo.rpm // The rpm known by debuginfod
+ - ...
+ - ARCHN
+ - data
+ - signed // Periodically purged (and not scanned by debuginfod)
+ - sigcache
+ - ARCH1
+ - foo.rpm.sig // An empty rpm header
+ - ...
+ - ARCHN
+ - PACKAGE_KEYID1
+ - ARCH1
+ - foo.rpm.sig // The header of the signed rpm. This is the file we need to extract the IMA signatures
+ - ...
+ - ARCHN
+ - ...
+ - PACKAGE_KEYIDN
+ We therefore need to do a mapping of P/V/R/A/N-V-R.A.rpm -> P/V/R/data/sigcache/KEYID/A/N-V-R.A.rpm.sig. There are 2 key insights here
+ 1. We need to go 2 directories down from sigcache to get to the rpm header. So to distinguish ARCH1/foo.rpm.sig and PACKAGE_KEYID1/ARCH1/foo.rpm.sig
+ we can look 2 directories down
+ 2. Its safe to assume that the user will have all of the required verification certs. So we can pick from any of the PACKAGE_KEYIDi directories.
+ For simplicity we choose first we match against
+ See: https://pagure.io/koji/issue/3670
+ */
+
+ // Do the mapping from b_source0 to the koji path for the signed rpm header
+ string signed_rpm_path = b_source0;
+ size_t insert_pos = string::npos;
+ for(int i = 0; i < 2; i++) insert_pos = signed_rpm_path.rfind("/", insert_pos) - 1;
+ string globbed_path = signed_rpm_path.insert(insert_pos + 1, "/data/sigcache/*").append(".sig"); // The globbed path we're seeking
+ glob_t pglob;
+ int grc;
+ if(0 != (grc = glob(globbed_path.c_str(), GLOB_NOSORT, NULL, &pglob)))
+ {
+ // Break out, but only report real errors
+ if (verbose && grc != GLOB_NOMATCH) obatched(clog) << "There was an error (" << strerror(errno) << ") globbing " << globbed_path << endl;
+ break; // Exit koji sigcache check
+ }
+ signed_rpm_path = pglob.gl_pathv[0]; // See insight 2 above
+ globfree(&pglob);
+
+ if (verbose > 2) obatched(clog) << "attempting IMA signature extraction from koji header " << signed_rpm_path << endl;
+
+ FD_t sig_rpm_fd;
+ if(NULL == (sig_rpm_fd = Fopen(signed_rpm_path.c_str(), "r")))
+ {
+ if (verbose) obatched(clog) << "There was an error while opening " << signed_rpm_path << endl;
+ break; // Exit koji sigcache check
+ }
+
+ Header sig_hdr = headerRead(sig_rpm_fd, HEADER_MAGIC_YES /* Validate magic too */ );
+ if (!sig_hdr || 1 != headerGet(sig_hdr, RPMSIGTAG_FILESIGNATURES, &sig_tag_data, HEADERGET_ALLOC))
+ {
+ if (verbose) obatched(clog) << "Unable to extract RPMSIGTAG_FILESIGNATURES from " << signed_rpm_path << endl;
+ }
+ headerFree(sig_hdr); // We can free here since sig_tag_data has an alloc'd copy of the data
+ Fclose(sig_rpm_fd);
+ }
+ }while(false);
+
+ if(0 == sig_tag_data.count)
+ {
+ // In the general case (or a fallback from the koji sigcache mapping not finding signatures)
+ // we can just (try) extract the signatures from the rpm header
+ if (1 != headerGet(rpm_hdr, RPMTAG_FILESIGNATURES, &sig_tag_data, HEADERGET_ALLOC))
+ {
+ if (verbose) obatched(clog) << "Unable to extract RPMTAG_FILESIGNATURES from " << b_source0 << endl;
+ }
+ }
+ // Search the array for the signature coresponding to b_source1
+ int idx = -1;
+ char *sig = NULL;
+ rpmfi hdr_fi = rpmfiNew(NULL, rpm_hdr, RPMTAG_BASENAMES, RPMFI_FLAGS_QUERY);
+ for(sig = (char*)rpmtdNextString(&sig_tag_data), idx = rpmfiNext(hdr_fi);
+ idx != -1 && 0 != strcmp(b_source1.c_str(), rpmfiFN(hdr_fi));
+ sig = (char*)rpmtdNextString(&sig_tag_data), idx = rpmfiNext(hdr_fi));
+ rpmfiFree(hdr_fi);
+
+ if(sig && 0 != strlen(sig) && idx != -1)
+ {
+ if (verbose > 2) obatched(clog) << "Found IMA signature for " << b_source1 << ":\n" << sig << endl;
+ ima_sig = sig;
+ inc_metric("http_responses_total","extra","ima-sigs-extracted");
+ }
+ else
+ {
+ if (verbose > 2) obatched(clog) << "Could not find IMA signature for " << b_source1 << endl;
+ }
+
+ rpmtdFreeData (&sig_tag_data);
+ headerFree(rpm_hdr);
+ Fclose(rpm_fd);
+ }
+ while(false);
+ #endif
+
// check for a match in the fdcache first
int fd = fdcache.lookup(b_source0, b_source1);
while (fd >= 0) // got one!; NB: this is really an if() with a possible branch out to the end
@@ -1990,11 +2128,13 @@ handle_buildid_r_match (bool internal_req_p,
to_string(fs.st_size).c_str());
add_mhd_response_header (r, "X-DEBUGINFOD-ARCHIVE", b_source0.c_str());
add_mhd_response_header (r, "X-DEBUGINFOD-FILE", b_source1.c_str());
+ if(!ima_sig.empty()) add_mhd_response_header(r, "X-DEBUGINFOD-IMASIGNATURE", ima_sig.c_str());
add_mhd_last_modified (r, fs.st_mtime);
if (verbose > 1)
obatched(clog) << "serving fdcache archive " << b_source0
<< " file " << b_source1
- << " section=" << section << endl;
+ << " section=" << section
+ << " IMA signature=" << ima_sig << endl;
/* libmicrohttpd will close it. */
if (result_fd)
*result_fd = fd;
@@ -2174,11 +2314,13 @@ handle_buildid_r_match (bool internal_req_p,
add_mhd_response_header (r, "X-DEBUGINFOD-ARCHIVE",
b_source0.c_str());
add_mhd_response_header (r, "X-DEBUGINFOD-FILE", file.c_str());
+ if(!ima_sig.empty()) add_mhd_response_header(r, "X-DEBUGINFOD-IMASIGNATURE", ima_sig.c_str());
add_mhd_last_modified (r, archive_entry_mtime(e));
if (verbose > 1)
obatched(clog) << "serving archive " << b_source0
<< " file " << b_source1
- << " section=" << section << endl;
+ << " section=" << section
+ << " IMA signature=" << ima_sig << endl;
/* libmicrohttpd will close it. */
if (result_fd)
*result_fd = fd;
diff --git a/debuginfod/debuginfod.h.in b/debuginfod/debuginfod.h.in
index 4a256ba9af1f..73f633f0b8e9 100644
--- a/debuginfod/debuginfod.h.in
+++ b/debuginfod/debuginfod.h.in
@@ -39,6 +39,7 @@
#define DEBUGINFOD_MAXSIZE_ENV_VAR "DEBUGINFOD_MAXSIZE"
#define DEBUGINFOD_MAXTIME_ENV_VAR "DEBUGINFOD_MAXTIME"
#define DEBUGINFOD_HEADERS_FILE_ENV_VAR "DEBUGINFOD_HEADERS_FILE"
+#define DEBUGINFOD_IMA_CERT_PATH_ENV_VAR "DEBUGINFOD_IMA_CERT_PATH"
/* The libdebuginfod soname. */
#define DEBUGINFOD_SONAME "@LIBDEBUGINFOD_SONAME@"
diff --git a/debuginfod/ima-certs/centosimabeta9.crt b/debuginfod/ima-certs/centosimabeta9.crt
new file mode 100644
index 000000000000..534f86a15ac5
--- /dev/null
+++ b/debuginfod/ima-certs/centosimabeta9.crt
@@ -0,0 +1,70 @@
+Certificate:
+ Data:
+ Version: 3 (0x2)
+ Serial Number:
+ d6:78:a4:e3:e7:d3:c5:aa
+ Signature Algorithm: sha256WithRSAEncryption
+ Issuer: O=RH-IMA-CA, CN=Red Hat IMA CA/emailAddress=secalert@redhat.com
+ Validity
+ Not Before: Jul 1 16:14:28 2023 GMT
+ Not After : Jan 18 16:14:28 2038 GMT
+ Subject: CN=CentOS IMA beta key (for verification)
+ Subject Public Key Info:
+ Public Key Algorithm: id-ecPublicKey
+ Public-Key: (384 bit)
+ pub:
+ 04:6c:6d:44:6d:eb:55:31:56:11:ab:a7:dc:cc:34:
+ 72:9c:9b:05:5c:d8:28:36:c8:04:05:5a:c8:33:b5:
+ ba:de:ba:f5:0d:a9:1f:02:c1:43:78:e8:0a:2f:05:
+ e9:a9:ce:c4:24:5b:d6:b2:ce:cf:06:52:7a:7a:d9:
+ 3f:dd:b3:09:b1:22:90:e4:95:4e:07:80:f4:9c:73:
+ e9:6c:00:09:ed:b1:68:76:c5:59:7e:74:a4:c3:cf:
+ a1:84:20:51:f6:29:2e
+ ASN1 OID: secp384r1
+ NIST CURVE: P-384
+ X509v3 extensions:
+ X509v3 Basic Constraints: critical
+ CA:FALSE
+ X509v3 Key Usage: critical
+ Digital Signature
+ X509v3 Extended Key Usage: critical
+ Code Signing
+ X509v3 Subject Key Identifier:
+ 61:BA:CE:C0:D7:97:EA:49:9F:D8:92:79:41:57:19:0A:D6:04:63:E1
+ X509v3 Authority Key Identifier:
+ keyid:FB:31:82:5D:D0:E0:73:68:5B:26:4E:30:38:96:36:73:F7:53:95:9A
+
+ Signature Algorithm: sha256WithRSAEncryption
+ 8b:28:5a:ca:d3:2a:ec:e2:c6:89:f8:4d:e9:ff:c8:31:47:7b:
+ 93:17:c1:55:48:40:ce:be:1c:58:ff:6a:83:55:07:72:70:c7:
+ a3:25:91:70:90:ab:9d:bc:d3:63:4e:aa:f2:2f:ee:0c:05:a5:
+ 8e:2e:98:7b:41:44:37:66:87:80:94:56:b1:1f:13:99:d0:80:
+ 0b:e8:ec:50:cf:8c:90:53:cb:b1:9c:35:cf:b7:75:1f:ad:77:
+ 52:d2:06:56:7a:37:16:fe:bb:c2:aa:de:72:5d:b3:4b:9f:d3:
+ 05:c6:4a:36:d2:14:6b:9a:b3:64:62:4b:d5:9d:aa:71:ab:11:
+ c0:08:17:e1:f9:88:49:c7:6f:74:ff:02:04:ee:cd:a2:75:5b:
+ 79:19:ed:ad:14:71:b4:19:56:ed:fe:7b:61:7e:d4:97:b5:c7:
+ a5:05:41:4c:9f:17:91:71:71:bc:12:2b:d8:6c:2b:c0:02:93:
+ ed:28:30:9b:7f:a1:93:8c:32:74:fe:6c:c0:ab:68:5e:e5:3e:
+ 22:c9:db:d4:4e:b2:5c:d6:49:e7:1d:91:78:61:3c:ca:5e:72:
+ 63:74:ff:af:0d:0d:32:ae:2a:cd:9b:a1:30:95:ea:33:53:64:
+ 2b:13:2d:96:e3:b6:6d:f2:c2:3c:24:c8:6b:87:f8:b4:7f:72:
+ 30:66:f8:5d
+-----BEGIN CERTIFICATE-----
+MIICzzCCAbegAwIBAgIJANZ4pOPn08WqMA0GCSqGSIb3DQEBCwUAMFExEjAQBgNV
+BAoMCVJILUlNQS1DQTEXMBUGA1UEAwwOUmVkIEhhdCBJTUEgQ0ExIjAgBgkqhkiG
+9w0BCQEWE3NlY2FsZXJ0QHJlZGhhdC5jb20wHhcNMjMwNzAxMTYxNDI4WhcNMzgw
+MTE4MTYxNDI4WjAxMS8wLQYDVQQDDCZDZW50T1MgSU1BIGJldGEga2V5IChmb3Ig
+dmVyaWZpY2F0aW9uKTB2MBAGByqGSM49AgEGBSuBBAAiA2IABGxtRG3rVTFWEaun
+3Mw0cpybBVzYKDbIBAVayDO1ut669Q2pHwLBQ3joCi8F6anOxCRb1rLOzwZSenrZ
+P92zCbEikOSVTgeA9Jxz6WwACe2xaHbFWX50pMPPoYQgUfYpLqN4MHYwDAYDVR0T
+AQH/BAIwADAOBgNVHQ8BAf8EBAMCB4AwFgYDVR0lAQH/BAwwCgYIKwYBBQUHAwMw
+HQYDVR0OBBYEFGG6zsDXl+pJn9iSeUFXGQrWBGPhMB8GA1UdIwQYMBaAFPsxgl3Q
+4HNoWyZOMDiWNnP3U5WaMA0GCSqGSIb3DQEBCwUAA4IBAQCLKFrK0yrs4saJ+E3p
+/8gxR3uTF8FVSEDOvhxY/2qDVQdycMejJZFwkKudvNNjTqryL+4MBaWOLph7QUQ3
+ZoeAlFaxHxOZ0IAL6OxQz4yQU8uxnDXPt3UfrXdS0gZWejcW/rvCqt5yXbNLn9MF
+xko20hRrmrNkYkvVnapxqxHACBfh+YhJx290/wIE7s2idVt5Ge2tFHG0GVbt/nth
+ftSXtcelBUFMnxeRcXG8EivYbCvAApPtKDCbf6GTjDJ0/mzAq2he5T4iydvUTrJc
+1knnHZF4YTzKXnJjdP+vDQ0yrirNm6EwleozU2QrEy2W47Zt8sI8JMhrh/i0f3Iw
+Zvhd
+-----END CERTIFICATE-----
diff --git a/debuginfod/ima-certs/centosimarelease9.crt b/debuginfod/ima-certs/centosimarelease9.crt
new file mode 100644
index 000000000000..db641453260f
--- /dev/null
+++ b/debuginfod/ima-certs/centosimarelease9.crt
@@ -0,0 +1,70 @@
+Certificate:
+ Data:
+ Version: 3 (0x2)
+ Serial Number:
+ d6:78:a4:e3:e7:d3:c5:ab
+ Signature Algorithm: sha256WithRSAEncryption
+ Issuer: O=RH-IMA-CA, CN=Red Hat IMA CA/emailAddress=secalert@redhat.com
+ Validity
+ Not Before: Jul 1 16:14:51 2023 GMT
+ Not After : Jan 18 16:14:51 2038 GMT
+ Subject: CN=CentOS IMA release key (for verification)
+ Subject Public Key Info:
+ Public Key Algorithm: id-ecPublicKey
+ Public-Key: (384 bit)
+ pub:
+ 04:d4:d0:31:08:09:0d:97:d0:5c:c8:49:ff:90:f4:
+ 3a:16:85:a3:73:a1:d9:c4:28:4c:f7:aa:a8:22:c2:
+ cf:0e:8b:d7:9a:ed:e6:f0:89:f8:85:95:72:c3:38:
+ 27:2a:29:97:6a:6b:2b:01:04:a3:32:ba:f4:75:f9:
+ e4:c8:48:2f:f5:36:69:44:27:f9:35:b3:0c:c3:22:
+ 24:67:51:06:d3:73:f1:56:94:20:a8:8c:82:34:c0:
+ 10:ef:ce:f9:b4:7a:42
+ ASN1 OID: secp384r1
+ NIST CURVE: P-384
+ X509v3 extensions:
+ X509v3 Basic Constraints: critical
+ CA:FALSE
+ X509v3 Key Usage: critical
+ Digital Signature
+ X509v3 Extended Key Usage: critical
+ Code Signing
+ X509v3 Subject Key Identifier:
+ 54:E5:A3:4F:16:2B:32:B7:77:FF:E3:4F:1E:8B:66:12:7C:43:5B:B5
+ X509v3 Authority Key Identifier:
+ keyid:FB:31:82:5D:D0:E0:73:68:5B:26:4E:30:38:96:36:73:F7:53:95:9A
+
+ Signature Algorithm: sha256WithRSAEncryption
+ c6:1d:92:0e:92:40:d6:ae:a5:5d:4e:5d:2a:e1:0f:92:42:20:
+ 89:e1:a9:82:87:35:42:c9:7f:77:dd:19:e3:cf:ef:be:8b:39:
+ 4f:99:2e:cd:cc:a3:18:23:7f:81:4b:7d:63:5d:71:b4:4b:9c:
+ ea:dc:2f:1d:16:da:4c:ed:98:bf:df:88:11:d0:8b:af:01:55:
+ 71:05:fe:d7:ac:78:4e:46:de:48:9f:04:74:42:c2:c8:1a:fc:
+ c5:46:6a:99:3e:9a:b0:e4:04:07:48:e2:4c:65:e5:01:a8:ad:
+ 3c:8d:c0:ca:c5:73:23:36:88:27:54:8b:90:f8:ea:55:fc:eb:
+ b8:69:a5:8b:a0:1d:8b:f1:93:dd:71:9e:e9:88:f0:2d:0e:7d:
+ 86:a4:8d:0b:fd:00:c9:c0:73:aa:b1:65:b1:60:6e:a4:09:1b:
+ 3e:30:d9:62:2a:15:d6:50:2a:6a:fd:24:e7:8c:93:78:4a:28:
+ d5:b1:d9:ba:1b:8d:ef:48:0d:f4:8c:79:90:0f:95:8d:79:39:
+ 8d:41:a5:fc:6f:e4:ef:5c:ee:3b:f4:c3:2c:c3:a0:b7:61:ac:
+ 7e:e9:eb:a0:3a:ba:05:2c:bd:aa:a9:1f:c5:b9:ee:72:f6:c4:
+ 54:1f:71:3b:e1:70:1a:30:f4:04:18:50:60:c4:5a:da:93:cd:
+ b6:f6:67:c8
+-----BEGIN CERTIFICATE-----
+MIIC0jCCAbqgAwIBAgIJANZ4pOPn08WrMA0GCSqGSIb3DQEBCwUAMFExEjAQBgNV
+BAoMCVJILUlNQS1DQTEXMBUGA1UEAwwOUmVkIEhhdCBJTUEgQ0ExIjAgBgkqhkiG
+9w0BCQEWE3NlY2FsZXJ0QHJlZGhhdC5jb20wHhcNMjMwNzAxMTYxNDUxWhcNMzgw
+MTE4MTYxNDUxWjA0MTIwMAYDVQQDDClDZW50T1MgSU1BIHJlbGVhc2Uga2V5IChm
+b3IgdmVyaWZpY2F0aW9uKTB2MBAGByqGSM49AgEGBSuBBAAiA2IABNTQMQgJDZfQ
+XMhJ/5D0OhaFo3Oh2cQoTPeqqCLCzw6L15rt5vCJ+IWVcsM4Jyopl2prKwEEozK6
+9HX55MhIL/U2aUQn+TWzDMMiJGdRBtNz8VaUIKiMgjTAEO/O+bR6QqN4MHYwDAYD
+VR0TAQH/BAIwADAOBgNVHQ8BAf8EBAMCB4AwFgYDVR0lAQH/BAwwCgYIKwYBBQUH
+AwMwHQYDVR0OBBYEFFTlo08WKzK3d//jTx6LZhJ8Q1u1MB8GA1UdIwQYMBaAFPsx
+gl3Q4HNoWyZOMDiWNnP3U5WaMA0GCSqGSIb3DQEBCwUAA4IBAQDGHZIOkkDWrqVd
+Tl0q4Q+SQiCJ4amChzVCyX933Rnjz+++izlPmS7NzKMYI3+BS31jXXG0S5zq3C8d
+FtpM7Zi/34gR0IuvAVVxBf7XrHhORt5InwR0QsLIGvzFRmqZPpqw5AQHSOJMZeUB
+qK08jcDKxXMjNognVIuQ+OpV/Ou4aaWLoB2L8ZPdcZ7piPAtDn2GpI0L/QDJwHOq
+sWWxYG6kCRs+MNliKhXWUCpq/STnjJN4SijVsdm6G43vSA30jHmQD5WNeTmNQaX8
+b+TvXO479MMsw6C3Yax+6eugOroFLL2qqR/Fue5y9sRUH3E74XAaMPQEGFBgxFra
+k8229mfI
+-----END CERTIFICATE-----
diff --git a/debuginfod/ima-certs/fedora-38-ima.der b/debuginfod/ima-certs/fedora-38-ima.der
new file mode 100644
index 000000000000..238ae6c30763
Binary files /dev/null and b/debuginfod/ima-certs/fedora-38-ima.der differ
diff --git a/debuginfod/ima-certs/fedora-39-ima.der b/debuginfod/ima-certs/fedora-39-ima.der
new file mode 100644
index 000000000000..0d13baa62d75
Binary files /dev/null and b/debuginfod/ima-certs/fedora-39-ima.der differ
diff --git a/debuginfod/ima-certs/redhatimabeta9.crt b/debuginfod/ima-certs/redhatimabeta9.crt
new file mode 100644
index 000000000000..4a3b6701d6c4
--- /dev/null
+++ b/debuginfod/ima-certs/redhatimabeta9.crt
@@ -0,0 +1,70 @@
+Certificate:
+ Data:
+ Version: 3 (0x2)
+ Serial Number:
+ d6:78:a4:e3:e7:d3:c5:a8
+ Signature Algorithm: sha256WithRSAEncryption
+ Issuer: O=RH-IMA-CA, CN=Red Hat IMA CA/emailAddress=secalert@redhat.com
+ Validity
+ Not Before: Jul 1 16:13:38 2023 GMT
+ Not After : Jan 18 16:13:38 2038 GMT
+ Subject: CN=Red Hat IMA beta key (for verification)
+ Subject Public Key Info:
+ Public Key Algorithm: id-ecPublicKey
+ Public-Key: (384 bit)
+ pub:
+ 04:af:d0:5e:29:27:3a:26:05:27:74:1b:07:62:20:
+ c3:3e:40:43:73:3e:ad:e4:24:3e:b1:24:9d:72:81:
+ e3:f0:43:40:7b:82:52:dc:57:66:ed:92:af:79:c2:
+ 6b:09:31:bf:50:1b:76:24:b5:2f:8f:ce:9c:65:20:
+ d4:e2:3d:37:41:0d:5d:b7:78:af:8c:d3:71:b7:10:
+ c9:70:15:77:d0:8f:09:97:3f:7e:83:7d:67:98:8f:
+ cf:be:16:b5:6d:73:7d
+ ASN1 OID: secp384r1
+ NIST CURVE: P-384
+ X509v3 extensions:
+ X509v3 Basic Constraints: critical
+ CA:FALSE
+ X509v3 Key Usage: critical
+ Digital Signature
+ X509v3 Extended Key Usage: critical
+ Code Signing
+ X509v3 Subject Key Identifier:
+ 7D:45:10:63:D2:89:7B:E3:6D:A6:62:D6:6B:36:15:E5:F0:F2:9C:D0
+ X509v3 Authority Key Identifier:
+ keyid:FB:31:82:5D:D0:E0:73:68:5B:26:4E:30:38:96:36:73:F7:53:95:9A
+
+ Signature Algorithm: sha256WithRSAEncryption
+ 68:9b:7d:29:e2:46:ed:03:6a:5d:d9:9e:f3:7a:0d:49:fa:22:
+ 40:02:3e:cb:af:a3:ee:aa:b5:43:fd:d9:1e:5b:82:89:e7:ad:
+ 48:03:d4:4c:50:7e:03:32:c3:eb:88:b8:b4:57:8c:1d:78:f9:
+ a4:ca:d2:55:78:a1:ec:67:c8:1b:90:40:be:98:5b:fd:77:91:
+ 6b:7c:ed:5f:e9:b0:af:6e:71:03:bc:03:5f:ad:f0:01:bc:24:
+ 0d:ea:1e:04:9e:10:ee:5c:97:17:49:3e:8e:7b:51:17:1a:e2:
+ 06:09:04:d4:6d:94:ea:30:57:fd:70:d0:56:6c:36:81:9f:81:
+ 75:25:98:34:9b:e8:03:14:b0:7c:8c:13:b5:59:0a:45:15:d8:
+ 75:c9:7a:01:a4:2d:71:a1:6d:53:b6:22:37:ab:98:d3:f0:76:
+ 28:8d:c3:3b:7d:c5:11:b1:89:7b:ac:41:e2:92:e9:38:4d:6d:
+ 07:d9:07:76:53:1a:87:ca:79:2e:5d:20:ec:8b:f2:55:56:dd:
+ b1:65:ec:4c:d7:86:7b:8e:9b:a7:7f:6d:6e:7a:3f:18:e3:e3:
+ e7:3f:1c:fb:44:ab:7e:0c:c2:1d:61:7c:92:73:07:a2:b4:5a:
+ 6b:45:64:dd:4d:c9:df:a3:e1:5f:39:db:36:f3:d0:ba:c6:be:
+ 97:3c:2f:e6
+-----BEGIN CERTIFICATE-----
+MIIC0DCCAbigAwIBAgIJANZ4pOPn08WoMA0GCSqGSIb3DQEBCwUAMFExEjAQBgNV
+BAoMCVJILUlNQS1DQTEXMBUGA1UEAwwOUmVkIEhhdCBJTUEgQ0ExIjAgBgkqhkiG
+9w0BCQEWE3NlY2FsZXJ0QHJlZGhhdC5jb20wHhcNMjMwNzAxMTYxMzM4WhcNMzgw
+MTE4MTYxMzM4WjAyMTAwLgYDVQQDDCdSZWQgSGF0IElNQSBiZXRhIGtleSAoZm9y
+IHZlcmlmaWNhdGlvbikwdjAQBgcqhkjOPQIBBgUrgQQAIgNiAASv0F4pJzomBSd0
+GwdiIMM+QENzPq3kJD6xJJ1ygePwQ0B7glLcV2btkq95wmsJMb9QG3YktS+Pzpxl
+INTiPTdBDV23eK+M03G3EMlwFXfQjwmXP36DfWeYj8++FrVtc32jeDB2MAwGA1Ud
+EwEB/wQCMAAwDgYDVR0PAQH/BAQDAgeAMBYGA1UdJQEB/wQMMAoGCCsGAQUFBwMD
+MB0GA1UdDgQWBBR9RRBj0ol7422mYtZrNhXl8PKc0DAfBgNVHSMEGDAWgBT7MYJd
+0OBzaFsmTjA4ljZz91OVmjANBgkqhkiG9w0BAQsFAAOCAQEAaJt9KeJG7QNqXdme
+83oNSfoiQAI+y6+j7qq1Q/3ZHluCieetSAPUTFB+AzLD64i4tFeMHXj5pMrSVXih
+7GfIG5BAvphb/XeRa3ztX+mwr25xA7wDX63wAbwkDeoeBJ4Q7lyXF0k+jntRFxri
+BgkE1G2U6jBX/XDQVmw2gZ+BdSWYNJvoAxSwfIwTtVkKRRXYdcl6AaQtcaFtU7Yi
+N6uY0/B2KI3DO33FEbGJe6xB4pLpOE1tB9kHdlMah8p5Ll0g7IvyVVbdsWXsTNeG
+e46bp39tbno/GOPj5z8c+0SrfgzCHWF8knMHorRaa0Vk3U3J36PhXznbNvPQusa+
+lzwv5g==
+-----END CERTIFICATE-----
diff --git a/debuginfod/ima-certs/redhatimarelease9.crt b/debuginfod/ima-certs/redhatimarelease9.crt
new file mode 100644
index 000000000000..cd0b66494da2
--- /dev/null
+++ b/debuginfod/ima-certs/redhatimarelease9.crt
@@ -0,0 +1,70 @@
+Certificate:
+ Data:
+ Version: 3 (0x2)
+ Serial Number:
+ d6:78:a4:e3:e7:d3:c5:a9
+ Signature Algorithm: sha256WithRSAEncryption
+ Issuer: O=RH-IMA-CA, CN=Red Hat IMA CA/emailAddress=secalert@redhat.com
+ Validity
+ Not Before: Jul 1 16:14:04 2023 GMT
+ Not After : Jan 18 16:14:04 2038 GMT
+ Subject: CN=Red Hat IMA release key (for verification)
+ Subject Public Key Info:
+ Public Key Algorithm: id-ecPublicKey
+ Public-Key: (384 bit)
+ pub:
+ 04:4f:0e:ef:bf:e2:23:89:91:27:4e:7c:32:a1:d0:
+ c0:26:92:de:37:8d:b0:5d:ea:7f:d6:27:18:9b:b4:
+ 62:be:06:85:3d:f9:cc:47:7e:c7:bd:91:54:53:62:
+ b4:c0:8a:43:48:c2:59:07:2b:88:d7:3d:4b:30:8d:
+ 6c:32:fb:a5:da:dc:8a:85:e9:61:44:18:fc:d9:8b:
+ f5:5e:38:c8:85:77:ca:73:68:ce:48:df:af:3d:06:
+ 43:2f:4b:6c:0c:cd:88
+ ASN1 OID: secp384r1
+ NIST CURVE: P-384
+ X509v3 extensions:
+ X509v3 Basic Constraints: critical
+ CA:FALSE
+ X509v3 Key Usage: critical
+ Digital Signature
+ X509v3 Extended Key Usage: critical
+ Code Signing
+ X509v3 Subject Key Identifier:
+ 22:FA:01:DC:0E:A0:26:9F:69:A8:67:E5:CF:E4:9C:FB:D3:32:04:49
+ X509v3 Authority Key Identifier:
+ keyid:FB:31:82:5D:D0:E0:73:68:5B:26:4E:30:38:96:36:73:F7:53:95:9A
+
+ Signature Algorithm: sha256WithRSAEncryption
+ 1a:1e:c1:2d:65:ad:f0:24:ec:9e:a7:fd:d4:ea:e1:54:dc:31:
+ 1c:62:8c:29:0b:7a:56:6e:f7:b4:87:92:3e:ff:d5:40:4b:24:
+ a1:68:6e:ee:9c:35:65:a1:3f:8e:f3:8b:9b:18:b1:03:ed:fb:
+ 50:2e:a3:23:d1:93:1d:d6:82:0a:10:6f:34:be:d6:3a:bd:76:
+ 8c:44:0e:ad:a7:2a:c4:8e:8d:c4:e4:8d:51:d8:26:b7:38:89:
+ d1:23:a0:23:88:76:fa:f1:27:91:57:3e:b2:0f:cf:73:53:db:
+ 20:40:5d:82:b9:e9:bc:a2:94:09:57:fb:85:0d:56:4b:dc:19:
+ 65:12:2f:6d:6a:3b:be:35:1f:d4:52:ea:e4:72:36:f9:fe:cb:
+ d4:1b:0f:e3:0e:88:7c:68:58:28:c3:06:5f:bd:d2:f9:2e:1a:
+ 30:f0:63:65:2d:55:e1:a4:fd:97:cf:ff:c0:52:22:1c:24:a3:
+ 6e:de:7a:c9:9d:75:d2:d0:82:b0:7f:6f:db:21:01:69:f0:54:
+ 76:04:19:68:2c:22:72:dd:3b:0d:04:d5:ad:5a:80:30:68:90:
+ 6e:c2:27:f4:28:af:1b:78:f6:0a:70:74:5c:3a:61:42:f5:63:
+ 7c:83:12:5a:1b:43:bc:d4:1b:28:b5:ef:98:c5:14:04:42:80:
+ dd:54:30:a4
+-----BEGIN CERTIFICATE-----
+MIIC0zCCAbugAwIBAgIJANZ4pOPn08WpMA0GCSqGSIb3DQEBCwUAMFExEjAQBgNV
+BAoMCVJILUlNQS1DQTEXMBUGA1UEAwwOUmVkIEhhdCBJTUEgQ0ExIjAgBgkqhkiG
+9w0BCQEWE3NlY2FsZXJ0QHJlZGhhdC5jb20wHhcNMjMwNzAxMTYxNDA0WhcNMzgw
+MTE4MTYxNDA0WjA1MTMwMQYDVQQDDCpSZWQgSGF0IElNQSByZWxlYXNlIGtleSAo
+Zm9yIHZlcmlmaWNhdGlvbikwdjAQBgcqhkjOPQIBBgUrgQQAIgNiAARPDu+/4iOJ
+kSdOfDKh0MAmkt43jbBd6n/WJxibtGK+BoU9+cxHfse9kVRTYrTAikNIwlkHK4jX
+PUswjWwy+6Xa3IqF6WFEGPzZi/VeOMiFd8pzaM5I3689BkMvS2wMzYijeDB2MAwG
+A1UdEwEB/wQCMAAwDgYDVR0PAQH/BAQDAgeAMBYGA1UdJQEB/wQMMAoGCCsGAQUF
+BwMDMB0GA1UdDgQWBBQi+gHcDqAmn2moZ+XP5Jz70zIESTAfBgNVHSMEGDAWgBT7
+MYJd0OBzaFsmTjA4ljZz91OVmjANBgkqhkiG9w0BAQsFAAOCAQEAGh7BLWWt8CTs
+nqf91OrhVNwxHGKMKQt6Vm73tIeSPv/VQEskoWhu7pw1ZaE/jvOLmxixA+37UC6j
+I9GTHdaCChBvNL7WOr12jEQOracqxI6NxOSNUdgmtziJ0SOgI4h2+vEnkVc+sg/P
+c1PbIEBdgrnpvKKUCVf7hQ1WS9wZZRIvbWo7vjUf1FLq5HI2+f7L1BsP4w6IfGhY
+KMMGX73S+S4aMPBjZS1V4aT9l8//wFIiHCSjbt56yZ110tCCsH9v2yEBafBUdgQZ
+aCwict07DQTVrVqAMGiQbsIn9CivG3j2CnB0XDphQvVjfIMSWhtDvNQbKLXvmMUU
+BEKA3VQwpA==
+-----END CERTIFICATE-----
diff --git a/doc/ChangeLog b/doc/ChangeLog
index 7f2d6ff4fd31..914f8f649511 100644
--- a/doc/ChangeLog
+++ b/doc/ChangeLog
@@ -1,3 +1,10 @@
+2023-08-14 Ryan Goldberg <rgoldber@redhat.com>
+
+ * debuginfod-client-config.7: Document DEBUGINFOD_IMA_CERT_PATH,
+ update DEBUGINFOD_URLS.
+ * debuginfod.8: Document --koji-sigcache
+ * debuginfod-find.1, debuginfod_find_debuginfo.3: Update SECURITY
+
2023-02-14 Mark Wielaard <mark@klomp.org>
* debuginfod.8: Add .TP before -g.
diff --git a/doc/debuginfod-client-config.7 b/doc/debuginfod-client-config.7
index 53d82806d395..11d2465858c5 100644
--- a/doc/debuginfod-client-config.7
+++ b/doc/debuginfod-client-config.7
@@ -27,6 +27,34 @@ debuginfod instances. Alternate URL prefixes are separated by space.
This environment variable may be set by /etc/profile.d scripts
reading /etc/debuginfod/*.urls files.
+This environment variable can also contain policy defining tags which
+dictate the response policy for verifying per-file IMA signatures in
+RPMs. As the space seperated list is read left to right, upon
+encountering a tag, subsequent URLs up to the next tag will be handled
+using that specified policy. All URLs before the first tag will use
+the default policy, \fIima:permissive\fP. For example:
+
+.in +4n
+.EX
+DEBUGINFOD_URLS="https://foo.com ima:enforcing https://bar.ca http://localhost:8002/ ima:permissive https://baz.org"
+.EE
+.in
+
+Where foo.com and baz.org use the default \fIpermissive\fP policy and
+bar.ca and localhost use an \fIenforcing\fP policy. The policy tag
+may be one of the following:
+.IP
+\fIima:enforcing\fP Every downloaded file requires a valid signature.
+.IP
+\fIima:permissive\fP Every downloaded file accompanied by a signature
+must be valid, but downloads without signatures are accepted.
+.IP
+\fIima:ignore\fP Skips verification altogether.
+.IP
+
+Alerts of validation failure will be directed as specified
+in $DEBUGINFOD_VERBOSE.
+
.TP
.B $DEBUGINFOD_CACHE_PATH
This environment variable governs the location of the cache where
@@ -82,6 +110,12 @@ outbound HTTP requests, one per line. The header lines shouldn't end with
CRLF, unless that's the system newline convention. Whitespace-only lines
are skipped.
+.TP
+.B $DEBUGINFOD_IMA_CERT_PATH
+This environment variable contains a list of absolute directory paths
+holding X.509 certificates for RPM per-file IMA-verification.
+Alternate paths are separated by colons.
+
.SH CACHE
Before each query, the debuginfod client library checks for a need to
diff --git a/doc/debuginfod-find.1 b/doc/debuginfod-find.1
index 7d577babeb89..89a706728cd6 100644
--- a/doc/debuginfod-find.1
+++ b/doc/debuginfod-find.1
@@ -129,10 +129,19 @@ and printing the http response headers from the server.
.SH "SECURITY"
-debuginfod-find \fBdoes not\fP include any particular security
-features. It trusts that the binaries returned by the debuginfod(s)
-are accurate. Therefore, the list of servers should include only
-trustworthy ones. If accessed across HTTP rather than HTTPS, the
+If IMA signature(s) are available from the RPMs that contain
+requested files, then
+.BR debuginfod
+will extract those signatures into response headers, and
+.BR debuginfod-find
+will perform verification upon the files.
+Validation policy is controlled via tags inserted into
+$DEBUGINFOD_URLS. By default,
+.BR debuginfod-find
+acts in permissive mode, only rejecting incorrect
+signatures. See below for details and other policy options.
+
+If accessed across HTTP rather than HTTPS, the
network should be trustworthy. Authentication information through
the internal \fIlibcurl\fP library is not currently enabled, except
for the basic plaintext \%\fIhttp[s]://userid:password@hostname/\fP style.
diff --git a/doc/debuginfod.8 b/doc/debuginfod.8
index d4316bec8175..3e73868715d7 100644
--- a/doc/debuginfod.8
+++ b/doc/debuginfod.8
@@ -289,6 +289,14 @@ completed archive or file scans. This may slow down parallel scanning
phase somewhat, but generate much smaller "-wal" temporary files on
busy servers. The default is 256. Disabled if 0.
+.TP
+.B "\-\-koji\-sigcache"
+Enable an additional step of RPM path mapping when extracting signatures for use
+in RPM per-file IMA verification on koji repositories. The signatures are retrieved
+from the Fedora koji sigcache rpm.sig files as opposed to the original RPM header.
+If a signature cannot be found in the sigcache rpm.sig file, the RPM will be
+tried as a fallback.
+
.TP
.B "\-v"
Increase verbosity of logging to the standard error file descriptor.
@@ -311,7 +319,8 @@ X-DEBUGINFOD-FILE is simply the unescaped filename and
X-DEBUGINFOD-SIZE is the size of the file. For files found in archives,
in addition to X-DEBUGINFOD-FILE and X-DEBUGINFOD-SIZE,
X-DEBUGINFOD-ARCHIVE is added. X-DEBUGINFOD-ARCHIVE is the name of the
-archive the file was found in.
+archive the file was found in. X-DEBUGINFOD-IMA-SIGNATURE contains the
+per-file IMA signature as a hexadecimal blob.
There are three requests. In each case, the buildid is encoded as a
lowercase hexadecimal string. For example, for a program \fI/bin/ls\fP,
diff --git a/doc/debuginfod_find_debuginfo.3 b/doc/debuginfod_find_debuginfo.3
index 6469a3dfb2db..e170b94367b5 100644
--- a/doc/debuginfod_find_debuginfo.3
+++ b/doc/debuginfod_find_debuginfo.3
@@ -250,13 +250,22 @@ void *debuginfod_so = dlopen(DEBUGINFOD_SONAME, RTLD_LAZY);
.in
.SH "SECURITY"
+
+If IMA signature(s) are available from the RPMs that contain
+requested files, then
+.BR debuginfod
+will extract those signatures into response headers, and
+.BR debuginfod_find_* ()
+will perform verification upon the files.
+Validation policy is controlled via tags inserted into
+$DEBUGINFOD_URLS. By default,
.BR debuginfod_find_* ()
-functions \fBdo not\fP include any particular security
-features. They trust that the binaries returned by the debuginfod(s)
-are accurate. Therefore, the list of servers should include only
-trustworthy ones. If accessed across HTTP rather than HTTPS, the
-network should be trustworthy. Passing user authentication information
-through the internal \fIlibcurl\fP library is not currently enabled, except
+acts in permissive mode, only rejecting incorrect
+signatures. See below for details and other policy options.
+
+If accessed across HTTP rather than HTTPS, the
+network should be trustworthy. Authentication information through
+the internal \fIlibcurl\fP library is not currently enabled, except
for the basic plaintext \%\fIhttp[s]://userid:password@hostname/\fP style.
(The debuginfod server does not perform authentication, but a front-end
proxy server could.)
diff --git a/tests/ChangeLog b/tests/ChangeLog
index d816873ce083..e52470c04f49 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,11 @@
+2023-08-14 Ryan Goldberg <rgoldber@redhat.com>
+
+ * run-debuginfod-ima-verification.sh: New test.
+ * hello2-1.0-1.x86_64.rpm. New test file.
+ * debuginfod-ima/koji. New test directory.
+ * imacert.der. New test file.
+ * Makefile.am (TESTS, EXTRA_DIST): Add it.
+
2023-04-21 Frank Ch. Eigler <fche@redhat.com>
* run-debuginfod-IXr.sh: New test.
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 32b18e6ef633..130cc992c42e 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -273,6 +273,9 @@ if !OLD_LIBMICROHTTPD
# Too many open file descriptors confuses libmicrohttpd < 0.9.51
TESTS += run-debuginfod-federation-metrics.sh
endif
+if ENABLE_IMA_VERIFICATION
+TESTS += run-debuginfod-ima-verification.sh
+endif
endif
if HAVE_CXX11
@@ -588,6 +591,7 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \
run-debuginfod-webapi-concurrency.sh \
run-debuginfod-section.sh \
run-debuginfod-IXr.sh \
+ run-debuginfod-ima-verification.sh \
debuginfod-rpms/fedora30/hello2-1.0-2.src.rpm \
debuginfod-rpms/fedora30/hello2-1.0-2.x86_64.rpm \
debuginfod-rpms/fedora30/hello2-debuginfo-1.0-2.x86_64.rpm \
@@ -611,6 +615,9 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \
debuginfod-rpms/rhel7/hello2-debuginfo-1.0-2.x86_64.rpm \
debuginfod-rpms/rhel7/hello2-two-1.0-2.x86_64.rpm \
debuginfod-rpms/rhel7/hello2-two-1.0-2.x86_64.rpm \
+ debuginfod-ima/rhel9/hello2-1.0-1.x86_64.rpm \
+ debuginfod-ima/rhel9/imacert.der \
+ debuginfod-ima/koji \
debuginfod-debs/hithere-dbgsym_1.0-1_amd64.ddeb \
debuginfod-debs/hithere_1.0-1.debian.tar.xz \
debuginfod-debs/hithere_1.0-1.dsc \
diff --git a/tests/debuginfod-ima/koji/arch/hello-2.10-9.fc38.x86_64.rpm b/tests/debuginfod-ima/koji/arch/hello-2.10-9.fc38.x86_64.rpm
new file mode 100644
index 000000000000..b04ad8c2af39
Binary files /dev/null and b/tests/debuginfod-ima/koji/arch/hello-2.10-9.fc38.x86_64.rpm differ
diff --git a/tests/debuginfod-ima/koji/data/sigcache/keyid/arch/hello-2.10-9.fc38.x86_64.rpm.sig b/tests/debuginfod-ima/koji/data/sigcache/keyid/arch/hello-2.10-9.fc38.x86_64.rpm.sig
new file mode 100644
index 000000000000..ee7eb8e467b4
Binary files /dev/null and b/tests/debuginfod-ima/koji/data/sigcache/keyid/arch/hello-2.10-9.fc38.x86_64.rpm.sig differ
diff --git a/tests/debuginfod-ima/rhel9/hello2-1.0-1.x86_64.rpm b/tests/debuginfod-ima/rhel9/hello2-1.0-1.x86_64.rpm
new file mode 100644
index 000000000000..0262ae2f0c4c
Binary files /dev/null and b/tests/debuginfod-ima/rhel9/hello2-1.0-1.x86_64.rpm differ
diff --git a/tests/debuginfod-ima/rhel9/imacert.der b/tests/debuginfod-ima/rhel9/imacert.der
new file mode 100644
index 000000000000..b0250b6c30d5
Binary files /dev/null and b/tests/debuginfod-ima/rhel9/imacert.der differ
diff --git a/tests/run-debuginfod-ima-verification.sh b/tests/run-debuginfod-ima-verification.sh
new file mode 100755
index 000000000000..45f3e7d037a6
--- /dev/null
+++ b/tests/run-debuginfod-ima-verification.sh
@@ -0,0 +1,177 @@
+#!/usr/bin/env bash
+#
+# Copyright (C) 2019-2023 Red Hat, Inc.
+# This file is part of elfutils.
+#
+# This file is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# elfutils is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+. $srcdir/debuginfod-subr.sh
+
+type rpmsign 2>/dev/null || { echo "need rpmsign"; exit 77; }
+cat << EoF > include.c
+#include <rpm/rpmlib.h>
+#include <rpm/rpmfi.h>
+#include <rpm/header.h>
+#include <imaevm.h>
+#include <openssl/evp.h>
+EoF
+tempfiles include.c
+gcc -H -fsyntax-only include.c 2> /dev/null || { echo "one or more devel packages are missing (rpm-devel, ima-evm-utils-devel, openssl-devel)"; exit 77; }
+
+DB=${PWD}/.debuginfod_tmp.sqlite
+tempfiles $DB
+export DEBUGINFOD_CACHE_PATH=${PWD}/.client_cache
+IMA_POLICY="enforcing"
+
+# This variable is essential and ensures no time-race for claiming ports occurs
+# set base to a unique multiple of 100 not used in any other 'run-debuginfod-*' test
+base=14000
+get_ports
+mkdir R
+env LD_LIBRARY_PATH=$ldpath DEBUGINFOD_URLS= ${abs_builddir}/../debuginfod/debuginfod $VERBOSE -R \
+ -d $DB -p $PORT1 -t0 -g0 R > vlog$PORT1 2>&1 &
+PID1=$!
+tempfiles vlog$PORT1
+errfiles vlog$PORT1
+
+########################################################################
+cp -pv ${abs_srcdir}/debuginfod-ima/rhel9/hello2-1.0-1.x86_64.rpm signed.rpm
+tempfiles signed.rpm
+RPM_BUILDID=460912dbc989106ec7325d243384df20c5ccec0c # /usr/local/bin/hello
+
+MIN_IMAEVM_MAJ_VERSION=3
+MIN_RPM_MAJ_VERSION=4
+# If the correct programs (and versions) exist sign the rpm in the test
+if (command -v openssl &> /dev/null) && \
+ (command -v rpmsign &> /dev/null) && \
+ (command -v gpg &> /dev/null) && \
+ [ $(ldd `which rpmsign` | grep libimaevm | awk -F'[^0-9]+' '{ print $2 }') -ge $MIN_IMAEVM_MAJ_VERSION ] && \
+ [ $(rpm --version | awk -F'[^0-9]+' '{ print $2 }') -ge $MIN_RPM_MAJ_VERSION ]
+then
+ # SIGN THE RPM
+ # First remove any old signatures
+ rpmsign --delsign signed.rpm &> /dev/null
+ rpmsign --delfilesign signed.rpm &> /dev/null
+
+ # Make a gpg keypair (with $PWD as the homedir)
+ mkdir -m 700 openpgp-revocs.d private-keys-v1.d
+ gpg --quick-gen-key --yes --homedir ${PWD} --batch --passphrase '' --no-default-keyring --keyring "${PWD}/pubring.kbx" example@elfutils.org 2> /dev/null
+
+ # Create a private DER signing key and a public X509 DER format verification key pair
+ openssl genrsa | openssl pkcs8 -topk8 -nocrypt -outform PEM -out signing.pem
+ openssl req -x509 -key signing.pem -out imacert.pem -days 365 -keyform PEM \
+ -subj "/C=CA/ST=ON/L=TO/O=Elfutils/CN=www.sourceware.org\/elfutils"
+
+ tempfiles openpgp-revocs.d/* private-keys-v1.d/* * openpgp-revocs.d private-keys-v1.d
+
+ rpmsign --addsign --signfiles --fskpath=signing.pem -D "_gpg_name example@elfutils.org" -D "_gpg_path ${PWD}" signed.rpm
+ cp signed.rpm R/signed.rpm
+ VERIFICATION_CERT_DIR=${PWD}
+
+ # Cleanup
+ rm -rf openpgp-revocs.d private-keys-v1.d
+else
+ # USE A PRESIGNED RPM
+ cp signed.rpm R/signed.rpm
+ # Note we test with no trailing /
+ VERIFICATION_CERT_DIR=${abs_srcdir}/debuginfod-ima/rhel9
+fi
+
+########################################################################
+# Server must become ready with R fully scanned and indexed
+wait_ready $PORT1 'ready' 1
+wait_ready $PORT1 'thread_work_total{role="traverse"}' 1
+wait_ready $PORT1 'thread_work_pending{role="scan"}' 0
+wait_ready $PORT1 'thread_busy{role="scan"}' 0
+
+export DEBUGINFOD_URLS="ima:$IMA_POLICY http://127.0.0.1:$PORT1"
+
+# Test 1: Without a certificate the verification should fail
+export DEBUGINFOD_IMA_CERT_PATH=
+RC=0
+testrun ${abs_top_builddir}/debuginfod/debuginfod-find -vv executable $RPM_BUILDID || RC=1
+test $RC -ne 0
+
+# Test 2: It should pass once the certificate is added to the path
+export DEBUGINFOD_IMA_CERT_PATH=$VERIFICATION_CERT_DIR
+rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
+kill -USR1 $PID1
+wait_ready $PORT1 'thread_work_total{role="traverse"}' 2
+wait_ready $PORT1 'thread_work_pending{role="scan"}' 0
+wait_ready $PORT1 'thread_busy{role="scan"}' 0
+testrun ${abs_top_builddir}/debuginfod/debuginfod-find -vv executable $RPM_BUILDID
+
+# Test 3: Corrupt the data and it should fail
+dd if=/dev/zero of=R/signed.rpm bs=1 count=128 seek=1024 conv=notrunc
+rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
+kill -USR1 $PID1
+wait_ready $PORT1 'thread_work_total{role="traverse"}' 3
+wait_ready $PORT1 'thread_work_pending{role="scan"}' 0
+wait_ready $PORT1 'thread_busy{role="scan"}' 0
+RC=0
+testrun ${abs_top_builddir}/debuginfod/debuginfod-find executable $RPM_BUILDID || RC=1
+test $RC -ne 0
+
+# Test 4: A rpm without a signature will fail
+cp signed.rpm R/signed.rpm
+rpmsign --delfilesign R/signed.rpm
+rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
+kill -USR1 $PID1
+wait_ready $PORT1 'thread_work_total{role="traverse"}' 4
+wait_ready $PORT1 'thread_work_pending{role="scan"}' 0
+wait_ready $PORT1 'thread_busy{role="scan"}' 0
+RC=0
+testrun ${abs_top_builddir}/debuginfod/debuginfod-find executable $RPM_BUILDID || RC=1
+test $RC -ne 0
+
+# Test 5: Only tests 1,2 will result in extracted signature
+[[ $(curl -s http://127.0.0.1:$PORT1/metrics | grep 'http_responses_total{extra="ima-sigs-extracted"}' | awk '{print $NF}') -eq 2 ]]
+
+kill $PID1
+wait $PID1
+PID1=0
+
+#######################################################################
+# We also test the --koji-sigcache
+cp -pR ${abs_srcdir}/debuginfod-ima/koji R/koji
+
+rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
+env LD_LIBRARY_PATH=$ldpath DEBUGINFOD_URLS= ${abs_builddir}/../debuginfod/debuginfod $VERBOSE -R \
+ -d $DB -p $PORT2 -t0 -g0 -X /data/ --koji-sigcache R/koji > vlog$PORT1 2>&1 &
+#reuse PID1
+PID1=$!
+tempfiles vlog$PORT2
+errfiles vlog$PORT2
+
+RPM_BUILDID=c592a95e45625d7891b90f6b86e63373d540461d #/usr/bin/hello
+# Note we test with a trailing slash and with the required directory as the third in the PATH
+VERIFICATION_CERT_DIR=/not/a/dir:${abs_srcdir}/debuginfod-ima/rhel9:${abs_srcdir}/../debuginfod/ima-certs/
+
+########################################################################
+# Server must become ready with koji fully scanned and indexed
+wait_ready $PORT2 'ready' 1
+wait_ready $PORT2 'thread_work_total{role="traverse"}' 1
+wait_ready $PORT2 'thread_work_pending{role="scan"}' 0
+wait_ready $PORT2 'thread_busy{role="scan"}' 0
+
+# Test 1: The path should be properly mapped and verified using the actual fedora 38 cert
+export DEBUGINFOD_URLS="ima:$IMA_POLICY http://127.0.0.1:$PORT2"
+export DEBUGINFOD_IMA_CERT_PATH=$VERIFICATION_CERT_DIR
+testrun ${abs_top_builddir}/debuginfod/debuginfod-find -vv executable $RPM_BUILDID
+
+kill $PID1
+wait $PID1
+PID1=0
+
+exit 0
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] PR28204, debuginfod IMA
2023-09-07 12:55 [PATCH] PR28204, debuginfod IMA Frank Ch. Eigler
@ 2023-10-23 19:33 ` Mark Wielaard
2023-10-24 13:27 ` Frank Ch. Eigler
2023-10-24 15:25 ` Mark Wielaard
1 sibling, 1 reply; 11+ messages in thread
From: Mark Wielaard @ 2023-10-23 19:33 UTC (permalink / raw)
To: Frank Ch. Eigler; +Cc: elfutils-devel
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 <rgoldber@redhat.com>
> 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 <rgoldber@redhat.com>
> Signed-off-by: Frank Ch. Eigler <fche@redhat.com>
>
> diff --git a/ChangeLog b/ChangeLog
> index 6aed95b6974e..b3b1a8ebc93a 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,9 @@
> +2023-08-14 Ryan Goldberg <rgoldber@redhat.com>
> +
> + * 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 <dichen@redhat.com>
>
> * 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 <rgoldber@redhat.com>
> +
> + * 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 <mark@klomp.org>
>
> * 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 <rpm/rpmlib.h>])
> +])
> +
> +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 <rgoldber@redhat.com>
> +
> + * 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 <fche@redhat.com>
>
> * 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 <sys/stat.h>
> #include <sys/utsname.h>
> #include <curl/curl.h>
> +#include <fnmatch.h>
>
> /* 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 <pthread.h>
>
> +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 ?
> + 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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] PR28204, debuginfod IMA
2023-10-23 19:33 ` Mark Wielaard
@ 2023-10-24 13:27 ` Frank Ch. Eigler
2023-10-24 21:03 ` Mark Wielaard
0 siblings, 1 reply; 11+ messages in thread
From: Frank Ch. Eigler @ 2023-10-24 13:27 UTC (permalink / raw)
To: Mark Wielaard; +Cc: elfutils-devel
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] PR28204, debuginfod IMA
2023-10-24 13:27 ` Frank Ch. Eigler
@ 2023-10-24 21:03 ` Mark Wielaard
2023-10-27 19:15 ` Frank Ch. Eigler
0 siblings, 1 reply; 11+ messages in thread
From: Mark Wielaard @ 2023-10-24 21:03 UTC (permalink / raw)
To: Frank Ch. Eigler; +Cc: elfutils-devel
Hi Frank,
On Tue, Oct 24, 2023 at 09:27:43AM -0400, Frank Ch. Eigler wrote:
> > 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.
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. It would also help with patchwork and pre-commit CI.
https://git-scm.com/docs/git-format-patch#_base_tree_information
> > [...]
> > > 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.
So instead permissive is more like a optional checksum. And the
checksum we are using is really a signature. So we do more than
checksumming, we also check the certificate (is it still valid,
expired, etc.) It is just imho a very "sloppy" concept. I don't
understand the permissive threat-model. And I don't really understand
the name "permissive". Which makes it hard to know if we have
implemented it correctly. e.g. 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.
On the other hand I think the enforcing mode is pretty clear. It
provides the user with those files that the distro provider really
meant to ship. It makes sure that the tools relying on debuginfod only
process known-good/trusted files.
> > 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.
Yeah, you are right, it wouldn't work for the executables, and for the
debug files it only works if you already have the executable on disk
(and trust them). In theory a debugfile can contain MD5 sums in the
DWARF line tables which could be used to check the source files (there
is also the time and size of the file). But in practice those aren't
added (at least not by gcc).
This probably shows how I am confused by the permissive/checking
concept.
> > > 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
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.
Option 3 really seems something different. An alternative would be
marking https servers as trusted as long as their TLS certificate is
trusted.
> > > 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.
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.
> > 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.
> > > + 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.
OK.
> > [...]
> > 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.
> > 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.
>
> > > +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.
Ah, yeah, I got that wrong. Sorry. Those are unused arguments.
> > > + // 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)
ewww. Does this pass ubsan (--enable-sanitize-undefined)? The issue
is that this seems to access structure values at a possibly unaligned
address.
> > > + 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
OK, reading 4K blocks is fine. But bleah, that is a very generic name
to use in an include header. Boo imaevm.h.
> > > + /* 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.
So same as above, this seems like it might trigger undefined behavior.
> > > + 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.
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.
Another possible issue might be the initialization of openssl in the
static constructure. How does that interact with how libcurl inits
openssl?
Cheers,
Mark
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] PR28204, debuginfod IMA
2023-10-24 21:03 ` Mark Wielaard
@ 2023-10-27 19:15 ` Frank Ch. Eigler
2023-10-31 13:20 ` Mark Wielaard
0 siblings, 1 reply; 11+ messages in thread
From: Frank Ch. Eigler @ 2023-10-27 19:15 UTC (permalink / raw)
To: Mark Wielaard; +Cc: elfutils-devel
Hi -
> > 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.
> > > [...]
> > > > 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.
> [...]
> > 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.
> > > 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.
> > > 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.
That leaves the user. Under what conditions do you think all of the
following might hold?
- a user designates a IMA-signature-serving debuginfod as his server
- the user enabled enforcing mode
- the user does not trust our copy of the certs
- the user does not download the target distro certs for himself
I think it would take all four of those conditions for there to be a
difference between having the elfutils copy of these distro certs be
installed or not.
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. If we do not
distribute certificates, who would? Distros may post their own on
their web sites, or include them in some packages. But no single
distro is likely to go to extra effort to package -other distro's-
certificates. Nor necessarily will any distro pack all their own
certificates (for versions old & new) into a standard package where
debuginfod-client can find them. We are uniquely positioned to
"federate" this aspect.
> > > [...]
> > > 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.)
> [...]
> > 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.
> 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?
> 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.
- FChE
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PR28204, debuginfod IMA
2023-10-27 19:15 ` Frank Ch. Eigler
@ 2023-10-31 13:20 ` Mark Wielaard
2023-10-31 15:46 ` Frank Ch. Eigler
0 siblings, 1 reply; 11+ messages in thread
From: Mark Wielaard @ 2023-10-31 13:20 UTC (permalink / raw)
To: Frank Ch. Eigler; +Cc: elfutils-devel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PR28204, debuginfod IMA
2023-10-31 13:20 ` Mark Wielaard
@ 2023-10-31 15:46 ` Frank Ch. Eigler
2023-11-01 14:59 ` Mark Wielaard
0 siblings, 1 reply; 11+ messages in thread
From: Frank Ch. Eigler @ 2023-10-31 15:46 UTC (permalink / raw)
To: Mark Wielaard; +Cc: elfutils-devel
Hi, Mark -
> > 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.
I see what you mean, but maybe that puts the cart ahead of the horse.
What's desirable is easy access to a runnable version of the patch,
not the choice of command line tooling to do that. A published git
branch containing the same patch is even simpler than using git
am/b4/pw. git is the most convenient transport protocol for patches.
> 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. [...]
In the current version of the users/fche/try-bz28204 branch, this
has been renamed "optimistic", and the man page blurb now says:
\fIima:optimistic\fP Every downloaded file with a known-invalid
signature is rejected, protecting against some types of corruption.
> > 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 sign our elfutils releases, and packagers often sign their builds
of our releases, which users can verify.
> So you propose we setup a curating process to decide which certificates
> to include?
Sure. We already curate a set of debuginfod servers.
> If we do then I would suggest we create a separate package for that
> (or just a separate tar ball or repository). [...]
Another compromise possibility is to keep shipping these certificates,
but not include them in the default $DEBUGINFOD_IMA_CERT_PATH. A user
wishing to trust our curation can add /etc/debuginfod/ima-certs.
> > > 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.
It's a brief fingerprint/hash of the key, convenient for
identification in diagnostics.
> > > [...]
> > > > 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.
An elfutils build with --enable-sanitize-undefined had no complaints.
> > 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. [...]
OK, added a mutex around the entire function.
> 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?
Ugh. I don't know of an alternative. There isn't an equivalent
command line wrapping of the library either (with respect to
certificate searching) that we could fork out to. (OTOH, GPLv2 is
compatible with GPLv2+.)
> > 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. [...]
Interesting issue. One openssl initialization call is deep inside
libcurl, another one in libimaevm's solib ctor. Neither is
parametrized such that we could influence them. However, things are
working(tm).
- FChE
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PR28204, debuginfod IMA
2023-10-31 15:46 ` Frank Ch. Eigler
@ 2023-11-01 14:59 ` Mark Wielaard
2023-11-14 16:45 ` Frank Ch. Eigler
0 siblings, 1 reply; 11+ messages in thread
From: Mark Wielaard @ 2023-11-01 14:59 UTC (permalink / raw)
To: Frank Ch. Eigler; +Cc: elfutils-devel
Hi Frank,
On Tue, 2023-10-31 at 11:46 -0400, Frank Ch. Eigler wrote:
> > 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.
>
> I see what you mean, but maybe that puts the cart ahead of the horse.
> What's desirable is easy access to a runnable version of the patch,
> not the choice of command line tooling to do that. A published git
> branch containing the same patch is even simpler than using git
> am/b4/pw. git is the most convenient transport protocol for patches.
Since we are discussing/reviewing patches on the mailinglist I think it
makes sense posting them in a form that makes them easy to apply. That
can be addition to some other way to also publish them.
> > 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. [...]
>
> In the current version of the users/fche/try-bz28204 branch, this
> has been renamed "optimistic", and the man page blurb now says:
>
> \fIima:optimistic\fP Every downloaded file with a known-invalid
> signature is rejected, protecting against some types of corruption.
I like this wording more. But maybe it would be helpful to split the
patch into one that implements ima:enforcing and another that adds the
ima:optimistic idea. That way we can more easily get the code in that
we seem to agree on. And it makes it more clear what extra code is
needed for the other policies.
> > > 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 sign our elfutils releases, and packagers often sign their builds
> of our releases, which users can verify.
Right, but those are files we write and distribute. These are
certificates that other use to sign files they distribute.
> > So you propose we setup a curating process to decide which certificates
> > to include?
>
> Sure. We already curate a set of debuginfod servers.
You mean the list of servers that https://debuginfod.elfutils.org/
federates? I don't mind that server also maintaining a list of ima
certs for those servers. But make sure the distros actually want
someone else to redistribute their certificates. I can imagine distros
wanting people to get their certificates from them directly.
I don't think we should redistribute them as part of the main elfutils
package though.
>
> > > > [...]
> > > > > 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.
>
> An elfutils build with --enable-sanitize-undefined had no complaints.
Good. I still don't like the code, but at least it seems the compiler
sees it as something that should work.
> > 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?
>
> Ugh. I don't know of an alternative. There isn't an equivalent
> command line wrapping of the library either (with respect to
> certificate searching) that we could fork out to. (OTOH, GPLv2 is
> compatible with GPLv2+.)
But GPLv2-only is not compatible with GPLv3 which is used by e.g. gdb.
This is a bit of a pickle :{
> > > 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. [...]
>
> Interesting issue. One openssl initialization call is deep inside
> libcurl, another one in libimaevm's solib ctor. Neither is
> parametrized such that we could influence them. However, things are
> working(tm).
Thanks for checking.
Cheers,
Mark
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] PR28204, debuginfod IMA
2023-11-01 14:59 ` Mark Wielaard
@ 2023-11-14 16:45 ` Frank Ch. Eigler
2023-11-15 16:00 ` Mark Wielaard
0 siblings, 1 reply; 11+ messages in thread
From: Frank Ch. Eigler @ 2023-11-14 16:45 UTC (permalink / raw)
To: Mark Wielaard; +Cc: elfutils-devel
Hi -
> > \fIima:optimistic\fP Every downloaded file with a known-invalid
> > signature is rejected, protecting against some types of corruption.
>
> I like this wording more. But maybe it would be helpful to split the
> patch into one that implements ima:enforcing and another that adds the
> ima:optimistic idea. That way we can more easily get the code in that
> we seem to agree on. And it makes it more clear what extra code is
> needed for the other policies.
I interpret this as a veto for the "optimistic" mode. Too bad, this
is going to reduce usability and utility. What mode do you want by
default then, "ignore" or "enforcing"?
> > We sign our elfutils releases, and packagers often sign their builds
> > of our releases, which users can verify.
>
> Right, but those are files we write and distribute. These are
> certificates that other use to sign files they distribute.
They use unpublished private keys to sign files. Public certificates
allow anyone to verify the signatures.
> > > So you propose we setup a curating process to decide which certificates
> > > to include?
> >
> > Sure. We already curate a set of debuginfod servers.
> You mean the list of servers that https://debuginfod.elfutils.org/
> federates? I don't mind that server also maintaining a list of ima
> certs for those servers.
I assume you mean to require users to manually download & install them
somehow.
> But make sure the distros actually want someone else to redistribute
> their certificates. I can imagine distros wanting people to get
> their certificates from them directly.
Certificates are public keys. They are literally designed for wide
distribution. They may be authenticated further by certificate
chains, but there is little harm to even crafted certificates. They
would not be usable to verify signatures, leaving the user no worse
off than without the certs.
> I don't think we should redistribute them as part of the main elfutils
> package though.
I interpret that as a veto.
> > Ugh. I don't know of an alternative. There isn't an equivalent
> > command line wrapping of the library either (with respect to
> > certificate searching) that we could fork out to. (OTOH, GPLv2 is
> > compatible with GPLv2+.)
>
> But GPLv2-only is not compatible with GPLv3 which is used by e.g. gdb.
> This is a bit of a pickle :{
I interpret that as a veto. OK, will have to set time aside to
rewrite this code.
- FChE
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] PR28204, debuginfod IMA
2023-11-14 16:45 ` Frank Ch. Eigler
@ 2023-11-15 16:00 ` Mark Wielaard
0 siblings, 0 replies; 11+ messages in thread
From: Mark Wielaard @ 2023-11-15 16:00 UTC (permalink / raw)
To: Frank Ch. Eigler; +Cc: elfutils-devel
Hi Frank,
On Tue, 2023-11-14 at 11:45 -0500, Frank Ch. Eigler wrote:
> > > \fIima:optimistic\fP Every downloaded file with a known-invalid
> > > signature is rejected, protecting against some types of corruption.
> >
> > I like this wording more. But maybe it would be helpful to split the
> > patch into one that implements ima:enforcing and another that adds the
> > ima:optimistic idea. That way we can more easily get the code in that
> > we seem to agree on. And it makes it more clear what extra code is
> > needed for the other policies.
>
> I interpret this as a veto for the "optimistic" mode. Too bad, this
> is going to reduce usability and utility. What mode do you want by
> default then, "ignore" or "enforcing"?
What I am proposing we do first is to add the code for enforcing, which
we seem to agree upon is useful and fairly simple to understand. Once
that is in we can see how we can extend it to also have an "optimistic"
mode. Because it seems we don't seem have consensus on what the exact
semantics are or should be for this.
> > > We sign our elfutils releases, and packagers often sign their builds
> > > of our releases, which users can verify.
> >
> > Right, but those are files we write and distribute. These are
> > certificates that other use to sign files they distribute.
>
> They use unpublished private keys to sign files. Public certificates
> allow anyone to verify the signatures.
Agreed.
> > > > So you propose we setup a curating process to decide which certificates
> > > > to include?
> > >
> > > Sure. We already curate a set of debuginfod servers.
>
> > You mean the list of servers that https://debuginfod.elfutils.org/
> > federates? I don't mind that server also maintaining a list of ima
> > certs for those servers.
>
> I assume you mean to require users to manually download & install them
> somehow.
Yes. I assume a distro maintainer will make sure the default
DEBUGINFOD_URLS and associated distro ima certificates are setup. But
that the user will explicitly setup other debuginfod servers (and/or
certificates for non-distro signed package files).
> > But make sure the distros actually want someone else to redistribute
> > their certificates. I can imagine distros wanting people to get
> > their certificates from them directly.
>
> Certificates are public keys. They are literally designed for wide
> distribution. They may be authenticated further by certificate
> chains, but there is little harm to even crafted certificates. They
> would not be usable to verify signatures, leaving the user no worse
> off than without the certs.
>
> > I don't think we should redistribute them as part of the main elfutils
> > package though.
>
> I interpret that as a veto.
More like a suggestion to collect these certificates and distribute
them in their own package or tar from debuginfod.elfutils.org.
Redistributing certificates seems something separate from the main code
package. Just like debuginfod.elfutils.org lists various debuginfod
server URLs that users could use.
> > > Ugh. I don't know of an alternative. There isn't an equivalent
> > > command line wrapping of the library either (with respect to
> > > certificate searching) that we could fork out to. (OTOH, GPLv2 is
> > > compatible with GPLv2+.)
> >
> > But GPLv2-only is not compatible with GPLv3 which is used by e.g. gdb.
> > This is a bit of a pickle :{
>
> I interpret that as a veto. OK, will have to set time aside to
> rewrite this code.
Yeah it is somewhat unfortunate this code is GPLv2-only. But it also
doesn't really look like it was meant to be used as a generic library.
If it was maybe you can ask the copyright holders to use LGPLv2+ or
similar so it can be used with a (L)GPLv3+ code base?
Cheers,
Mark
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PR28204, debuginfod IMA
2023-09-07 12:55 [PATCH] PR28204, debuginfod IMA Frank Ch. Eigler
2023-10-23 19:33 ` Mark Wielaard
@ 2023-10-24 15:25 ` Mark Wielaard
1 sibling, 0 replies; 11+ messages in thread
From: Mark Wielaard @ 2023-10-24 15:25 UTC (permalink / raw)
To: Frank Ch. Eigler, elfutils-devel
Hi,
Continued review...
On Thu, 2023-09-07 at 08:55 -0400, Frank Ch. Eigler via Elfutils-devel
wrote:
> diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
> index d72d2ad16960..8c3298586672 100644
> --- a/debuginfod/debuginfod.cxx
> +++ b/debuginfod/debuginfod.cxx
> @@ -113,6 +113,13 @@ using namespace std;
> #define MHD_RESULT int
> #endif
>
> +#ifdef ENABLE_IMA_VERIFICATION
> + #include <rpm/rpmlib.h>
> + #include <rpm/rpmfi.h>
> + #include <rpm/header.h>
> + #include <glob.h>
> +#endif
> +
> #include <curl/curl.h>
> #include <archive.h>
> #include <archive_entry.h>
OK.
> @@ -431,6 +438,8 @@ static const struct argp_option options[] =
> { "disable-source-scan", ARGP_KEY_DISABLE_SOURCE_SCAN, NULL, 0, "Do not scan dwarf source info.", 0 },
> #define ARGP_SCAN_CHECKPOINT 0x100A
> { "scan-checkpoint", ARGP_SCAN_CHECKPOINT, "NUM", 0, "Number of files scanned before a WAL checkpoint.", 0 },
> +#define ARGP_KEY_KOJI_SIGCACHE 0x100B
> + { "koji-sigcache", ARGP_KEY_KOJI_SIGCACHE, NULL, 0, "Do a koji specific mapping of rpm paths to get IMA signatures.", 0 },
> { NULL, 0, NULL, 0, NULL, 0 },
> };
OK. Is actually documented below. But first couldn't find it because of
the escaped '-' (good), which apparently is a real issue:
https://lwn.net/SubscriberLink/947941/b427a4409847108d/
> @@ -486,6 +495,7 @@ static bool scan_source_info = true;
> static string tmpdir;
> static bool passive_p = false;
> static long scan_checkpoint = 256;
> +static bool requires_koji_sigcache_mapping = false;
>
> static void set_metric(const string& key, double value);
> // static void inc_metric(const string& key);
> @@ -692,6 +702,9 @@ parse_opt (int key, char *arg,
> if (scan_checkpoint < 0)
> argp_failure(state, 1, EINVAL, "scan checkpoint");
> break;
> + case ARGP_KEY_KOJI_SIGCACHE:
> + requires_koji_sigcache_mapping = true;
> + break;
> // case 'h': argp_state_help (state, stderr, ARGP_HELP_LONG|ARGP_HELP_EXIT_OK);
> default: return ARGP_ERR_UNKNOWN;
> }
OK. Maybe produce a warning/error ifndef ENABLE_IMA_VERIFICATION ?
> @@ -1933,6 +1946,131 @@ handle_buildid_r_match (bool internal_req_p,
> return 0;
> }
>
> + // Extract the IMA per-file signature (if it exists)
> + string ima_sig = "";
> + #ifdef ENABLE_IMA_VERIFICATION
> + do
> + {
> + FD_t rpm_fd;
> + if(!(rpm_fd = Fopen(b_source0.c_str(), "r.ufdio")))
> + {
> + if (verbose) obatched(clog) << "There was an error while opening " << b_source0 << endl;
> + break; // Exit IMA extraction
> + }
So Fopen is part of rpm devel? Is there documentation?
Specifically what "r.ufdio" means?
> + Header rpm_hdr;
> + if(RPMRC_FAIL == rpmReadPackageFile(NULL, rpm_fd, b_source0.c_str(), &rpm_hdr))
> + {
> + if (verbose) obatched(clog) << "There was an error while reading the header of " << b_source0 << endl;
> + Fclose(rpm_fd);
> + break; // Exit IMA extraction
> + }
OK.
> + // Fill sig_tag_data with an alloc'd copy of the array of IMA signatures (if they exist)
> + struct rpmtd_s sig_tag_data;
> + rpmtdReset(&sig_tag_data);
> + do{ /* A do-while so we can break out of the koji sigcache checking on failure */
> + if(requires_koji_sigcache_mapping)
> + {
> + /* NB: Koji builds result in a directory structure like the following
> + - PACKAGE/VERSION/RELEASE
> + - ARCH1
> + - foo.rpm // The rpm known by debuginfod
> + - ...
> + - ARCHN
> + - data
> + - signed // Periodically purged (and not scanned by debuginfod)
> + - sigcache
> + - ARCH1
> + - foo.rpm.sig // An empty rpm header
> + - ...
> + - ARCHN
> + - PACKAGE_KEYID1
> + - ARCH1
> + - foo.rpm.sig // The header of the signed rpm. This is the file we need to extract the IMA signatures
> + - ...
> + - ARCHN
> + - ...
> + - PACKAGE_KEYIDN
> + We therefore need to do a mapping of P/V/R/A/N-V-R.A.rpm -> P/V/R/data/sigcache/KEYID/A/N-V-R.A.rpm.sig. There are 2 key insights here
> + 1. We need to go 2 directories down from sigcache to get to the rpm header. So to distinguish ARCH1/foo.rpm.sig and PACKAGE_KEYID1/ARCH1/foo.rpm.sig
> + we can look 2 directories down
> + 2. Its safe to assume that the user will have all of the required verification certs. So we can pick from any of the PACKAGE_KEYIDi directories.
> + For simplicity we choose first we match against
> + See: https://pagure.io/koji/issue/3670
> + */
Nice comment, thanks for the bug reference.
Note tiny typo "PACKAGE_KEYIDi" (extra 'i').
> + // Do the mapping from b_source0 to the koji path for the signed rpm header
> + string signed_rpm_path = b_source0;
> + size_t insert_pos = string::npos;
> + for(int i = 0; i < 2; i++) insert_pos = signed_rpm_path.rfind("/", insert_pos) - 1;
> + string globbed_path = signed_rpm_path.insert(insert_pos + 1, "/data/sigcache/*").append(".sig"); // The globbed path we're seeking
> + glob_t pglob;
> + int grc;
> + if(0 != (grc = glob(globbed_path.c_str(), GLOB_NOSORT, NULL, &pglob)))
> + {
> + // Break out, but only report real errors
> + if (verbose && grc != GLOB_NOMATCH) obatched(clog) << "There was an error (" << strerror(errno) << ") globbing " << globbed_path << endl;
> + break; // Exit koji sigcache check
> + }
> + signed_rpm_path = pglob.gl_pathv[0]; // See insight 2 above
> + globfree(&pglob);
> +
> + if (verbose > 2) obatched(clog) << "attempting IMA signature extraction from koji header " << signed_rpm_path << endl;
> +
> + FD_t sig_rpm_fd;
> + if(NULL == (sig_rpm_fd = Fopen(signed_rpm_path.c_str(), "r")))
> + {
> + if (verbose) obatched(clog) << "There was an error while opening " << signed_rpm_path << endl;
> + break; // Exit koji sigcache check
> + }
> +
> + Header sig_hdr = headerRead(sig_rpm_fd, HEADER_MAGIC_YES /* Validate magic too */ );
> + if (!sig_hdr || 1 != headerGet(sig_hdr, RPMSIGTAG_FILESIGNATURES, &sig_tag_data, HEADERGET_ALLOC))
> + {
> + if (verbose) obatched(clog) << "Unable to extract RPMSIGTAG_FILESIGNATURES from " << signed_rpm_path << endl;
> + }
> + headerFree(sig_hdr); // We can free here since sig_tag_data has an alloc'd copy of the data
> + Fclose(sig_rpm_fd);
> + }
> + }while(false);
> +
> + if(0 == sig_tag_data.count)
> + {
> + // In the general case (or a fallback from the koji sigcache mapping not finding signatures)
> + // we can just (try) extract the signatures from the rpm header
> + if (1 != headerGet(rpm_hdr, RPMTAG_FILESIGNATURES, &sig_tag_data, HEADERGET_ALLOC))
> + {
> + if (verbose) obatched(clog) << "Unable to extract RPMTAG_FILESIGNATURES from " << b_source0 << endl;
> + }
> + }
OK. A bit convoluted, but that is just rpm being rpm.
> + // Search the array for the signature coresponding to b_source1
> + int idx = -1;
> + char *sig = NULL;
> + rpmfi hdr_fi = rpmfiNew(NULL, rpm_hdr, RPMTAG_BASENAMES, RPMFI_FLAGS_QUERY);
> + for(sig = (char*)rpmtdNextString(&sig_tag_data), idx = rpmfiNext(hdr_fi);
> + idx != -1 && 0 != strcmp(b_source1.c_str(), rpmfiFN(hdr_fi));
> + sig = (char*)rpmtdNextString(&sig_tag_data), idx = rpmfiNext(hdr_fi));
> + rpmfiFree(hdr_fi);
That is a complicated for statement, not easily understood. Partially
because of indentation. Advise to at least put ';' on its own line.
> + if(sig && 0 != strlen(sig) && idx != -1)
> + {
> + if (verbose > 2) obatched(clog) << "Found IMA signature for " << b_source1 << ":\n" << sig << endl;
> + ima_sig = sig;
> + inc_metric("http_responses_total","extra","ima-sigs-extracted");
> + }
> + else
> + {
> + if (verbose > 2) obatched(clog) << "Could not find IMA signature for " << b_source1 << endl;
> + }
> +
> + rpmtdFreeData (&sig_tag_data);
> + headerFree(rpm_hdr);
> + Fclose(rpm_fd);
> + }
> + while(false);
> + #endif
OK.
BTW. Shouldn't we cache this image_sig somewhere?
It seems a lot of work to do each time handle_buildid_r_match is
called.
> // check for a match in the fdcache first
> int fd = fdcache.lookup(b_source0, b_source1);
> while (fd >= 0) // got one!; NB: this is really an if() with a possible branch out to the end
> @@ -1990,11 +2128,13 @@ handle_buildid_r_match (bool internal_req_p,
> to_string(fs.st_size).c_str());
> add_mhd_response_header (r, "X-DEBUGINFOD-ARCHIVE", b_source0.c_str());
> add_mhd_response_header (r, "X-DEBUGINFOD-FILE", b_source1.c_str());
> + if(!ima_sig.empty()) add_mhd_response_header(r, "X-DEBUGINFOD-IMASIGNATURE", ima_sig.c_str());
> add_mhd_last_modified (r, fs.st_mtime);
> if (verbose > 1)
> obatched(clog) << "serving fdcache archive " << b_source0
> << " file " << b_source1
> - << " section=" << section << endl;
> + << " section=" << section
> + << " IMA signature=" << ima_sig << endl;
> /* libmicrohttpd will close it. */
> if (result_fd)
> *result_fd = fd;
> @@ -2174,11 +2314,13 @@ handle_buildid_r_match (bool internal_req_p,
> add_mhd_response_header (r, "X-DEBUGINFOD-ARCHIVE",
> b_source0.c_str());
> add_mhd_response_header (r, "X-DEBUGINFOD-FILE", file.c_str());
> + if(!ima_sig.empty()) add_mhd_response_header(r, "X-DEBUGINFOD-IMASIGNATURE", ima_sig.c_str());
> add_mhd_last_modified (r, archive_entry_mtime(e));
> if (verbose > 1)
> obatched(clog) << "serving archive " << b_source0
> << " file " << b_source1
> - << " section=" << section << endl;
> + << " section=" << section
> + << " IMA signature=" << ima_sig << endl;
> /* libmicrohttpd will close it. */
> if (result_fd)
> *result_fd = fd;
OK.
> diff --git a/debuginfod/debuginfod.h.in b/debuginfod/debuginfod.h.in
> index 4a256ba9af1f..73f633f0b8e9 100644
> --- a/debuginfod/debuginfod.h.in
> +++ b/debuginfod/debuginfod.h.in
> @@ -39,6 +39,7 @@
> #define DEBUGINFOD_MAXSIZE_ENV_VAR "DEBUGINFOD_MAXSIZE"
> #define DEBUGINFOD_MAXTIME_ENV_VAR "DEBUGINFOD_MAXTIME"
> #define DEBUGINFOD_HEADERS_FILE_ENV_VAR "DEBUGINFOD_HEADERS_FILE"
> +#define DEBUGINFOD_IMA_CERT_PATH_ENV_VAR "DEBUGINFOD_IMA_CERT_PATH"
>
> /* The libdebuginfod soname. */
> #define DEBUGINFOD_SONAME "@LIBDEBUGINFOD_SONAME@"
OK.
> diff --git a/debuginfod/ima-certs/centosimabeta9.crt b/debuginfod/ima-certs/centosimabeta9.crt
> new file mode 100644
> index 000000000000..534f86a15ac5
> --- /dev/null
> +++ b/debuginfod/ima-certs/centosimabeta9.crt
> @@ -0,0 +1,70 @@
> +Certificate:
> + Data:
> + Version: 3 (0x2)
> + Serial Number:
> + d6:78:a4:e3:e7:d3:c5:aa
> + Signature Algorithm: sha256WithRSAEncryption
> + Issuer: O=RH-IMA-CA, CN=Red Hat IMA CA/emailAddress=secalert@redhat.com
> + Validity
> + Not Before: Jul 1 16:14:28 2023 GMT
> + Not After : Jan 18 16:14:28 2038 GMT
> + Subject: CN=CentOS IMA beta key (for verification)
> + Subject Public Key Info:
> + Public Key Algorithm: id-ecPublicKey
> + Public-Key: (384 bit)
> + pub:
> + 04:6c:6d:44:6d:eb:55:31:56:11:ab:a7:dc:cc:34:
> + 72:9c:9b:05:5c:d8:28:36:c8:04:05:5a:c8:33:b5:
> + ba:de:ba:f5:0d:a9:1f:02:c1:43:78:e8:0a:2f:05:
> + e9:a9:ce:c4:24:5b:d6:b2:ce:cf:06:52:7a:7a:d9:
> + 3f:dd:b3:09:b1:22:90:e4:95:4e:07:80:f4:9c:73:
> + e9:6c:00:09:ed:b1:68:76:c5:59:7e:74:a4:c3:cf:
> + a1:84:20:51:f6:29:2e
> + ASN1 OID: secp384r1
> + NIST CURVE: P-384
> + X509v3 extensions:
> + X509v3 Basic Constraints: critical
> + CA:FALSE
> + X509v3 Key Usage: critical
> + Digital Signature
> + X509v3 Extended Key Usage: critical
> + Code Signing
> + X509v3 Subject Key Identifier:
> + 61:BA:CE:C0:D7:97:EA:49:9F:D8:92:79:41:57:19:0A:D6:04:63:E1
> + X509v3 Authority Key Identifier:
> + keyid:FB:31:82:5D:D0:E0:73:68:5B:26:4E:30:38:96:36:73:F7:53:95:9A
> +
> + Signature Algorithm: sha256WithRSAEncryption
> + 8b:28:5a:ca:d3:2a:ec:e2:c6:89:f8:4d:e9:ff:c8:31:47:7b:
> + 93:17:c1:55:48:40:ce:be:1c:58:ff:6a:83:55:07:72:70:c7:
> + a3:25:91:70:90:ab:9d:bc:d3:63:4e:aa:f2:2f:ee:0c:05:a5:
> + 8e:2e:98:7b:41:44:37:66:87:80:94:56:b1:1f:13:99:d0:80:
> + 0b:e8:ec:50:cf:8c:90:53:cb:b1:9c:35:cf:b7:75:1f:ad:77:
> + 52:d2:06:56:7a:37:16:fe:bb:c2:aa:de:72:5d:b3:4b:9f:d3:
> + 05:c6:4a:36:d2:14:6b:9a:b3:64:62:4b:d5:9d:aa:71:ab:11:
> + c0:08:17:e1:f9:88:49:c7:6f:74:ff:02:04:ee:cd:a2:75:5b:
> + 79:19:ed:ad:14:71:b4:19:56:ed:fe:7b:61:7e:d4:97:b5:c7:
> + a5:05:41:4c:9f:17:91:71:71:bc:12:2b:d8:6c:2b:c0:02:93:
> + ed:28:30:9b:7f:a1:93:8c:32:74:fe:6c:c0:ab:68:5e:e5:3e:
> + 22:c9:db:d4:4e:b2:5c:d6:49:e7:1d:91:78:61:3c:ca:5e:72:
> + 63:74:ff:af:0d:0d:32:ae:2a:cd:9b:a1:30:95:ea:33:53:64:
> + 2b:13:2d:96:e3:b6:6d:f2:c2:3c:24:c8:6b:87:f8:b4:7f:72:
> + 30:66:f8:5d
> +-----BEGIN CERTIFICATE-----
> +MIICzzCCAbegAwIBAgIJANZ4pOPn08WqMA0GCSqGSIb3DQEBCwUAMFExEjAQBgNV
> +BAoMCVJILUlNQS1DQTEXMBUGA1UEAwwOUmVkIEhhdCBJTUEgQ0ExIjAgBgkqhkiG
> +9w0BCQEWE3NlY2FsZXJ0QHJlZGhhdC5jb20wHhcNMjMwNzAxMTYxNDI4WhcNMzgw
> +MTE4MTYxNDI4WjAxMS8wLQYDVQQDDCZDZW50T1MgSU1BIGJldGEga2V5IChmb3Ig
> +dmVyaWZpY2F0aW9uKTB2MBAGByqGSM49AgEGBSuBBAAiA2IABGxtRG3rVTFWEaun
> +3Mw0cpybBVzYKDbIBAVayDO1ut669Q2pHwLBQ3joCi8F6anOxCRb1rLOzwZSenrZ
> +P92zCbEikOSVTgeA9Jxz6WwACe2xaHbFWX50pMPPoYQgUfYpLqN4MHYwDAYDVR0T
> +AQH/BAIwADAOBgNVHQ8BAf8EBAMCB4AwFgYDVR0lAQH/BAwwCgYIKwYBBQUHAwMw
> +HQYDVR0OBBYEFGG6zsDXl+pJn9iSeUFXGQrWBGPhMB8GA1UdIwQYMBaAFPsxgl3Q
> +4HNoWyZOMDiWNnP3U5WaMA0GCSqGSIb3DQEBCwUAA4IBAQCLKFrK0yrs4saJ+E3p
> +/8gxR3uTF8FVSEDOvhxY/2qDVQdycMejJZFwkKudvNNjTqryL+4MBaWOLph7QUQ3
> +ZoeAlFaxHxOZ0IAL6OxQz4yQU8uxnDXPt3UfrXdS0gZWejcW/rvCqt5yXbNLn9MF
> +xko20hRrmrNkYkvVnapxqxHACBfh+YhJx290/wIE7s2idVt5Ge2tFHG0GVbt/nth
> +ftSXtcelBUFMnxeRcXG8EivYbCvAApPtKDCbf6GTjDJ0/mzAq2he5T4iydvUTrJc
> +1knnHZF4YTzKXnJjdP+vDQ0yrirNm6EwleozU2QrEy2W47Zt8sI8JMhrh/i0f3Iw
> +Zvhd
> +-----END CERTIFICATE-----
> diff --git a/debuginfod/ima-certs/centosimarelease9.crt b/debuginfod/ima-certs/centosimarelease9.crt
> new file mode 100644
> index 000000000000..db641453260f
> --- /dev/null
> +++ b/debuginfod/ima-certs/centosimarelease9.crt
> @@ -0,0 +1,70 @@
> +Certificate:
> + Data:
> + Version: 3 (0x2)
> + Serial Number:
> + d6:78:a4:e3:e7:d3:c5:ab
> + Signature Algorithm: sha256WithRSAEncryption
> + Issuer: O=RH-IMA-CA, CN=Red Hat IMA CA/emailAddress=secalert@redhat.com
> + Validity
> + Not Before: Jul 1 16:14:51 2023 GMT
> + Not After : Jan 18 16:14:51 2038 GMT
> + Subject: CN=CentOS IMA release key (for verification)
> + Subject Public Key Info:
> + Public Key Algorithm: id-ecPublicKey
> + Public-Key: (384 bit)
> + pub:
> + 04:d4:d0:31:08:09:0d:97:d0:5c:c8:49:ff:90:f4:
> + 3a:16:85:a3:73:a1:d9:c4:28:4c:f7:aa:a8:22:c2:
> + cf:0e:8b:d7:9a:ed:e6:f0:89:f8:85:95:72:c3:38:
> + 27:2a:29:97:6a:6b:2b:01:04:a3:32:ba:f4:75:f9:
> + e4:c8:48:2f:f5:36:69:44:27:f9:35:b3:0c:c3:22:
> + 24:67:51:06:d3:73:f1:56:94:20:a8:8c:82:34:c0:
> + 10:ef:ce:f9:b4:7a:42
> + ASN1 OID: secp384r1
> + NIST CURVE: P-384
> + X509v3 extensions:
> + X509v3 Basic Constraints: critical
> + CA:FALSE
> + X509v3 Key Usage: critical
> + Digital Signature
> + X509v3 Extended Key Usage: critical
> + Code Signing
> + X509v3 Subject Key Identifier:
> + 54:E5:A3:4F:16:2B:32:B7:77:FF:E3:4F:1E:8B:66:12:7C:43:5B:B5
> + X509v3 Authority Key Identifier:
> + keyid:FB:31:82:5D:D0:E0:73:68:5B:26:4E:30:38:96:36:73:F7:53:95:9A
> +
> + Signature Algorithm: sha256WithRSAEncryption
> + c6:1d:92:0e:92:40:d6:ae:a5:5d:4e:5d:2a:e1:0f:92:42:20:
> + 89:e1:a9:82:87:35:42:c9:7f:77:dd:19:e3:cf:ef:be:8b:39:
> + 4f:99:2e:cd:cc:a3:18:23:7f:81:4b:7d:63:5d:71:b4:4b:9c:
> + ea:dc:2f:1d:16:da:4c:ed:98:bf:df:88:11:d0:8b:af:01:55:
> + 71:05:fe:d7:ac:78:4e:46:de:48:9f:04:74:42:c2:c8:1a:fc:
> + c5:46:6a:99:3e:9a:b0:e4:04:07:48:e2:4c:65:e5:01:a8:ad:
> + 3c:8d:c0:ca:c5:73:23:36:88:27:54:8b:90:f8:ea:55:fc:eb:
> + b8:69:a5:8b:a0:1d:8b:f1:93:dd:71:9e:e9:88:f0:2d:0e:7d:
> + 86:a4:8d:0b:fd:00:c9:c0:73:aa:b1:65:b1:60:6e:a4:09:1b:
> + 3e:30:d9:62:2a:15:d6:50:2a:6a:fd:24:e7:8c:93:78:4a:28:
> + d5:b1:d9:ba:1b:8d:ef:48:0d:f4:8c:79:90:0f:95:8d:79:39:
> + 8d:41:a5:fc:6f:e4:ef:5c:ee:3b:f4:c3:2c:c3:a0:b7:61:ac:
> + 7e:e9:eb:a0:3a:ba:05:2c:bd:aa:a9:1f:c5:b9:ee:72:f6:c4:
> + 54:1f:71:3b:e1:70:1a:30:f4:04:18:50:60:c4:5a:da:93:cd:
> + b6:f6:67:c8
> +-----BEGIN CERTIFICATE-----
> +MIIC0jCCAbqgAwIBAgIJANZ4pOPn08WrMA0GCSqGSIb3DQEBCwUAMFExEjAQBgNV
> +BAoMCVJILUlNQS1DQTEXMBUGA1UEAwwOUmVkIEhhdCBJTUEgQ0ExIjAgBgkqhkiG
> +9w0BCQEWE3NlY2FsZXJ0QHJlZGhhdC5jb20wHhcNMjMwNzAxMTYxNDUxWhcNMzgw
> +MTE4MTYxNDUxWjA0MTIwMAYDVQQDDClDZW50T1MgSU1BIHJlbGVhc2Uga2V5IChm
> +b3IgdmVyaWZpY2F0aW9uKTB2MBAGByqGSM49AgEGBSuBBAAiA2IABNTQMQgJDZfQ
> +XMhJ/5D0OhaFo3Oh2cQoTPeqqCLCzw6L15rt5vCJ+IWVcsM4Jyopl2prKwEEozK6
> +9HX55MhIL/U2aUQn+TWzDMMiJGdRBtNz8VaUIKiMgjTAEO/O+bR6QqN4MHYwDAYD
> +VR0TAQH/BAIwADAOBgNVHQ8BAf8EBAMCB4AwFgYDVR0lAQH/BAwwCgYIKwYBBQUH
> +AwMwHQYDVR0OBBYEFFTlo08WKzK3d//jTx6LZhJ8Q1u1MB8GA1UdIwQYMBaAFPsx
> +gl3Q4HNoWyZOMDiWNnP3U5WaMA0GCSqGSIb3DQEBCwUAA4IBAQDGHZIOkkDWrqVd
> +Tl0q4Q+SQiCJ4amChzVCyX933Rnjz+++izlPmS7NzKMYI3+BS31jXXG0S5zq3C8d
> +FtpM7Zi/34gR0IuvAVVxBf7XrHhORt5InwR0QsLIGvzFRmqZPpqw5AQHSOJMZeUB
> +qK08jcDKxXMjNognVIuQ+OpV/Ou4aaWLoB2L8ZPdcZ7piPAtDn2GpI0L/QDJwHOq
> +sWWxYG6kCRs+MNliKhXWUCpq/STnjJN4SijVsdm6G43vSA30jHmQD5WNeTmNQaX8
> +b+TvXO479MMsw6C3Yax+6eugOroFLL2qqR/Fue5y9sRUH3E74XAaMPQEGFBgxFra
> +k8229mfI
> +-----END CERTIFICATE-----
> diff --git a/debuginfod/ima-certs/fedora-38-ima.der b/debuginfod/ima-certs/fedora-38-ima.der
> new file mode 100644
> index 000000000000..238ae6c30763
> Binary files /dev/null and b/debuginfod/ima-certs/fedora-38-ima.der differ
> diff --git a/debuginfod/ima-certs/fedora-39-ima.der b/debuginfod/ima-certs/fedora-39-ima.der
> new file mode 100644
> index 000000000000..0d13baa62d75
> Binary files /dev/null and b/debuginfod/ima-certs/fedora-39-ima.der differ
> diff --git a/debuginfod/ima-certs/redhatimabeta9.crt b/debuginfod/ima-certs/redhatimabeta9.crt
> new file mode 100644
> index 000000000000..4a3b6701d6c4
> --- /dev/null
> +++ b/debuginfod/ima-certs/redhatimabeta9.crt
> @@ -0,0 +1,70 @@
> +Certificate:
> + Data:
> + Version: 3 (0x2)
> + Serial Number:
> + d6:78:a4:e3:e7:d3:c5:a8
> + Signature Algorithm: sha256WithRSAEncryption
> + Issuer: O=RH-IMA-CA, CN=Red Hat IMA CA/emailAddress=secalert@redhat.com
> + Validity
> + Not Before: Jul 1 16:13:38 2023 GMT
> + Not After : Jan 18 16:13:38 2038 GMT
> + Subject: CN=Red Hat IMA beta key (for verification)
> + Subject Public Key Info:
> + Public Key Algorithm: id-ecPublicKey
> + Public-Key: (384 bit)
> + pub:
> + 04:af:d0:5e:29:27:3a:26:05:27:74:1b:07:62:20:
> + c3:3e:40:43:73:3e:ad:e4:24:3e:b1:24:9d:72:81:
> + e3:f0:43:40:7b:82:52:dc:57:66:ed:92:af:79:c2:
> + 6b:09:31:bf:50:1b:76:24:b5:2f:8f:ce:9c:65:20:
> + d4:e2:3d:37:41:0d:5d:b7:78:af:8c:d3:71:b7:10:
> + c9:70:15:77:d0:8f:09:97:3f:7e:83:7d:67:98:8f:
> + cf:be:16:b5:6d:73:7d
> + ASN1 OID: secp384r1
> + NIST CURVE: P-384
> + X509v3 extensions:
> + X509v3 Basic Constraints: critical
> + CA:FALSE
> + X509v3 Key Usage: critical
> + Digital Signature
> + X509v3 Extended Key Usage: critical
> + Code Signing
> + X509v3 Subject Key Identifier:
> + 7D:45:10:63:D2:89:7B:E3:6D:A6:62:D6:6B:36:15:E5:F0:F2:9C:D0
> + X509v3 Authority Key Identifier:
> + keyid:FB:31:82:5D:D0:E0:73:68:5B:26:4E:30:38:96:36:73:F7:53:95:9A
> +
> + Signature Algorithm: sha256WithRSAEncryption
> + 68:9b:7d:29:e2:46:ed:03:6a:5d:d9:9e:f3:7a:0d:49:fa:22:
> + 40:02:3e:cb:af:a3:ee:aa:b5:43:fd:d9:1e:5b:82:89:e7:ad:
> + 48:03:d4:4c:50:7e:03:32:c3:eb:88:b8:b4:57:8c:1d:78:f9:
> + a4:ca:d2:55:78:a1:ec:67:c8:1b:90:40:be:98:5b:fd:77:91:
> + 6b:7c:ed:5f:e9:b0:af:6e:71:03:bc:03:5f:ad:f0:01:bc:24:
> + 0d:ea:1e:04:9e:10:ee:5c:97:17:49:3e:8e:7b:51:17:1a:e2:
> + 06:09:04:d4:6d:94:ea:30:57:fd:70:d0:56:6c:36:81:9f:81:
> + 75:25:98:34:9b:e8:03:14:b0:7c:8c:13:b5:59:0a:45:15:d8:
> + 75:c9:7a:01:a4:2d:71:a1:6d:53:b6:22:37:ab:98:d3:f0:76:
> + 28:8d:c3:3b:7d:c5:11:b1:89:7b:ac:41:e2:92:e9:38:4d:6d:
> + 07:d9:07:76:53:1a:87:ca:79:2e:5d:20:ec:8b:f2:55:56:dd:
> + b1:65:ec:4c:d7:86:7b:8e:9b:a7:7f:6d:6e:7a:3f:18:e3:e3:
> + e7:3f:1c:fb:44:ab:7e:0c:c2:1d:61:7c:92:73:07:a2:b4:5a:
> + 6b:45:64:dd:4d:c9:df:a3:e1:5f:39:db:36:f3:d0:ba:c6:be:
> + 97:3c:2f:e6
> +-----BEGIN CERTIFICATE-----
> +MIIC0DCCAbigAwIBAgIJANZ4pOPn08WoMA0GCSqGSIb3DQEBCwUAMFExEjAQBgNV
> +BAoMCVJILUlNQS1DQTEXMBUGA1UEAwwOUmVkIEhhdCBJTUEgQ0ExIjAgBgkqhkiG
> +9w0BCQEWE3NlY2FsZXJ0QHJlZGhhdC5jb20wHhcNMjMwNzAxMTYxMzM4WhcNMzgw
> +MTE4MTYxMzM4WjAyMTAwLgYDVQQDDCdSZWQgSGF0IElNQSBiZXRhIGtleSAoZm9y
> +IHZlcmlmaWNhdGlvbikwdjAQBgcqhkjOPQIBBgUrgQQAIgNiAASv0F4pJzomBSd0
> +GwdiIMM+QENzPq3kJD6xJJ1ygePwQ0B7glLcV2btkq95wmsJMb9QG3YktS+Pzpxl
> +INTiPTdBDV23eK+M03G3EMlwFXfQjwmXP36DfWeYj8++FrVtc32jeDB2MAwGA1Ud
> +EwEB/wQCMAAwDgYDVR0PAQH/BAQDAgeAMBYGA1UdJQEB/wQMMAoGCCsGAQUFBwMD
> +MB0GA1UdDgQWBBR9RRBj0ol7422mYtZrNhXl8PKc0DAfBgNVHSMEGDAWgBT7MYJd
> +0OBzaFsmTjA4ljZz91OVmjANBgkqhkiG9w0BAQsFAAOCAQEAaJt9KeJG7QNqXdme
> +83oNSfoiQAI+y6+j7qq1Q/3ZHluCieetSAPUTFB+AzLD64i4tFeMHXj5pMrSVXih
> +7GfIG5BAvphb/XeRa3ztX+mwr25xA7wDX63wAbwkDeoeBJ4Q7lyXF0k+jntRFxri
> +BgkE1G2U6jBX/XDQVmw2gZ+BdSWYNJvoAxSwfIwTtVkKRRXYdcl6AaQtcaFtU7Yi
> +N6uY0/B2KI3DO33FEbGJe6xB4pLpOE1tB9kHdlMah8p5Ll0g7IvyVVbdsWXsTNeG
> +e46bp39tbno/GOPj5z8c+0SrfgzCHWF8knMHorRaa0Vk3U3J36PhXznbNvPQusa+
> +lzwv5g==
> +-----END CERTIFICATE-----
> diff --git a/debuginfod/ima-certs/redhatimarelease9.crt b/debuginfod/ima-certs/redhatimarelease9.crt
> new file mode 100644
> index 000000000000..cd0b66494da2
> --- /dev/null
> +++ b/debuginfod/ima-certs/redhatimarelease9.crt
> @@ -0,0 +1,70 @@
> +Certificate:
> + Data:
> + Version: 3 (0x2)
> + Serial Number:
> + d6:78:a4:e3:e7:d3:c5:a9
> + Signature Algorithm: sha256WithRSAEncryption
> + Issuer: O=RH-IMA-CA, CN=Red Hat IMA CA/emailAddress=secalert@redhat.com
> + Validity
> + Not Before: Jul 1 16:14:04 2023 GMT
> + Not After : Jan 18 16:14:04 2038 GMT
> + Subject: CN=Red Hat IMA release key (for verification)
> + Subject Public Key Info:
> + Public Key Algorithm: id-ecPublicKey
> + Public-Key: (384 bit)
> + pub:
> + 04:4f:0e:ef:bf:e2:23:89:91:27:4e:7c:32:a1:d0:
> + c0:26:92:de:37:8d:b0:5d:ea:7f:d6:27:18:9b:b4:
> + 62:be:06:85:3d:f9:cc:47:7e:c7:bd:91:54:53:62:
> + b4:c0:8a:43:48:c2:59:07:2b:88:d7:3d:4b:30:8d:
> + 6c:32:fb:a5:da:dc:8a:85:e9:61:44:18:fc:d9:8b:
> + f5:5e:38:c8:85:77:ca:73:68:ce:48:df:af:3d:06:
> + 43:2f:4b:6c:0c:cd:88
> + ASN1 OID: secp384r1
> + NIST CURVE: P-384
> + X509v3 extensions:
> + X509v3 Basic Constraints: critical
> + CA:FALSE
> + X509v3 Key Usage: critical
> + Digital Signature
> + X509v3 Extended Key Usage: critical
> + Code Signing
> + X509v3 Subject Key Identifier:
> + 22:FA:01:DC:0E:A0:26:9F:69:A8:67:E5:CF:E4:9C:FB:D3:32:04:49
> + X509v3 Authority Key Identifier:
> + keyid:FB:31:82:5D:D0:E0:73:68:5B:26:4E:30:38:96:36:73:F7:53:95:9A
> +
> + Signature Algorithm: sha256WithRSAEncryption
> + 1a:1e:c1:2d:65:ad:f0:24:ec:9e:a7:fd:d4:ea:e1:54:dc:31:
> + 1c:62:8c:29:0b:7a:56:6e:f7:b4:87:92:3e:ff:d5:40:4b:24:
> + a1:68:6e:ee:9c:35:65:a1:3f:8e:f3:8b:9b:18:b1:03:ed:fb:
> + 50:2e:a3:23:d1:93:1d:d6:82:0a:10:6f:34:be:d6:3a:bd:76:
> + 8c:44:0e:ad:a7:2a:c4:8e:8d:c4:e4:8d:51:d8:26:b7:38:89:
> + d1:23:a0:23:88:76:fa:f1:27:91:57:3e:b2:0f:cf:73:53:db:
> + 20:40:5d:82:b9:e9:bc:a2:94:09:57:fb:85:0d:56:4b:dc:19:
> + 65:12:2f:6d:6a:3b:be:35:1f:d4:52:ea:e4:72:36:f9:fe:cb:
> + d4:1b:0f:e3:0e:88:7c:68:58:28:c3:06:5f:bd:d2:f9:2e:1a:
> + 30:f0:63:65:2d:55:e1:a4:fd:97:cf:ff:c0:52:22:1c:24:a3:
> + 6e:de:7a:c9:9d:75:d2:d0:82:b0:7f:6f:db:21:01:69:f0:54:
> + 76:04:19:68:2c:22:72:dd:3b:0d:04:d5:ad:5a:80:30:68:90:
> + 6e:c2:27:f4:28:af:1b:78:f6:0a:70:74:5c:3a:61:42:f5:63:
> + 7c:83:12:5a:1b:43:bc:d4:1b:28:b5:ef:98:c5:14:04:42:80:
> + dd:54:30:a4
> +-----BEGIN CERTIFICATE-----
> +MIIC0zCCAbugAwIBAgIJANZ4pOPn08WpMA0GCSqGSIb3DQEBCwUAMFExEjAQBgNV
> +BAoMCVJILUlNQS1DQTEXMBUGA1UEAwwOUmVkIEhhdCBJTUEgQ0ExIjAgBgkqhkiG
> +9w0BCQEWE3NlY2FsZXJ0QHJlZGhhdC5jb20wHhcNMjMwNzAxMTYxNDA0WhcNMzgw
> +MTE4MTYxNDA0WjA1MTMwMQYDVQQDDCpSZWQgSGF0IElNQSByZWxlYXNlIGtleSAo
> +Zm9yIHZlcmlmaWNhdGlvbikwdjAQBgcqhkjOPQIBBgUrgQQAIgNiAARPDu+/4iOJ
> +kSdOfDKh0MAmkt43jbBd6n/WJxibtGK+BoU9+cxHfse9kVRTYrTAikNIwlkHK4jX
> +PUswjWwy+6Xa3IqF6WFEGPzZi/VeOMiFd8pzaM5I3689BkMvS2wMzYijeDB2MAwG
> +A1UdEwEB/wQCMAAwDgYDVR0PAQH/BAQDAgeAMBYGA1UdJQEB/wQMMAoGCCsGAQUF
> +BwMDMB0GA1UdDgQWBBQi+gHcDqAmn2moZ+XP5Jz70zIESTAfBgNVHSMEGDAWgBT7
> +MYJd0OBzaFsmTjA4ljZz91OVmjANBgkqhkiG9w0BAQsFAAOCAQEAGh7BLWWt8CTs
> +nqf91OrhVNwxHGKMKQt6Vm73tIeSPv/VQEskoWhu7pw1ZaE/jvOLmxixA+37UC6j
> +I9GTHdaCChBvNL7WOr12jEQOracqxI6NxOSNUdgmtziJ0SOgI4h2+vEnkVc+sg/P
> +c1PbIEBdgrnpvKKUCVf7hQ1WS9wZZRIvbWo7vjUf1FLq5HI2+f7L1BsP4w6IfGhY
> +KMMGX73S+S4aMPBjZS1V4aT9l8//wFIiHCSjbt56yZ110tCCsH9v2yEBafBUdgQZ
> +aCwict07DQTVrVqAMGiQbsIn9CivG3j2CnB0XDphQvVjfIMSWhtDvNQbKLXvmMUU
> +BEKA3VQwpA==
> +-----END CERTIFICATE-----
So not sure I like distributing this imho random collection of
certificates.
> diff --git a/doc/ChangeLog b/doc/ChangeLog
> index 7f2d6ff4fd31..914f8f649511 100644
> --- a/doc/ChangeLog
> +++ b/doc/ChangeLog
> @@ -1,3 +1,10 @@
> +2023-08-14 Ryan Goldberg <rgoldber@redhat.com>
> +
> + * debuginfod-client-config.7: Document DEBUGINFOD_IMA_CERT_PATH,
> + update DEBUGINFOD_URLS.
> + * debuginfod.8: Document --koji-sigcache
> + * debuginfod-find.1, debuginfod_find_debuginfo.3: Update SECURITY
> +
> 2023-02-14 Mark Wielaard <mark@klomp.org>
>
> * debuginfod.8: Add .TP before -g.
> diff --git a/doc/debuginfod-client-config.7 b/doc/debuginfod-client-config.7
> index 53d82806d395..11d2465858c5 100644
> --- a/doc/debuginfod-client-config.7
> +++ b/doc/debuginfod-client-config.7
> @@ -27,6 +27,34 @@ debuginfod instances. Alternate URL prefixes are separated by space.
> This environment variable may be set by /etc/profile.d scripts
> reading /etc/debuginfod/*.urls files.
>
> +This environment variable can also contain policy defining tags which
> +dictate the response policy for verifying per-file IMA signatures in
> +RPMs. As the space seperated list is read left to right, upon
> +encountering a tag, subsequent URLs up to the next tag will be handled
> +using that specified policy. All URLs before the first tag will use
> +the default policy, \fIima:permissive\fP. For example:
> +
> +.in +4n
> +.EX
> +DEBUGINFOD_URLS="https://foo.com ima:enforcing https://bar.ca http://localhost:8002/ ima:permissive https://baz.org"
> +.EE
> +.in
> +
> +Where foo.com and baz.org use the default \fIpermissive\fP policy and
> +bar.ca and localhost use an \fIenforcing\fP policy. The policy tag
> +may be one of the following:
> +.IP
> +\fIima:enforcing\fP Every downloaded file requires a valid signature.
> +.IP
> +\fIima:permissive\fP Every downloaded file accompanied by a signature
> +must be valid, but downloads without signatures are accepted.
> +.IP
> +\fIima:ignore\fP Skips verification altogether.
> +.IP
> +
> +Alerts of validation failure will be directed as specified
> +in $DEBUGINFOD_VERBOSE.
> +
> .TP
> .B $DEBUGINFOD_CACHE_PATH
> This environment variable governs the location of the cache where
> @@ -82,6 +110,12 @@ outbound HTTP requests, one per line. The header lines shouldn't end with
> CRLF, unless that's the system newline convention. Whitespace-only lines
> are skipped.
>
> +.TP
> +.B $DEBUGINFOD_IMA_CERT_PATH
> +This environment variable contains a list of absolute directory paths
> +holding X.509 certificates for RPM per-file IMA-verification.
> +Alternate paths are separated by colons.
> +
> .SH CACHE
>
> Before each query, the debuginfod client library checks for a need to
OK. Nice documentation. Thanks.
> diff --git a/doc/debuginfod-find.1 b/doc/debuginfod-find.1
> index 7d577babeb89..89a706728cd6 100644
> --- a/doc/debuginfod-find.1
> +++ b/doc/debuginfod-find.1
> @@ -129,10 +129,19 @@ and printing the http response headers from the server.
>
> .SH "SECURITY"
>
> -debuginfod-find \fBdoes not\fP include any particular security
> -features. It trusts that the binaries returned by the debuginfod(s)
> -are accurate. Therefore, the list of servers should include only
> -trustworthy ones. If accessed across HTTP rather than HTTPS, the
> +If IMA signature(s) are available from the RPMs that contain
> +requested files, then
> +.BR debuginfod
> +will extract those signatures into response headers, and
> +.BR debuginfod-find
> +will perform verification upon the files.
> +Validation policy is controlled via tags inserted into
> +$DEBUGINFOD_URLS. By default,
> +.BR debuginfod-find
> +acts in permissive mode, only rejecting incorrect
> +signatures. See below for details and other policy options.
> +
> +If accessed across HTTP rather than HTTPS, the
> network should be trustworthy. Authentication information through
> the internal \fIlibcurl\fP library is not currently enabled, except
> for the basic plaintext \%\fIhttp[s]://userid:password@hostname/\fP style.
OK.
> diff --git a/doc/debuginfod.8 b/doc/debuginfod.8
> index d4316bec8175..3e73868715d7 100644
> --- a/doc/debuginfod.8
> +++ b/doc/debuginfod.8
> @@ -289,6 +289,14 @@ completed archive or file scans. This may slow down parallel scanning
> phase somewhat, but generate much smaller "-wal" temporary files on
> busy servers. The default is 256. Disabled if 0.
>
> +.TP
> +.B "\-\-koji\-sigcache"
> +Enable an additional step of RPM path mapping when extracting signatures for use
> +in RPM per-file IMA verification on koji repositories. The signatures are retrieved
> +from the Fedora koji sigcache rpm.sig files as opposed to the original RPM header.
> +If a signature cannot be found in the sigcache rpm.sig file, the RPM will be
> +tried as a fallback.
> +
> .TP
> .B "\-v"
> Increase verbosity of logging to the standard error file descriptor.
> @@ -311,7 +319,8 @@ X-DEBUGINFOD-FILE is simply the unescaped filename and
> X-DEBUGINFOD-SIZE is the size of the file. For files found in archives,
> in addition to X-DEBUGINFOD-FILE and X-DEBUGINFOD-SIZE,
> X-DEBUGINFOD-ARCHIVE is added. X-DEBUGINFOD-ARCHIVE is the name of the
> -archive the file was found in.
> +archive the file was found in. X-DEBUGINFOD-IMA-SIGNATURE contains the
> +per-file IMA signature as a hexadecimal blob.
>
> There are three requests. In each case, the buildid is encoded as a
> lowercase hexadecimal string. For example, for a program \fI/bin/ls\fP,
OK.
> diff --git a/doc/debuginfod_find_debuginfo.3 b/doc/debuginfod_find_debuginfo.3
> index 6469a3dfb2db..e170b94367b5 100644
> --- a/doc/debuginfod_find_debuginfo.3
> +++ b/doc/debuginfod_find_debuginfo.3
> @@ -250,13 +250,22 @@ void *debuginfod_so = dlopen(DEBUGINFOD_SONAME, RTLD_LAZY);
> .in
>
> .SH "SECURITY"
> +
> +If IMA signature(s) are available from the RPMs that contain
> +requested files, then
> +.BR debuginfod
> +will extract those signatures into response headers, and
> +.BR debuginfod_find_* ()
> +will perform verification upon the files.
> +Validation policy is controlled via tags inserted into
> +$DEBUGINFOD_URLS. By default,
> .BR debuginfod_find_* ()
> -functions \fBdo not\fP include any particular security
> -features. They trust that the binaries returned by the debuginfod(s)
> -are accurate. Therefore, the list of servers should include only
> -trustworthy ones. If accessed across HTTP rather than HTTPS, the
> -network should be trustworthy. Passing user authentication information
> -through the internal \fIlibcurl\fP library is not currently enabled, except
> +acts in permissive mode, only rejecting incorrect
> +signatures. See below for details and other policy options.
> +
> +If accessed across HTTP rather than HTTPS, the
> +network should be trustworthy. Authentication information through
> +the internal \fIlibcurl\fP library is not currently enabled, except
> for the basic plaintext \%\fIhttp[s]://userid:password@hostname/\fP style.
> (The debuginfod server does not perform authentication, but a front-end
> proxy server could.)
OK.
> diff --git a/tests/ChangeLog b/tests/ChangeLog
> index d816873ce083..e52470c04f49 100644
> --- a/tests/ChangeLog
> +++ b/tests/ChangeLog
> @@ -1,3 +1,11 @@
> +2023-08-14 Ryan Goldberg <rgoldber@redhat.com>
> +
> + * run-debuginfod-ima-verification.sh: New test.
> + * hello2-1.0-1.x86_64.rpm. New test file.
> + * debuginfod-ima/koji. New test directory.
> + * imacert.der. New test file.
> + * Makefile.am (TESTS, EXTRA_DIST): Add it.
> +
> 2023-04-21 Frank Ch. Eigler <fche@redhat.com>
>
> * run-debuginfod-IXr.sh: New test.
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 32b18e6ef633..130cc992c42e 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -273,6 +273,9 @@ if !OLD_LIBMICROHTTPD
> # Too many open file descriptors confuses libmicrohttpd < 0.9.51
> TESTS += run-debuginfod-federation-metrics.sh
> endif
> +if ENABLE_IMA_VERIFICATION
> +TESTS += run-debuginfod-ima-verification.sh
> +endif
> endif
>
> if HAVE_CXX11
> @@ -588,6 +591,7 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \
> run-debuginfod-webapi-concurrency.sh \
> run-debuginfod-section.sh \
> run-debuginfod-IXr.sh \
> + run-debuginfod-ima-verification.sh \
> debuginfod-rpms/fedora30/hello2-1.0-2.src.rpm \
> debuginfod-rpms/fedora30/hello2-1.0-2.x86_64.rpm \
> debuginfod-rpms/fedora30/hello2-debuginfo-1.0-2.x86_64.rpm \
> @@ -611,6 +615,9 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \
> debuginfod-rpms/rhel7/hello2-debuginfo-1.0-2.x86_64.rpm \
> debuginfod-rpms/rhel7/hello2-two-1.0-2.x86_64.rpm \
> debuginfod-rpms/rhel7/hello2-two-1.0-2.x86_64.rpm \
> + debuginfod-ima/rhel9/hello2-1.0-1.x86_64.rpm \
> + debuginfod-ima/rhel9/imacert.der \
> + debuginfod-ima/koji \
> debuginfod-debs/hithere-dbgsym_1.0-1_amd64.ddeb \
> debuginfod-debs/hithere_1.0-1.debian.tar.xz \
> debuginfod-debs/hithere_1.0-1.dsc \
Should hello-2.10-9.fc38.x86_64.rpm.sig also be added to EXTRA_DIST?
> diff --git a/tests/debuginfod-ima/koji/arch/hello-2.10-9.fc38.x86_64.rpm b/tests/debuginfod-ima/koji/arch/hello-2.10-9.fc38.x86_64.rpm
> new file mode 100644
> index 000000000000..b04ad8c2af39
> Binary files /dev/null and b/tests/debuginfod-ima/koji/arch/hello-2.10-9.fc38.x86_64.rpm differ
> diff --git a/tests/debuginfod-ima/koji/data/sigcache/keyid/arch/hello-2.10-9.fc38.x86_64.rpm.sig b/tests/debuginfod-ima/koji/data/sigcache/keyid/arch/hello-2.10-9.fc38.x86_64.rpm.sig
> new file mode 100644
> index 000000000000..ee7eb8e467b4
> Binary files /dev/null and b/tests/debuginfod-ima/koji/data/sigcache/keyid/arch/hello-2.10-9.fc38.x86_64.rpm.sig differ
> diff --git a/tests/debuginfod-ima/rhel9/hello2-1.0-1.x86_64.rpm b/tests/debuginfod-ima/rhel9/hello2-1.0-1.x86_64.rpm
> new file mode 100644
> index 000000000000..0262ae2f0c4c
> Binary files /dev/null and b/tests/debuginfod-ima/rhel9/hello2-1.0-1.x86_64.rpm differ
> diff --git a/tests/debuginfod-ima/rhel9/imacert.der b/tests/debuginfod-ima/rhel9/imacert.der
> new file mode 100644
> index 000000000000..b0250b6c30d5
> Binary files /dev/null and b/tests/debuginfod-ima/rhel9/imacert.der differ
> diff --git a/tests/run-debuginfod-ima-verification.sh b/tests/run-debuginfod-ima-verification.sh
> new file mode 100755
> index 000000000000..45f3e7d037a6
> --- /dev/null
> +++ b/tests/run-debuginfod-ima-verification.sh
> @@ -0,0 +1,177 @@
> +#!/usr/bin/env bash
> +#
> +# Copyright (C) 2019-2023 Red Hat, Inc.
> +# This file is part of elfutils.
> +#
> +# This file is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# elfutils is distributed in the hope that it will be useful, but
> +# WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +. $srcdir/debuginfod-subr.sh
> +
> +type rpmsign 2>/dev/null || { echo "need rpmsign"; exit 77; }
Add rpmsign to elfutils.spec.in as BuildRequires?
> +cat << EoF > include.c
> +#include <rpm/rpmlib.h>
> +#include <rpm/rpmfi.h>
> +#include <rpm/header.h>
> +#include <imaevm.h>
> +#include <openssl/evp.h>
> +EoF
> +tempfiles include.c
> +gcc -H -fsyntax-only include.c 2> /dev/null || { echo "one or more devel packages are missing (rpm-devel, ima-evm-utils-devel, openssl-devel)"; exit 77; }
Isn't this redundant with the configure.ac checks?
> +DB=${PWD}/.debuginfod_tmp.sqlite
> +tempfiles $DB
> +export DEBUGINFOD_CACHE_PATH=${PWD}/.client_cache
> +IMA_POLICY="enforcing"
> +
> +# This variable is essential and ensures no time-race for claiming ports occurs
> +# set base to a unique multiple of 100 not used in any other 'run-debuginfod-*' test
> +base=14000
> +get_ports
> +mkdir R
> +env LD_LIBRARY_PATH=$ldpath DEBUGINFOD_URLS= ${abs_builddir}/../debuginfod/debuginfod $VERBOSE -R \
> + -d $DB -p $PORT1 -t0 -g0 R > vlog$PORT1 2>&1 &
> +PID1=$!
> +tempfiles vlog$PORT1
> +errfiles vlog$PORT1
> +
> +########################################################################
> +cp -pv ${abs_srcdir}/debuginfod-ima/rhel9/hello2-1.0-1.x86_64.rpm signed.rpm
> +tempfiles signed.rpm
> +RPM_BUILDID=460912dbc989106ec7325d243384df20c5ccec0c # /usr/local/bin/hello
> +
> +MIN_IMAEVM_MAJ_VERSION=3
> +MIN_RPM_MAJ_VERSION=4
> +# If the correct programs (and versions) exist sign the rpm in the test
> +if (command -v openssl &> /dev/null) && \
> + (command -v rpmsign &> /dev/null) && \
> + (command -v gpg &> /dev/null) && \
> + [ $(ldd `which rpmsign` | grep libimaevm | awk -F'[^0-9]+' '{ print $2 }') -ge $MIN_IMAEVM_MAJ_VERSION ] && \
> + [ $(rpm --version | awk -F'[^0-9]+' '{ print $2 }') -ge $MIN_RPM_MAJ_VERSION ]
> +then
> + # SIGN THE RPM
> + # First remove any old signatures
> + rpmsign --delsign signed.rpm &> /dev/null
> + rpmsign --delfilesign signed.rpm &> /dev/null
> +
> + # Make a gpg keypair (with $PWD as the homedir)
> + mkdir -m 700 openpgp-revocs.d private-keys-v1.d
> + gpg --quick-gen-key --yes --homedir ${PWD} --batch --passphrase '' --no-default-keyring --keyring "${PWD}/pubring.kbx" example@elfutils.org 2> /dev/null
> +
> + # Create a private DER signing key and a public X509 DER format verification key pair
> + openssl genrsa | openssl pkcs8 -topk8 -nocrypt -outform PEM -out signing.pem
> + openssl req -x509 -key signing.pem -out imacert.pem -days 365 -keyform PEM \
> + -subj "/C=CA/ST=ON/L=TO/O=Elfutils/CN=www.sourceware.org\/elfutils"
> +
> + tempfiles openpgp-revocs.d/* private-keys-v1.d/* * openpgp-revocs.d private-keys-v1.d
> +
> + rpmsign --addsign --signfiles --fskpath=signing.pem -D "_gpg_name example@elfutils.org" -D "_gpg_path ${PWD}" signed.rpm
> + cp signed.rpm R/signed.rpm
> + VERIFICATION_CERT_DIR=${PWD}
> +
> + # Cleanup
> + rm -rf openpgp-revocs.d private-keys-v1.d
> +else
> + # USE A PRESIGNED RPM
> + cp signed.rpm R/signed.rpm
> + # Note we test with no trailing /
> + VERIFICATION_CERT_DIR=${abs_srcdir}/debuginfod-ima/rhel9
> +fi
It seems simpler to just add openssl and gpg to BuildRequires and the
at the start of the test. If you really want a presigned RPM add
another test?
> +########################################################################
> +# Server must become ready with R fully scanned and indexed
> +wait_ready $PORT1 'ready' 1
> +wait_ready $PORT1 'thread_work_total{role="traverse"}' 1
> +wait_ready $PORT1 'thread_work_pending{role="scan"}' 0
> +wait_ready $PORT1 'thread_busy{role="scan"}' 0
> +
> +export DEBUGINFOD_URLS="ima:$IMA_POLICY http://127.0.0.1:$PORT1"
> +
> +# Test 1: Without a certificate the verification should fail
> +export DEBUGINFOD_IMA_CERT_PATH=
> +RC=0
> +testrun ${abs_top_builddir}/debuginfod/debuginfod-find -vv executable $RPM_BUILDID || RC=1
> +test $RC -ne 0
> +
> +# Test 2: It should pass once the certificate is added to the path
> +export DEBUGINFOD_IMA_CERT_PATH=$VERIFICATION_CERT_DIR
> +rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
> +kill -USR1 $PID1
> +wait_ready $PORT1 'thread_work_total{role="traverse"}' 2
> +wait_ready $PORT1 'thread_work_pending{role="scan"}' 0
> +wait_ready $PORT1 'thread_busy{role="scan"}' 0
> +testrun ${abs_top_builddir}/debuginfod/debuginfod-find -vv executable $RPM_BUILDID
> +
> +# Test 3: Corrupt the data and it should fail
> +dd if=/dev/zero of=R/signed.rpm bs=1 count=128 seek=1024 conv=notrunc
> +rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
> +kill -USR1 $PID1
> +wait_ready $PORT1 'thread_work_total{role="traverse"}' 3
> +wait_ready $PORT1 'thread_work_pending{role="scan"}' 0
> +wait_ready $PORT1 'thread_busy{role="scan"}' 0
> +RC=0
> +testrun ${abs_top_builddir}/debuginfod/debuginfod-find executable $RPM_BUILDID || RC=1
> +test $RC -ne 0
> +
> +# Test 4: A rpm without a signature will fail
> +cp signed.rpm R/signed.rpm
> +rpmsign --delfilesign R/signed.rpm
> +rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
> +kill -USR1 $PID1
> +wait_ready $PORT1 'thread_work_total{role="traverse"}' 4
> +wait_ready $PORT1 'thread_work_pending{role="scan"}' 0
> +wait_ready $PORT1 'thread_busy{role="scan"}' 0
> +RC=0
> +testrun ${abs_top_builddir}/debuginfod/debuginfod-find executable $RPM_BUILDID || RC=1
> +test $RC -ne 0
> +
> +# Test 5: Only tests 1,2 will result in extracted signature
> +[[ $(curl -s http://127.0.0.1:$PORT1/metrics | grep 'http_responses_total{extra="ima-sigs-extracted"}' | awk '{print $NF}') -eq 2 ]]
> +
> +kill $PID1
> +wait $PID1
> +PID1=0
> +
> +#######################################################################
> +# We also test the --koji-sigcache
> +cp -pR ${abs_srcdir}/debuginfod-ima/koji R/koji
> +
> +rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
> +env LD_LIBRARY_PATH=$ldpath DEBUGINFOD_URLS= ${abs_builddir}/../debuginfod/debuginfod $VERBOSE -R \
> + -d $DB -p $PORT2 -t0 -g0 -X /data/ --koji-sigcache R/koji > vlog$PORT1 2>&1 &
> +#reuse PID1
> +PID1=$!
> +tempfiles vlog$PORT2
> +errfiles vlog$PORT2
> +
> +RPM_BUILDID=c592a95e45625d7891b90f6b86e63373d540461d #/usr/bin/hello
> +# Note we test with a trailing slash and with the required directory as the third in the PATH
> +VERIFICATION_CERT_DIR=/not/a/dir:${abs_srcdir}/debuginfod-ima/rhel9:${abs_srcdir}/../debuginfod/ima-certs/
> +
> +########################################################################
> +# Server must become ready with koji fully scanned and indexed
> +wait_ready $PORT2 'ready' 1
> +wait_ready $PORT2 'thread_work_total{role="traverse"}' 1
> +wait_ready $PORT2 'thread_work_pending{role="scan"}' 0
> +wait_ready $PORT2 'thread_busy{role="scan"}' 0
> +
> +# Test 1: The path should be properly mapped and verified using the actual fedora 38 cert
> +export DEBUGINFOD_URLS="ima:$IMA_POLICY http://127.0.0.1:$PORT2"
> +export DEBUGINFOD_IMA_CERT_PATH=$VERIFICATION_CERT_DIR
> +testrun ${abs_top_builddir}/debuginfod/debuginfod-find -vv executable $RPM_BUILDID
> +
> +kill $PID1
> +wait $PID1
> +PID1=0
> +
> +exit 0
I haven't ran the tests but it looks OK.
Thanks,
Mark
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-11-15 16:00 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-07 12:55 [PATCH] PR28204, debuginfod IMA Frank Ch. Eigler
2023-10-23 19:33 ` Mark Wielaard
2023-10-24 13:27 ` Frank Ch. Eigler
2023-10-24 21:03 ` Mark Wielaard
2023-10-27 19:15 ` Frank Ch. Eigler
2023-10-31 13:20 ` Mark Wielaard
2023-10-31 15:46 ` Frank Ch. Eigler
2023-11-01 14:59 ` Mark Wielaard
2023-11-14 16:45 ` Frank Ch. Eigler
2023-11-15 16:00 ` Mark Wielaard
2023-10-24 15:25 ` Mark Wielaard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).