From 824fb03cdb08ca2cfb23ba0acf37a12b77f54b24 Mon Sep 17 00:00:00 2001
From: Morten Linderud <morten@linderud.pw>
Date: Mon, 12 Aug 2024 20:38:36 +0200
Subject: [PATCH] keys: ensure we compare the input/output files

We only used the path for this instead of comparing the authenticode
hashes.

Signed-off-by: Morten Linderud <morten@linderud.pw>
---
 backend/backend.go |  9 ++-----
 keys.go            | 65 +++++++++++++++++++++++++++++++++-------------
 2 files changed, 49 insertions(+), 25 deletions(-)

diff --git a/backend/backend.go b/backend/backend.go
index 072bdb0..ce32407 100644
--- a/backend/backend.go
+++ b/backend/backend.go
@@ -180,16 +180,11 @@ func (k *KeyHierarchy) VerifyFile(hier hierarchy.Hierarchy, r io.ReaderAt) (bool
 	return ok, nil
 }
 
-func (k *KeyHierarchy) SignFile(hier hierarchy.Hierarchy, r io.ReaderAt) ([]byte, error) {
+func (k *KeyHierarchy) SignFile(hier hierarchy.Hierarchy, peBinary *authenticode.PECOFFBinary) ([]byte, error) {
 	kk := k.GetKeyBackend(hier.Efivar())
 	signer := kk.Signer()
 
-	peBinary, err := authenticode.Parse(r)
-	if err != nil {
-		return nil, err
-	}
-
-	_, err = peBinary.Sign(signer, kk.Certificate())
+	_, err := peBinary.Sign(signer, kk.Certificate())
 	if err != nil {
 		return nil, err
 	}
diff --git a/keys.go b/keys.go
index 019cb51..650c821 100644
--- a/keys.go
+++ b/keys.go
@@ -1,6 +1,8 @@
 package sbctl
 
 import (
+	"bytes"
+	"crypto"
 	"errors"
 	"fmt"
 	"os"
@@ -32,6 +34,8 @@ func VerifyFile(state *config.State, kh *backend.KeyHierarchy, ev hierarchy.Hier
 var ErrAlreadySigned = errors.New("already signed file")
 
 func SignFile(state *config.State, kh *backend.KeyHierarchy, ev hierarchy.Hierarchy, file, output string) error {
+	// Check to see if input and output binary is the same
+	var same bool
 
 	// Make sure that output is always populated by atleast the file path
 	if output == "" {
@@ -43,23 +47,6 @@ func SignFile(state *config.State, kh *backend.KeyHierarchy, ev hierarchy.Hierar
 		return fmt.Errorf("%s does not exist", file)
 	}
 
-	// Let's check if we have signed it already AND the original file hasn't changed
-	ok, err := VerifyFile(state, kh, ev, output)
-	if errors.Is(err, os.ErrNotExist) && (file != output) {
-		// if the file does not exist and file is not the same as output
-		// then we just catch the error and continue. This is expected
-		// behaviour
-	} else if errors.Is(err, authenticode.ErrNoValidSignatures) {
-		// If we tried to verify the file, but it has signatures but nothing signed
-		// by our key, we catch the error and continue.
-	} else if err != nil {
-		return err
-	}
-
-	if ok {
-		return ErrAlreadySigned
-	}
-
 	// We want to write the file back with correct permissions
 	si, err := state.Fs.Stat(file)
 	if err != nil {
@@ -72,7 +59,49 @@ func SignFile(state *config.State, kh *backend.KeyHierarchy, ev hierarchy.Hierar
 	}
 	defer peFile.Close()
 
-	b, err := kh.SignFile(ev, peFile)
+	inputBinary, err := authenticode.Parse(peFile)
+	if err != nil {
+		return err
+	}
+
+	// Check if the files are identical
+	if file != output {
+		outputFile, err := state.Fs.Open(output)
+		if err != nil {
+			return err
+		}
+		defer outputFile.Close()
+		outputBinary, err := authenticode.Parse(outputFile)
+		if err != nil {
+			return err
+		}
+		b := outputBinary.Hash(crypto.SHA256)
+		bb := inputBinary.Hash(crypto.SHA256)
+		if bytes.Equal(b, bb) {
+			same = true
+		}
+	}
+
+	if file == output {
+		same = true
+	}
+
+	// Let's check if we have signed it already AND the original file hasn't changed
+	// TODO: This will run authenticode.Parse again, *and* open the file
+	// this should be refactored to be nicer
+	ok, err := VerifyFile(state, kh, ev, output)
+	if errors.Is(err, authenticode.ErrNoValidSignatures) {
+		// If we tried to verify the file, but it has signatures but nothing signed
+		// by our key, we catch the error and continue.
+	} else if ok && same {
+		// If already signed, and the input/output binaries are identical,
+		// we can just assume everything is fine.
+		return ErrAlreadySigned
+	} else if err != nil {
+		return err
+	}
+
+	b, err := kh.SignFile(ev, inputBinary)
 	if err != nil {
 		return err
 	}