[REST] basic input/output sanity check #5492

jonasschnelli commented at 7:59 PM on December 16, 2014: contributor

Mentioned by SergioDemianLerner at https://github.com/bitcoin/bitcoin/commit/f676c80f437e15ab0ee190e93baea733a88db0f2#commitcomment-8996114

Make sure that we not send back raw request parameters.

jgarzik commented at 8:04 PM on December 16, 2014: contributor

What about rethinking the logic a bit? This is only needed for error responses that echo back to the client a piece of information it already knows.

It seems like each case could be simplified by removing the variable portion of each error string.

jonasschnelli commented at 8:18 PM on December 16, 2014: contributor

Removing the variable from the error string would also be feasible method IMO. Depends if we'd like to keep the "more information" in the error string. But i agree with just have "invalid hash" instead of "invalid hash: /inputhash/", etc.

On the other hand it could be handy and save(r) for upcoming implementations to make sure input parameters gets a minimalistic check.

In the current implementation there is a minimalistic very tiny risk of exploiting IsHex (eating cpu over rest with horrible path lengths, path_max is not specified in RFC as far as i know).

Somehow i have a feeling we should truncate and SanitizeString() all input parameters to prevent any possible upcoming damage.

laanwj added the label REST on Jan 8, 2015

laanwj commented at 11:56 AM on January 26, 2015: member

+1 for better input validation.

However I don't like the approach of using sanitize for input strings. It should either accept the input as-is or reject it and throw an error immediately. Don't try to proceed at all using 'sanitized' input.

Using sanitizestring for logged messages makes sense.

gavinandresen commented at 7:21 PM on March 11, 2015: contributor

I agree with @laanwj -- throw an error if given unsanitary input, don't try to fix it. (but sanitize if input is logged as part of the error handling, of course)

jonasschnelli commented at 10:32 PM on March 11, 2015: contributor

Agreed. I'll update the PR to detect unsanitary inputs and throw exception in such cases.

jonasschnelli force-pushed on Mar 12, 2015

jonasschnelli commented at 1:03 PM on March 12, 2015: contributor

Updated. Now it will detect unsafe inputs. Outputs still go though SanitizeString().

[REST] basic input/output sanity check

Mentioned by SergioDemianLerner at https://github.com/bitcoin/bitcoin/commit/f676c80f437e15ab0ee190e93baea733a88db0f2#commitcomment-8996114

367790d5cf

jonasschnelli force-pushed on Mar 13, 2015

in src/rest.cpp:None in 367790d5cf outdated

 276 | @@ -260,15 +277,15 @@ static bool rest_tx(AcceptedConnection* conn,
 277 |      vector<string> params;
 278 |      const RetFormat rf = ParseDataFormat(params, strReq);
 279 |  
 280 | -    string hashStr = params[0];
 281 | +    CheckRequestString(params[0], IT_TXHASH);

laanwj commented at 3:13 PM on March 20, 2015:

Isn't this pre-check redundant? e.g., ParseHashStr will already fail on invalid hashes. Or should, at least.

jonasschnelli commented at 2:32 PM on March 21, 2015: contributor

@laanwj: Right. Is was redundant.

I just updated the whole PR. I did also remove SanitizeString() from error replies because we are now sure that hashes and amounts are correct formatted, etc. How does it look now?

[REST] basic input/output sanity check (overhaul)

- removed SanitizeString() because we now make sure that input parameters are correct.
- more precise parameter check including more details error string.

d5b73e1360

in src/rest.cpp:None in 114c13cfdb outdated

  58 | @@ -52,6 +59,15 @@ static RestErr RESTERR(enum HTTPStatusCode status, string message)
  59 |      return re;
  60 |  }
  61 |  
  62 | +void CheckRequestString(const string inputString, enum InputType inputType = IT_UNDEF, size_t maxLength = 0)

Diapolo commented at 5:47 PM on March 21, 2015:

Why are you not using const std::string& inputString?

jonasschnelli commented at 7:00 PM on March 21, 2015:

Right. The rest.cpp class was introduced with using namespace std; which is somehow inconsistent with other classes/files. This PR just follows the current rest.cpp implementation code style. But i'm pretty sure someone will open a PR to fix this. :)

Diapolo commented at 6:39 AM on March 23, 2015:

The important part wasn't the std:: it was the & ;-).

jonasschnelli commented at 8:11 AM on March 23, 2015:

Ha. Yes. Right. will change that to pass-by-ref.

jonasschnelli commented at 10:40 AM on March 23, 2015:

Why are you not using const std::string& inputString?

Fixed.

jonasschnelli force-pushed on Mar 23, 2015

laanwj commented at 8:36 AM on March 24, 2015: member

The redundant CheckRequestString check still seems to be there?

I'd personally have kept the SanitizeStrings on output just in case, as a kind of belt-and-suspenders, but no strong opinion there.

jonasschnelli commented at 8:39 AM on March 24, 2015: contributor

@laanwj There is no more redundancy. CheckRequestString() will ensure input correctness, ParseHashStr() will no longer check for correctness of the hashstr to parse. I removed SanitizeStrings because it was no longer possible to output a non hex hash or unlimited-non-digi-value.

laanwj commented at 9:46 AM on March 24, 2015: member

ParseHashStr() will no longer check for correctness of the hashstr to parse.

To me it seems that was an excellent place to do the check. Where possible, combining checking input and parsing makes sense, as it reduces the scope for error.

jonasschnelli commented at 6:14 PM on March 24, 2015: contributor

I'm also not happy with the solution. Let me try to optimize and shuffle around some things.

laanwj closed this on Jul 10, 2015

MarcoFalke locked this on Sep 8, 2021