Attachment 531762 Details for Bug 808594

[patch] backported upstream patch

pesign-bnc808594-align-signatures.patch (text/plain), 16.62 KB, created by Gary Ching-Pang Lin on 2013-03-26 08:44:34 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Gary Ching-Pang Lin

Created: 2013-03-26 08:44:34 UTC

Size: 16.62 KB

patch

obsolete

>From 21cec8feac92a8cda788eaf3f9e9aee9d1b92672 Mon Sep 17 00:00:00 2001
>From: Peter Jones <pjones@redhat.com>
>Date: Mon, 25 Mar 2013 11:34:45 -0400
>Subject: [PATCH 1/6] If the last hunk of the file isn't 16-byte aligned, pad
> before digesting.
>
>When we (or MS) create a data directory section, we pad it to 16-bytes.
>This means that when you add that and then hash, you'll have that
>0-extension before the data directory (in this case, the cert list) in
>the checksum.
>
>If we do -h without embedding the signature in the binary, we still need
>to take that into account.
>
>Signed-off-by: Peter Jones <pjones@redhat.com>
>---
> src/cms_common.c |   24 +++++++++++++++++++++++-
> 1 file changed, 23 insertions(+), 1 deletion(-)
>
>diff --git a/src/cms_common.c b/src/cms_common.c
>index 9ab2021..306d53e 100644
>--- a/src/cms_common.c
>+++ b/src/cms_common.c
>@@ -795,6 +795,12 @@ err:
> 	return -1;
> }
> 
>+#if 1
>+#define dprintf(fmt, ...)
>+#else
>+#define dprintf(fmt, args...) printf(fmt, ## args)
>+#endif
>+
> int
> generate_digest(cms_context *cms, Pe *pe)
> {
>@@ -860,6 +866,8 @@ generate_digest(cms_context *cms, Pe *pe)
> 		cms->log(cms, LOG_ERR, "Pe header is invalid");
> 		goto error;
> 	}
>+	dprintf("beginning of hash\n");
>+	dprintf("digesting %lx + %lx\n", hash_base - map, hash_size);
> 	generate_digest_step(cms, hash_base, hash_size);
> 
> 	/* 5. Skip over the image checksum
>@@ -882,6 +890,7 @@ generate_digest(cms_context *cms, Pe *pe)
> 		goto error;
> 	}
> 	generate_digest_step(cms, hash_base, hash_size);
>+	dprintf("digesting %lx + %lx\n", hash_base - map, hash_size);
> 
> 	/* 8. Skip over the crt dir
> 	 * 9. Hash everything up to the end of the image header. */
>@@ -895,6 +904,7 @@ generate_digest(cms_context *cms, Pe *pe)
> 		goto error;
> 	}
> 	generate_digest_step(cms, hash_base, hash_size);
>+	dprintf("digesting %lx + %lx\n", hash_base - map, hash_size);
> 
> 	/* 10. Set SUM_OF_BYTES_HASHED to the size of the header. */
> 	hashed_bytes = pe32opthdr ? pe32opthdr->header_size
>@@ -926,6 +936,7 @@ generate_digest(cms_context *cms, Pe *pe)
> 		}
> 
> 		generate_digest_step(cms, hash_base, hash_size);
>+		dprintf("digesting %lx + %lx\n", hash_base - map, hash_size);
> 
> 		hashed_bytes += hash_size;
> 	}
>@@ -938,8 +949,19 @@ generate_digest(cms_context *cms, Pe *pe)
> 			cms->log(cms, LOG_ERR, "Pe has invalid trailing data");
> 			goto error_shdrs;
> 		}
>-		generate_digest_step(cms, hash_base, hash_size);
>+		if (hash_size % 16 != 0) {
>+			size_t tmp_size = hash_size + (16 - (hash_size % 16));
>+			uint8_t tmp_array[tmp_size];
>+			memset(tmp_array, '\0', tmp_size);
>+			memcpy(tmp_array, hash_base, hash_size);
>+			generate_digest_step(cms, tmp_array, tmp_size);
>+			dprintf("digesting %lx + %lx\n", (unsigned long)tmp_array, tmp_size);
>+		} else {
>+			generate_digest_step(cms, hash_base, hash_size);
>+			dprintf("digesting %lx + %lx\n", hash_base - map, hash_size);
>+		}
> 	}
>+	dprintf("end of hash\n");
> 
> 	rc = generate_digest_finish(cms);
> 	if (rc < 0)
>-- 
>1.7.10.4
>
>
>From d07c91cffaeaaa1b0f0a0dbc684e073d976ee9f3 Mon Sep 17 00:00:00 2001
>From: Peter Jones <pjones@redhat.com>
>Date: Mon, 25 Mar 2013 12:53:05 -0400
>Subject: [PATCH 2/6] Pad signatures everywhere /except/ -h.
>
>If you run -h, you may be using that hash in a db/dbx variable, in which
>case the padding isn't appropriate.  Everywhere else, it's for
>implanting at some stage.
>
>Signed-off-by: Peter Jones <pjones@redhat.com>
>---
> src/cms_common.c |    4 ++--
> src/cms_common.h |    2 +-
> src/daemon.c     |    6 +++---
> src/pesign.c     |   15 ++++++++-------
> 4 files changed, 14 insertions(+), 13 deletions(-)
>
>diff --git a/src/cms_common.c b/src/cms_common.c
>index 306d53e..f2ee684 100644
>--- a/src/cms_common.c
>+++ b/src/cms_common.c
>@@ -802,7 +802,7 @@ err:
> #endif
> 
> int
>-generate_digest(cms_context *cms, Pe *pe)
>+generate_digest(cms_context *cms, Pe *pe, int padded)
> {
> 	void *hash_base;
> 	size_t hash_size;
>@@ -949,7 +949,7 @@ generate_digest(cms_context *cms, Pe *pe)
> 			cms->log(cms, LOG_ERR, "Pe has invalid trailing data");
> 			goto error_shdrs;
> 		}
>-		if (hash_size % 16 != 0) {
>+		if (hash_size % 16 != 0 && padded) {
> 			size_t tmp_size = hash_size + (16 - (hash_size % 16));
> 			uint8_t tmp_array[tmp_size];
> 			memset(tmp_array, '\0', tmp_size);
>diff --git a/src/cms_common.h b/src/cms_common.h
>index a3848cd..d819aab 100644
>--- a/src/cms_common.h
>+++ b/src/cms_common.h
>@@ -105,7 +105,7 @@ extern int generate_spc_link(cms_context *cms, SpcLink *slp,
> 
> extern int generate_spc_string(cms_context *cms, SECItem *ssp, char *str,
> 				int len);
>-extern int generate_digest(cms_context *cms, Pe *pe);
>+extern int generate_digest(cms_context *cms, Pe *pe, int padded);
> extern int generate_signature(cms_context *ctx);
> extern int unlock_nss_token(cms_context *ctx);
> extern int find_certificate(cms_context *ctx);
>diff --git a/src/daemon.c b/src/daemon.c
>index 92ae856..69821ba 100644
>--- a/src/daemon.c
>+++ b/src/daemon.c
>@@ -433,7 +433,7 @@ malformed:
> 		if (rc < 0)
> 			goto finish;
> 
>-		rc = generate_digest(ctx->cms, outpe);
>+		rc = generate_digest(ctx->cms, outpe, 1);
> 		if (rc < 0) {
> err_attached:
> 			pe_end(outpe);
>@@ -448,7 +448,7 @@ err_attached:
> 		if (sigspace < 0)
> 			goto err_attached;
> 		allocate_signature_space(outpe, sigspace);
>-		rc = generate_digest(ctx->cms, outpe);
>+		rc = generate_digest(ctx->cms, outpe, 1);
> 		if (rc < 0)
> 			goto err_attached;
> 		rc = generate_signature(ctx->cms);
>@@ -463,7 +463,7 @@ err_attached:
> 			ctx->cms->log(ctx->cms, ctx->priority|LOG_ERR,
> 				"pesignd: could not truncate output file: %m");
> 		}
>-		rc = generate_digest(ctx->cms, inpe);
>+		rc = generate_digest(ctx->cms, inpe, 1);
> 		if (rc < 0) {
> err_detached:
> 			if (ftruncate(outfd, 0) != 0) {
>diff --git a/src/pesign.c b/src/pesign.c
>index fcb2dca..81515d2 100644
>--- a/src/pesign.c
>+++ b/src/pesign.c
>@@ -473,7 +473,8 @@ main(int argc, char *argv[])
> 			"force overwriting of output file", NULL },
> 		{"sign", 's', POPT_ARG_VAL, &ctxp->sign, 1,
> 			"create a new signature", NULL },
>-		{"hash", 'h', POPT_ARG_VAL, &ctxp->hash, 1, "hash binary", NULL },
>+		{"hash", 'h', POPT_ARG_VAL, &ctxp->hash, 1,
>+			"hash binary", NULL },
> 		{"digest_type", 'd', POPT_ARG_STRING|POPT_ARGFLAG_SHOW_DEFAULT,
> 			&digest_name, 0, "digest type to use for pe hash" },
> 		{"import-signed-certificate", 'm',
>@@ -669,7 +670,7 @@ main(int argc, char *argv[])
> 			open_input(ctxp);
> 			open_output(ctxp);
> 			close_input(ctxp);
>-			generate_digest(ctxp->cms_ctx, ctxp->outpe);
>+			generate_digest(ctxp->cms_ctx, ctxp->outpe, 1);
> 			sigspace = calculate_signature_space(ctxp->cms_ctx,
> 								ctxp->outpe);
> 			allocate_signature_space(ctxp->outpe, sigspace);
>@@ -683,7 +684,7 @@ main(int argc, char *argv[])
> 		case EXPORT_SATTRS:
> 			open_input(ctxp);
> 			open_sattr_output(ctxp);
>-			generate_digest(ctxp->cms_ctx, ctxp->inpe);
>+			generate_digest(ctxp->cms_ctx, ctxp->inpe, 1);
> 			generate_sattr_blob(ctxp);
> 			close_sattr_output(ctxp);
> 			close_input(ctxp);
>@@ -779,7 +780,7 @@ main(int argc, char *argv[])
> 			break;
> 		case GENERATE_DIGEST|PRINT_DIGEST:
> 			open_input(ctxp);
>-			generate_digest(ctxp->cms_ctx, ctxp->inpe);
>+			generate_digest(ctxp->cms_ctx, ctxp->inpe, 0);
> 			print_digest(ctxp);
> 			break;
> 		/* generate a signature and save it in a separate file */
>@@ -793,7 +794,7 @@ main(int argc, char *argv[])
> 			}
> 			open_input(ctxp);
> 			open_sig_output(ctxp);
>-			generate_digest(ctxp->cms_ctx, ctxp->inpe);
>+			generate_digest(ctxp->cms_ctx, ctxp->inpe, 1);
> 			generate_signature(ctxp->cms_ctx);
> 			export_signature(ctxp->cms_ctx, ctxp->outsigfd, ctxp->ascii);
> 			break;
>@@ -814,11 +815,11 @@ main(int argc, char *argv[])
> 			open_input(ctxp);
> 			open_output(ctxp);
> 			close_input(ctxp);
>-			generate_digest(ctxp->cms_ctx, ctxp->outpe);
>+			generate_digest(ctxp->cms_ctx, ctxp->outpe, 1);
> 			sigspace = calculate_signature_space(ctxp->cms_ctx,
> 							     ctxp->outpe);
> 			allocate_signature_space(ctxp->outpe, sigspace);
>-			generate_digest(ctxp->cms_ctx, ctxp->outpe);
>+			generate_digest(ctxp->cms_ctx, ctxp->outpe, 1);
> 			generate_signature(ctxp->cms_ctx);
> 			insert_signature(ctxp->cms_ctx, ctxp->signum);
> 			finalize_signatures(ctxp->cms_ctx->signatures,
>-- 
>1.7.10.4
>
>
>From 29a593849964bb89c29bb40dd6a1f4bb5a90e675 Mon Sep 17 00:00:00 2001
>From: Peter Jones <pjones@redhat.com>
>Date: Thu, 21 Mar 2013 11:02:43 -0400
>Subject: [PATCH 3/6] Deal with PE-COFF 8.2+ alignment restrictions for the
> certificate list.
>
>PE-COFF 8.2 and newer finally specify the certificate list as a proper
>array, but they kindly made a new rule that each entry has to be 8-byte
>aligned.  So align them now :/
>
>Signed-off-by: Peter Jones <pjones@redhat.com>
>---
> src/wincert.c |   22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
>diff --git a/src/wincert.c b/src/wincert.c
>index 4197a87..3686918 100644
>--- a/src/wincert.c
>+++ b/src/wincert.c
>@@ -42,6 +42,11 @@ generate_cert_list(SECItem **signatures, int num_signatures,
> 	*cert_list_size = cl_size;
> 
> 	for (int i = 0; i < num_signatures; i++) {
>+		/* pe-coff 8.2 adds some text that says each cert list
>+		 * entry is 8-byte aligned, so that means we need to align
>+		 * them here. */
>+		if ((intptr_t)data % 8 != 0)
>+			data = (uint8_t *)((intptr_t)data + (8 - ((intptr_t)data % 8)));
> 		struct cert_list_entry *cle = (struct cert_list_entry *)data;
> 		cle->wc.length = signatures[i]->len +
> 			sizeof (win_certificate);
>@@ -170,6 +175,11 @@ done:
> 
> 		iter->n += sizeof (*tmpcert) + length;
> 
>+		/* each cert list entry must be aligned to an 8-byte
>+		 * boundary */
>+		if (iter->n % 8 != 0)
>+			iter->n += 8 - (iter->n % 8);
>+
> 		return 1;
> 	}
> }
>@@ -208,8 +218,13 @@ size_t
> get_reserved_sig_space(cms_context *cms, Pe *pe)
> {
> 	size_t ret = 0;
>-	for (int i = 0; i < cms->num_signatures; i++)
>+	for (int i = 0; i < cms->num_signatures; i++) {
> 		ret += cms->signatures[i]->len + sizeof (win_certificate);
>+		/* each certificate list entry must be 8-byte aligned,
>+		 * so we need to account for that in our space calculation */
>+		if (ret % 8 != 0)
>+			ret += 8 - (ret % 8);
>+	}
> 	return ret;
> }
> 
>@@ -238,6 +253,11 @@ err:
> 
> 	size_t res = get_reserved_sig_space(cms, pe);
> 
>+	/* pe-coff 8.2 adds some text that says each cert list entry is
>+	 * 8-byte aligned, so that means we need alignment space here. */
>+	if (res % 8 != 0)
>+		res += 8 - (res % 8);
>+
> 	ssize_t ret = res + sig.len + sizeof(win_certificate) -
> 						available_cert_space(pe);
> 
>-- 
>1.7.10.4
>
>
>From 731aa2ac9012a39fd4ccee813c77a9e75235606c Mon Sep 17 00:00:00 2001
>From: Peter Jones <pjones@redhat.com>
>Date: Fri, 22 Mar 2013 09:56:23 -0400
>Subject: [PATCH 4/6] More certificate list alignment fixups (based on work by
> Gary Lin)
>
>There was still some chance the first entry wasn't aligned right, and
>doing it ad-hoc every time wasn't that great.  So fix that.
>
>This is really all Gary's work, I've just reformatted it a little bit.
>
>Signed-off-by: Peter Jones <pjones@redhat.com>
>---
> libdpe/common.h        |    2 ++
> libdpe/pe_allocspace.c |    4 ++--
> src/wincert.c          |   24 ++++++++++--------------
> 3 files changed, 14 insertions(+), 16 deletions(-)
>
>diff --git a/libdpe/common.h b/libdpe/common.h
>index 5d379e8..be42738 100644
>--- a/libdpe/common.h
>+++ b/libdpe/common.h
>@@ -31,6 +31,8 @@
> 
> #define is_64_bit(pe) ((pe)->flags & IMAGE_FILE_32BIT_MACHINE)
> 
>+#define ALIGNMENT_PADDING(address, align) ((align - (address % align)) % align)
>+
> #define xfree(x) ({if (x) { free(x); x = NULL; }})
> #define xmunmap(addr, size) ({if (addr) { munmap(addr,size); addr = NULL; }})
> 
>diff --git a/libdpe/pe_allocspace.c b/libdpe/pe_allocspace.c
>index 0ae1f5d..716373c 100644
>--- a/libdpe/pe_allocspace.c
>+++ b/libdpe/pe_allocspace.c
>@@ -86,7 +86,7 @@ pe_extend_file(Pe *pe, size_t size, uint32_t *new_space, int align)
> 	void *new = NULL;
> 
> 	if (align)
>-		align = (pe->maximum_size + size) % align;
>+		align = ALIGNMENT_PADDING(pe->maximum_size, align);
> 	int extra = size + align;
> 
> 	int rc = ftruncate(pe->fildes, pe->maximum_size + extra);
>@@ -119,7 +119,7 @@ pe_allocspace(Pe *pe, size_t size, uint32_t *offset)
> 
> 	/* XXX PJFIX TODO: this should try to find space in the already
> 	 * mapped regions. */
>-	rc = pe_extend_file(pe, size, offset, 0);
>+	rc = pe_extend_file(pe, size, offset, 8);
> 	if (rc < 0)
> 		return -1;
> 	return 0;
>diff --git a/src/wincert.c b/src/wincert.c
>index 3686918..cc612b6 100644
>--- a/src/wincert.c
>+++ b/src/wincert.c
>@@ -19,6 +19,8 @@
> 
> #include "pesign.h"
> 
>+#define ALIGNMENT_PADDING(address, align) ((align - (address % align)) % align)
>+
> struct cert_list_entry {
> 	win_certificate wc;
> 	uint8_t data[];
>@@ -32,6 +34,7 @@ generate_cert_list(SECItem **signatures, int num_signatures,
> 	for (int i = 0; i < num_signatures; i++) {
> 		cl_size += sizeof (win_certificate);
> 		cl_size += signatures[i]->len;
>+		cl_size += ALIGNMENT_PADDING(cl_size, 8);
> 	}
> 
> 	uint8_t *data = malloc(cl_size);
>@@ -45,16 +48,16 @@ generate_cert_list(SECItem **signatures, int num_signatures,
> 		/* pe-coff 8.2 adds some text that says each cert list
> 		 * entry is 8-byte aligned, so that means we need to align
> 		 * them here. */
>-		if ((intptr_t)data % 8 != 0)
>-			data = (uint8_t *)((intptr_t)data + (8 - ((intptr_t)data % 8)));
> 		struct cert_list_entry *cle = (struct cert_list_entry *)data;
> 		cle->wc.length = signatures[i]->len +
>+			ALIGNMENT_PADDING(signatures[i]->len, 8) +
> 			sizeof (win_certificate);
> 		cle->wc.revision = WIN_CERT_REVISION_2_0;
> 		cle->wc.cert_type = WIN_CERT_TYPE_PKCS_SIGNED_DATA;
> 		memcpy(&cle->data[0], signatures[i]->data,
> 					signatures[i]->len);
> 		data += sizeof (win_certificate) + signatures[i]->len;
>+		data += ALIGNMENT_PADDING(signatures[i]->len, 8);
> 	}
> 
> 	return 0;
>@@ -175,11 +178,6 @@ done:
> 
> 		iter->n += sizeof (*tmpcert) + length;
> 
>-		/* each cert list entry must be aligned to an 8-byte
>-		 * boundary */
>-		if (iter->n % 8 != 0)
>-			iter->n += 8 - (iter->n % 8);
>-
> 		return 1;
> 	}
> }
>@@ -222,8 +220,7 @@ get_reserved_sig_space(cms_context *cms, Pe *pe)
> 		ret += cms->signatures[i]->len + sizeof (win_certificate);
> 		/* each certificate list entry must be 8-byte aligned,
> 		 * so we need to account for that in our space calculation */
>-		if (ret % 8 != 0)
>-			ret += 8 - (ret % 8);
>+		ret += ALIGNMENT_PADDING(ret, 8);
> 	}
> 	return ret;
> }
>@@ -253,14 +250,13 @@ err:
> 
> 	size_t res = get_reserved_sig_space(cms, pe);
> 
>-	/* pe-coff 8.2 adds some text that says each cert list entry is
>-	 * 8-byte aligned, so that means we need alignment space here. */
>-	if (res % 8 != 0)
>-		res += 8 - (res % 8);
>-
> 	ssize_t ret = res + sig.len + sizeof(win_certificate) -
> 						available_cert_space(pe);
> 
>+	/* pe-coff 8.2 adds some text that says each cert list entry is
>+	 * 8-byte aligned, so that means we need alignment space here. */
>+	ret += ALIGNMENT_PADDING(ret, 8);
>+
> 	//free(sig.data);
> 
> 	return ret;
>-- 
>1.7.10.4
>
>
>From 12595de05a873712a76e6118f00f324fd257d0f6 Mon Sep 17 00:00:00 2001
>From: Gary Ching-Pang Lin <glin@suse.com>
>Date: Tue, 26 Mar 2013 11:28:57 +0800
>Subject: [PATCH 5/6] Pad the file to be 16-byte aligned, instead of 8-byte
>
>---
> libdpe/pe_allocspace.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/libdpe/pe_allocspace.c b/libdpe/pe_allocspace.c
>index 716373c..8b09153 100644
>--- a/libdpe/pe_allocspace.c
>+++ b/libdpe/pe_allocspace.c
>@@ -119,7 +119,7 @@ pe_allocspace(Pe *pe, size_t size, uint32_t *offset)
> 
> 	/* XXX PJFIX TODO: this should try to find space in the already
> 	 * mapped regions. */
>-	rc = pe_extend_file(pe, size, offset, 8);
>+	rc = pe_extend_file(pe, size, offset, 16);
> 	if (rc < 0)
> 		return -1;
> 	return 0;
>-- 
>1.7.10.4
>
>
>From deb5811f7e718d8d0d9c41ad18d2302876334e7a Mon Sep 17 00:00:00 2001
>From: Gary Ching-Pang Lin <glin@suse.com>
>Date: Tue, 26 Mar 2013 11:34:33 +0800
>Subject: [PATCH 6/6] Add an option, -padding, for -h to pad signatures
>
>We are using "-h" to check the integrity of the file after inserting
>a raw signature. Add this option to make the digests consistent.
>---
> src/pesign.c |    5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
>diff --git a/src/pesign.c b/src/pesign.c
>index 81515d2..57fe96e 100644
>--- a/src/pesign.c
>+++ b/src/pesign.c
>@@ -440,6 +440,7 @@ main(int argc, char *argv[])
> 	int remove = 0;
> 	int daemon = 0;
> 	int fork = 1;
>+	int padding = 0;
> 
> 	char *digest_name = "sha256";
> 	char *tokenname = "NSS Certificate DB";
>@@ -518,6 +519,8 @@ main(int argc, char *argv[])
> 			"run as a daemon process", NULL },
> 		{"nofork", 'N', POPT_ARG_VAL, &fork, 0,
> 			"don't fork when daemonizing", NULL },
>+		{"padding", 'P', POPT_ARG_VAL|POPT_ARGFLAG_DOC_HIDDEN,
>+			&padding, 1, "pad data section", NULL },
> 		POPT_AUTOALIAS
> 		POPT_AUTOHELP
> 		POPT_TABLEEND
>@@ -780,7 +783,7 @@ main(int argc, char *argv[])
> 			break;
> 		case GENERATE_DIGEST|PRINT_DIGEST:
> 			open_input(ctxp);
>-			generate_digest(ctxp->cms_ctx, ctxp->inpe, 0);
>+			generate_digest(ctxp->cms_ctx, ctxp->inpe, padding);
> 			print_digest(ctxp);
> 			break;
> 		/* generate a signature and save it in a separate file */
>-- 
>1.7.10.4
>

Actions: View | Diff

Attachments on bug 808594: 529841 | 531762 | 532050