Add sanity checks to FIPS module construction.
If -ffunction-sections or -fdata-sections is enabled when doing a FIPS
shared build, the linker script won't do what's expected and will
silently end up including very little (or nothing) in the integrity
check.
This changes alters the linker script to discard any text or data
sections other than the main one, which should make this failure much
more obvious.
Also, add assertions (that are always enabled) in the module to check
that a few obvious things that should be inside the module boundaries
actually are.
Change-Id: I91178e213a28a7c0c4a38155974e452cd9d558d1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/38324
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/fipsmodule/bcm.c b/crypto/fipsmodule/bcm.c
index f3f66f4..91b9170 100644
--- a/crypto/fipsmodule/bcm.c
+++ b/crypto/fipsmodule/bcm.c
@@ -115,6 +115,26 @@
extern const uint8_t BORINGSSL_bcm_rodata_end[];
#endif
+// assert_within is used to sanity check that certain symbols are within the
+// bounds of the integrity check. It checks that start <= symbol < end and
+// aborts otherwise.
+static void assert_within(const void *start, const void *symbol,
+ const void *end) {
+ const uintptr_t start_val = (uintptr_t) start;
+ const uintptr_t symbol_val = (uintptr_t) symbol;
+ const uintptr_t end_val = (uintptr_t) end;
+
+ if (start_val <= symbol_val && symbol_val < end_val) {
+ return;
+ }
+
+ fprintf(
+ stderr,
+ "FIPS module doesn't span expected symbol. Expected %p <= %p < %p\n",
+ start, symbol, end);
+ BORINGSSL_FIPS_abort();
+}
+
#if defined(OPENSSL_ANDROID) && defined(OPENSSL_AARCH64)
static void BORINGSSL_maybe_set_module_text_permissions(int permission) {
// Android may be compiled in execute-only-memory mode, in which case the
@@ -147,11 +167,29 @@
// .text section, which triggers the global-buffer overflow detection.
const uint8_t *const start = BORINGSSL_bcm_text_start;
const uint8_t *const end = BORINGSSL_bcm_text_end;
+
+ assert_within(start, AES_encrypt, end);
+ assert_within(start, RSA_sign, end);
+ assert_within(start, RAND_bytes, end);
+ assert_within(start, EC_GROUP_cmp, end);
+ assert_within(start, SHA256_Update, end);
+ assert_within(start, ECDSA_do_verify, end);
+ assert_within(start, EVP_AEAD_CTX_seal, end);
+
#if defined(BORINGSSL_SHARED_LIBRARY)
const uint8_t *const rodata_start = BORINGSSL_bcm_rodata_start;
const uint8_t *const rodata_end = BORINGSSL_bcm_rodata_end;
+#else
+ // In the static build, read-only data is placed within the .text segment.
+ const uint8_t *const rodata_start = BORINGSSL_bcm_text_start;
+ const uint8_t *const rodata_end = BORINGSSL_bcm_text_end;
#endif
+ assert_within(rodata_start, kPrimes, rodata_end);
+ assert_within(rodata_start, des_skb, rodata_end);
+ assert_within(rodata_start, kP256Params, rodata_end);
+ assert_within(rodata_start, kPKCS1SigPrefixes, rodata_end);
+
#if defined(OPENSSL_ANDROID)
uint8_t result[SHA256_DIGEST_LENGTH];
const EVP_MD *const kHashFunction = EVP_sha256();
diff --git a/crypto/fipsmodule/fips_shared.lds b/crypto/fipsmodule/fips_shared.lds
index 6d44abc..c3db101 100644
--- a/crypto/fipsmodule/fips_shared.lds
+++ b/crypto/fipsmodule/fips_shared.lds
@@ -15,5 +15,11 @@
*(.rela.dyn)
*(.data)
*(.rel.ro)
+ *(*.text.*)
+ *(*.data.*)
+
+ /* This should be included to catch any unexpected rodata subsections, but
+ it crashes the linker!
+ *(*.rodata.*) */
}
}
diff --git a/util/fipstools/delocate/delocate.go b/util/fipstools/delocate/delocate.go
index a43b428..92b4c31 100644
--- a/util/fipstools/delocate/delocate.go
+++ b/util/fipstools/delocate/delocate.go
@@ -803,6 +803,8 @@
instrCombine
// instrThreeArg merges two sources into a destination in some fashion.
instrThreeArg
+ // instrCompare takes two arguments and writes outputs to the flags register.
+ instrCompare
instrOther
)
@@ -833,6 +835,11 @@
return instrCombine
}
+ case "cmpq":
+ if len(args) == 2 {
+ return instrCompare
+ }
+
case "sarxq", "shlxq", "shrxq":
if len(args) == 3 {
return instrThreeArg
@@ -855,6 +862,13 @@
}
}
+func compare(w stringWriter, instr, a, b string) wrapperFunc {
+ return func(k func()) {
+ k()
+ w.WriteString(fmt.Sprintf("\t%s %s, %s\n", instr, a, b))
+ }
+}
+
func (d *delocation) loadFromGOT(w stringWriter, destination, symbol, section string, redzoneCleared bool) wrapperFunc {
d.gotExternalsNeeded[symbol+"@"+section] = struct{}{}
@@ -1074,7 +1088,7 @@
}
classification := classifyInstruction(instructionName, argNodes)
- if classification != instrThreeArg && i != 0 {
+ if classification != instrThreeArg && classification != instrCompare && i != 0 {
return nil, errors.New("GOT access must be source operand")
}
@@ -1091,6 +1105,17 @@
case instrMove:
assertNodeType(argNodes[1], ruleRegisterOrConstant)
targetReg = d.contents(argNodes[1])
+ case instrCompare:
+ otherSource := d.contents(argNodes[i^1])
+ saveRegWrapper, tempReg := saveRegister(d.output, []string{otherSource})
+ redzoneCleared = true
+ wrappers = append(wrappers, saveRegWrapper)
+ if i == 0 {
+ wrappers = append(wrappers, compare(d.output, instructionName, tempReg, otherSource))
+ } else {
+ wrappers = append(wrappers, compare(d.output, instructionName, otherSource, tempReg))
+ }
+ targetReg = tempReg
case instrTransformingMove:
assertNodeType(argNodes[1], ruleRegisterOrConstant)
targetReg = d.contents(argNodes[1])
@@ -1114,7 +1139,7 @@
return nil, fmt.Errorf("three-argument instruction has %d arguments", n)
}
if i != 0 && i != 1 {
- return nil, errors.New("GOT access must be from soure operand")
+ return nil, errors.New("GOT access must be from source operand")
}
targetReg = d.contents(argNodes[2])
diff --git a/util/fipstools/delocate/testdata/x86_64-GOTRewrite/in.s b/util/fipstools/delocate/testdata/x86_64-GOTRewrite/in.s
index ccbc0bf..398032f 100644
--- a/util/fipstools/delocate/testdata/x86_64-GOTRewrite/in.s
+++ b/util/fipstools/delocate/testdata/x86_64-GOTRewrite/in.s
@@ -44,4 +44,7 @@
vpbroadcastq stderr@GOTPCREL(%rip), %xmm0
vpbroadcastq foo@GOTPCREL(%rip), %xmm0
+ cmpq foo@GOTPCREL(%rip), %rax
+ cmpq %rax, foo@GOTPCREL(%rip)
+
.comm foobar,64,32
diff --git a/util/fipstools/delocate/testdata/x86_64-GOTRewrite/out.s b/util/fipstools/delocate/testdata/x86_64-GOTRewrite/out.s
index 273cfe9..e14a71e 100644
--- a/util/fipstools/delocate/testdata/x86_64-GOTRewrite/out.s
+++ b/util/fipstools/delocate/testdata/x86_64-GOTRewrite/out.s
@@ -172,6 +172,21 @@
leaq 128(%rsp), %rsp
vpbroadcastq %xmm0, %xmm0
+# WAS cmpq foo@GOTPCREL(%rip), %rax
+ leaq -128(%rsp), %rsp
+ pushq %rbx
+ leaq .Lfoo_local_target(%rip), %rbx
+ cmpq %rbx, %rax
+ popq %rbx
+ leaq 128(%rsp), %rsp
+# WAS cmpq %rax, foo@GOTPCREL(%rip)
+ leaq -128(%rsp), %rsp
+ pushq %rbx
+ leaq .Lfoo_local_target(%rip), %rbx
+ cmpq %rax, %rbx
+ popq %rbx
+ leaq 128(%rsp), %rsp
+
.comm foobar,64,32
.text
.loc 1 2 0
