Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xbps-install: add --verify-sig option. #384

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ericonr
Copy link
Member

@ericonr ericonr commented Mar 6, 2021

Forcing signature verification for local packages can be useful for
innumerous reasons, the simplest one being the possibility of adding a
test suite for this part of the code without requiring a test setup
running a server or similar.

For this, it was necessary to add a new flag value to xbps_handle, and I
took the opportunity to re-organize the code a bit, including always
checking sha256 for all packages and reporting when remove(3) fails.

Somewhat WIP, lacks man page docs. But it works :)

Forcing signature verification for local packages can be useful for
innumerous reasons, the simplest one being the possibility of adding a
test suite for this part of the code without requiring a test setup
running a server or similar.

For this, it was necessary to add a new flag value to xbps_handle, and I
took the opportunity to re-organize the code a bit, including always
checking sha256 for all packages and reporting when remove(3) fails.
lib/transaction_fetch.c Outdated Show resolved Hide resolved
free(sigfile);
xbps_set_cb_state(xhp, XBPS_STATE_VERIFY_FAIL, rv, pkgver,
"%s: %s.", pkgver, errmsg);
}
goto out;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This goto is, and was, excessive.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to keep it so people didn't forget to add it back if it became necessary to add more stuff to do after the conditionals are done. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine, let it stay.

@Duncaen
Copy link
Member

Duncaen commented Mar 6, 2021

Checking sha256sum of packages that are going to be verified by signature is pointless, now read the whole file twice to generate the same checksum once for just checking the checksum and once for verifying the signature.

@ericonr
Copy link
Member Author

ericonr commented Mar 6, 2021

Checking sha256sum of packages that are going to be verified by signature is pointless, now read the whole file twice to generate the same checksum once for just checking the checksum and once for verifying the signature.

I had it in my head that the signature checking happened with the checksum stored in repodata, for some reason. Would it be ok to add another parameter to xbps_verify_file_signature with the checksum so it can be checked anyway? It's basically a free check, once that's done.

@Duncaen
Copy link
Member

Duncaen commented Mar 6, 2021

There is already xbps_verify_signature which takes a signature file and a digest.

This function allows us to check the sha256 sum for a particular file,
while at the same storing the calculated digest in the provided struct.

This allows us to always check the sha256 sum for packages during
installation, but also not have to scan the package twice when checking
its signature.

In order to simplify the code and necessary error checking, and because
some functions take a length parameter for the binary hash, while others
don't, introduce the xbps_sha256_digest struct, which guarantees we are
always passing a binary digest to functions which require one and
guarantees it has the necessary length to hold the digest, therefore
providing some form of type and memory safety and avoiding the need to
pass and propagate length parameters everywhere.
@lgtm-com
Copy link

lgtm-com bot commented Mar 6, 2021

This pull request introduces 1 alert when merging 2e427b0 into 01180f9 - view on LGTM.com

new alerts:

  • 1 for FIXME comment

ericonr added 3 commits March 7, 2021 00:21
Useful to print a signature path into a string and returning a pointer
to the start of the ".sig" suffix. Also checks if the whole format
string actually fit into the destination.

Replace the manual computation in download_binpkg with it.
verify_binpkg was using the package path instead of the signature path
in xbps_verify_signature. Compute the signature path using the new
xbps_file_sig_path, and as a bonus avoid new allocations.
This actually checks that the resulting string fit inside the buffer, so
we avoid using a truncated path.
@lgtm-com
Copy link

lgtm-com bot commented Mar 7, 2021

This pull request introduces 2 alerts when merging 1ac0716 into 01180f9 - view on LGTM.com

new alerts:

  • 2 for FIXME comment

Also add documentation to man page.
@lgtm-com
Copy link

lgtm-com bot commented Mar 7, 2021

This pull request introduces 2 alerts when merging d5a7640 into 01180f9 - view on LGTM.com

new alerts:

  • 2 for FIXME comment

@ericonr
Copy link
Member Author

ericonr commented Mar 7, 2021

I added at least two FIXME and one XXX comments, I can try to address them in this PR, or work on them afterwards. I believe @Duncaen mentioned he had a stash for the sha256 not being able to start with 0, at least.

I haven't addressed @Chocimier's comment on the goto, waiting for an answer to my response.

Could add some tests for signing here already, to lay the groundwork for a potential move to a different signature scheme.

Would appreciate some review.

@Duncaen
Copy link
Member

Duncaen commented Mar 7, 2021

From c9c4353ffc2482e8765d6e37653e16118342238d Mon Sep 17 00:00:00 2001
From: Duncan Overbruck <[email protected]>
Date: Sat, 6 Mar 2021 20:22:35 +0100
Subject: [PATCH] WIP: lib/download.c: always digest file if digest is
 requested

---
 lib/download.c          |  6 ++++++
 lib/transaction_fetch.c | 31 +++++++------------------------
 2 files changed, 13 insertions(+), 24 deletions(-)

diff --git a/lib/download.c b/lib/download.c
index b3dcc02d..dfef9d05 100644
--- a/lib/download.c
+++ b/lib/download.c
@@ -182,6 +182,12 @@ xbps_fetch_file_dest_sha256(struct xbps_handle *xhp, const char *uri, const char
 	if (fio == NULL) {
 		if (fetchLastErrCode == FETCH_UNCHANGED) {
 			/* Last-Modified matched */
+			if (digest) {
+				if (!xbps_file_sha256_raw(digest, digestlen, filename)) {
+					errno = EIO;
+					rv = -1;
+				}
+			}
 			goto fetch_file_out;
 		} else if (fetchLastErrCode == FETCH_PROTO && url_st.size == stp->st_size) {
 			/* 413, requested offset == length */
diff --git a/lib/transaction_fetch.c b/lib/transaction_fetch.c
index 1df27c58..9a387c0b 100644
--- a/lib/transaction_fetch.c
+++ b/lib/transaction_fetch.c
@@ -129,8 +129,7 @@ download_binpkg(struct xbps_handle *xhp, xbps_dictionary_t repo_pkgd)
 	xbps_set_cb_state(xhp, XBPS_STATE_DOWNLOAD, 0, pkgver,
 		"Downloading `%s' package (from `%s')...", pkgver, repoloc);
 
-	if ((rv = xbps_fetch_file_sha256(xhp, buf, NULL, digest,
-	    sizeof digest)) == -1) {
+	if ((rv = xbps_fetch_file_sha256(xhp, buf, NULL, digest, sizeof digest)) == -1) {
 		rv = fetchLastErrCode ? fetchLastErrCode : errno;
 		fetchstr = xbps_fetch_error_string();
 		xbps_set_cb_state(xhp, XBPS_STATE_DOWNLOAD_FAIL, rv,
@@ -153,29 +152,13 @@ download_binpkg(struct xbps_handle *xhp, xbps_dictionary_t repo_pkgd)
 		return rv;
 	}
 
-	/*
-	 * If digest is not set, binary package was not downloaded,
-	 * i.e. 304 not modified, verify by file instead.
-	 */
-	if (*digest) {
+	if (!xbps_verify_signature(repo, buf, digest)) {
+		rv = EPERM;
+		/* remove signature */
+		(void)remove(buf);
+		/* remove binpkg */
 		*sigsuffix = '\0';
-		if (!xbps_verify_file_signature(repo, buf)) {
-			rv = EPERM;
-			/* remove binpkg */
-			(void)remove(buf);
-			/* remove signature */
-			*sigsuffix = '.';
-			(void)remove(buf);
-		}
-	} else {
-		if (!xbps_verify_signature(repo, buf, digest)) {
-			rv = EPERM;
-			/* remove signature */
-			(void)remove(buf);
-			/* remove binpkg */
-			*sigsuffix = '\0';
-			(void)remove(buf);
-		}
+		(void)remove(buf);
 	}
 
 	if (rv == EPERM) {
-- 
2.30.1

There is the other codepath in lib/download.c, fetchLastErrCode == FETCH_PROTO && url_st.size == stp->st_size which also needs to do this.
Not sure about that one, wasn't happy about assuming that if it returns 413 that the file is good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants