From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id EED833858D37 for ; Tue, 24 Oct 2023 13:27:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EED833858D37 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org EED833858D37 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698154071; cv=none; b=Ip8ujoejt++bSSzUY7jBmPN2N7S5RBY6tAze0wa20kENxpnFTfEuqIQb6FzaNvIPSC0QSjOF4Jrgrzx/M0pMi85oKSwTkg0sHtQWTXih4mPHO0oXZkLP32F6+i1gTeyhvJw78ebvka+b1O7WuJhhiLuPQJe9EraO409pQI5Rk58= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698154071; c=relaxed/simple; bh=G/3LT7cvsR+C/qxTl6KLAWqVOwAn5xHa0sPxfaW/K+Y=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=WD/8EX+kU2NgmA5jjqwSI3pCnXip0WI7Mpw9bNg9HR5JhjG8q//DFlMofmnj/+FEIDAHVUIG4+58WQ72kj86WsXWhIazo+FI42bHMPwDJwkuj7iQF7xishb6WnqYSGefddi1Ohi3vIYuOmt3yJ7hWPd2YTCHgLANqoJ/7gCLV70= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1698154068; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Y3PXM4i+ntUnjEagJVg7rUJcvYkp8ryCll+kp0rznpQ=; b=WygPTv1DnSAzJ9VyNuHQPw3DxsVKaVUtirGlIttZG562ueSK88tUy2M3YhCxkBFRIfIEql dgEo2EkI8JDCkAXeO0cWPEAqAEmsP3xA4x3jy8y65uiKBv9lk2jaW2AK9zvj9m2WnOMEpR X8KupddxOzlkOrCRl04G5FkS/din6sA= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-477-omrS32B9ObSy4TTn_ifOYQ-1; Tue, 24 Oct 2023 09:27:45 -0400 X-MC-Unique: omrS32B9ObSy4TTn_ifOYQ-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 3C01C88D281; Tue, 24 Oct 2023 13:27:45 +0000 (UTC) Received: from redhat.com (unknown [10.22.8.19]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 1D51625C0; Tue, 24 Oct 2023 13:27:45 +0000 (UTC) Received: from fche by redhat.com with local (Exim 4.94.2) (envelope-from ) id 1qvHRg-000446-1K; Tue, 24 Oct 2023 09:27:44 -0400 Date: Tue, 24 Oct 2023 09:27:43 -0400 From: "Frank Ch. Eigler" To: Mark Wielaard Cc: elfutils-devel@sourceware.org Subject: Re: [PATCH] PR28204, debuginfod IMA Message-ID: <20231024132743.GC9683@redhat.com> References: <20231023193347.GB2863@gnu.wildebeest.org> MIME-Version: 1.0 In-Reply-To: <20231023193347.GB2863@gnu.wildebeest.org> User-Agent: Mutt/1.12.0 (2019-05-25) X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-7.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi - 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 > > + #include > > + #include > > + #include > > + #include > > 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