fuzz: scriptpubkeyman: heap-buffer-overflow miniscript.cpp in CScript BuildScript #30864

1. 
   dergoegge commented at 5:25 pm on September 10, 2024: member

   0$ echo "dHIoJTE3LzwyOzM+LGw6cGsoJTA4KSk=" | base64 --decode > scriptpubkeyman.crash
   1$ FUZZ=scriptpubkeyman src/test/fuzz/fuzz scriptpubkeyman.crash
   2...
   3SUMMARY: AddressSanitizer: heap-buffer-overflow miniscript.cpp in CScript BuildScript<opcodetype, CScript&, opcodetype, CScript&, opcodetype>(opcodetype&&, CScript&, opcodetype&&, CScript&, opcodetype&&)
   4...
   

2. 
   achow101 commented at 5:43 pm on September 10, 2024: member

   I believe this is only an issue with the fuzzer as I can’t trigger this crash outside of it. However, it does reveal an actual issue in the handling of multipath key expressions with miniscript.

   The issue appears to be because MiniscriptDescriptor’s m_node is shallow copied, and when cloned fragments belonging to the multipath components are destroyed later, various shared_ptrs end up also being destroyed.

   The following diff fixes this particular crash, but I think it is insufficient and possibly incorrect:

    0diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
    1index 5026470edcf..c1b898610c7 100644
    2--- a/src/script/descriptor.cpp
    3+++ b/src/script/descriptor.cpp
    4@@ -1360,7 +1360,7 @@ public:
    5         for (const auto& arg : m_pubkey_args) {
    6             providers.push_back(arg->Clone());
    7         }
    8-        return std::make_unique<MiniscriptDescriptor>(std::move(providers), miniscript::MakeNodeRef<uint32_t>(*m_node));
    9+        return std::make_unique<MiniscriptDescriptor>(std::move(providers), m_node);
   10     }
   11 };
   

   I think the proper way to fix this is to deep copy the entire node, but this has the possibility of triggering a stack overflow.

3. 
   achow101 commented at 7:52 pm on September 10, 2024: member

   #30866

Contributors
dergoegge achow101