Improve validation of txprepare arguments
#4259
Conversation
cdecker
requested review from
rustyrussell and
niftynei
and removed request for
rustyrussell
common/json_tok.c
Outdated
| cmd, JSONRPC2_INVALID_PARAMS, | ||
| "Invalid element in %s: element %zu is not a valid " | ||
| "outpoint \"%s\"", | ||
| name, i, curr); |
There was a problem hiding this comment.
Yesh, can we implement json_to_outpoint and use this instead?
common/json_tok.h
Outdated
| const char *name, | ||
| const char *buffer, | ||
| const jsmntok_t *tok, | ||
| const char ***strings); |
There was a problem hiding this comment.
Despite the inefficiency, I prefer converting rather than leaving strings. Maybe we need a struct outpoint as a separate patch though.
There was a problem hiding this comment.
I was unsure whether the added effort and conversions would cause some pushback, but generally I agree that we should always verify as much as possible by doing all conversions, even when doing passthru of arguments (which caused this issue in the first place).
cdecker
force-pushed
the
issue-4258
branch
2 times, most recently
from
d681793 to
a178680
Compare
|
Ok, took two steps back and one forward: we now have a |
cdecker
force-pushed
the
issue-4258
branch
2 times, most recently
from
3fd4e4d to
83bd126
Compare
|
Couple of force-pushes to remove some drive-by fixes. |
|
Added a couple of cleanups for you feedback |
We promised to deprecate the old "destination satoshi feerate utxos" variant some time on mid-2020 so let's do just that. `txprepare` doesn't parse it either anymore. Changelog-Changed: pyln: `txprepare` no longer supports the deprecated `destination satoshi feerate utxos` call format.
In a couple of places we accept arrays of strings and don't validate them. If we forward them, e.g., call a JSON-RPC method from the plugin, we end up embedding the unverified string in the JSON-RPC call without escaping, which then leads to invalid JSON being passed on. This at least partially causes ElementsProject#4238
We were not checking that outputs is indeed an array, and just going ahead creating the array of outputs. Since `tok->size` for a string is 0 we ended up ignoring the argument altogether and thus the created transaction would end up only with a single change output. Fixes ElementsProject#4258
We were masquerading errors when parsing the request by reporting only a bogus malformed `id` field in the response, when the real issue was that we were unable to parse the request in the first place (which caused the null-id error to be returned). Fixes ElementsProject#4238
If some parameter is malformed on the command line we could end up with a malformed JSON-RPC request, which would then result in very unhelpful error messages. Fixes ElementsProject#4238 Changelog-Changed: cli: `lightning-cli` now performs better sanity checks on the JSON-RPC requests it sends.
Signed-off-by: Rusty Russell <[email protected]>
No reason to use a temp var here. Signed-off-by: Rusty Russell <[email protected]>
There are two issues we address in this PR:
id is nullerror because the side receiving the mangled request cannot extract theidand returns a generic error instead, which doesn't match up with our expectations.In a couple of places we accept arrays of strings and don't validate
them. If we forward them, e.g., call a JSON-RPC method from the
plugin, we end up embedding the unverified string in the JSON-RPC
call without escaping, which then leads to invalid JSON being passed
on.
We were not checking that outputs is indeed an array, and just going
ahead creating the array of outputs. Since
tok->sizefor a string is0 we ended up ignoring the argument altogether and thus the created
transaction would end up only with a single change output.
Since I was looking at the cli params already I also added some more feedback
to
lightning-cli. It now parses the request before sending it tolightningd, so ifa malformed param results in a malformed request we catch it early and tell the
user to check the formatting of the params.
Fixes #4238
Fixes #4258