From 6256bdb15a7c43b644132ae8c78adf31bcae4d28 Mon Sep 17 00:00:00 2001 From: Christian Grothoff Date: Wed, 12 Aug 2020 20:12:39 +0200 Subject: implement #6478, but untested as shown by FIXMEs --- contrib/gana | 2 +- src/include/taler_amount_lib.h | 18 ++ src/lib/exchange_api_common.c | 12 +- src/lib/exchange_api_refund.c | 379 ++++++++++++++++++++++++++++++++++++++++- src/util/amount.c | 29 ++++ 5 files changed, 430 insertions(+), 10 deletions(-) diff --git a/contrib/gana b/contrib/gana index 73f613235..c36107f80 160000 --- a/contrib/gana +++ b/contrib/gana @@ -1 +1 @@ -Subproject commit 73f61323554df47079e19cd4236d148e2c17a1b3 +Subproject commit c36107f80f136f0a512e62c7b992ea18218280bb diff --git a/src/include/taler_amount_lib.h b/src/include/taler_amount_lib.h index 63c5dba40..4f6d14004 100644 --- a/src/include/taler_amount_lib.h +++ b/src/include/taler_amount_lib.h @@ -205,6 +205,24 @@ TALER_amount_cmp (const struct TALER_Amount *a1, const struct TALER_Amount *a2); +/** + * Compare the value/fraction of two amounts. Does not compare the currency. + * Comparing amounts of different currencies will cause the program to abort(). + * If unsure, check with #TALER_amount_cmp_currency() first to be sure that + * the currencies of the two amounts are identical. NBO variant. + * + * @param a1 first amount + * @param a2 second amount + * @return result of the comparison + * -1 if `a1 < a2` + * 1 if `a1 > a2` + * 0 if `a1 == a2`. + */ +int +TALER_amount_cmp_nbo (const struct TALER_AmountNBO *a1, + const struct TALER_AmountNBO *a2); + + /** * Test if @a a1 and @a a2 are the same currency. * diff --git a/src/lib/exchange_api_common.c b/src/lib/exchange_api_common.c index 743adb0ff..b07ac1112 100644 --- a/src/lib/exchange_api_common.c +++ b/src/lib/exchange_api_common.c @@ -516,7 +516,11 @@ TALER_EXCHANGE_verify_coin_history ( if (0 == strcasecmp (type, "DEPOSIT")) { - struct TALER_DepositRequestPS dr; + struct TALER_DepositRequestPS dr = { + .purpose.size = htonl (sizeof (dr)), + .purpose.purpose = htonl (TALER_SIGNATURE_WALLET_COIN_DEPOSIT), + .coin_pub = *coin_pub + }; struct TALER_CoinSpendSignatureP sig; struct GNUNET_JSON_Specification spec[] = { GNUNET_JSON_spec_fixed_auto ("coin_sig", @@ -546,11 +550,8 @@ TALER_EXCHANGE_verify_coin_history ( GNUNET_break_op (0); return GNUNET_SYSERR; } - dr.purpose.size = htonl (sizeof (dr)); - dr.purpose.purpose = htonl (TALER_SIGNATURE_WALLET_COIN_DEPOSIT); TALER_amount_hton (&dr.amount_with_fee, &amount); - dr.coin_pub = *coin_pub; if (GNUNET_OK != GNUNET_CRYPTO_eddsa_verify (TALER_SIGNATURE_WALLET_COIN_DEPOSIT, &dr, @@ -657,7 +658,7 @@ TALER_EXCHANGE_verify_coin_history ( GNUNET_JSON_spec_fixed_auto ("merchant_pub", &rr.merchant), GNUNET_JSON_spec_uint64 ("rtransaction_id", - &rr.rtransaction_id), + &rr.rtransaction_id), // FIXME: shouldn't this be NBO!? GNUNET_JSON_spec_end () }; @@ -669,6 +670,7 @@ TALER_EXCHANGE_verify_coin_history ( GNUNET_break_op (0); return GNUNET_SYSERR; } + abort (); // FIXME: this shows the case is not tested! ... TALER_amount_hton (&rr.refund_amount, &amount); if (GNUNET_OK != diff --git a/src/lib/exchange_api_refund.c b/src/lib/exchange_api_refund.c index 37c60e07e..7c758ba2a 100644 --- a/src/lib/exchange_api_refund.c +++ b/src/lib/exchange_api_refund.c @@ -129,6 +129,363 @@ verify_refund_signature_ok (struct TALER_EXCHANGE_RefundHandle *rh, } +/** + * Verify that the information in the "409 Conflict" response + * from the exchange is valid and indeed shows that the refund + * amount requested is too high. + * + * @param[in,out] rh refund handle (refund fee added) + * @param json json reply with the coin transaction history + * @return #GNUNET_OK if the signature is valid, #GNUNET_SYSERR if not + */ +static int +verify_conflict_history_ok (struct TALER_EXCHANGE_RefundHandle *rh, + const json_t *json) +{ + json_t *history; + struct GNUNET_JSON_Specification spec[] = { + GNUNET_JSON_spec_json ("history", + &history), + GNUNET_JSON_spec_end () + }; + size_t len; + struct TALER_Amount dtotal; + bool have_deposit; + struct TALER_Amount rtotal; + bool have_refund; + + if (GNUNET_OK != + GNUNET_JSON_parse (json, + spec, + NULL, NULL)) + { + GNUNET_break_op (0); + return GNUNET_SYSERR; + } + // FIXME: check that 'history' contains a coin history that + // demonstrates that another refund would exceed the deposit amount! + + + len = json_array_size (history); + if (0 == len) + { + GNUNET_break_op (0); + return GNUNET_SYSERR; + } + have_deposit = false; + have_refund = false; + for (size_t off = 0; offdepconf.coin_pub + }; + struct TALER_CoinSpendSignatureP sig; + struct GNUNET_JSON_Specification spec[] = { + GNUNET_JSON_spec_fixed_auto ("coin_sig", + &sig), + GNUNET_JSON_spec_fixed_auto ("h_contract_terms", + &dr.h_contract_terms), + GNUNET_JSON_spec_fixed_auto ("h_wire", + &dr.h_wire), + GNUNET_JSON_spec_fixed_auto ("h_denom_pub", + &dr.h_denom_pub), + TALER_JSON_spec_absolute_time_nbo ("timestamp", + &dr.wallet_timestamp), + TALER_JSON_spec_absolute_time_nbo ("refund_deadline", + &dr.refund_deadline), + TALER_JSON_spec_amount_nbo ("deposit_fee", + &dr.deposit_fee), + GNUNET_JSON_spec_fixed_auto ("merchant_pub", + &dr.merchant), + GNUNET_JSON_spec_end () + }; + + if (GNUNET_OK != + GNUNET_JSON_parse (transaction, + spec, + NULL, NULL)) + { + GNUNET_break_op (0); + return GNUNET_SYSERR; + } + TALER_amount_hton (&dr.amount_with_fee, + &amount); + if (GNUNET_OK != + GNUNET_CRYPTO_eddsa_verify (TALER_SIGNATURE_WALLET_COIN_DEPOSIT, + &dr, + &sig.eddsa_signature, + &rh->depconf.coin_pub.eddsa_pub)) + { + GNUNET_break_op (0); + return GNUNET_SYSERR; + } + if ( (0 != GNUNET_memcmp (&rh->depconf.h_contract_terms, + &dr.h_contract_terms)) || + (0 != GNUNET_memcmp (&rh->depconf.merchant, + &dr.merchant)) ) + { + /* deposit information is about a different merchant/contract */ + GNUNET_break_op (0); + return GNUNET_SYSERR; + } + if (have_deposit) + { + /* this cannot really happen, but we conservatively support it anyway */ + GNUNET_break (0 <= + TALER_amount_add (&dtotal, + &dtotal, + &amount)); + } + else + { + dtotal = amount; + have_deposit = true; + } + } + else if (0 == strcasecmp (type, + "REFUND")) + { + struct TALER_MerchantSignatureP sig; + struct TALER_Amount refund_fee; + struct TALER_RefundRequestPS rr = { + .purpose.size = htonl (sizeof (rr)), + .purpose.purpose = htonl (TALER_SIGNATURE_MERCHANT_REFUND), + .coin_pub = rh->depconf.coin_pub + }; + struct GNUNET_JSON_Specification spec[] = { + TALER_JSON_spec_amount ("refund_fee", + &refund_fee), + GNUNET_JSON_spec_fixed_auto ("merchant_sig", + &sig), + GNUNET_JSON_spec_fixed_auto ("h_contract_terms", + &rr.h_contract_terms), + GNUNET_JSON_spec_fixed_auto ("merchant_pub", + &rr.merchant), + GNUNET_JSON_spec_uint64 ("rtransaction_id", + &rr.rtransaction_id), // FIXME: shouldn't this be NBO!? + GNUNET_JSON_spec_end () + }; + + if (GNUNET_OK != + GNUNET_JSON_parse (transaction, + spec, + NULL, NULL)) + { + GNUNET_break_op (0); + return GNUNET_SYSERR; + } + TALER_amount_hton (&rr.refund_amount, + &amount); + if (GNUNET_OK != + GNUNET_CRYPTO_eddsa_verify (TALER_SIGNATURE_MERCHANT_REFUND, + &rr, + &sig.eddsa_sig, + &rr.merchant.eddsa_pub)) + { + GNUNET_break_op (0); + return GNUNET_SYSERR; + } + + if ( (0 != GNUNET_memcmp (&rh->depconf.h_contract_terms, + &rr.h_contract_terms)) || + (0 != GNUNET_memcmp (&rh->depconf.merchant, + &rr.merchant)) ) + { + /* refund is about a different merchant/contract */ + GNUNET_break_op (0); + return GNUNET_SYSERR; + } + if (rr.rtransaction_id == rh->depconf.rtransaction_id) + { + /* Eh, this shows either a dependency failure or idempotency, + but must not happen in a conflict reply. Fail! */ + GNUNET_break_op (0); + return GNUNET_SYSERR; + } + + if (have_refund) + { + GNUNET_break (0 <= + TALER_amount_add (&rtotal, + &rtotal, + &amount)); + } + else + { + rtotal = amount; + have_refund = true; + } + } + else + { + /* unexpected type, new version on server? */ + GNUNET_break_op (0); + GNUNET_log (GNUNET_ERROR_TYPE_ERROR, + "Unexpected type `%s' in response\n", + type); + return GNUNET_SYSERR; + } + } + + { + struct TALER_Amount amount; + + TALER_amount_ntoh (&amount, + &rh->depconf.refund_amount); + if (0 > + TALER_amount_add (&rtotal, + &rtotal, + &amount)) + { + GNUNET_break (0); + return GNUNET_SYSERR; + } + } + if (-1 == TALER_amount_cmp (&dtotal, + &rtotal)) + { + /* dtotal < rtotal: good! */ + return GNUNET_OK; + } + /* this fails to prove a conflict */ + GNUNET_break_op (0); + return GNUNET_SYSERR; +} + + +/** + * Verify that the information on the "412 Dependency Failed" response + * from the exchange is valid and indeed shows that there is a refund + * transaction ID reuse going on. + * + * @param[in,out] rh refund handle (refund fee added) + * @param json json reply with the signature + * @return #GNUNET_OK if the signature is valid, #GNUNET_SYSERR if not + */ +static int +verify_failed_dependency_ok (struct TALER_EXCHANGE_RefundHandle *rh, + const json_t *json) +{ + json_t *h; + json_t *e; + struct GNUNET_JSON_Specification spec[] = { + GNUNET_JSON_spec_json ("history", &h), + GNUNET_JSON_spec_end () + }; + + if (GNUNET_OK != + GNUNET_JSON_parse (json, + spec, + NULL, NULL)) + { + GNUNET_break_op (0); + return GNUNET_SYSERR; + } + if ( (! json_is_array (h)) || + (1 != json_array_size (h) ) ) + { + GNUNET_break_op (0); + return GNUNET_SYSERR; + } + e = json_array_get (h, 0); + { + struct TALER_Amount amount; + const char *type; + struct TALER_MerchantSignatureP sig; + struct TALER_Amount refund_fee; + struct TALER_RefundRequestPS rr = { + .purpose.size = htonl (sizeof (rr)), + .purpose.purpose = htonl (TALER_SIGNATURE_MERCHANT_REFUND), + .coin_pub = rh->depconf.coin_pub + }; + uint64_t rtransaction_id; + struct GNUNET_JSON_Specification spec[] = { + TALER_JSON_spec_amount ("amount", + &amount), + GNUNET_JSON_spec_string ("type", + &type), + TALER_JSON_spec_amount ("refund_fee", + &refund_fee), + GNUNET_JSON_spec_fixed_auto ("merchant_sig", + &sig), + GNUNET_JSON_spec_fixed_auto ("h_contract_terms", + &rr.h_contract_terms), + GNUNET_JSON_spec_fixed_auto ("merchant_pub", + &rr.merchant), + GNUNET_JSON_spec_uint64 ("rtransaction_id", + &rtransaction_id), + GNUNET_JSON_spec_end () + }; + + if (GNUNET_OK != + GNUNET_JSON_parse (e, + spec, + NULL, NULL)) + { + GNUNET_break_op (0); + return GNUNET_SYSERR; + } + rr.rtransaction_id = GNUNET_htonll (rtransaction_id); + TALER_amount_hton (&rr.refund_amount, + &amount); + if (GNUNET_OK != + GNUNET_CRYPTO_eddsa_verify (TALER_SIGNATURE_MERCHANT_REFUND, + &rr, + &sig.eddsa_sig, + &rh->depconf.merchant.eddsa_pub)) + { + GNUNET_break_op (0); + return GNUNET_SYSERR; + } + if ( (rr.rtransaction_id != rh->depconf.rtransaction_id) || + (0 != GNUNET_memcmp (&rh->depconf.h_contract_terms, + &rr.h_contract_terms)) || + (0 != GNUNET_memcmp (&rh->depconf.merchant, + &rr.merchant)) || + (0 == TALER_amount_cmp_nbo (&rh->depconf.refund_amount, + &rr.refund_amount)) ) + { + GNUNET_break_op (0); + return GNUNET_SYSERR; + } + } + return GNUNET_OK; +} + + /** * Function called when we're done processing the * HTTP /refund request. @@ -199,10 +556,17 @@ handle_refund_finished (void *cls, break; case MHD_HTTP_CONFLICT: /* Requested total refunds exceed deposited amount */ + if (GNUNET_OK != + verify_conflict_history_ok (rh, + j)) + { + GNUNET_break (0); + hr.ec = TALER_EC_REFUND_INVALID_FAILURE_PROOF_BY_EXCHANGE; + hr.hint = "conflict information provided by exchange is invalid"; + break; + } hr.ec = TALER_JSON_get_error_code (j); hr.hint = TALER_JSON_get_error_hint (j); - // FIXME: check that 'history' contains a coin history that - // demonstrates that another refund would exceed the deposit amount! break; case MHD_HTTP_GONE: /* Kind of normal: the money was already sent to the merchant @@ -211,12 +575,19 @@ handle_refund_finished (void *cls, hr.hint = TALER_JSON_get_error_hint (j); break; case MHD_HTTP_PRECONDITION_FAILED: + if (GNUNET_OK != + verify_failed_dependency_ok (rh, + j)) + { + GNUNET_break (0); + hr.ec = TALER_EC_REFUND_INVALID_FAILURE_PROOF_BY_EXCHANGE; + hr.hint = "failed precondition proof returned by exchange is invalid"; + break; + } /* Two different refund requests were made about the same deposit, but carrying identical refund transaction ids. */ hr.ec = TALER_JSON_get_error_code (j); hr.hint = TALER_JSON_get_error_hint (j); - // FIXME: check that 'history' contains a duly signed refund request - // for the same rtransaction ID but a different amount! break; case MHD_HTTP_INTERNAL_SERVER_ERROR: /* Server had an internal issue; we should retry, but this API diff --git a/src/util/amount.c b/src/util/amount.c index b5e28051c..5b0c3af53 100644 --- a/src/util/amount.c +++ b/src/util/amount.c @@ -390,6 +390,35 @@ TALER_amount_cmp (const struct TALER_Amount *a1, } +/** + * Compare the value/fraction of two amounts. Does not compare the currency. + * Comparing amounts of different currencies will cause the program to abort(). + * If unsure, check with #TALER_amount_cmp_currency() first to be sure that + * the currencies of the two amounts are identical. NBO variant. + * + * @param a1 first amount + * @param a2 second amount + * @return result of the comparison + * -1 if `a1 < a2` + * 1 if `a1 > a2` + * 0 if `a1 == a2`. + */ +int +TALER_amount_cmp_nbo (const struct TALER_AmountNBO *a1, + const struct TALER_AmountNBO *a2) +{ + struct TALER_Amount h1; + struct TALER_Amount h2; + + TALER_amount_ntoh (&h1, + a1); + TALER_amount_ntoh (&h2, + a2); + return TALER_amount_cmp (&h1, + &h2); +} + + /** * Perform saturating subtraction of amounts. * -- cgit v1.2.3