Sietch random `memo` fields leak stack memory and `rand()` state

HarryR commented 4 years ago

Please make sure you have the latest SilentDragonLite.

Describe the bug The fillTxJsonParams and encryptDecrypt functions utilise the rand() function,

See:

The fillTxJsonParams function creates 8 random 'dust' public z-keys using the light client RPC, then it generates a readable ASCII string with a length between 10 and 129 characters long - it uses the cryptographically secure QT QRandomGenerator to choose the content of the string, this is randomString.

Then, for each of the 8 output memo fields it:

creates a new byte buffer, enough to contain the random string and the null terminator byte, containing a copy of the above random readable ASCII string.
the buffer is at offset randomHash, probably on the heap
chooses a new number between 10 and 129, using rand(), lets call this x
then it allocates crypto_aead_xchacha20poly1305_ietf_ABYTES bytes (16) on the stack
then it uses crypto_generichash, aka BLAKE2b, with an output size of 32
as input it takes the pointer to randomHash, the length of randomString plus x, so it hashes randomString and a suffix if x bytes of heap memory after randomString.
the output of crypto_generichash is twice as long as the stack allocated buffer, therefore it writes the remaining 16 bytes beyond the output hash buffer into the stack
then... ok so far we have stack overflow, heap leakage, using rand() as in cryptographic functions... *facepalm* but wait... it gets better
then encryptDecrypt function xors the stack buffer hash (of length 16) against a sequence of characters generated by rand()... but the sequence is len(randomString)+x, so it's modifying up to 113 bytes of stack memory beyond the end of the hash buffer.
then it hex hex encodes it and includes it as the memo field.

TL;DR

This code is not memory safe: it accesses ranges of heap allocated memory out of bounds
This code is not cryptographically safe: it uses rand() as a source of entropy in encryptDecrypt.
This code is not stack safe, it usually writes beyond the end of a stack allocated buffer
This code leaks internal program state, namely bytes beyond the ends of buffers allocated on either stack or heap...

Given that rand() is a LCG with a known and easily searchable period, and this is done in a loop 8 times in a row, if there is any easy to predict pattern of zeros or otherwise fixed bytes in the included output then the internal state of the rand() LCG can be recovered using SAT solver (like Z3) given two handfuls of occurrences (seeing as you only get the lower 8 bits after the xor) of known bytes xor'd with some state.

This will leak memory and internal state of the program in the memo field...

There are buffer overflows...

The crypto is WTF...

What more can I say

leto commented 4 years ago

@HarryR thanks for the bug report. Do you have a suggested patch?

To clarify, this code is just attempting to generate "random looking" data and there is no cryptographic use of this data. It's meant to make an empty memo field not look empty.

I don't see a buffer overflow, you will have to give proof of that. There is no attacker controlled data. And any program state that is leaked is sent to addresses for which no person has the private key. If you have a better way to not leak internal state, patches are welcome

HarryR commented 4 years ago

For me this kind of code is indicative of larger problems. I will not write patches for you.

I don't see a buffer overflow, you will have to give proof of that.

... right, see below, and my description above.

There is no attacker controlled data.

not in this instance, will you find the other ones though?

And any program state that is leaked is sent to addresses for which no person has the private key.

For the lightclient... yes; for hushd, see: https://github.com/MyHush/hush3/blob/master/src/sietch.h - aren't backdoors supposed to be less obvious?

anyway...

1) Look at these memo fields... do they look 'random' to you?

[
  {
    "address": "zs1k8tew505at5qnlq6vr75s8mgdcdd2vjhkvl5nxufjvgyyeef9yqxtuatwh72w9y4dnjgcrl5jek",
    "amount": 0,
    "memo": "7ea3039b8710300c81315481224072a2e7070dbc0be2b7b7b7f6a75aefe7ba2b4764e9b548c8b7b747070dbc0be2b7b7b76fe9b548c8b7b7976fe9b548c8b7b7870910bc0be2b7b7979474cbbec8b7b79762e9b548c8b7b7dc6f"
  },
  {
    "address": "zs1ntnysrfdsmef0py58gj5qnnl03sr5sl9vyej83zdtgemt8mlfhdq7wqz87pa4jtqqsepjffp206",
    "amount": 0,
    "memo": "bd5a16a9f66d1f59549417225dfab5eb12afa514a34a1f1f1f5e0ff2474f1283efcc411de0601f1fefafa514a34a1f1f1fc7411de0601f1f3fc7411de0601f1f2fa1b814a34a1f1f3f3cdc6316601f1f3fca411de0601f1f74c7e91ba34a1f1f1f1f1f1f1f1f1f1f4f90d614a34a1f1f1dcb411de0601f1f7fb4d414a34a1f1fff1a996316601f1f2f986714a34a1f1f0f38"
  },
  {
    "address": "zs13r6ygceplvchnm3fhdtz8gqyty7252xzmuq6pkdlhk5evz2e3ha7ev4wuztlelq0d39ccr4rcxf",
    "amount": 0,
    "memo": "49aa534dea79c9ce35cbf8ac113b3f3d01070dbc0be2b7b7b7f6a75aefe7ba2b4764e9b548c8b7b747070dbc0be2b7b7b76fe9b548c8b7b7"
  },
  {
    "address": "zs1dvd2q4dcsf5dj2a287rajkq3m48wnhuqfedswqrulagz2dkudm53x3huhpm75qdrxt6zy746mkk",
    "amount": 0,
    "memo": "5abb7781fb9d5b90a4c039dd2306682433999322957c2929296839c4717924b5d9fa772bd6562929d9999322957c292929f1772bd656292909f1772bd656292919978e22957c2929090aea552056292909fc772bd656292942f1df2d957c2929292929292929292979a6e022957c29292bfd772bd65629294982e222957c"
  },
  {
    "address": "zs15g7gutt2rszg4x5p4xmca4lavpg9elfhxwh7y0lwg6qq9k4erzw50p2q7x3ayrzueh24k7w7e77",
    "amount": 0,
    "memo": "dba8fc7aaf9babcf315f5d797b1319781a1d17a611f8adadadecbd40f5fda0315d7ef3af52d2adad5d1d17a611f8adadad75f3af52d2adad8d75f3af52d2adad9d130aa611f8adad8d8e6ed1a4d2adad8d78f3af52d2adadc6755ba911f8adadadadadadadadadadfd2264a611f8adadaf79f3af52d2adadcd0666a611f8adad4da82bd1a4d2adad9d2ad5a611f8adadbd8aadadadadadadef2849a911f8adadede135d6a4d2adad4d"
  },
  {
    "address": "zs13fv743lhw565hp5m4tpstvk4ezy7fapvtdyp3ea7t3h52rvc2p6axcygvajwsp233fw7s6ypmu2",
    "amount": 0,
    "memo": "d2c06999da96e5aec06f727898f8e54e51555fee59b0e5e5e5a4f508bdb5e8791536bbe71a9ae5e515555fee59b0e5e5e53dbbe71a9ae5e5c53dbbe71a9ae5e5d55b42ee59b0e5e5c5c62699ec9ae5e5c530bbe71a9ae5e58e3d13e159b0e5e5e5e5e5e5"
  },
  {
    "address": "zs1cllfsvjerrnl5dnf6rgzfc08zrjhfhdt32q773dhvwnmfwrgedcq6cr9gu2qlfcgjtwf6vpt7fu",
    "amount": 0
  }
]

2) Now look at the valgrind log, it is as I described above

==14778== Invalid read of size 8
==14778==    at 0x55D57EE: memmove (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14778==    by 0xB1FF59: crypto_generichash_blake2b__update (in /x/MyHush/SilentDragonLite/SilentDragonLite)
==14778==    by 0xB20604: crypto_generichash_blake2b__blake2b (in /x/MyHush/SilentDragonLite/SilentDragonLite)
==14778==    by 0x303690: Controller::fillTxJsonParams(nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer>&, Tx) (controller.cpp:185)
==14778==    by 0x31186A: Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>)::{lambda()#1}::operator()() const (controller.cpp:1591)
==14778==    by 0x319CEE: std::_Function_handler<void (), Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>)::{lambda()#1}>::_M_invoke(std::_Any_data const&) (std_function.h:316)
==14778==    by 0x213521: std::function<void ()>::operator()() const (std_function.h:706)
==14778==    by 0x310DD1: Controller::unlockIfEncrypted(std::function<void ()>, std::function<void ()>) (controller.cpp:1551)
==14778==    by 0x311DF9: Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>) (controller.cpp:1588)
==14778==    by 0x2A2C81: MainWindow::sendButton() (sendtab.cpp:865)
==14778==    by 0x282439: QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (MainWindow::*)()>::call(void (MainWindow::*)(), MainWindow*, void**) (qobjectdefs_impl.h:136)
==14778==    by 0x280A70: void QtPrivate::FunctionPointer<void (MainWindow::*)()>::call<QtPrivate::List<>, void>(void (MainWindow::*)(), MainWindow*, void**) (qobjectdefs_impl.h:169)
==14778==  Address 0x19b0bcc0 is 1 bytes after a block of size 95 alloc'd
==14778==    at 0x55CF89F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14778==    by 0x3035F9: Controller::fillTxJsonParams(nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer>&, Tx) (controller.cpp:177)
==14778==    by 0x31186A: Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>)::{lambda()#1}::operator()() const (controller.cpp:1591)
==14778==    by 0x319CEE: std::_Function_handler<void (), Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>)::{lambda()#1}>::_M_invoke(std::_Any_data const&) (std_function.h:316)
==14778==    by 0x213521: std::function<void ()>::operator()() const (std_function.h:706)
==14778==    by 0x310DD1: Controller::unlockIfEncrypted(std::function<void ()>, std::function<void ()>) (controller.cpp:1551)
==14778==    by 0x311DF9: Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>) (controller.cpp:1588)
==14778==    by 0x2A2C81: MainWindow::sendButton() (sendtab.cpp:865)
==14778==    by 0x282439: QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (MainWindow::*)()>::call(void (MainWindow::*)(), MainWindow*, void**) (qobjectdefs_impl.h:136)
==14778==    by 0x280A70: void QtPrivate::FunctionPointer<void (MainWindow::*)()>::call<QtPrivate::List<>, void>(void (MainWindow::*)(), MainWindow*, void**) (qobjectdefs_impl.h:169)
==14778==    by 0x27EA2B: QtPrivate::QSlotObject<void (MainWindow::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (qobject_impl.h:120)
==14778==    by 0x720266E: QMetaObject::activate(QObject*, int, int, void**) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.9.5)
==14778== 
==14778== Invalid read of size 8
==14778==    at 0x55D57E0: memmove (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14778==    by 0xB1FF59: crypto_generichash_blake2b__update (in /x/MyHush/SilentDragonLite/SilentDragonLite)
==14778==    by 0xB20604: crypto_generichash_blake2b__blake2b (in /x/MyHush/SilentDragonLite/SilentDragonLite)
==14778==    by 0x303690: Controller::fillTxJsonParams(nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer>&, Tx) (controller.cpp:185)
==14778==    by 0x31186A: Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>)::{lambda()#1}::operator()() const (controller.cpp:1591)
==14778==    by 0x319CEE: std::_Function_handler<void (), Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>)::{lambda()#1}>::_M_invoke(std::_Any_data const&) (std_function.h:316)
==14778==    by 0x213521: std::function<void ()>::operator()() const (std_function.h:706)
==14778==    by 0x310DD1: Controller::unlockIfEncrypted(std::function<void ()>, std::function<void ()>) (controller.cpp:1551)
==14778==    by 0x311DF9: Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>) (controller.cpp:1588)
==14778==    by 0x2A2C81: MainWindow::sendButton() (sendtab.cpp:865)
==14778==    by 0x282439: QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (MainWindow::*)()>::call(void (MainWindow::*)(), MainWindow*, void**) (qobjectdefs_impl.h:136)
==14778==    by 0x280A70: void QtPrivate::FunctionPointer<void (MainWindow::*)()>::call<QtPrivate::List<>, void>(void (MainWindow::*)(), MainWindow*, void**) (qobjectdefs_impl.h:169)
==14778==  Address 0x19b0bcc8 is 9 bytes after a block of size 95 alloc'd
==14778==    at 0x55CF89F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14778==    by 0x3035F9: Controller::fillTxJsonParams(nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer>&, Tx) (controller.cpp:177)
==14778==    by 0x31186A: Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>)::{lambda()#1}::operator()() const (controller.cpp:1591)
==14778==    by 0x319CEE: std::_Function_handler<void (), Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>)::{lambda()#1}>::_M_invoke(std::_Any_data const&) (std_function.h:316)
==14778==    by 0x213521: std::function<void ()>::operator()() const (std_function.h:706)
==14778==    by 0x310DD1: Controller::unlockIfEncrypted(std::function<void ()>, std::function<void ()>) (controller.cpp:1551)
==14778==    by 0x311DF9: Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>) (controller.cpp:1588)
==14778==    by 0x2A2C81: MainWindow::sendButton() (sendtab.cpp:865)
==14778==    by 0x282439: QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (MainWindow::*)()>::call(void (MainWindow::*)(), MainWindow*, void**) (qobjectdefs_impl.h:136)
==14778==    by 0x280A70: void QtPrivate::FunctionPointer<void (MainWindow::*)()>::call<QtPrivate::List<>, void>(void (MainWindow::*)(), MainWindow*, void**) (qobjectdefs_impl.h:169)
==14778==    by 0x27EA2B: QtPrivate::QSlotObject<void (MainWindow::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (qobject_impl.h:120)
==14778==    by 0x720266E: QMetaObject::activate(QObject*, int, int, void**) (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.9.5)
==14778== 
==14778== Invalid read of size 2
==14778==    at 0x55D5750: memmove (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14778==    by 0xB1FF59: crypto_generichash_blake2b__update (in /x/MyHush/SilentDragonLite/SilentDragonLite)
==14778==    by 0xB20604: crypto_generichash_blake2b__blake2b (in /x/MyHush/SilentDragonLite/SilentDragonLite)
==14778==    by 0x303690: Controller::fillTxJsonParams(nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer>&, Tx) (controller.cpp:185)
==14778==    by 0x31186A: Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>)::{lambda()#1}::operator()() const (controller.cpp:1591)
==14778==    by 0x319CEE: std::_Function_handler<void (), Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>)::{lambda()#1}>::_M_invoke(std::_Any_data const&) (std_function.h:316)
==14778==    by 0x213521: std::function<void ()>::operator()() const (std_function.h:706)
==14778==    by 0x310DD1: Controller::unlockIfEncrypted(std::function<void ()>, std::function<void ()>) (controller.cpp:1551)
==14778==    by 0x311DF9: Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>) (controller.cpp:1588)
==14778==    by 0x2A2C81: MainWindow::sendButton() (sendtab.cpp:865)
==14778==    by 0x282439: QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (MainWindow::*)()>::call(void (MainWindow::*)(), MainWindow*, void**) (qobjectdefs_impl.h:136)
==14778==    by 0x280A70: void QtPrivate::FunctionPointer<void (MainWindow::*)()>::call<QtPrivate::List<>, void>(void (MainWindow::*)(), MainWindow*, void**) (qobjectdefs_impl.h:169)
==14778==  Address 0x19b0bd10 is 16 bytes before a block of size 80 alloc'd
==14778==    at 0x55CF89F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14778==    by 0xDBD050D: ??? (in /usr/lib/x86_64-linux-gnu/libQt5XcbQpa.so.5.9.5)
==14778==    by 0xDBD26E6: ??? (in /usr/lib/x86_64-linux-gnu/libQt5XcbQpa.so.5.9.5)
==14778==    by 0xDBD430D: ??? (in /usr/lib/x86_64-linux-gnu/libQt5XcbQpa.so.5.9.5)
==14778==    by 0x658A585: QRasterPaintEngine::drawCachedGlyphs(int, unsigned int const*, QFixedPoint const*, QFontEngine*) (in /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5.9.5)
==14778==    by 0x658E1F7: QRasterPaintEngine::drawTextItem(QPointF const&, QTextItem const&) (in /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5.9.5)
==14778==    by 0x65A9650: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5.9.5)
==14778==    by 0x641AF52: QTextLine::draw(QPainter*, QPointF const&, QTextLayout::FormatRange const*) const (in /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5.9.5)
==14778==    by 0x65A037C: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5.9.5)
==14778==    by 0x65A6E6A: QPainter::drawText(QRect const&, int, QString const&, QRect*) (in /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5.9.5)
==14778==    by 0x5CF63FF: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.9.5)
==14778==    by 0x5CF647E: QTextEdit::paintEvent(QPaintEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.9.5)
==14778== 
==14778== Use of uninitialised value of size 8
==14778==    at 0x6FFE00A: QByteArray::toHex(char) const (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.9.5)
==14778==    by 0x6FFE11E: QByteArray::toHex() const (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.9.5)
==14778==    by 0x303771: Controller::fillTxJsonParams(nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer>&, Tx) (controller.cpp:191)
==14778==    by 0x31186A: Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>)::{lambda()#1}::operator()() const (controller.cpp:1591)
==14778==    by 0x319CEE: std::_Function_handler<void (), Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>)::{lambda()#1}>::_M_invoke(std::_Any_data const&) (std_function.h:316)
==14778==    by 0x213521: std::function<void ()>::operator()() const (std_function.h:706)
==14778==    by 0x310DD1: Controller::unlockIfEncrypted(std::function<void ()>, std::function<void ()>) (controller.cpp:1551)
==14778==    by 0x311DF9: Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>) (controller.cpp:1588)
==14778==    by 0x2A2C81: MainWindow::sendButton() (sendtab.cpp:865)
==14778==    by 0x282439: QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (MainWindow::*)()>::call(void (MainWindow::*)(), MainWindow*, void**) (qobjectdefs_impl.h:136)
==14778==    by 0x280A70: void QtPrivate::FunctionPointer<void (MainWindow::*)()>::call<QtPrivate::List<>, void>(void (MainWindow::*)(), MainWindow*, void**) (qobjectdefs_impl.h:169)
==14778==    by 0x27EA2B: QtPrivate::QSlotObject<void (MainWindow::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (qobject_impl.h:120)
==14778== 
==14778== Use of uninitialised value of size 8
==14778==    at 0x6FFE024: QByteArray::toHex(char) const (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.9.5)
==14778==    by 0x6FFE11E: QByteArray::toHex() const (in /usr/lib/x86_64-linux-gnu/libQt5Core.so.5.9.5)
==14778==    by 0x303771: Controller::fillTxJsonParams(nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer>&, Tx) (controller.cpp:191)
==14778==    by 0x31186A: Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>)::{lambda()#1}::operator()() const (controller.cpp:1591)
==14778==    by 0x319CEE: std::_Function_handler<void (), Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>)::{lambda()#1}>::_M_invoke(std::_Any_data const&) (std_function.h:316)
==14778==    by 0x213521: std::function<void ()>::operator()() const (std_function.h:706)
==14778==    by 0x310DD1: Controller::unlockIfEncrypted(std::function<void ()>, std::function<void ()>) (controller.cpp:1551)
==14778==    by 0x311DF9: Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>) (controller.cpp:1588)
==14778==    by 0x2A2C81: MainWindow::sendButton() (sendtab.cpp:865)
==14778==    by 0x282439: QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (MainWindow::*)()>::call(void (MainWindow::*)(), MainWindow*, void**) (qobjectdefs_impl.h:136)
==14778==    by 0x280A70: void QtPrivate::FunctionPointer<void (MainWindow::*)()>::call<QtPrivate::List<>, void>(void (MainWindow::*)(), MainWindow*, void**) (qobjectdefs_impl.h:169)
==14778==    by 0x27EA2B: QtPrivate::QSlotObject<void (MainWindow::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (qobject_impl.h:120)
==14778== 
==14778== Invalid read of size 1
==14778==    at 0x55D5788: memmove (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14778==    by 0xB1FF59: crypto_generichash_blake2b__update (in /x/MyHush/SilentDragonLite/SilentDragonLite)
==14778==    by 0xB20604: crypto_generichash_blake2b__blake2b (in /x/MyHush/SilentDragonLite/SilentDragonLite)
==14778==    by 0x303690: Controller::fillTxJsonParams(nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer>&, Tx) (controller.cpp:185)
==14778==    by 0x31186A: Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>)::{lambda()#1}::operator()() const (controller.cpp:1591)
==14778==    by 0x319CEE: std::_Function_handler<void (), Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>)::{lambda()#1}>::_M_invoke(std::_Any_data const&) (std_function.h:316)
==14778==    by 0x213521: std::function<void ()>::operator()() const (std_function.h:706)
==14778==    by 0x310DD1: Controller::unlockIfEncrypted(std::function<void ()>, std::function<void ()>) (controller.cpp:1551)
==14778==    by 0x311DF9: Controller::executeTransaction(Tx, std::function<void (QString)>, std::function<void (QString, QString)>) (controller.cpp:1588)
==14778==    by 0x2A2C81: MainWindow::sendButton() (sendtab.cpp:865)
==14778==    by 0x282439: QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (MainWindow::*)()>::call(void (MainWindow::*)(), MainWindow*, void**) (qobjectdefs_impl.h:136)
==14778==    by 0x280A70: void QtPrivate::FunctionPointer<void (MainWindow::*)()>::call<QtPrivate::List<>, void>(void (MainWindow::*)(), MainWindow*, void**) (qobjectdefs_impl.h:169)
==14778==  Address 0x19e5df9a is 6 bytes before a block of size 108 alloc'd
==14778==    at 0x55CF89F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14778==    by 0xDBD050D: ??? (in /usr/lib/x86_64-linux-gnu/libQt5XcbQpa.so.5.9.5)
==14778==    by 0xDBD26E6: ??? (in /usr/lib/x86_64-linux-gnu/libQt5XcbQpa.so.5.9.5)
==14778==    by 0xDBD430D: ??? (in /usr/lib/x86_64-linux-gnu/libQt5XcbQpa.so.5.9.5)
==14778==    by 0x658A585: QRasterPaintEngine::drawCachedGlyphs(int, unsigned int const*, QFixedPoint const*, QFontEngine*) (in /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5.9.5)
==14778==    by 0x658E1F7: QRasterPaintEngine::drawTextItem(QPointF const&, QTextItem const&) (in /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5.9.5)
==14778==    by 0x65A9650: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5.9.5)
==14778==    by 0x641AF52: QTextLine::draw(QPainter*, QPointF const&, QTextLayout::FormatRange const*) const (in /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5.9.5)
==14778==    by 0x65A037C: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5.9.5)
==14778==    by 0x65A6E6A: QPainter::drawText(QRect const&, int, QString const&, QRect*) (in /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5.9.5)
==14778==    by 0x5CF63FF: ??? (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.9.5)
==14778==    by 0x5CF647E: QTextEdit::paintEvent(QPaintEvent*) (in /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5.9.5)
==14778==

leto commented 4 years ago

@HarryR I think you have found some valid bugs, and if you want to send us a zaddr, we will pay you a bounty, like we do with all people who report bugs.

It's a valid bug that these memos are not all 512 bytes long, which they should be. It also causes further errors that some of them are not aligned to 8 bytes, which 512 would be.

This code is much more complex than it needs to be. A simple XOR of vaguely random data would be enough. We do not need cryptographic hash functions or cryptographic randomness in this instance.

@HarryR as for your accusations about hushd, this issues is about SDL and I am talking about this code. The equivalent code in hushd, which randomizes this memo data, is much newer, and works differently. Feel free to check that code out and tell me if there are similar bugs. I implemented it independently and didn't use this SDL code as a basis for that.

Additionally, it's mentioned in the whitepaper that there are FOUR implementations of Sietch, and the improvements and drawbacks of each. You can't call it a backdoor if we talk about it in the whitepaper, lulz. In any case, within one week of these ideas coming out, jl777 and others made similar critiques of the fixed 200 zaddrs, which is why this SDL implemenation with randomized seedphrases was written, and why I wrote the --sietch-dynamic CLI option, which accomplishes something similar but still not as good as the way SDL does things. There will likely be a 5th implementation that emulates exactly the technique I instructed @DenioD to implement. At that time, both the full node and lite wallet will have equivalent features and privacy mitigations. Currently, the hushd implementation of memo randomization is better, but SDL's method of using random seedphrases is better for zout address generation. This is related to the fact that seedphrase generation is already part of the SDL codebase, but it's not in the full node codebase.

@HarryR to beg the question, why are the memos not random? Did you do statistical analysis on the distribution of values? None of my issues above relate to their lack of random-ness.

leto commented 4 years ago

To clarify, I don't think leaking the state of rand() is considered a valid bug. We have very specific threat models, and we never use rand() for cryptographic purposes, so that is just not a valid issue for us. Leaking memory and non-aligned stuff and wrong-sized memo fields are indeed valid bugs, but I still would not consider them to have any security impact.

Our threat model is the same as Bitcoin and all other Zcash Protocol coins: If you run untrusted software on the same hardware as your full node wallet, you have already lost. None of the internals are side-channel resistant, so it's not a threat model that concerns us.

If you find bugs which are related to attacker-controlled data coming from the network, then those are very interesting to us.

leto commented 4 years ago

@DenioD I suggest you emulate my memo-randomization code that recently landed in hushd. Simplify things as much as possible.

leto commented 4 years ago

@DenioD that code is here: https://github.com/MyHush/hush3/blob/dev/src/sietch.h#L29

MyHush / SilentDragonLite

Sietch random `memo` fields leak stack memory and `rand()` state #192