Crash when trying to set alternate alphabet for Base32 decoder

[noloader]

Brad from the mailing list reports the following program causes a segmentation fault on Linux. The issue was also confirmed on OS X.

#include <iostream>
#include <sstream>
#include <string>

#include <cryptopp/base32.h>

// This file is intended to debug base32 decoding issues in CryptoPP
// when using an alternate alphabet. By default, CryptoPP uses DUDE
// for base32 encoding/decoding. Most other libraries use RFC4648.
// This creates compatibility issues.

// DUDE - http://tools.ietf.org/html/draft-ietf-idn-dude-02
// RFC 4648 - http://tools.ietf.org/html/rfc4648

// Note - Alternate alphabet encoding works, but decoding fails
// with a stack overflow.


static const byte ALPHABET[]          = "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567"; // RFC4648

static const std::string test_encoded = "GEZDGNBVGY3TQOJQGEZDGNBVGY3TQOJQ";
static const std::string test_raw     = "12345678901234567890";

const std::string encode( const std::string& raw )
{
    // This function correctly encodes RFC4648

    std::string encoded;

    CryptoPP::Base32Encoder b32encoder;
    CryptoPP::AlgorithmParameters ep = CryptoPP::MakeParameters(
                                       CryptoPP::Name::EncodingLookupArray(),
                                       (const byte *)ALPHABET,
                                       false);
    b32encoder.IsolatedInitialize(ep);

    b32encoder.Attach( new CryptoPP::StringSink( encoded ) );
    b32encoder.Put( (std::uint8_t*)raw.c_str(), raw.size() );
    b32encoder.MessageEnd();

    return encoded;
}

const std::string decode( const std::string& encoded )
{
    std::string decoded;

    static int decoding_array[256];
    CryptoPP::Base32Decoder::InitializeDecodingLookupArray(decoding_array, 
                               ALPHABET, 
                               32, 
                               true); // false = case insensitive

    CryptoPP::Base32Decoder b32decoder;
    CryptoPP::AlgorithmParameters dp = CryptoPP::MakeParameters(
                                       CryptoPP::Name::DecodingLookupArray(),
                                       (const int *)decoding_array,
                                       false);
    b32decoder.IsolatedInitialize(dp); 

    b32decoder.Attach( new CryptoPP::StringSink( decoded ) );
    b32decoder.Put( (std::uint8_t*)encoded.c_str(), encoded.size() );
    b32decoder.MessageEnd();

    return decoded;
}


int main()
{
    // Test encoding
    const std::string e = encode( test_raw );
    if( e == test_encoded )
    {
        std::cout << "'" << test_raw << "' encodes to '" << e << "' RFC4648 Encoding works!\n";
    }
    else
    {
        std::cout << e << " RFC4648 Encoding failed\n";
    }

    // Test decoding
    const std::string d = decode( test_encoded );
    if( d == test_raw )
    {
        std::cout << d << " RFC4648 Decoding works!\n";
    }
    else
    {
        std::cout << d << " RFC4648 Decoding failed\n";
    }

    return 0;
}

[noloader]

Here's what I am seeing under a debugger. it looks like things went recursive.

$ lldb ./test.exe
(lldb) target create "./test.exe"
Current executable set to './test.exe' (x86_64).
(lldb) r
Process 91827 launched: './test.exe' (x86_64)
'12345678901234567890' encodes to 'GEZDGNBVGY3TQOJQGEZDGNBVGY3TQOJQ' RFC4648 Encoding works!
...
(lldb) bt all
* thread #1: tid = 0x540d05, 0x00007fff8ece5297 libsystem_malloc.dylib`szone_malloc_should_clear + 20, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=2, address=0x7fff5f3fffe4)
  * frame #0: 0x00007fff8ece5297 libsystem_malloc.dylib`szone_malloc_should_clear + 20
    frame #1: 0x00007fff8ece7868 libsystem_malloc.dylib`malloc_zone_malloc + 71
    frame #2: 0x00007fff8ece827c libsystem_malloc.dylib`malloc + 42
    frame #3: 0x00007fff8e87b28e libc++.1.dylib`operator new(unsigned long) + 30
    frame #4: 0x0000000100003bd2 test.exe`CryptoPP::AlgorithmParameters& CryptoPP::AlgorithmParameters::operator(this=0x00007fff5f4001f0, name=0x000000010019e4fd, value=0x00007fff5f400278, throwIfNotUsed=false)<int const*>(char const*, int const* const&, bool) + 50 at algparam.h:514
    frame #5: 0x0000000100002dfa test.exe`CryptoPP::AlgorithmParameters CryptoPP::MakeParameters<int const*>(name=0x000000010019e4fd, value=0x00007fff5f400278, throwIfNotUsed=false) + 74 at algparam.h:556
    frame #6: 0x00000001000346f4 test.exe`CryptoPP::Base32Decoder::IsolatedInitialize(this=0x00007fff5fbff370, parameters=0x00007fff5f400368) + 68 at base32.cpp:23
    frame #7: 0x000000010014e65d test.exe`CryptoPP::Filter::Initialize(this=0x00007fff5fbff370, parameters=0x00007fff5f400368, propagation=-1) + 61 at filters.cpp:74
    frame #8: 0x0000000100034750 test.exe`CryptoPP::Base32Decoder::IsolatedInitialize(this=0x00007fff5fbff370, parameters=0x00007fff5f400438) + 160 at base32.cpp:21
    ...
    frame #80623: 0x000000010014e65d test.exe`CryptoPP::Filter::Initialize(this=0x00007fff5fbff370, parameters=0x00007fff5fbff1a8, propagation=-1) + 61 at filters.cpp:74
    frame #80624: 0x0000000100034750 test.exe`CryptoPP::Base32Decoder::IsolatedInitialize(this=0x00007fff5fbff370, parameters=0x00007fff5fbff278) + 160 at base32.cpp:21
    frame #80625: 0x000000010014e65d test.exe`CryptoPP::Filter::Initialize(this=0x00007fff5fbff370, parameters=0x00007fff5fbff278, propagation=-1) + 61 at filters.cpp:74
    frame #80626: 0x0000000100034750 test.exe`CryptoPP::Base32Decoder::IsolatedInitialize(this=0x00007fff5fbff370, parameters=0x00007fff5fbff358) + 160 at base32.cpp:21

Testing with a Base64 (with appropriate table changes) does not witness the problem. It seems Base64Decoder side steps it by making IsolatedInitialize a nop.

class Base64Decoder : public BaseN_Decoder
{
public:
    Base64Decoder(BufferedTransformation *attachment = NULL)
        : BaseN_Decoder(GetDecodingLookupArray(), 6, attachment) {}

    void IsolatedInitialize(const NameValuePairs &parameters)
        {CRYPTOPP_UNUSED(parameters);}

private:
    static const int * CRYPTOPP_API GetDecodingLookupArray();
};

[noloader]

OK, so it looks like this issue has been around for some time. Going back to the Crypto++ 5.6.2 implementation of Base32Decoder::IsolatedInitialize, it appears the issue existed there, also.

The fix appears to be quite simple:

$ cat git.diff
diff --git a/base32.cpp b/base32.cpp
index 80ca456..3c7e6ee 100644
--- a/base32.cpp
+++ b/base32.cpp
@@ -18,7 +18,7 @@ void Base32Encoder::IsolatedInitialize(const NameValuePairs &parameters)

 void Base32Decoder::IsolatedInitialize(const NameValuePairs &parameters)
 {
-   BaseN_Decoder::Initialize(CombinedNameValuePairs(
+   BaseN_Decoder::IsolatedInitialize(CombinedNameValuePairs(
        parameters,
        MakeParameters(Name::DecodingLookupArray(), GetDefaultDecodingLookupArray(), false)(Name::Log2Base(), 5, true)));
 }

After the change, the self-tests and Brad's driver completed successfully.

Now, the problem we seem to have is this class is not really tested, so its not clear to me if we are breaking things in other places:

hilbert:cryptopp$ egrep "(Base32Encoder|Base32Decoder)" *.h *.cpp | grep test
hilbert:cryptopp$

[noloader]

... the problem we seem to have is this class is not really tested, so its not clear to me if we are breaking things in other places...

OK, I believe this test case exercises Base32 and Base64 sufficiently to commit the change.

---

#include <iostream>
#include <string>
#include "filters.h"
#include "files.h"
#include "base32.h"
#include "base64.h"
#include "osrng.h"

using namespace std;
using namespace CryptoPP;

int main(int argc, char* argv[])
{
    {
    cout << "**********" << endl;
    Base64Encoder base64a(new FileSink(cout));
    const byte m1[] = "Now is the time for all good men...";
    const size_t l1 = COUNTOF(m1);

    base64a.Put(m1, l1);
    base64a.MessageEnd();
    cout << endl;

    Base64Decoder base64b(new FileSink(cout));
    const byte m2[] = "Tm93IGlzIHRoZSB0aW1lIGZvciBhbGwgZ29vZCBtZW4uLi4A";
    const size_t l2 = COUNTOF(m2);

    base64b.Put(m2, l2);
    base64b.MessageEnd();
    cout << endl;
    }

    // *********************************** //

    {
    cout << "**********" << endl;
    Base64Encoder base64a(new FileSink(cout));
    const byte m1[] = "Now is the time for all good men...";
    const size_t l1 = COUNTOF(m1);

    AlgorithmParameters params = MakeParameters(Name::InsertLineBreaks(), false);
    base64a.IsolatedInitialize(params);

    base64a.Put(m1, l1);
    base64a.MessageEnd();
    cout << endl;

    Base64Decoder base64b(new FileSink(cout));
    const byte m2[] = "Tm93IGlzIHRoZSB0aW1lIGZvciBhbGwgZ29vZCBtZW4uLi4A";
    const size_t l2 = COUNTOF(m2);

    base64b.Put(m2, l2);
    base64b.MessageEnd();
    cout << endl;
    }

    // *********************************** //

    {
    cout << "**********" << endl;
    Base64Encoder base64a(new FileSink(cout));
    const byte m1[] = "Now is the time for all good men...";
    const size_t l1 = COUNTOF(m1);

    const byte ALPHABET[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_";
    AlgorithmParameters p1 = MakeParameters(Name::EncodingLookupArray(),(const byte *)ALPHABET);
    base64a.IsolatedInitialize(p1);

    base64a.Put(m1, l1);
    base64a.MessageEnd();
    cout << endl;

    Base64Decoder base64b(new FileSink(cout));
    const byte m2[] = "Tm93IGlzIHRoZSB0aW1lIGZvciBhbGwgZ29vZCBtZW4uLi4A";
    const size_t l2 = COUNTOF(m2);

    int lookup[256];
    Base64Decoder::InitializeDecodingLookupArray(lookup, ALPHABET, 64, false /*insensitive*/);

    AlgorithmParameters p2 = MakeParameters(Name::DecodingLookupArray(),(const int *)lookup);
    base64b.IsolatedInitialize(p2);

    base64b.Put(m2, l2);
    base64b.MessageEnd();
    cout << endl;
    }

    {
    cout << "**********" << endl;
    string random, s1, s2, s3;
    random.resize(10000);
    OS_GenerateRandomBlock(false, (byte*)&random[0], random.size());

    Base64Encoder base64a(new StringSink(s1));

    // This is the WebSafe alphabet; we can use Base64URLEncoder and Base64URLDecoder 
    const byte ALPHABET[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_";
    AlgorithmParameters p1 = MakeParameters(Name::EncodingLookupArray(),(const byte *)ALPHABET);
    base64a.IsolatedInitialize(p1);

    base64a.Put((byte*)&random[0], random.size());
    base64a.MessageEnd();

    Base64Decoder base64b(new StringSink(s2));

    int lookup[256];
    Base64Decoder::InitializeDecodingLookupArray(lookup, ALPHABET, 64, false /*insensitive*/);

    AlgorithmParameters p2 = MakeParameters(Name::DecodingLookupArray(),(const int *)lookup);
    base64b.IsolatedInitialize(p2);

    base64b.Put((byte*)&s1[0], s1.size());
    base64b.MessageEnd();

    Base64URLDecoder base64c(new StringSink(s3));
    base64c.Put((byte*)&s1[0], s1.size());
    base64c.MessageEnd();

    if(random == s2 && random == s3)
        cout << "Round trip'ed 10000 bytes" << endl;
    else
        cout << "Failed to round trip 10000 bytes" << endl;
    }

    {
    Base32Encoder base32a(new FileSink(cout));
    const byte m1[] = "Now is the time for all good men...";
    const size_t l1 = COUNTOF(m1);

    base32a.Put(m1, l1);
    base32a.MessageEnd();
    cout << endl;

    Base32Decoder base32b(new FileSink(cout));
    const byte m2[] = "J3ZZQIDJQNSHI4DFEB4GU5MFEBVG86TANFYG2IDHP7ZYIIDPNXZC6MTQAA";
    const size_t l2 = COUNTOF(m2);

    base32b.Put(m2, l2);
    base32b.MessageEnd();
    cout << endl;
    }

    // *********************************** //

    {
    Base32Encoder base32a(new FileSink(cout));
    const byte m1[] = "Now is the time for all good men...";
    const size_t l1 = COUNTOF(m1);

    AlgorithmParameters params = MakeParameters(Name::Uppercase(), false);
    base32a.IsolatedInitialize(params);

    base32a.Put(m1, l1);
    base32a.MessageEnd();
    cout << endl;

    Base32Decoder base32b(new FileSink(cout));
    const byte m2[] = "j3zzqidjqnshi4dfeb4gu5mfebvg86tanfyg2idhp7zyiidpnxzc6mtqaa";
    const size_t l2 = COUNTOF(m2);

    base32b.Put(m2, l2);
    base32b.MessageEnd();
    cout << endl;
    }

    // *********************************** //

    {
    Base32Encoder base32a(new FileSink(cout));
    const byte m1[] = "Now is the time for all good men...";
    const size_t l1 = COUNTOF(m1);

    const byte ALPHABET[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567";
    AlgorithmParameters p1 = MakeParameters(Name::EncodingLookupArray(),(const byte *)ALPHABET);
    base32a.IsolatedInitialize(p1);

    base32a.Put(m1, l1);
    base32a.MessageEnd();
    cout << endl;

    Base32Decoder base32b(new FileSink(cout));
    const byte m2[] = "JZXXOIDJOMQHI2DFEB2GS3LFEBTG64RAMFWGYIDHN5XWIIDNMVXC4LROAA";
    const size_t l2 = COUNTOF(m2);

    int lookup[256];
    Base32Decoder::InitializeDecodingLookupArray(lookup, ALPHABET, 32, true /*insensitive*/);

    AlgorithmParameters p2 = MakeParameters(Name::DecodingLookupArray(),(const int *)lookup);
    base32b.IsolatedInitialize(p2);

    base32b.Put(m2, l2);
    base32b.MessageEnd();
    cout << endl;
    }

    return 0;
}

[noloader]

Cleared at Commit 9a5e359bb3795bf3

[ghost]

Thanks Jeff. I appreciate it. I'm brad from the mailing list ;)

Until this is released (I imagine it may be a year or more looking at former Crypto++ releases), I'll continue to use your old alt_base32 header and cpp file. They were removed from the Crypto++ wiki, but I have them in my repo here: https://github.com/w8rbt/oathgen/tree/master/alt_base32

Thanks again,

Brad (w8rbt)

[ghost]

Curious if Crypto++ 7 fixes this. I did not see it mentioned in the changelog.

[noloader]

Yes, this should have been fixed at Crypto++ 5.6.4.

Are you still seeing it in the latest versions of the library?