Kovri: new base32/64 impl / radix interface / unit-test #788

anonimal · 2018-01-10T02:57:48Z

Removes i2pd's base32/64 impl, introduces new impl using crypto++
Includes related rewrites, refactors, new documentation, and dead code removal

Resolves #786. Works into #785.

By submitting this pull-request, I confirm the following:

I have read and understood the contributor guide in kovri-docs.
I have checked that another pull-request for this purpose does not exist.
I have considered and confirmed that this submission will be valuable to others.
I accept that this submission may not be used and that this pull-request may be closed by the will of the maintainer.
I give this submission freely under the BSD 3-clause license.

anonimal · 2018-01-10T02:59:55Z

Live tests and all new test-cases pass (only tested on Linux, awaiting buildbot).

coneiric

Overall, except for a couple minor things, the changes lgtm.

The new interface and code are really clean, and the exception handling is very nice.

coneiric · 2018-01-10T07:30:29Z

src/core/crypto/impl/cryptopp/radix.cc

+std::string Base32::Encode(const std::uint8_t* in, const std::uint64_t len)
+{
+  // Prepare encoder
+  CryptoPP::AlgorithmParameters static const params(CryptoPP::MakeParameters(


would you be open to adding a documentation line pointing to the CryptoPP Manual page for AlgorithmParameters (https://www.cryptopp.com/docs/ref/class_algorithm_parameters.html)?

Behavior for MakeParameters and AlgorithmParameters was hard to understand from reading the code, until I read the manual page, and it became very obvious.

Mainly the confusing bit was where AlgorithmParameters overrides the () operator to append a key-value pair, and return an AlgorithmParameters ref (allowing one to append an arbitrary number of key-value pairs).

would you be open to adding a documentation line pointing to the CryptoPP Manual page for AlgorithmParameters

This isn't necessary because this type of c++ / crypto++ question is outside the scope of kovri. Adding links to code is generally a bad idea too (links become outdated) and doing so would create a precedent for endless links. Crypto++'s manual merely produces doxygen output so you can read the same docs within the code (see deps/cryptopp) or visit their website (as you did).

coneiric · 2018-01-10T07:33:49Z

src/core/router/info.cc

+                    }
+
+                  //TODO(anonimal): let's try to avoid a memcpy
+                  std::memcpy(address.key, key.data(), sizeof(address.key));


not easily searchable for a secure alternative. from cppreference:

If either dest or src is a null pointer, the behavior is undefined, even if count is zero.

If the objects are not TriviallyCopyable, the behavior of memcpy is not specified and may be undefined

For the first case, checking that buffers are non-null seems like an easy fix.

Maybe for the TriviallyCopyable case, could use:

try { bool address_key_trivially_copyable = is_trivially_copyable<address.key>::value; bool key_trivially_copyable ... if !address_key_trivially_copyable || !key_trivially_copyable throw } catch (...)

Know this doesn't fully address the "secure memcpy" problem, but would it help make the current situation more secure?

This TODO was a note for me to implement address.key as a container and/or implement an overloaded operator for Tag and/or rewrite/throw-out Tag. See also #366 and #784 (note: a public address key in this context should not need #784).

The resolution of any of those aforementioned points should also resolve your concerns Edit: but is outside the scope of this PR.

anonimal · 2018-01-10T23:27:48Z

FreeBSD's unit-test fails in test-case ParseACL though all other platforms pass. Hold on merging.

anonimal · 2018-01-11T23:23:14Z

Here's a backtrace (note: the issue is reproducible with clang++39):

#0  0x000000080349339a in thr_kill () from /lib/libc.so.7
#1  0x0000000803493386 in raise () from /lib/libc.so.7
#2  0x0000000803493309 in abort () from /lib/libc.so.7
#3  0x000000000047a78f in __clang_call_terminate () at ios:507
#4  0x000000000059651e in CryptoPP::member_ptr<CryptoPP::AlgorithmParametersBase>::~member_ptr (this=0x7fffffffca80) at smartptr.h:71
#5  0x0000000000595481 in CryptoPP::AlgorithmParameters::~AlgorithmParameters (this=0x7fffffffca78) at algparam.h:433
#6  0x0000000000594e5d in kovri::core::Base32::Decode (in=0x805032a99 "", len=0) at /usr/home/anonimal/kovri/src/core/crypto/impl/cryptopp/radix.cc:76
#7  0x00000000007a2183 in kovri::core::Tag<32>::FromBase32 (this=0x7fffffffd1c0, encoded=@0x805032a98) at identity.h:124
#8  0x000000000092bf90 in kovri::client::ParseACL (list=
      {<std::__1::__basic_string_common<true>> = {<No data fields>}, __r_ = {<std::__1::__libcpp_compressed_pair_imp<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::__rep, std::__1::allocator<char>, 2>> = {<std::__1::allocator<char>> = {<No data fields>}, __first_ = {{__l = {__cap_ = 177, __size_ = 164, __data_ = 0x805045300 "7ooubi6turxmjmtrpx2sq2mfpmjaywr3aqzs6skii4lwiwipcmoa,,,,65m4tm6zwzn3wkj2c56abig7vjrxnucszbsdpwrog3cocpxru2hq,,,,3pz774osmoosan7opdjyzld2knlyizwnggfounbzmd6m5tz5brla"}, __s = {{__size_ = 177 '', __lx = -79 ''}, __data_ = 0x7fffffffd531 ""}, __r = {__words = 0x7fffffffd530}}}}, <No data fields>}, static npos = <optimized out>}) at /usr/home/anonimal/kovri/src/client/util/parse.cc:56
#9  0x00000000004de97c in ClientParsing::ParseACL::test_method (this=0x7fffffffddb0) at /usr/home/anonimal/kovri/tests/unit_tests/client/util/parse.cc:77
#10 0x00000000004ddf17 in ClientParsing::ParseACL_invoker () at /usr/home/anonimal/kovri/tests/unit_tests/client/util/parse.cc:45

At least two immediate working solutions:

I don't like this because there's no apparent reason why we should ever throw:

diff --git a/deps/cryptopp b/deps/cryptopp
--- a/deps/cryptopp
+++ b/deps/cryptopp
@@ -1 +1 @@
-Subproject commit f1a80e6a58e6cf72c969dda6825b9781eb300927
+Subproject commit f1a80e6a58e6cf72c969dda6825b9781eb300927-dirty
diff --git a/src/core/crypto/impl/cryptopp/radix.cc b/src/core/crypto/impl/cryptopp/radix.cc
index 4a37cdd..eac9f97 100644
--- a/src/core/crypto/impl/cryptopp/radix.cc
+++ b/src/core/crypto/impl/cryptopp/radix.cc
@@ -69,7 +69,11 @@ std::vector<std::uint8_t> Base32::Decode(
 
   CryptoPP::AlgorithmParameters const params(CryptoPP::MakeParameters(
       CryptoPP::Name::DecodingLookupArray(),
+#if defined(__FreeBSD__)  // See #788
+      reinterpret_cast<const int*>(lookup), false));
+#else
       reinterpret_cast<const int*>(lookup)));
+#endif
 
   // Decode
   return Radix::Decode<CryptoPP::Base32Decoder>(params, in, len);

I don't like this because it's more work for us:

diff --git a/src/core/router/identity.h b/src/core/router/identity.h
index e16f8975..c3716080 100644
--- a/src/core/router/identity.h
+++ b/src/core/router/identity.h
@@ -118,6 +118,9 @@ class Tag {
 
   void FromBase32(const std::string& encoded)
   {
+    if (encoded.empty())
+      throw std::length_error("Tag: null encoded length for b32");
+
     std::vector<std::uint8_t> const decoded =
         core::Base32::Decode(encoded.c_str(), encoded.length());

and then point 2 should be applied elsewhere. I'm personally vying for point 1.

After reviewing cryptopp's smartptr and algparam, I don't see what patches could be applied (at least ones that wouldn't be unnecessary or intrusive). Maybe freebsd clang needs to work out another bug?

Pinging @noloader.

anonimal · 2018-01-12T00:56:05Z

All tests now pass with the pre-processor patch applied. I'd like to wait for a resolution to weidai11/cryptopp#562 until merging.

noloader · 2018-01-12T20:43:06Z

@anonimal, @coneiric,

I'm crafting some self test for Crypto++ to test swapping an alphabet. Regarding this change, I think you may have been on the right track:

   CryptoPP::AlgorithmParameters const params(CryptoPP::MakeParameters(
       CryptoPP::Name::DecodingLookupArray(),
+#if defined(__FreeBSD__)  // See #788
+      reinterpret_cast<const int*>(lookup), false));
+#else
       reinterpret_cast<const int*>(lookup)));
+#endif

In fact, the library makes its parameters using false, too. Its due to a hidden CombinedNameValuePairs, which allows you two combine two sets of NameValuePairs. Below is from the Base64Decoder source file around line 85, but the same applies to other encoders and decoders:

void Base64Decoder::IsolatedInitialize(const NameValuePairs &parameters)
{
    BaseN_Decoder::IsolatedInitialize(CombinedNameValuePairs(
        parameters,
        MakeParameters(Name::DecodingLookupArray(), GetDecodingLookupArray(), false)(Name::Log2Base(), 6, true)));
}

So the quickest way to side step the issue and get on with your real work is probably:

    CryptoPP::AlgorithmParameters const params(CryptoPP::MakeParameters(
        CryptoPP::Name::DecodingLookupArray(),
            reinterpret_cast<const int*>(lookup), false));

We added new self tests to the library. The are active when -DDEBUG is in effect.

Unfortunately, they don't duplicate the problems.

anonimal · 2018-01-13T00:53:31Z

MakeParameters(Name::DecodingLookupArray(), GetDecodingLookupArray(), false)(Name::Log2Base(), 6, true)));

@noloader Ah, I see now. Thank you for clarifying!

Though I see that false is harmless, I'd like to keep the pre-processor cruft until weidai11/cryptopp#562 is resolved (since I'll need to leave a comment anyway regarding the explicit use of false).

noloader · 2018-01-13T14:15:03Z

@anonimal,

Forgive my ignorance... In this:

     std::vector<std::uint8_t> const decoded =
         core::Base32::Decode(encoded.c_str(), encoded.length());

What is encoded? Is it a std::string or something else?

If it is a std::string you should be OK. If it is another type, then be sure c_str() returns a pointer to something rather than NULL, nullptr_t or something else.

anonimal · 2018-01-13T23:43:22Z

What is encoded? Is it a std::string or something else?

Yes, std::string.

anonimal · 2018-01-14T08:35:43Z

On an unrelated note, I've added and rebased a few small changes:

added an alphabet test-case
moved the alphabet getter into translation unit (will silence clang warnings)
clarified the names of fixture data

On a related note, this PR should be safe to merge while @noloader and I continue debugging weidai11/cryptopp#562.

- Removes i2pd's base32/64 impl, introduces new impl using crypto++ - Includes related rewrites, refactors, new documentation, and dead code removal Referencing monero-project#788

0750b84 Kovri: new base32/64 impl / radix interface / unit-test (anonimal)

This may have something to do with monero-project/kovri#788

ac06218 Fix compile error due to T = int [256] ...again. static_cast the array, switch back to 'static const' AlgorithmParameters. (Jeffrey Walton) 75f8ca1 Fix compile error due to T = int [256] (Jeffrey Walton) 32cfb4b Unroll DecodingLookupArray for Base32 and Base64 decoders This is a time/space tradeoff, but it also provides 1-time intialization while avoiding a race. It should also clear weidai11/cryptopp#562 and #788 once and for all (famous last words) (Jeffrey Walton)

anonimal force-pushed the radix branch from 2507498 to ca50f84 Compare January 10, 2018 03:13

coneiric suggested changes Jan 10, 2018

View reviewed changes

anonimal force-pushed the radix branch from ca50f84 to bd13596 Compare January 11, 2018 23:27

anonimal mentioned this pull request Jan 12, 2018

FreeBSD/Clang++38: AlgorithmParameters/MakeParameters throws when used weidai11/cryptopp#562

Closed

noloader referenced this pull request in weidai11/cryptopp Jan 13, 2018

add self test for custom Encoder and Decoder alphabet (GH #562)

746fd6a

noloader referenced this pull request in weidai11/cryptopp Jan 13, 2018

Add additional Encoder and Decoder alphabet test (GH #562)

29a15f6

noloader referenced this pull request in weidai11/cryptopp Jan 13, 2018

Add additional Encoder and Decoder alphabet test (GH #562)

23f3328

anonimal force-pushed the radix branch from bd13596 to 9f68a8d Compare January 14, 2018 08:34

Kovri: new base32/64 impl / radix interface / unit-test

0750b84

- Removes i2pd's base32/64 impl, introduces new impl using crypto++ - Includes related rewrites, refactors, new documentation, and dead code removal Referencing monero-project#788

anonimal force-pushed the radix branch from 9f68a8d to 0750b84 Compare January 14, 2018 08:36

anonimal merged commit 0750b84 into monero-project:master Jan 16, 2018

anonimal added a commit that referenced this pull request Jan 16, 2018

Merge pull request #788

056b410

0750b84 Kovri: new base32/64 impl / radix interface / unit-test (anonimal)

anonimal deleted the radix branch January 16, 2018 07:45

anonimal mentioned this pull request Jan 29, 2018

AddressBook: decode base32 substring from valid host name #799

Merged

noloader added a commit to weidai11/cryptopp that referenced this pull request Jan 29, 2018

Fix misaligned SSE2 allocations on FreeBSD (GH #562)

c4392c4

This may have something to do with monero-project/kovri#788

anonimal mentioned this pull request Jan 29, 2018

Radix: throw length error, not runtime, when decoding #803

Merged

noloader mentioned this pull request Jan 31, 2018

Unroll DecodingLookupArray for Base32 and Base64 decoders #804

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kovri: new base32/64 impl / radix interface / unit-test #788

Kovri: new base32/64 impl / radix interface / unit-test #788

anonimal commented Jan 10, 2018

anonimal commented Jan 10, 2018

coneiric left a comment

coneiric Jan 10, 2018

anonimal Jan 10, 2018

coneiric Jan 10, 2018

anonimal Jan 10, 2018 •

edited

Loading

anonimal commented Jan 10, 2018

anonimal commented Jan 11, 2018

anonimal commented Jan 12, 2018

noloader commented Jan 12, 2018 •

edited

Loading

anonimal commented Jan 13, 2018

noloader commented Jan 13, 2018

anonimal commented Jan 13, 2018

anonimal commented Jan 14, 2018

Kovri: new base32/64 impl / radix interface / unit-test #788

Kovri: new base32/64 impl / radix interface / unit-test #788

Conversation

anonimal commented Jan 10, 2018

anonimal commented Jan 10, 2018

coneiric left a comment

Choose a reason for hiding this comment

coneiric Jan 10, 2018

Choose a reason for hiding this comment

anonimal Jan 10, 2018

Choose a reason for hiding this comment

coneiric Jan 10, 2018

Choose a reason for hiding this comment

anonimal Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

anonimal commented Jan 10, 2018

anonimal commented Jan 11, 2018

anonimal commented Jan 12, 2018

noloader commented Jan 12, 2018 • edited Loading

anonimal commented Jan 13, 2018

noloader commented Jan 13, 2018

anonimal commented Jan 13, 2018

anonimal commented Jan 14, 2018

anonimal Jan 10, 2018 •

edited

Loading

noloader commented Jan 12, 2018 •

edited

Loading