Skip to content
Snippets Groups Projects

apk2/apk3 fetch not checking package signature

    • Closed Issue created by Sertonix @sertonix

    apk fetch copies the downloaded data to the output file with out any correctness check and without verifying the signature. Scripts like mkimage.sh consider the output of apk fetch to be trusted/verified data. Considering that packages may be fetched over http (and not https) this could be abused.

    I just hope the alpine images were always created with https repos :sweat_smile:

    A small example showing the problem. The file just needs to have the correct size, nothing more.

    mkdir x86_64
wget -O x86_64/APKINDEX.tar.gz https://dl-cdn.alpinelinux.org/alpine/edge/main/x86_64/APKINDEX.tar.gz
printf %1238s invalid > x86_64/build-base-0.5-r3.apk
apk fetch --from none -X. build-base --stdout | grep -o invalid

    CC @ncopa @fabled

    Edited by Sertonix

    Designs

      Child items 0

      2. No child items are currently assigned. Use child items to break down this issue into smaller parts.

      Activity

      • Sertonix changed the description

        changed the description

      • Natanael Copa
        Natanael Copa @ncopa ·
        Owner

        I wonder if we could run apk verify after apk fetch in mkimage.sh script?

      • Timo Teräs
        Timo Teräs @fabled ·
        Maintainer

        apk index verifies the signatures before signing index since 2.14.2 so the image creation would fail at this step.

        But makes sense to add the signature verification here.

      • Sertonix
        Sertonix @sertonix ·
        Author Contributor

        I wonder if we could run apk verify after apk fetch in mkimage.sh script?

        I think it would be better to have apk fetch always check signatures. It makes sense cause it already checks the signature of the index.

        apk index verifies the signatures before signing index since 2.14.2 so the image creation would fail at this step.

        Not all fetched packages are run through apk index. Some are just extracted in place. For example like this:

        apk fetch --root "$APKROOT" --quiet --stdout grub-efi | tar -C "$WORKDIR" -zx usr/lib/grub
      • Sertonix
        Sertonix @sertonix ·
        Author Contributor

        Would this patch be ok?

        --- a/src/app_fetch.c
+++ b/src/app_fetch.c
@@ -16,6 +16,7 @@
 
 #include "apk_applet.h"
 #include "apk_database.h"
+#include "apk_extract.h"
 #include "apk_io.h"
 #include "apk_print.h"
 #include "apk_solver.h"
@@ -153,6 +154,7 @@ static int fetch_package(struct apk_database *db, const char *match, struct apk_
 	struct apk_ostream *os;
 	struct apk_repository *repo;
 	struct apk_file_info fi;
+	struct apk_extract_ctx ectx;
 	char url[PATH_MAX], filename[256];
 	int r, urlfd;
 
@@ -209,10 +211,9 @@ static int fetch_package(struct apk_database *db, const char *match, struct apk_
 		goto err;
 	}
 
-	apk_stream_copy(is, os, pkg->size, progress_cb, ctx, 0);
-	apk_ostream_copy_meta(os, is);
-	apk_istream_close(is);
-	r = apk_ostream_close(os);
+	is = apk_istream_tee(is, os, APK_ISTREAM_TEE_COPY_META, progress_cb, ctx);
+	apk_extract_init(&ectx, db->ctx, NULL);
+	r = apk_extract(&ectx, is);
 	if (r) goto err;
 	goto done;
      • Sertonix
        Sertonix @sertonix ·
        Author Contributor

        The patch above in combination with aports!72610 (merged) should ensure that mkimage.sh checks the signatures everywhere.

        Edited by Sertonix
      • Timo Teräs
        Timo Teräs @fabled ·
        Maintainer

        I wonder if the piece from apk_cache_download:

        	if (pkg) apk_extract_verify_identity(&ectx, pkg->digest_alg, apk_pkg_digest_blob(pkg));

        should be included to instead verify the index hash. This would then agree with how things are doing during install time.

        Perhaps do separate apk_repository_download helper and convert both apk_cache_download and fetch_package to use it. This would probably be slightly simpler to maintain and backport. Does this sound like a good plan to you?

      • Sertonix
        Sertonix @sertonix ·
        Author Contributor

        Sounds good.

      • Timo Teräs
        Timo Teräs @fabled ·
        Maintainer

        Sorry on the latency on this. I have had unusually many other things again on the plate. I now have the following commits (Happy to adjust the author of master commit to Sertonix if you want):

        Master:

        Subject: [PATCH] fetch: validate downloaded package against repository

Use the repository hash to validate the package.

fixes #11027
---
 src/app_fetch.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/app_fetch.c b/src/app_fetch.c
index c602223..34e8060 100644
--- a/src/app_fetch.c
+++ b/src/app_fetch.c
@@ -16,6 +16,7 @@
 
 #include "apk_applet.h"
 #include "apk_database.h"
+#include "apk_extract.h"
 #include "apk_io.h"
 #include "apk_print.h"
 #include "apk_solver.h"
@@ -153,6 +154,7 @@ static int fetch_package(struct apk_database *db, const char *match, struct apk_
 	struct apk_ostream *os;
 	struct apk_repository *repo;
 	struct apk_file_info fi;
+	struct apk_extract_ctx ectx;
 	char url[PATH_MAX], filename[256];
 	int r, urlfd;
 
@@ -207,13 +209,11 @@ static int fetch_package(struct apk_database *db, const char *match, struct apk_
 		goto err;
 	}
 
-	apk_stream_copy(is, os, pkg->size, progress_cb, ctx, 0);
-	apk_ostream_copy_meta(os, is);
-	apk_istream_close(is);
-	r = apk_ostream_close(os);
-	if (r) goto err;
-	goto done;
-
+	is = apk_istream_tee(is, os, APK_ISTREAM_TEE_COPY_META, progress_cb, ctx);
+	apk_extract_init(&ectx, db->ctx, NULL);
+	apk_extract_verify_identity(&ectx, pkg->digest_alg, apk_pkg_digest_blob(pkg));
+	r = apk_extract(&ectx, is);
+	if (r == 0) goto done;
 err:
 	apk_err(out, PKG_VER_FMT ": %s", PKG_VER_PRINTF(pkg), apk_error_str(r));
 	ctx->errors++;
-- 
2.46.2

        2.14-stable:

        Subject: [PATCH] fetch: validate downloaded package against repository

Use the repository hash to validate the package.

fixes #11027

(cherry picked from commit xxx)
---
 src/apk_io.h    |  2 ++
 src/app_fetch.c | 32 +++++++++++---------------------
 src/io.c        | 44 +++++++++++++++++++++++++++++---------------
 3 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/src/apk_io.h b/src/apk_io.h
index 8f789e4..b11ec8e 100644
--- a/src/apk_io.h
+++ b/src/apk_io.h
@@ -133,6 +133,8 @@ struct apk_segment_istream {
 struct apk_istream *apk_istream_segment(struct apk_segment_istream *sis, struct apk_istream *is, size_t len, time_t mtime);
 struct apk_istream *apk_istream_tee(struct apk_istream *from, int atfd, const char *to, int copy_meta,
 				    apk_progress_cb cb, void *cb_ctx);
+struct apk_istream *apk_istream_tee_fd(struct apk_istream *from, int fd, int copy_meta,
+				       apk_progress_cb cb, void *cb_ctx);
 
 struct apk_ostream_ops {
 	ssize_t (*write)(struct apk_ostream *os, const void *buf, size_t size);
diff --git a/src/app_fetch.c b/src/app_fetch.c
index 4bbf081..8a4a7c0 100644
--- a/src/app_fetch.c
+++ b/src/app_fetch.c
@@ -146,12 +146,13 @@ static void progress_cb(void *pctx, size_t bytes_done)
 
 static int fetch_package(struct apk_database *db, const char *match, struct apk_package *pkg, void *pctx)
 {
+	struct apk_sign_ctx sctx;
 	struct fetch_ctx *ctx = pctx;
 	struct apk_istream *is;
 	struct apk_repository *repo;
 	struct apk_file_info fi;
 	char url[PATH_MAX], filename[256];
-	int r, fd, urlfd;
+	int r, fd, urlfd, copy_meta = 1;
 
 	if (!pkg->marked)
 		return 0;
@@ -186,6 +187,7 @@ static int fetch_package(struct apk_database *db, const char *match, struct apk_
 
 	if (ctx->flags & FETCH_STDOUT) {
 		fd = STDOUT_FILENO;
+		copy_meta = 0;
 	} else {
 		if ((ctx->flags & FETCH_LINK) && urlfd >= 0) {
 			const char *urlfile = apk_url_local_file(url);
@@ -201,27 +203,15 @@ static int fetch_package(struct apk_database *db, const char *match, struct apk_
 		}
 	}
 
+	apk_sign_ctx_init(&sctx, APK_SIGN_VERIFY_IDENTITY, &pkg->csum, db->keys_fd);
 	is = apk_istream_from_fd_url(urlfd, url);
-	if (IS_ERR_OR_NULL(is)) {
-		r = PTR_ERR(is) ?: -EIO;
-		goto err;
-	}
-
-	r = apk_istream_splice(is, fd, pkg->size, progress_cb, ctx);
-	if (fd != STDOUT_FILENO) {
-		struct apk_file_meta meta;
-		apk_istream_get_meta(is, &meta);
-		apk_file_meta_to_fd(fd, &meta);
-		close(fd);
-	}
-	apk_istream_close(is);
-
-	if (r != pkg->size) {
-		unlinkat(ctx->outdir_fd, filename, 0);
-		if (r >= 0) r = -EIO;
-		goto err;
-	}
-	goto done;
+	is = apk_istream_tee_fd(is, fd, copy_meta, progress_cb, pctx);
+	is = apk_istream_gunzip_mpart(is, apk_sign_ctx_mpart_cb, &sctx);
+	r = apk_tar_parse(is, apk_sign_ctx_verify_tar, &sctx, &db->id_cache);
+	r = apk_sign_ctx_status(&sctx, r);
+	apk_sign_ctx_free(&sctx);
+	if (r == 0) goto done;
+	if (fd != STDOUT_FILENO) unlinkat(ctx->outdir_fd, filename, 0);
 
 err:
 	apk_error(PKG_VER_FMT ": %s", PKG_VER_PRINTF(pkg), apk_error_str(r));
diff --git a/src/io.c b/src/io.c
index e47048b..ff338c7 100644
--- a/src/io.c
+++ b/src/io.c
@@ -305,7 +305,7 @@ static int tee_close(struct apk_istream *is)
 	}
 
 	r = apk_istream_close(tee->inner_is);
-	close(tee->fd);
+	if (tee->fd > STDERR_FILENO) close(tee->fd);
 	free(tee);
 	return r;
 }
@@ -316,25 +316,24 @@ static const struct apk_istream_ops tee_istream_ops = {
 	.close = tee_close,
 };
 
-struct apk_istream *apk_istream_tee(struct apk_istream *from, int atfd, const char *to, int copy_meta, apk_progress_cb cb, void *cb_ctx)
+struct apk_istream *apk_istream_tee_fd(struct apk_istream *from, int fd, int copy_meta, apk_progress_cb cb, void *cb_ctx)
 {
 	struct apk_tee_istream *tee;
-	int fd, r;
-
-	if (IS_ERR_OR_NULL(from)) return ERR_CAST(from);
-	if (atfd_error(atfd)) return ERR_PTR(atfd);
+	int r;
 
-	fd = openat(atfd, to, O_CREAT | O_RDWR | O_TRUNC | O_CLOEXEC,
-		    S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
+	if (IS_ERR_OR_NULL(from)) {
+		r = PTR_ERR(from);
+		goto err;
+	}
 	if (fd < 0) {
-		r = -errno;
-		goto err_is;
+		r = -EBADFD;
+		goto err;
 	}
 
 	tee = malloc(sizeof *tee);
 	if (!tee) {
 		r = -ENOMEM;
-		goto err_fd;
+		goto err;
 	}
 
 	*tee = (struct apk_tee_istream) {
@@ -358,13 +357,28 @@ struct apk_istream *apk_istream_tee(struct apk_istream *from, int atfd, const ch
 	return &tee->is;
 err_free:
 	free(tee);
-err_fd:
-	close(fd);
-err_is:
-	apk_istream_close(from);
+err:
+	if (fd > STDERR_FILENO) close(fd);
+	if (!IS_ERR_OR_NULL(from)) apk_istream_close(from);
 	return ERR_PTR(r);
 }
 
+struct apk_istream *apk_istream_tee(struct apk_istream *from, int atfd, const char *to, int copy_meta, apk_progress_cb cb, void *cb_ctx)
+{
+	int fd;
+
+	if (atfd_error(atfd)) {
+		apk_istream_close(from);
+		return ERR_PTR(atfd);
+	}
+	fd = openat(atfd, to, O_CREAT | O_RDWR | O_TRUNC | O_CLOEXEC, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
+	if (fd < 0) {
+		apk_istream_close(from);
+		return ERR_PTR(-errno);
+	}
+	return apk_istream_tee_fd(from, fd, copy_meta, cb, cb_ctx);
+}
+
 struct apk_mmap_istream {
 	struct apk_istream is;
 	int fd;
-- 
2.46.2

        I would like to tag new master pre release and 2.14-stable after posting these, so I'll push those tags on the same go.

        Given I have tight schedule this week (I was hoping to do this on Tuesday but ran out of time), I'll probably do this next Monday afternoon if that sounds good time wise.

        Edited by Timo Teräs
        • Sertonix
          Sertonix @sertonix ·
          Author Contributor

          After the r = apk_tar_parse(is, apk_sign_ctx_verify_tar, &sctx, &db->id_cache); line there needs to be something like if (r < 0) goto err;. And then the apk_sign_ctx_free(&sctx); needs to be moved after done:.

          Otherwise this looks good.

        • Timo Teräs
          Timo Teräs @fabled ·
          Maintainer

          No this is the construct used else where, apk_sign_ctx_status takes r as argument and takes it into account processing the status. This way the function call group has no jumps and the free is also there in the correct place.

        • Please register or sign in to reply
      • Timo Teräs closed with commit 769bb19b

        closed with commit 769bb19b

      • Timo Teräs mentioned in commit 7763d4ed

        mentioned in commit 7763d4ed

      • Sertonix
        Sertonix @sertonix ·
        Author Contributor

        This issue should probably marked as not confidential now.

      • Timo Teräs made the issue visible to everyone

        made the issue visible to everyone

      Please register or sign in to reply