lbryio / lbrycrd

The blockchain that provides the digital content namespace for the LBRY protocol
https://lbry.com
MIT License
2.57k stars 178 forks source link

Sanitize RPC commands #125

Closed shyba closed 6 years ago

shyba commented 6 years ago

It's more of a low priority feature request and maybe a good first issue, but something I noticed is that we could validate the parameters in a better way.

Some of our methods doesn't check for validity of claim id or how the parameters comes: https://github.com/lbryio/lbrycrd/blob/master/src/rpc/claimtrie.cpp#L498

In opposition, some of Bitcoin commands does the check and returns an error if it's not a proper hash. For instance getrawtransaction calls ParseHashV for checking it: https://github.com/lbryio/lbrycrd/blob/270307a29070a4c10095528e232ad0f8e251ec5a/src/rpc/rawtransaction.cpp#L183

Which is implemented here, where we could also add our validators: https://github.com/lbryio/lbrycrd/blob/270307a29070a4c10095528e232ad0f8e251ec5a/src/rpc/server.cpp#L144

Aside from the usability of the cli for people giving the wrong input to commands, this should avoid calling methods over bogus data with sometimes unpredictable outcomes.

kauffj commented 6 years ago

Thank you @shyba! And yes, this is a good first issue.

If it's not too much work, could you add a list of methods that need this, or a way someone else could identify the ones that do?

shyba commented 6 years ago

Sure! Commands are implemented here: https://github.com/lbryio/lbrycrd/blob/master/src/rpc/claimtrie.cpp The ones that needs sanitizing are:

See how it's done in here: https://github.com/lbryio/lbrycrd/blob/270307a29070a4c10095528e232ad0f8e251ec5a/src/rpc/rawtransaction.cpp#L183

For the name parameter in getclaimsforname and getvalueforname no sanitization is necessary since they are expected to be strings in any format.

BrannonKing commented 6 years ago

Fixed via https://github.com/lbryio/lbrycrd/commit/e3813362815556634046970d978ba66cef84fea9