From 1068f4a2bfcc82aefc307fc2114f52e5ac9e61ee Mon Sep 17 00:00:00 2001 From: Gavin Andresen Date: Fri, 19 Apr 2013 13:27:33 -0400 Subject: [PATCH] Fix use-after-free bug, and better error handling --- README.txt | 25 +- TODO.txt | 76 -- c++/README.txt | 21 + c++/paymentrequest-create.cpp | 34 +- c++/paymentrequest-dump.cpp | 2 +- php/demo_website/createpaymentrequest.php | 7 +- php/demo_website/include/paymentrequest.php | 1227 ++++++++++++++++++- spec.rst | 297 ----- 8 files changed, 1282 insertions(+), 407 deletions(-) delete mode 100644 TODO.txt create mode 100644 c++/README.txt mode change 120000 => 100644 php/demo_website/include/paymentrequest.php delete mode 100644 spec.rst diff --git a/README.txt b/README.txt index 3a58cd8..b50a549 100644 --- a/README.txt +++ b/README.txt @@ -1,25 +1,16 @@ Code implementing a simple payment protocol for Bitcoin (PaymentRequest/etc). -See https://gist.github.com/4120476 +See https://en.bitcoin.it/wiki/BIP_0070 -Dependencies: - OpenSSL (library and openssl command-line tool) - Google Protocol Buffers (library and protoc command-line compiler) +Files here: -Debian/Ubuntu: - apt-get install openssl protobuf -OSX MacPorts: - port install openssl protobuf +paymentrequest.proto : Google protocol buffer definition of messages -To compile: - make +Subdirectories here: -The Makefile will create a "certificate authority in a box" in ca_in_a_box/ and -compile command-line tools: +c++ : command-line utilities for creating/validating PaymentRequests -paymentrequest-create # Prototype code: create a PaymentRequest message -paymentrequest-verify # Prototype code: verify a PaymentRequest message +php : php code and a demo website for creating/validating PaymentRequests - -Example usage: - paymentrequest-create paytoaddress=1BTCorgHwCg6u2YSAWKgS17qUad6kHmtQW memo="Just Testing" amount=11.0 | paymentrequest-dump +ca_in_a_box : "certificate authority in a box", used to generate + certificates and certificate chains for testing. diff --git a/TODO.txt b/TODO.txt deleted file mode 100644 index badb238..0000000 --- a/TODO.txt +++ /dev/null @@ -1,76 +0,0 @@ -Task list for implementing PaymentRequest/etc - -SERVER SIDE: - -DONE: 1. Create command-line, intended-to-run-on-a-web-server merchant tool - that creates PaymentRequest messages: paymentrequest-create - -Arguments: - --paytoaddress=BTC_ADDRESS - --amount=BTC (e.g. 1.0) - --certificates=path/to/certleaf.pem,path/to/certintermediate.pem - --privatekey=path/to/key.pem - --memo="message" - --expires=timestamp - --payment_url=URL - --out=path_to_file - ---certificates is the certificate chain, farthest-from-root first. ---privatekey must be the key used by the first --certificate. ---out defaults to stdout - -See https://gist.github.com/4120476 for a description of the other arguments. - -DONE: 2. Create a command-line tools for debugging/testing: - "paymentrequest-dump" - -3. Create a testing website that generates -testnet PaymentRequests signed - by a valid certificate (allow user to choose paytoaddress/amount/etc). - DONE: https://bitcoincore.org/~gavin/createpaymentrequest.php - -4. Modify testing website so it will generate invalid PaymentRequests - Conditions: - Self-signed - Expired leaf certificate - Valid-certificates but expired PaymentRequest - Invalid signature - ??? what else ??? UTF-8 tricks with the common name? (e.g. attacker^H^H^H^H^H^H^H^Hamazon.com ) - Is that taken care of by the root CA ? - -5. Add to testing website to support a payment_url that generates PaymentACK messages: - DONE: Add an id number to generated PaymentRequests, add to PaymentRequest.merchant_data - and store them as they're generated - DONE: Handle Payment messages submitted to payment_url: - get id from merchant_data, lookup associated PaymentRequest - - -CLIENT SIDE - -1. DONE/PULLED: Rework Bitcoin-Qt bitcoin: URI handling to use QLocalSocket instead of the flaky - boost::interprocess mechanism. - -2. Teach Bitcoin-Qt to handle application/x-bitcoin-payment-request MIME type: - DONE Add protobuf library as dependency to .pro file, doc/build-*.txt, gitian dependencies, etc. - DONE Use system root certs, but allow overriding with own list. - DONE Display Send dialog with info from PaymentRequest - (displayed in Send tab) - - DONE: Unit tests for valid/invalid certs - -3. Modify installers to: - register Bitcoin-Qt as x-bitcoin-payment-request handler - OSX: DONE - Windows - Linux (??? just ppa ???) - - TEST: OSX/Linux/Windows - -4. Teach Bitcoin-Qt the extended bitcoin: URI syntax: - DONE if request= given, fetch payment-request from URL - - TEST: OSX/Linux/Windows - -5. Teach Bitcoin-Qt to create and submit Payment messages when given - a PaymentRequest with a payment_url: - DONE Code to POST to a TLS-protected url (let user know if ERROR) - DONE Code to parse response and show PaymentACK.memo diff --git a/c++/README.txt b/c++/README.txt new file mode 100644 index 0000000..16bd6dc --- /dev/null +++ b/c++/README.txt @@ -0,0 +1,21 @@ +Dependencies: + OpenSSL (library and openssl command-line tool) + Google Protocol Buffers (library and protoc command-line compiler) + +Debian/Ubuntu: + apt-get install openssl protobuf +OSX MacPorts: + port install openssl protobuf + +To compile: + make + +The Makefile will create a "certificate authority in a box" in ../ca_in_a_box/ and +compile command-line tools: + +paymentrequest-create # Prototype code: create a PaymentRequest message +paymentrequest-verify # Prototype code: verify a PaymentRequest message + + +Example usage: + paymentrequest-create paytoaddress=1BTCorgHwCg6u2YSAWKgS17qUad6kHmtQW memo="Just Testing" amount=11.0 | paymentrequest-dump diff --git a/c++/paymentrequest-create.cpp b/c++/paymentrequest-create.cpp index 0de27c0..5fdbc8c 100644 --- a/c++/paymentrequest-create.cpp +++ b/c++/paymentrequest-create.cpp @@ -37,7 +37,7 @@ #include #include -#include "paymentrequest.pb.h"; +#include "paymentrequest.pb.h" #include // For string-to-uint64 conversion #include "util.h" @@ -352,25 +352,28 @@ int main(int argc, char **argv) { // Certificate chain: X509Certificates certChain; - X509 *first_cert = NULL; - X509 *last_cert = NULL; - EVP_PKEY *pubkey = NULL; std::list certFiles = split(params["certificates"], ","); + std::vector certs; for (std::list::iterator it = certFiles.begin(); it != certFiles.end(); it++) { X509 *cert = parse_pem_cert(load_file(*it)); - certChain.add_certificate(x509_to_der(cert)); - if (first_cert == NULL) { - first_cert = cert; // Don't free this yet, need pubkey to stay valid - pubkey = X509_get_pubkey(cert); - } - else { - X509_free(cert); + if (cert == NULL) { + fprintf(stderr, "Could not read %s\n", it->c_str()); + continue; } - last_cert = cert; + certs.push_back(cert); + certChain.add_certificate(x509_to_der(cert)); + } + + if (certs.empty()) { + fprintf(stderr, "Could not load any certificates\n"); + exit(1); } + // Public key is first certificate: + EVP_PKEY *pubkey = X509_get_pubkey(certs.front()); + // Fetch any missing intermediate certificates, if we can: - fetchParentCerts(certChain, last_cert); + fetchParentCerts(certChain, certs.back()); string certChainBytes; certChain.SerializeToString(&certChainBytes); @@ -410,7 +413,10 @@ int main(int argc, char **argv) { } EVP_MD_CTX_destroy(ctx); EVP_PKEY_free(pubkey); - X509_free(first_cert); + for (std::vector::iterator it = certs.begin(); it != certs.end(); it++) { + X509_free(*it); + } + certs.clear(); // We got here, so the signature is self-consistent. request.set_signature(signature, actual_signature_len); diff --git a/c++/paymentrequest-dump.cpp b/c++/paymentrequest-dump.cpp index 1c4864a..d20e400 100644 --- a/c++/paymentrequest-dump.cpp +++ b/c++/paymentrequest-dump.cpp @@ -15,7 +15,7 @@ #include #include -#include "paymentrequest.pb.h"; +#include "paymentrequest.pb.h" #include "util.h" using std::string; diff --git a/php/demo_website/createpaymentrequest.php b/php/demo_website/createpaymentrequest.php index bd4aec7..299819d 100755 --- a/php/demo_website/createpaymentrequest.php +++ b/php/demo_website/createpaymentrequest.php @@ -66,7 +66,7 @@ function createPaymentRequest($params) if (!empty($params[$field])) { $output = new \payments\Output(); $r = address_to_script($params["address".$i]); - if ($r[0]) $testnet = true; + $testnet = $r[0]; $output->setScript($r[1]); $output->setAmount($params["amount".$i]*1.0e8); $totalAmount += $params["amount".$i]; @@ -183,6 +183,11 @@ function createPaymentRequest($params) $validationData['amount3'] = array('type' => 'btcamount'); if (isset($request['submit'])) { + // For debugging Tor connections: replace $CLIENT_IP + // in memo/ACK_message with client's IP address: + $request['memo'] = str_replace('$CLIENT_IP', $_SERVER['REMOTE_ADDR'], $request['memo']); + $request['ACK_message'] = str_replace('$CLIENT_IP', $_SERVER['REMOTE_ADDR'], $request['ACK_message']); + $formErrors = validateForm($request, $validationData); if (count($formErrors) == 0) { diff --git a/php/demo_website/include/paymentrequest.php b/php/demo_website/include/paymentrequest.php deleted file mode 120000 index 64c7e5c..0000000 --- a/php/demo_website/include/paymentrequest.php +++ /dev/null @@ -1 +0,0 @@ -../../paymentrequest.php \ No newline at end of file diff --git a/php/demo_website/include/paymentrequest.php b/php/demo_website/include/paymentrequest.php new file mode 100644 index 0000000..48f9780 --- /dev/null +++ b/php/demo_website/include/paymentrequest.php @@ -0,0 +1,1226 @@ +number = 1; + $f->name = "amount"; + $f->type = \DrSlump\Protobuf::TYPE_UINT64; + $f->rule = \DrSlump\Protobuf::RULE_OPTIONAL; + $f->default = 0; + $descriptor->addField($f); + + // REQUIRED BYTES script = 2 + $f = new \DrSlump\Protobuf\Field(); + $f->number = 2; + $f->name = "script"; + $f->type = \DrSlump\Protobuf::TYPE_BYTES; + $f->rule = \DrSlump\Protobuf::RULE_REQUIRED; + $descriptor->addField($f); + + foreach (self::$__extensions as $cb) { + $descriptor->addField($cb(), true); + } + + return $descriptor; + } + + /** + * Check if has a value + * + * @return boolean + */ + public function hasAmount(){ + return $this->_has(1); + } + + /** + * Clear value + * + * @return \payments\Output + */ + public function clearAmount(){ + return $this->_clear(1); + } + + /** + * Get value + * + * @return int + */ + public function getAmount(){ + return $this->_get(1); + } + + /** + * Set value + * + * @param int $value + * @return \payments\Output + */ + public function setAmount( $value){ + return $this->_set(1, $value); + } + + /** + * Check if