Repository: randombit/botan (cpp)

Issue:
FFI: botan_cipher_get_update_granularity() returns 17
....which is quite unlogical if application relies on this value.
Root of this is in the `ffi_choose_update_size()` function and the fact that `mode.update_granularity()` returns 1.

Pull request description:
FIX: Better scaling for botan_cipher_get_update_granularity()
This is an alternative suggestion to address #4090. Essentially, it keeps the constraint that `botan_cipher_get_update_granularity()` should always return a size greater than the `minimal_final_size()` of the underlying cipher mode. But, it tries to produce more sensible outputs that actually honor the documented behavior ("ideal granularity" being a multiple of this).

_To the best of my understanding, this is needed to handle the authentication tag of AEADs without special treatment of the final API buffer. But frankly, I don't fully understand the (historic) reasoning about it._

With the changes that came in #3951 this extra complexity is not needed anymore. At the expense of users handling the `BOTAN_FFI_ERROR_INSUFFICIENT_BUFFER_SPACE` error code properly. Therefore, I strongly suggest we implement the (vaguely) API changing suggestion of #4094 in the next major release. (How do we ensure that this won't be forgotten?)

Closes #4090.

Patch:
diff --git a/src/lib/ffi/ffi_cipher.cpp b/src/lib/ffi/ffi_cipher.cpp
index a72ea50f5..c619fd718 100644
--- a/src/lib/ffi/ffi_cipher.cpp
+++ b/src/lib/ffi/ffi_cipher.cpp
@@ -7,9 +7,12 @@
 #include <botan/ffi.h>
 
 #include <botan/aead.h>
+#include <botan/internal/bit_ops.h>
 #include <botan/internal/ffi_util.h>
 #include <botan/internal/stl_util.h>
 
+#include <limits>
+
 extern "C" {
 
 using namespace Botan_FFI;
@@ -38,31 +41,54 @@ struct botan_cipher_struct final : public botan_struct<Botan::Cipher_Mode, 0xB4A
 
 namespace {
 
+/**
+ * Select an update size so that the following constraints are satisfies:
+ *
+ *   - greater than or equal to the mode's update granularity
+ *   - greater than the mode's minimum final size
+ *   - the mode's ideal update granularity is a multiple of this size
+ *   - (optional) a power of 2
+ *
+ * Note that this is necessary mostly for backward-compatibility with previous
+ * versions of the FFI (prior to Botan 3.5.0). For Botan 4.0.0 we should just
+ * directly return the update granularity of the cipher mode and instruct users
+ * to switch to botan_cipher_get_ideal_update_granularity() instead. See also
+ * the discussion in GitHub Issue #4090.
+ */
 size_t ffi_choose_update_size(Botan::Cipher_Mode& mode) {
    const size_t update_granularity = mode.update_granularity();
+   const size_t ideal_update_granularity = mode.ideal_granularity();
    const size_t minimum_final_size = mode.minimum_final_size();
 
-   /*
-   * Return the minimum possible granularity given the FFI API constraints that
-   * we require the returned size be > minimum final size.
-   *
-   * If the minimum final size is zero, or the update_granularity is
-   * already greater, just use that.
-   *
-   * Otherwise scale the update_granularity to a sufficient size
-   * to be greater than the minimum.
-   */
+   // If the minimum final size is zero, or the update_granularity is
+   // already greater, just use that.
    if(minimum_final_size == 0 || update_granularity > minimum_final_size) {
       BOTAN_ASSERT_NOMSG(update_granularity > 0);
       return update_granularity;
    }
 
-   size_t buf_size = std::max(update_granularity, minimum_final_size + 1);
-   if(buf_size % update_granularity != 0) {
-      buf_size += update_granularity - (buf_size % update_granularity);
+   // If the ideal granularity is a multiple of the minimum final size, we
+   // might be able to use that if it stays within the ideal granularity.
+   if(ideal_update_granularity % minimum_final_size == 0 && minimum_final_size * 2 <= ideal_update_granularity) {
+      return minimum_final_size * 2;
+   }
+
+   // Otherwise, try to use the next power of two greater than the minimum
+   // final size, if the ideal granularity is a multiple of that.
+   BOTAN_ASSERT_NOMSG(minimum_final_size <= std::numeric_limits<uint16_t>::max());
+   const size_t b1 = size_t(1) << Botan::ceil_log2(static_cast<uint16_t>(minimum_final_size));
+   if(ideal_update_granularity % b1 == 0) {
+      return b1;
    }
 
-   return buf_size;
+   // Last resort: Find the next integer greater than the minimum final size
+   //              for which the ideal granularity is a multiple of.
+   //              Most sensible cipher modes should never reach this point.
+   BOTAN_ASSERT_NOMSG(minimum_final_size < ideal_update_granularity);
+   size_t b2 = minimum_final_size + 1;
+   for(; b2 < ideal_update_granularity && ideal_update_granularity % b2 != 0; ++b2) {}
+
+   return b2;
 }
 
 }  // namespace


Test patch:
diff --git a/src/tests/test_ffi.cpp b/src/tests/test_ffi.cpp
index d74c41500..d43920cba 100644
--- a/src/tests/test_ffi.cpp
+++ b/src/tests/test_ffi.cpp
@@ -792,6 +792,18 @@ class FFI_CBC_Cipher_Test final : public FFI_Test {
                result.test_eq("Expected size of padded message", ctext_len, plaintext.size() + 15);
                std::vector<uint8_t> ciphertext(ctext_len);
 
+               size_t update_granularity = 0;
+               size_t ideal_granularity = 0;
+               size_t taglen = 0;
+
+               TEST_FFI_OK(botan_cipher_get_update_granularity, (cipher_encrypt, &update_granularity));
+               TEST_FFI_OK(botan_cipher_get_ideal_update_granularity, (cipher_encrypt, &ideal_granularity));
+               TEST_FFI_OK(botan_cipher_get_tag_length, (cipher_encrypt, &taglen));
+
+               result.test_eq(
+                  "ideal granularity is a multiple of update granularity", ideal_granularity % update_granularity, 0);
+               result.test_eq("not an AEAD, hence no tag", taglen, 0);
+
                TEST_FFI_OK(botan_cipher_set_key, (cipher_encrypt, symkey.data(), symkey.size()));
                TEST_FFI_OK(botan_cipher_start, (cipher_encrypt, nonce.data(), nonce.size()));
                TEST_FFI_OK(botan_cipher_update,
@@ -827,6 +839,15 @@ class FFI_CBC_Cipher_Test final : public FFI_Test {
 
                   TEST_FFI_RC(0, botan_cipher_is_authenticated, (cipher_encrypt));
 
+                  TEST_FFI_OK(botan_cipher_get_update_granularity, (cipher_decrypt, &update_granularity));
+                  TEST_FFI_OK(botan_cipher_get_ideal_update_granularity, (cipher_decrypt, &ideal_granularity));
+                  TEST_FFI_OK(botan_cipher_get_tag_length, (cipher_decrypt, &taglen));
+
+                  result.test_eq("ideal granularity is a multiple of update granularity (decrypt)",
+                                 ideal_granularity % update_granularity,
+                                 0);
+                  result.test_eq("not an AEAD, hence no tag (decrypt)", taglen, 0);
+
                   TEST_FFI_OK(botan_cipher_set_key, (cipher_decrypt, symkey.data(), symkey.size()));
                   TEST_FFI_OK(botan_cipher_start, (cipher_decrypt, nonce.data(), nonce.size()));
                   TEST_FFI_OK(botan_cipher_update,
@@ -875,6 +896,14 @@ class FFI_GCM_Test final : public FFI_Test {
             size_t max_keylen = 0;
             size_t nonce_len = 0;
             size_t tag_len = 0;
+            size_t update_granularity = 0;
+            size_t ideal_granularity = 0;
+
+            TEST_FFI_OK(botan_cipher_get_update_granularity, (cipher_encrypt, &update_granularity));
+            TEST_FFI_OK(botan_cipher_get_ideal_update_granularity, (cipher_encrypt, &ideal_granularity));
+
+            result.test_eq(
+               "ideal granularity is a multiple of update granularity", ideal_granularity % update_granularity, 0);
 
             TEST_FFI_OK(botan_cipher_query_keylen, (cipher_encrypt, &min_keylen, &max_keylen));
             result.test_int_eq(min_keylen, 16, "Min key length");
@@ -948,6 +977,13 @@ class FFI_GCM_Test final : public FFI_Test {
                if(TEST_FFI_OK(botan_cipher_init, (&cipher_decrypt, "AES-128/GCM", BOTAN_CIPHER_INIT_FLAG_DECRYPT))) {
                   std::vector<uint8_t> decrypted(plaintext.size());
 
+                  TEST_FFI_OK(botan_cipher_get_update_granularity, (cipher_decrypt, &update_granularity));
+                  TEST_FFI_OK(botan_cipher_get_ideal_update_granularity, (cipher_decrypt, &ideal_granularity));
+
+                  result.test_eq("ideal granularity is a multiple of update granularity (decrypt)",
+                                 ideal_granularity % update_granularity,
+                                 0);
+
                   TEST_FFI_OK(botan_cipher_set_key, (cipher_decrypt, symkey.data(), symkey.size()));
                   TEST_FFI_OK(botan_cipher_set_associated_data, (cipher_decrypt, aad.data(), aad.size()));
                   TEST_FFI_OK(botan_cipher_start, (cipher_decrypt, nonce.data(), nonce.size()));
@@ -997,6 +1033,14 @@ class FFI_ChaCha20Poly1305_Test final : public FFI_Test {
             size_t max_keylen = 0;
             size_t nonce_len = 0;
             size_t tag_len = 0;
+            size_t update_granularity = 0;
+            size_t ideal_granularity = 0;
+
+            TEST_FFI_OK(botan_cipher_get_update_granularity, (cipher_encrypt, &update_granularity));
+            TEST_FFI_OK(botan_cipher_get_ideal_update_granularity, (cipher_encrypt, &ideal_granularity));
+
+            result.test_eq(
+               "ideal granularity is a multiple of update granularity", ideal_granularity % update_granularity, 0);
 
             TEST_FFI_OK(botan_cipher_query_keylen, (cipher_encrypt, &min_keylen, &max_keylen));
             result.test_int_eq(min_keylen, 32, "Min key length");
@@ -1065,6 +1109,13 @@ class FFI_ChaCha20Poly1305_Test final : public FFI_Test {
                               (&cipher_decrypt, "ChaCha20Poly1305", BOTAN_CIPHER_INIT_FLAG_DECRYPT))) {
                   std::vector<uint8_t> decrypted(plaintext.size());
 
+                  TEST_FFI_OK(botan_cipher_get_update_granularity, (cipher_decrypt, &update_granularity));
+                  TEST_FFI_OK(botan_cipher_get_ideal_update_granularity, (cipher_decrypt, &ideal_granularity));
+
+                  result.test_eq("ideal granularity is a multiple of update granularity (decrypt)",
+                                 ideal_granularity % update_granularity,
+                                 0);
+
                   TEST_FFI_OK(botan_cipher_set_key, (cipher_decrypt, symkey.data(), symkey.size()));
                   TEST_FFI_OK(botan_cipher_set_associated_data, (cipher_decrypt, aad.data(), aad.size()));
                   TEST_FFI_OK(botan_cipher_start, (cipher_decrypt, nonce.data(), nonce.size()));
@@ -1104,6 +1155,14 @@ class FFI_EAX_Test final : public FFI_Test {
             size_t mod_keylen = 0;
             size_t nonce_len = 0;
             size_t tag_len = 0;
+            size_t update_granularity = 0;
+            size_t ideal_granularity = 0;
+
+            TEST_FFI_OK(botan_cipher_get_update_granularity, (cipher_encrypt, &update_granularity));
+            TEST_FFI_OK(botan_cipher_get_ideal_update_granularity, (cipher_encrypt, &ideal_granularity));
+
+            result.test_eq(
+               "ideal granularity is a multiple of update granularity", ideal_granularity % update_granularity, 0);
 
             TEST_FFI_OK(botan_cipher_query_keylen, (cipher_encrypt, &min_keylen, &max_keylen));
             result.test_int_eq(min_keylen, 16, "Min key length");
@@ -1171,6 +1230,13 @@ class FFI_EAX_Test final : public FFI_Test {
                if(TEST_FFI_OK(botan_cipher_init, (&cipher_decrypt, "AES-128/EAX", BOTAN_CIPHER_INIT_FLAG_DECRYPT))) {
                   std::vector<uint8_t> decrypted(plaintext.size());
 
+                  TEST_FFI_OK(botan_cipher_get_update_granularity, (cipher_decrypt, &update_granularity));
+                  TEST_FFI_OK(botan_cipher_get_ideal_update_granularity, (cipher_decrypt, &ideal_granularity));
+
+                  result.test_eq("ideal granularity is a multiple of update granularity (decrypt)",
+                                 ideal_granularity % update_granularity,
+                                 0);
+
                   TEST_FFI_OK(botan_cipher_set_key, (cipher_decrypt, symkey.data(), symkey.size()));
                   TEST_FFI_OK(botan_cipher_start, (cipher_decrypt, nonce.data(), nonce.size()));
                   TEST_FFI_OK(botan_cipher_update,
@@ -1221,15 +1287,20 @@ class FFI_AEAD_Test final : public FFI_Test {
 
             size_t min_keylen = 0;
             size_t max_keylen = 0;
+            size_t update_granularity = 0;
             size_t ideal_granularity = 0;
             size_t noncelen = 0;
             size_t taglen = 0;
             constexpr size_t pt_multiplier = 5;
             TEST_FFI_OK(botan_cipher_query_keylen, (cipher_encrypt, &min_keylen, &max_keylen));
+            TEST_FFI_OK(botan_cipher_get_update_granularity, (cipher_encrypt, &update_granularity));
             TEST_FFI_OK(botan_cipher_get_ideal_update_granularity, (cipher_encrypt, &ideal_granularity));
             TEST_FFI_OK(botan_cipher_get_default_nonce_length, (cipher_encrypt, &noncelen));
             TEST_FFI_OK(botan_cipher_get_tag_length, (cipher_encrypt, &taglen));
 
+            result.test_eq(
+               "ideal granularity is a multiple of update granularity", ideal_granularity % update_granularity, 0);
+
             std::vector<uint8_t> key(max_keylen);
             TEST_FFI_OK(botan_rng_get, (rng, key.data(), key.size()));
             TEST_FFI_OK(botan_cipher_set_key, (cipher_encrypt, key.data(), key.size()));
@@ -1333,6 +1404,14 @@ class FFI_AEAD_Test final : public FFI_Test {
             // ----------------------------------------------------------------
 
             TEST_FFI_INIT(botan_cipher_init, (&cipher_decrypt, aead.c_str(), BOTAN_CIPHER_INIT_FLAG_DECRYPT));
+
+            TEST_FFI_OK(botan_cipher_get_update_granularity, (cipher_decrypt, &update_granularity));
+            TEST_FFI_OK(botan_cipher_get_ideal_update_granularity, (cipher_decrypt, &ideal_granularity));
+
+            result.test_eq("ideal granularity is a multiple of update granularity (decrypt)",
+                           ideal_granularity % update_granularity,
+                           0);
+
             TEST_FFI_OK(botan_cipher_set_key, (cipher_decrypt, key.data(), key.size()));
             TEST_FFI_OK(botan_cipher_start, (cipher_decrypt, nonce.data(), nonce.size()));
 
@@ -1425,6 +1504,15 @@ class FFI_StreamCipher_Test final : public FFI_Test {
 
             std::vector<uint8_t> ct(pt.size());
 
+            size_t update_granularity = 0;
+            size_t ideal_granularity = 0;
+
+            TEST_FFI_OK(botan_cipher_get_update_granularity, (ctr, &update_granularity));
+            TEST_FFI_OK(botan_cipher_get_ideal_update_granularity, (ctr, &ideal_granularity));
+
+            result.test_eq(
+               "ideal granularity is a multiple of update granularity", ideal_granularity % update_granularity, 0);
+
             TEST_FFI_RC(0, botan_cipher_is_authenticated, (ctr));
 
             size_t input_consumed = 0;