Skip to content
This repository has been archived by the owner on Apr 17, 2024. It is now read-only.

Commit

Permalink
Use volatile reads/writes in Tink constant-time comparisons.
Browse files Browse the repository at this point in the history
1) The byte reads for the comparison are made volatile, so that the loop doesn't terminate early once it finds a difference.
2) The difference accumulator writes are made volatile, so that writes don't terminate early once it finds a difference. (Even with the reads being volatile, it could, for example, continue accessing memory but not do anything with that memory, as an optimization. Aside from removing xors, this could still improve performance quite a bit, by eliminating data dependency and allowing execution to race ahead of the volatile reads, so I think it isn't any more unrealistic of an optimization. h/t to Richard Smith who pointed this possible optimization out.)

Background:

The C++ and C standards both require that volatile memory accesses occur ~ basically as the programmer expects. Unlike normal reads and normal writes, which can be removed if the compiler deduces that they aren't necessary, volatile memory accesses are considered an observable behavior.

This is one of the only ways in which the standard confesses that an underlying hardware exists, but it handwaves away the exact details as implementation-defined, so even a volatile read is not necessarily enough -- an implementation can define the semantics of a volatile read in a sufficiently convoluted way that it still can enable weird optimizations. In general, though, they aren't that cruel.

So while this doesn't guarantee anything, it at least makes the compiler less likely to introduce a covert channel through clever optimization.

As prior art, consider e.g. CPython's compare_digest function, which employs volatile reads (but not volatile writes): https://github.com/python/cpython/blob/master/Modules/_operator.c#L716-L762

PiperOrigin-RevId: 312400090
  • Loading branch information
ise-crypto authored and Copybara-Service committed May 20, 2020
1 parent 89a1628 commit 335291c
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 7 deletions.
8 changes: 6 additions & 2 deletions cc/subtle/aes_cmac_boringssl.cc
Expand Up @@ -76,9 +76,13 @@ util::Status AesCmacBoringSsl::VerifyMac(absl::string_view mac,
return util::Status(util::error::INTERNAL,
"BoringSSL failed to compute CMAC");
}
uint8_t diff = 0;
// The compiler is required by the standard not to elide volatile reads,
// which helps avoid unfortunate optimizations.
const volatile uint8_t* volatile_buf = buf;
const volatile char* volatile_mac = mac.data();
volatile uint8_t diff = 0;
for (uint32_t i = 0; i < tag_size_; i++) {
diff |= buf[i] ^ static_cast<uint8_t>(mac[i]);
diff |= volatile_buf[i] ^ static_cast<uint8_t>(volatile_mac[i]);
}
if (diff != 0) {
return util::Status(util::error::INVALID_ARGUMENT, "verification failed");
Expand Down
10 changes: 7 additions & 3 deletions cc/subtle/aes_siv_boringssl.cc
Expand Up @@ -226,10 +226,14 @@ util::StatusOr<std::string> AesSivBoringSsl::DecryptDeterministically(
S2v(absl::MakeSpan(reinterpret_cast<const uint8_t*>(additional_data.data()),
additional_data.size()),
absl::MakeSpan(pt), s2v);
// Compare the siv from the ciphertext with the recomputed siv
uint8_t diff = 0;
// Compare the siv from the ciphertext with the recomputed siv.
// The compiler is required by the standard not to elide volatile reads,
// which helps avoid unfortunate optimizations.
const volatile uint8_t* volatile_siv = siv;
const volatile uint8_t* volatile_s2v = s2v;
volatile uint8_t diff = 0;
for (int i = 0; i < kBlockSize; ++i) {
diff |= siv[i] ^ s2v[i];
diff |= volatile_siv[i] ^ volatile_s2v[i];
}
if (diff != 0) {
return util::Status(util::error::INVALID_ARGUMENT, "invalid ciphertext");
Expand Down
8 changes: 6 additions & 2 deletions cc/subtle/hmac_boringssl.cc
Expand Up @@ -96,9 +96,13 @@ util::Status HmacBoringSsl::VerifyMac(
return util::Status(util::error::INTERNAL,
"BoringSSL failed to compute HMAC");
}
uint8_t diff = 0;
// The compiler is required by the standard not to elide volatile reads,
// which helps avoid unfortunate optimizations.
const volatile uint8_t* volatile_buf = buf;
const volatile char* volatile_mac = mac.data();
volatile uint8_t diff = 0;
for (uint32_t i = 0; i < tag_size_; i++) {
diff |= buf[i] ^ static_cast<uint8_t>(mac[i]);
diff |= volatile_buf[i] ^ static_cast<uint8_t>(volatile_mac[i]);
}
if (diff != 0) {
return util::Status(util::error::INVALID_ARGUMENT, "verification failed");
Expand Down

0 comments on commit 335291c

Please sign in to comment.