Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve validation of txprepare arguments #4259

Merged
merged 10 commits into from
Dec 8, 2020
Merged

Improve validation of txprepare arguments #4259

merged 10 commits into from
Dec 8, 2020

Conversation

cdecker
Copy link
Member

@cdecker cdecker commented Dec 4, 2020 •
edited
Loading

There are two issues we address in this PR:

  • Broken JSON strings when forwarding params as raw strings to further JSON-RPC calls, causing the infamous id is null error because the side receiving the mangled request cannot extract the id and returns a generic error instead, which doesn't match up with our expectations.
  • Interpreting a string param as an array, resulting in an empty array, and thus only adding the change output to a prepared tx.

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->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.

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 to lightningd, so if
a 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

@cdecker cdecker self-assigned this Dec 4, 2020
@cdecker cdecker mentioned this pull request Dec 4, 2020
Closed
@cdecker cdecker marked this pull request as ready for review December 4, 2020 13:51
@cdecker cdecker requested review from rustyrussell and niftynei and removed request for rustyrussell December 4, 2020 13:51
@cdecker cdecker mentioned this pull request Dec 4, 2020
Closed
niftynei
niftynei reviewed Dec 5, 2020
View reviewed changes
Copy link
Collaborator

@niftynei niftynei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 10f842e

Awesome work finding this!

rustyrussell
rustyrussell approved these changes Dec 7, 2020
View reviewed changes
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will fix the others as a separate series.

Ack 6c308bf

common/json_tok.c Outdated Show resolved Hide resolved
common/json_tok.c Outdated
cmd, JSONRPC2_INVALID_PARAMS,
"Invalid element in %s: element %zu is not a valid "
"outpoint \"%s\"",
name, i, curr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite the inefficiency, I prefer converting rather than leaving strings. Maybe we need a struct outpoint as a separate patch though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 cdecker force-pushed the issue-4258 branch 2 times, most recently from d681793 to a178680 Compare December 7, 2020 13:17
@cdecker
Copy link
Member Author

cdecker commented Dec 7, 2020

Ok, took two steps back and one forward: we now have a struct bitcoin_outpoint (distinct from outpoiint in wallet which also contains metadata used when spending), json_to_outpoint and param_outpoint_arr. We now fully parse the txprepare utxos and report errors correctly.

@cdecker cdecker force-pushed the issue-4258 branch 2 times, most recently from 3fd4e4d to 83bd126 Compare December 7, 2020 14:03
@cdecker
Copy link
Member Author

cdecker commented Dec 7, 2020

Couple of force-pushes to remove some drive-by fixes.

rustyrussell
rustyrussell reviewed Dec 8, 2020
View reviewed changes
bitcoin/tx.h Outdated Show resolved Hide resolved
@rustyrussell
Copy link
Contributor

Added a couple of cleanups for you feedback

@cdecker
pyln: Remove deprecated txprepare variant
d78192f 
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.
@cdecker
json: Add two param functions to parse string arrs and outpoint arrs
a84d570 
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
@cdecker
txprepare: Use param_outpoint_arr helper to validate input
25d7158
@cdecker
cleanup: Remove stdio header from param.c
23d1670
@cdecker
txprepare: Verify that outputs arg is an array
c6b0f15 
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
@cdecker
pytest: Add a test to reproduce ElementsProject#4258
e80f298
cdecker and others added 4 commits December 8, 2020 12:18
@cdecker
jsonrpc: Forward errors on malformed requests to cli
2652fae 
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
@cdecker
cli: Add a JSON parsing sanity check before sending request
067aff8 
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.
@rustyrussell @cdecker
bitcoin/tx: fix type of outpoint (n is a u32), simplify json_to_outpoint
f7568ea 
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell @cdecker
common/json_tok: minor cleanup to bitcoin_outpoint array parsing.
859bb42 
No reason to use a temp var here.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell merged commit 094889f into ElementsProject:master Dec 8, 2020
@cdecker cdecker deleted the issue-4258 branch December 16, 2020 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rustyrussell rustyrussell rustyrussell approved these changes

@niftynei niftynei Awaiting requested review from niftynei

Assignees

@cdecker cdecker

Labels
bug plugin
Projects
None yet
Milestone
No milestone
3 participants
@cdecker @rustyrussell @niftynei