Repository: randombit/botan
Pull request #1854 (2019-03-10 04:24:14)

Replace upper_bound with lower_bound in pgp_s2k
Fix: RFC4880_encode_count doesn't return consistent results when processing exact iterations. It returns RFC4880 code + 1.

Fixes: #1853 

codecov-io: # [Codecov](https://codecov.io/gh/randombit/botan/pull/1854?src=pr&el=h1) Report
> Merging [#1854](https://codecov.io/gh/randombit/botan/pull/1854?src=pr&el=desc) into [master](https://codecov.io/gh/randombit/botan/commit/2920510d2b519b2a020b937804890dd62b915940?src=pr&el=desc) will **increase** coverage by `1.21%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/randombit/botan/pull/1854/graphs/tree.svg?width=650&token=VR6iYmSPH8&height=150&src=pr)](https://codecov.io/gh/randombit/botan/pull/1854?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##           master   #1854      +/-   ##
=========================================
+ Coverage   90.18%   91.4%   +1.21%     
=========================================
  Files         527     527              
  Lines       55964   55964              
  Branches     5830    5830              
=========================================
+ Hits        50473   51154     +681     
+ Misses       5491    4810     -681
```


| [Impacted Files](https://codecov.io/gh/randombit/botan/pull/1854?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/lib/pbkdf/pgp\_s2k/pgp\_s2k.cpp](https://codecov.io/gh/randombit/botan/pull/1854/diff?src=pr&el=tree#diff-c3JjL2xpYi9wYmtkZi9wZ3BfczJrL3BncF9zMmsuY3Bw) | `87.5% <100%> (ø)` | :arrow_up: |
| [src/lib/pk\_pad/emsa\_raw/emsa\_raw.cpp](https://codecov.io/gh/randombit/botan/pull/1854/diff?src=pr&el=tree#diff-c3JjL2xpYi9wa19wYWQvZW1zYV9yYXcvZW1zYV9yYXcuY3Bw) | `71.42% <0%> (-2.86%)` | :arrow_down: |
| [src/lib/pubkey/dl\_group/dl\_group.cpp](https://codecov.io/gh/randombit/botan/pull/1854/diff?src=pr&el=tree#diff-c3JjL2xpYi9wdWJrZXkvZGxfZ3JvdXAvZGxfZ3JvdXAuY3Bw) | `91.33% <0%> (-1.19%)` | :arrow_down: |
| [src/lib/stream/chacha/chacha.cpp](https://codecov.io/gh/randombit/botan/pull/1854/diff?src=pr&el=tree#diff-c3JjL2xpYi9zdHJlYW0vY2hhY2hhL2NoYWNoYS5jcHA=) | `100% <0%> (+0.93%)` | :arrow_up: |
| [src/lib/hash/sha2\_64/sha2\_64.cpp](https://codecov.io/gh/randombit/botan/pull/1854/diff?src=pr&el=tree#diff-c3JjL2xpYi9oYXNoL3NoYTJfNjQvc2hhMl82NC5jcHA=) | `100% <0%> (+1.11%)` | :arrow_up: |
| [src/lib/hash/sha2\_32/sha2\_32.cpp](https://codecov.io/gh/randombit/botan/pull/1854/diff?src=pr&el=tree#diff-c3JjL2xpYi9oYXNoL3NoYTJfMzIvc2hhMl8zMi5jcHA=) | `98.63% <0%> (+1.36%)` | :arrow_up: |
| [src/lib/hash/sha3/sha3.cpp](https://codecov.io/gh/randombit/botan/pull/1854/diff?src=pr&el=tree#diff-c3JjL2xpYi9oYXNoL3NoYTMvc2hhMy5jcHA=) | `100% <0%> (+1.44%)` | :arrow_up: |
| [src/lib/pubkey/elgamal/elgamal.cpp](https://codecov.io/gh/randombit/botan/pull/1854/diff?src=pr&el=tree#diff-c3JjL2xpYi9wdWJrZXkvZWxnYW1hbC9lbGdhbWFsLmNwcA==) | `96.92% <0%> (+1.53%)` | :arrow_up: |
| [src/lib/misc/cryptobox/cryptobox.cpp](https://codecov.io/gh/randombit/botan/pull/1854/diff?src=pr&el=tree#diff-c3JjL2xpYi9taXNjL2NyeXB0b2JveC9jcnlwdG9ib3guY3Bw) | `95.23% <0%> (+1.58%)` | :arrow_up: |
| [src/lib/block/threefish\_512/threefish\_512.cpp](https://codecov.io/gh/randombit/botan/pull/1854/diff?src=pr&el=tree#diff-c3JjL2xpYi9ibG9jay90aHJlZWZpc2hfNTEyL3RocmVlZmlzaF81MTIuY3Bw) | `100% <0%> (+3.12%)` | :arrow_up: |
| ... and [8 more](https://codecov.io/gh/randombit/botan/pull/1854/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/randombit/botan/pull/1854?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/randombit/botan/pull/1854?src=pr&el=footer). Last update [2920510...4e240e8](https://codecov.io/gh/randombit/botan/pull/1854?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

randombit: Sorry about that and thanks for the patch. Can you add something to the test (in `test_pbkdf.cpp`) that would detect this issue?
evpo: No worries, man. I will look at the test.

Patch:
diff --git a/src/lib/pbkdf/pgp_s2k/pgp_s2k.cpp b/src/lib/pbkdf/pgp_s2k/pgp_s2k.cpp
index d04fb47b3..8bcf9239f 100644
--- a/src/lib/pbkdf/pgp_s2k/pgp_s2k.cpp
+++ b/src/lib/pbkdf/pgp_s2k/pgp_s2k.cpp
@@ -61,7 +61,7 @@ uint8_t RFC4880_encode_count(size_t desired_iterations)
    if(desired_iterations >= OPENPGP_S2K_ITERS[255])
       return 255;
 
-   auto i = std::upper_bound(OPENPGP_S2K_ITERS, OPENPGP_S2K_ITERS + 256, desired_iterations);
+   auto i = std::lower_bound(OPENPGP_S2K_ITERS, OPENPGP_S2K_ITERS + 256, desired_iterations);
 
    return static_cast<uint8_t>(i - OPENPGP_S2K_ITERS);
    }


Test patch:
diff --git a/src/tests/test_pbkdf.cpp b/src/tests/test_pbkdf.cpp
index 20a25165b..34ed64c3f 100644
--- a/src/tests/test_pbkdf.cpp
+++ b/src/tests/test_pbkdf.cpp
@@ -214,6 +214,9 @@ class PGP_S2K_Iter_Test final : public Test
             const size_t dec = Botan::RFC4880_decode_count(static_cast<uint8_t>(c));
             const size_t comp_dec = (16 + (c & 0x0F)) << ((c >> 4) + 6);
             result.test_eq("Decoded value matches PGP formula", dec, comp_dec);
+
+            const size_t enc = Botan::RFC4880_encode_count(comp_dec);
+            result.test_eq("Encoded value matches PGP formula", enc, c);
             }
 
          uint8_t last_enc = 0;