Skip to content

util: Limit decimal range of numbers ParseScript accepts #18416

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

Merged
merged 1 commit into from
Mar 27, 2020
Merged

util: Limit decimal range of numbers ParseScript accepts #18416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,8 @@ EXTRA_DIST += \
test/util/data/txcreatescript3.json \
test/util/data/txcreatescript4.hex \
test/util/data/txcreatescript4.json \
test/util/data/txcreatescript5.hex \
test/util/data/txcreatescript6.hex \
test/util/data/txcreatesignv1.hex \
test/util/data/txcreatesignv1.json \
test/util/data/txcreatesignv2.hex \
Expand Down
8 changes: 8 additions & 0 deletions src/core_read.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,14 @@ CScript ParseScript(const std::string& s)
{
// Number
int64_t n = atoi64(*w);

//limit the range of numbers ParseScript accepts in decimal
//since numbers outside -0xFFFFFFFF...0xFFFFFFFF are illegal in scripts
if (n > int64_t{0xffffffff} || n < -1 * int64_t{0xffffffff}) {
throw std::runtime_error("script parse error: decimal numeric value only allowed in the "
"range -0xFFFFFFFF...0xFFFFFFFF");
}

result << n;
}
else if (w->substr(0,2) == "0x" && w->size() > 2 && IsHex(std::string(w->begin()+2, w->end())))
Expand Down
26 changes: 13 additions & 13 deletions src/test/data/script_tests.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,9 @@
["8388608", "SIZE 4 EQUAL", "P2SH,STRICTENC", "OK"],
["2147483647", "SIZE 4 EQUAL", "P2SH,STRICTENC", "OK"],
["2147483648", "SIZE 5 EQUAL", "P2SH,STRICTENC", "OK"],
["549755813887", "SIZE 5 EQUAL", "P2SH,STRICTENC", "OK"],
["549755813888", "SIZE 6 EQUAL", "P2SH,STRICTENC", "OK"],
["9223372036854775807", "SIZE 8 EQUAL", "P2SH,STRICTENC", "OK"],
["0x05ffffffff7f", "SIZE 5 EQUAL", "P2SH,STRICTENC", "OK"],
["0x06000000008000", "SIZE 6 EQUAL", "P2SH,STRICTENC", "OK"],
["0x08ffffffffffffff7f", "SIZE 8 EQUAL", "P2SH,STRICTENC", "OK"],
["-1", "SIZE 1 EQUAL", "P2SH,STRICTENC", "OK"],
["-127", "SIZE 1 EQUAL", "P2SH,STRICTENC", "OK"],
["-128", "SIZE 2 EQUAL", "P2SH,STRICTENC", "OK"],
Expand All @@ -133,9 +133,9 @@
["-8388608", "SIZE 4 EQUAL", "P2SH,STRICTENC", "OK"],
["-2147483647", "SIZE 4 EQUAL", "P2SH,STRICTENC", "OK"],
["-2147483648", "SIZE 5 EQUAL", "P2SH,STRICTENC", "OK"],
["-549755813887", "SIZE 5 EQUAL", "P2SH,STRICTENC", "OK"],
["-549755813888", "SIZE 6 EQUAL", "P2SH,STRICTENC", "OK"],
["-9223372036854775807", "SIZE 8 EQUAL", "P2SH,STRICTENC", "OK"],
["0x05ffffffffff", "SIZE 5 EQUAL", "P2SH,STRICTENC", "OK"],
["0x06000000008080", "SIZE 6 EQUAL", "P2SH,STRICTENC", "OK"],
["0x08ffffffffffffffff", "SIZE 8 EQUAL", "P2SH,STRICTENC", "OK"],
["'abcdefghijklmnopqrstuvwxyz'", "SIZE 26 EQUAL", "P2SH,STRICTENC", "OK"],

["42", "SIZE 1 EQUALVERIFY 42 EQUAL", "P2SH,STRICTENC", "OK", "SIZE does not consume argument"],
Expand Down Expand Up @@ -360,9 +360,9 @@
["8388608", "0x04 0x00008000 EQUAL", "P2SH,STRICTENC", "OK"],
["2147483647", "0x04 0xFFFFFF7F EQUAL", "P2SH,STRICTENC", "OK"],
["2147483648", "0x05 0x0000008000 EQUAL", "P2SH,STRICTENC", "OK"],
["549755813887", "0x05 0xFFFFFFFF7F EQUAL", "P2SH,STRICTENC", "OK"],
["549755813888", "0x06 0x000000008000 EQUAL", "P2SH,STRICTENC", "OK"],
["9223372036854775807", "0x08 0xFFFFFFFFFFFFFF7F EQUAL", "P2SH,STRICTENC", "OK"],
["0x05ffffffff7f", "0x05 0xFFFFFFFF7F EQUAL", "P2SH,STRICTENC", "OK"],
["0x06000000008000", "0x06 0x000000008000 EQUAL", "P2SH,STRICTENC", "OK"],
["0x08ffffffffffffff7f", "0x08 0xFFFFFFFFFFFFFF7F EQUAL", "P2SH,STRICTENC", "OK"],
["-1", "0x01 0x81 EQUAL", "P2SH,STRICTENC", "OK", "Numbers are little-endian with the MSB being a sign bit"],
["-127", "0x01 0xFF EQUAL", "P2SH,STRICTENC", "OK"],
["-128", "0x02 0x8080 EQUAL", "P2SH,STRICTENC", "OK"],
Expand All @@ -373,9 +373,9 @@
["-2147483647", "0x04 0xFFFFFFFF EQUAL", "P2SH,STRICTENC", "OK"],
["-2147483648", "0x05 0x0000008080 EQUAL", "P2SH,STRICTENC", "OK"],
["-4294967295", "0x05 0xFFFFFFFF80 EQUAL", "P2SH,STRICTENC", "OK"],
["-549755813887", "0x05 0xFFFFFFFFFF EQUAL", "P2SH,STRICTENC", "OK"],
["-549755813888", "0x06 0x000000008080 EQUAL", "P2SH,STRICTENC", "OK"],
["-9223372036854775807", "0x08 0xFFFFFFFFFFFFFFFF EQUAL", "P2SH,STRICTENC", "OK"],
["0x05ffffffffff", "0x05 0xFFFFFFFFFF EQUAL", "P2SH,STRICTENC", "OK"],
["0x06000000008080", "0x06 0x000000008080 EQUAL", "P2SH,STRICTENC", "OK"],
["0x08ffffffffffffffff", "0x08 0xFFFFFFFFFFFFFFFF EQUAL", "P2SH,STRICTENC", "OK"],

["2147483647", "1ADD 2147483648 EQUAL", "P2SH,STRICTENC", "OK", "We can do math on 4-byte integers, and compare 5-byte ones"],
["2147483647", "1ADD 1", "P2SH,STRICTENC", "OK"],
Expand Down Expand Up @@ -2521,7 +2521,7 @@
["-1", "CHECKSEQUENCEVERIFY", "CHECKSEQUENCEVERIFY", "NEGATIVE_LOCKTIME", "CSV automatically fails if stack top is negative"],
["0x0100", "CHECKSEQUENCEVERIFY", "CHECKSEQUENCEVERIFY,MINIMALDATA", "UNKNOWN_ERROR", "CSV fails if stack top is not minimally encoded"],
["0", "CHECKSEQUENCEVERIFY", "CHECKSEQUENCEVERIFY", "UNSATISFIED_LOCKTIME", "CSV fails if stack top bit 1 << 31 is set and the tx version < 2"],
["4294967296", "CHECKSEQUENCEVERIFY", "CHECKSEQUENCEVERIFY", "UNSATISFIED_LOCKTIME",
["0x050000000001", "CHECKSEQUENCEVERIFY", "CHECKSEQUENCEVERIFY", "UNSATISFIED_LOCKTIME",
Copy link
Member

Choose a reason for hiding this comment

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

Why are you changing all the values when translating from dec to hex? It might be fine, but at least for locktime values there are different code paths depending on the value.

Copy link
Contributor Author

@pierreN pierreN Mar 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ow, good point. I double checked with the value returned by ParseScript in the tests before/after the PR - so that the behavior of the tests doesn't change at all (i.e. before this PR the script built by ParseScript from 4294967296 contained 0x050000000001)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, oops. My bad

"CSV fails if stack top bit 1 << 31 is not set, and tx version < 2"],

["MINIMALIF tests"],
Expand Down
2 changes: 1 addition & 1 deletion src/test/data/tx_invalid.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@
"0100000001000100000000000000000000000000000000000000000000000000000000000000000000000000000001000000000000000000ff64cd1d", "P2SH,CHECKLOCKTIMEVERIFY"],

["Argument 2^32 with nLockTime=2^32-1"],
[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4294967296 CHECKLOCKTIMEVERIFY 1"]],
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this version be kept as an example of an _invalid script?

Copy link
Contributor Author

@pierreN pierreN Mar 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

OK, thanks. This is a good idea I think, I was hesitant to add more tests. If I understood you correctly, the tests are invalids here, so I guess you mean adding something like:

["Argument 2^32 in decimal (hence clamped to 0xffffffff) with nLockTime=2^32-2"],
[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "4294967296 CHECKLOCKTIMEVERIFY 1"]],
"0100000001000100000000000000000000000000000000000000000000000000000000000000000000000000000001000000000000000000fffffffe", "P2SH,CHECKLOCKTIMEVERIFY"],

If this is correct, I guess I should add similar tests for other modified test cases too ?

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to add as many tests as you like, as long as all existing proper test cases also stay there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK great, I added 5 more tests in the branch.

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is that this shouldn't be silently clamped. The test here looks right. Why would it be safe to parse a script and silently change it while parsing?

Copy link
Contributor

@practicalswift practicalswift Mar 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

std::runtime_error is thrown on other script parse errors in ParseScript(…). Shouldn't that be thrown also for this type of script parse error in ParseScript?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, thanks, this is clearly a possibility.

Note that for now, no test trigger the already existing std::runtime_error. script_json_test doesn't handle exceptions, so it should be modified to catch them (maybe in a different PR then ?). Testcases will have to be modified to handle the exception too I guess.

(the fuzzer harness already handle exceptions well)

I'm not sure which possibility (clamping or throwing an error) fits more the style of the codebase. Maybe getting an ACK from @sipa for the exception first ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@practicalswift hm sorry but I guess it's better to ask first: if you return an std::runtime_error, how to describe the error in the JSON test file ?

I guess that adding a new value to the ScriptError_t enum is too intrusive.
Adding a special if/else case in script_test.cpp:ParseScriptError seems too hackish.
Reusing an existing ScriptError_t seems the less odd option - something like SCRIPT_ERR_BAD_OPCODE or SCRIPT_ERR_OP_COUNT ?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there is a need. (tx/script)_(valid/invalid) are testing the transaction and script validity logic - not the script parser.

If a particular test now causes an exception to be thrown, just write that test differently. It's still testing the same thing.

Copy link
Contributor Author

@pierreN pierreN Mar 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

OK, so as stated below, if a test contains 4294967296 CHECKLOCKTIMEVERIFY 1 or 999...999, this is a script parser error which should be covered in bitcoin-util-test.json, not here. I believe this resolve this issue ?

[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "0x050000000001 CHECKLOCKTIMEVERIFY 1"]],
"0100000001000100000000000000000000000000000000000000000000000000000000000000000000000000000001000000000000000000ffffffff", "P2SH,CHECKLOCKTIMEVERIFY"],

["Same, but with nLockTime=2^31-1"],
Expand Down
6 changes: 3 additions & 3 deletions src/test/data/tx_valid.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,11 +292,11 @@
"020000000100010000000000000000000000000000000000000000000000000000000000000000000000ffffffff0100000000000000000000000000", "P2SH,CHECKSEQUENCEVERIFY"],

["Argument 3<<31 with various nSequence"],
[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "6442450944 CHECKSEQUENCEVERIFY 1"]],
[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "0x050000008001 CHECKSEQUENCEVERIFY 1"]],
"020000000100010000000000000000000000000000000000000000000000000000000000000000000000ffffbf7f0100000000000000000000000000", "P2SH,CHECKSEQUENCEVERIFY"],
[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "6442450944 CHECKSEQUENCEVERIFY 1"]],
[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "0x050000008001 CHECKSEQUENCEVERIFY 1"]],
"020000000100010000000000000000000000000000000000000000000000000000000000000000000000ffffff7f0100000000000000000000000000", "P2SH,CHECKSEQUENCEVERIFY"],
[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "6442450944 CHECKSEQUENCEVERIFY 1"]],
[[["0000000000000000000000000000000000000000000000000000000000000100", 0, "0x050000008001 CHECKSEQUENCEVERIFY 1"]],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that using an exception allowed to raise new errors in tx_valid - before the clamped value also valided the test...

"020000000100010000000000000000000000000000000000000000000000000000000000000000000000ffffffff0100000000000000000000000000", "P2SH,CHECKSEQUENCEVERIFY"],

["5 byte non-minimally-encoded operandss are valid"],
Expand Down
34 changes: 34 additions & 0 deletions test/util/data/bitcoin-util-test.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,40 @@
"output_cmp": "txcreatescript4.json",
"description": "Create a new transaction with a single output script (OP_DROP) in a P2SH, wrapped in a P2SH (output as json)"
},
{ "exec": "./bitcoin-tx",
"args": ["-create", "outscript=0:9999999999"],
"return_code": 1,
"error_txt": "error: script parse error: decimal numeric value only allowed in the range -0xFFFFFFFF...0xFFFFFFFF",
"description": "Try to parse an output script with a decimal number above the allowed range"
},
{ "exec": "./bitcoin-tx",
"args": ["-create", "outscript=0:4294967296"],
"return_code": 1,
"error_txt": "error: script parse error: decimal numeric value only allowed in the range -0xFFFFFFFF...0xFFFFFFFF",
"description": "Try to parse an output script with a decimal number just above the allowed range"
},
{ "exec": "./bitcoin-tx",
"args": ["-create", "outscript=0:4294967295"],
"output_cmp": "txcreatescript5.hex",
"description": "Try to parse an output script with a decimal number at the upper limit of the allowed range"
},
{ "exec": "./bitcoin-tx",
"args": ["-create", "outscript=0:-9999999999"],
"return_code": 1,
"error_txt": "error: script parse error: decimal numeric value only allowed in the range -0xFFFFFFFF...0xFFFFFFFF",
"description": "Try to parse an output script with a decimal number below the allowed range"
},
{ "exec": "./bitcoin-tx",
"args": ["-create", "outscript=0:-4294967296"],
"return_code": 1,
"error_txt": "error: script parse error: decimal numeric value only allowed in the range -0xFFFFFFFF...0xFFFFFFFF",
"description": "Try to parse an output script with a decimal number just below the allowed range"
},
{ "exec": "./bitcoin-tx",
"args": ["-create", "outscript=0:-4294967295"],
"output_cmp": "txcreatescript6.hex",
"description": "Try to parse an output script with a decimal number at the lower limit of the allowed range"
},
{ "exec": "./bitcoin-tx",
"args":
["-create", "nversion=1",
Expand Down
1 change: 1 addition & 0 deletions test/util/data/txcreatescript5.hex
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
02000000000100000000000000000605ffffffff0000000000
1 change: 1 addition & 0 deletions test/util/data/txcreatescript6.hex
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
02000000000100000000000000000605ffffffff8000000000