Transaction details endpoint should validate hash parameter

[tamirms]

The endpoint which processes /transactions/{hash} requests does not validate the hash parameter.

If you use a hash which is not a valid utf8 sequence this will result in horizon responding with a 500 status. Here is an example of this error which was logged recently:

time="2020-03-12T17:30:39.411Z" level=error msg="loading transaction record: get failed: pq: invalid byte sequence for encoding \"UTF8\": 0x00" pid=5777 req=horizon-testnet-green-002c/xuy67jVTC6-44865331 stack="[main.go:43 transaction.go:118 action.go:217 value.go:460 value.go:321 handler.go:100 handler.go:69 handler.go:222 server.go:2007 mux.go:418 server.go:2007 mux.go:69 mux.go:295 server.go:2007 mux.go:418 server.go:2007 mux.go:69 mux.go:295 server.go:2007 mux.go:418 server.go:2007 http.go:73 server.go:2007 cors.go:190 server.go:2007 compress.go:66 server.go:2007 middleware.go:220 server.go:2007 middleware.go:231 timer.go:227 middleware.go:230]"

[2opremio]

I can reproduce this with curl http://horizonhost/transactions/%00

@tamirms did you stumble into this through unescaped URLs as well?

[tamirms]

yeah, this is the request which triggered the error earlier:

/transactions/%00%1E4%5E%EF%BF%BD%EF%BF%BD%EF%BF%BDpVP%EF%BF%BDI&R%0BK%EF%BF%BD%1D%EF%BF%BD%EF%BF%BD=%EF%BF%BD%3F%23%EF%BF%BD%EF%BF%BDl%EF%BF%BD%1El%EF%BF%BD%EF%BF%BD```

[2opremio]

The URL parameter does already get validated as utf8 (i.e. 0x00 is a valid string, see https://play.golang.org/p/rkcxiep0rFO ). I think the problem is the leading 0 when passing the transaction ID on.

Looking into it further ...

[2opremio]

I suspect there is a bug in Squirrel's parameter substitution (or the way we use it).

When doing:

go/services/horizon/internal/db2/history/transaction.go

Lines 22 to 24 in 06b6a58

	sql := selectTransaction.
	Limit(1).
	Where("ht.transaction_hash = ?", hash)

With a 0x00-led hash, the resulting string is:

SELECT ht.id, ht.transaction_hash, ht.ledger_sequence, ht.application_order, ht.account, ht.account_sequence, ht.max_fee, COALESCE(ht.fee_charged, ht.max_fee) as fee_charged, ht.operation_count, ht.tx_envelope, ht.tx_result, ht.tx_meta, ht.tx_fee_meta, ht.created_at, ht.updated_at, ht.successful, array_to_string(ht.signatures, ',') AS signatures, ht.memo_type, ht.memo, lower(ht.time_bounds) AS valid_after, upper(ht.time_bounds) AS valid_before, hl.closed_at AS ledger_close_time FROM history_transactions ht LEFT JOIN history_ledgers hl ON ht.ledger_sequence = hl.sequence WHERE ht.transaction_hash = ? LIMIT 1

(I think that the zero is injected after the ?, which is preserved)

I could simply fix the problem for transactions by ensuring transaction IDs only have hex chars.

However, I think we should understand the Squirrel better and verify that it doesn't happen for other parameters, since it could potentially constitute a security problem.

[2opremio]

There is nothing wrong with Squirrel. It turns out that Postgres uses 0x00 internally as a terminator and it cannot be part of text (text/char/varchar) passed to it, even if it's a valid utf8 :S.

Security-wise, I don't think this is a problem, because we are using placeholders for the parameters.

So, I will just parse the hash IDs as hex characters to validate them.

[2opremio]

Uhm, maybe this should be fixed when merging back to master?

[tamirms]

@2opremio we usually close issues when we have merged them into a release branch

[2opremio]

👍

It seems that GitHub's automatic closing of issues only works when merging onto master. I'll investigate.

[ire-and-curses]

Yeah automatic closing doesn't work except to the chosen repo root branch. It sucks but there you go.