diff --git a/CHANGELOG.md b/CHANGELOG.md index 301daa8457..7d0ba786bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +## Next RC + +### Additions and Improvements +- Genesis file parameter `blockperiodseconds` is validated as a positive integer on startup to prevent unexpected runtime behaviour [#3186](https://github.com/hyperledger/besu/pull/3186) + ## 22.1.0-RC2 ### 22.1.0-RC2 Breaking Changes @@ -7,10 +12,10 @@ ### Additions and Improvements - Re-order external services (e.g JsonRpcHttpService) to start before blocks start processing [#3118](https://github.com/hyperledger/besu/pull/3118) -- Stream JSON RPC responses to avoid creating big JSON strings in memory [#3076] (https://github.com/hyperledger/besu/pull/3076) +- Stream JSON RPC responses to avoid creating big JSON strings in memory [#3076](https://github.com/hyperledger/besu/pull/3076) ### Bug Fixes -- Make 'to' field optional in eth_call method according to the spec [#3177] (https://github.com/hyperledger/besu/pull/3177) +- Make 'to' field optional in eth_call method according to the spec [#3177](https://github.com/hyperledger/besu/pull/3177) - Update to log4j 2.17.1. Resolves potential vulnerability only exploitable when using custom log4j configurations that are writable by untrusted users. ## 21.10.6 diff --git a/config/build.gradle b/config/build.gradle index c572d10309..04e1588b75 100644 --- a/config/build.gradle +++ b/config/build.gradle @@ -29,6 +29,7 @@ jar { dependencies { implementation project(':datatypes') + implementation project(':util') implementation 'com.fasterxml.jackson.core:jackson-databind' implementation 'com.google.guava:guava' diff --git a/config/src/main/java/org/hyperledger/besu/config/BftFork.java b/config/src/main/java/org/hyperledger/besu/config/BftFork.java index 478e5120a9..a6785c25c8 100644 --- a/config/src/main/java/org/hyperledger/besu/config/BftFork.java +++ b/config/src/main/java/org/hyperledger/besu/config/BftFork.java @@ -48,7 +48,7 @@ public class BftFork { } public OptionalInt getBlockPeriodSeconds() { - return JsonUtil.getInt(forkConfigRoot, BLOCK_PERIOD_SECONDS_KEY); + return JsonUtil.getPositiveInt(forkConfigRoot, BLOCK_PERIOD_SECONDS_KEY); } public Optional getBlockRewardWei() { diff --git a/config/src/main/java/org/hyperledger/besu/config/CliqueConfigOptions.java b/config/src/main/java/org/hyperledger/besu/config/CliqueConfigOptions.java index 5b43d226cd..a23a2cbd6c 100644 --- a/config/src/main/java/org/hyperledger/besu/config/CliqueConfigOptions.java +++ b/config/src/main/java/org/hyperledger/besu/config/CliqueConfigOptions.java @@ -38,7 +38,8 @@ public class CliqueConfigOptions { } public int getBlockPeriodSeconds() { - return JsonUtil.getInt(cliqueConfigRoot, "blockperiodseconds", DEFAULT_BLOCK_PERIOD_SECONDS); + return JsonUtil.getPositiveInt( + cliqueConfigRoot, "blockperiodseconds", DEFAULT_BLOCK_PERIOD_SECONDS); } Map asMap() { diff --git a/config/src/main/java/org/hyperledger/besu/config/IbftLegacyConfigOptions.java b/config/src/main/java/org/hyperledger/besu/config/IbftLegacyConfigOptions.java index 898c6121c0..9eb5944189 100644 --- a/config/src/main/java/org/hyperledger/besu/config/IbftLegacyConfigOptions.java +++ b/config/src/main/java/org/hyperledger/besu/config/IbftLegacyConfigOptions.java @@ -40,7 +40,8 @@ public class IbftLegacyConfigOptions { } public int getBlockPeriodSeconds() { - return JsonUtil.getInt(ibftConfigRoot, "blockperiodseconds", DEFAULT_BLOCK_PERIOD_SECONDS); + return JsonUtil.getPositiveInt( + ibftConfigRoot, "blockperiodseconds", DEFAULT_BLOCK_PERIOD_SECONDS); } public int getRequestTimeoutSeconds() { diff --git a/config/src/main/java/org/hyperledger/besu/config/JsonBftConfigOptions.java b/config/src/main/java/org/hyperledger/besu/config/JsonBftConfigOptions.java index f1e2d6c6db..17a203bd77 100644 --- a/config/src/main/java/org/hyperledger/besu/config/JsonBftConfigOptions.java +++ b/config/src/main/java/org/hyperledger/besu/config/JsonBftConfigOptions.java @@ -51,7 +51,8 @@ public class JsonBftConfigOptions implements BftConfigOptions { @Override public int getBlockPeriodSeconds() { - return JsonUtil.getInt(bftConfigRoot, "blockperiodseconds", DEFAULT_BLOCK_PERIOD_SECONDS); + return JsonUtil.getPositiveInt( + bftConfigRoot, "blockperiodseconds", DEFAULT_BLOCK_PERIOD_SECONDS); } @Override diff --git a/config/src/main/java/org/hyperledger/besu/config/JsonUtil.java b/config/src/main/java/org/hyperledger/besu/config/JsonUtil.java index bccf2e0c39..178e81482d 100644 --- a/config/src/main/java/org/hyperledger/besu/config/JsonUtil.java +++ b/config/src/main/java/org/hyperledger/besu/config/JsonUtil.java @@ -14,6 +14,8 @@ */ package org.hyperledger.besu.config; +import org.hyperledger.besu.util.number.PositiveNumber; + import java.io.IOException; import java.util.Locale; import java.util.Map; @@ -141,6 +143,27 @@ public class JsonUtil { return getInt(node, key).orElse(defaultValue); } + public static OptionalInt getPositiveInt(final ObjectNode node, final String key) { + return getValueAsString(node, key) + .map(v -> OptionalInt.of(parsePositiveInt(key, v))) + .orElse(OptionalInt.empty()); + } + + public static int getPositiveInt( + final ObjectNode node, final String key, final int defaultValue) { + final String value = getValueAsString(node, key, String.valueOf(defaultValue)); + return parsePositiveInt(key, value); + } + + private static int parsePositiveInt(final String key, final String value) { + try { + return PositiveNumber.fromString(value).getValue(); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException( + "Invalid property value, " + key + " should be a positive integer: " + value); + } + } + public static OptionalLong getLong(final ObjectNode json, final String key) { return getValue(json, key) .filter(jsonNode -> validateType(jsonNode, JsonNodeType.NUMBER)) diff --git a/config/src/test/java/org/hyperledger/besu/config/CliqueConfigOptionsTest.java b/config/src/test/java/org/hyperledger/besu/config/CliqueConfigOptionsTest.java index c8c1938e91..b919568bdc 100644 --- a/config/src/test/java/org/hyperledger/besu/config/CliqueConfigOptionsTest.java +++ b/config/src/test/java/org/hyperledger/besu/config/CliqueConfigOptionsTest.java @@ -17,6 +17,7 @@ package org.hyperledger.besu.config; import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import java.util.Map; @@ -30,7 +31,7 @@ public class CliqueConfigOptionsTest { @Test public void shouldGetEpochLengthFromConfig() { - final CliqueConfigOptions config = fromConfigOptions(singletonMap("EpochLength", 10_000)); + final CliqueConfigOptions config = fromConfigOptions(singletonMap("epochlength", 10_000)); assertThat(config.getEpochLength()).isEqualTo(10_000); } @@ -48,7 +49,7 @@ public class CliqueConfigOptionsTest { @Test public void shouldGetBlockPeriodFromConfig() { - final CliqueConfigOptions config = fromConfigOptions(singletonMap("BlockPeriodSeconds", 5)); + final CliqueConfigOptions config = fromConfigOptions(singletonMap("blockperiodseconds", 5)); assertThat(config.getBlockPeriodSeconds()).isEqualTo(5); } @@ -64,6 +65,13 @@ public class CliqueConfigOptionsTest { .isEqualTo(EXPECTED_DEFAULT_BLOCK_PERIOD); } + @Test + public void shouldThrowOnNonPositiveBlockPeriod() { + final CliqueConfigOptions config = fromConfigOptions(singletonMap("blockperiodseconds", -1)); + assertThatThrownBy(() -> config.getBlockPeriodSeconds()) + .isInstanceOf(IllegalArgumentException.class); + } + private CliqueConfigOptions fromConfigOptions(final Map cliqueConfigOptions) { final ObjectNode rootNode = JsonUtil.createEmptyObjectNode(); final ObjectNode configNode = JsonUtil.createEmptyObjectNode(); diff --git a/config/src/test/java/org/hyperledger/besu/config/JsonBftConfigOptionsTest.java b/config/src/test/java/org/hyperledger/besu/config/JsonBftConfigOptionsTest.java index cbe43fe524..c15aa5b12c 100644 --- a/config/src/test/java/org/hyperledger/besu/config/JsonBftConfigOptionsTest.java +++ b/config/src/test/java/org/hyperledger/besu/config/JsonBftConfigOptionsTest.java @@ -17,6 +17,7 @@ package org.hyperledger.besu.config; import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import java.util.Map; @@ -36,7 +37,7 @@ public class JsonBftConfigOptionsTest { @Test public void shouldGetEpochLengthFromConfig() { - final BftConfigOptions config = fromConfigOptions(singletonMap("EpochLength", 10_000)); + final BftConfigOptions config = fromConfigOptions(singletonMap("epochlength", 10_000)); assertThat(config.getEpochLength()).isEqualTo(10_000); } @@ -54,7 +55,7 @@ public class JsonBftConfigOptionsTest { @Test public void shouldGetBlockPeriodFromConfig() { - final BftConfigOptions config = fromConfigOptions(singletonMap("BlockPeriodSeconds", 5)); + final BftConfigOptions config = fromConfigOptions(singletonMap("blockperiodseconds", 5)); assertThat(config.getBlockPeriodSeconds()).isEqualTo(5); } @@ -70,9 +71,16 @@ public class JsonBftConfigOptionsTest { .isEqualTo(EXPECTED_DEFAULT_BLOCK_PERIOD); } + @Test + public void shouldThrowOnNonPositiveBlockPeriod() { + final BftConfigOptions config = fromConfigOptions(singletonMap("blockperiodseconds", -1)); + assertThatThrownBy(() -> config.getBlockPeriodSeconds()) + .isInstanceOf(IllegalArgumentException.class); + } + @Test public void shouldGetRequestTimeoutFromConfig() { - final BftConfigOptions config = fromConfigOptions(singletonMap("RequestTimeoutSeconds", 5)); + final BftConfigOptions config = fromConfigOptions(singletonMap("requesttimeoutseconds", 5)); assertThat(config.getRequestTimeoutSeconds()).isEqualTo(5); } @@ -90,7 +98,7 @@ public class JsonBftConfigOptionsTest { @Test public void shouldGetGossipedHistoryLimitFromConfig() { - final BftConfigOptions config = fromConfigOptions(singletonMap("GossipedHistoryLimit", 100)); + final BftConfigOptions config = fromConfigOptions(singletonMap("gossipedhistorylimit", 100)); assertThat(config.getGossipedHistoryLimit()).isEqualTo(100); } @@ -108,7 +116,7 @@ public class JsonBftConfigOptionsTest { @Test public void shouldGetMessageQueueLimitFromConfig() { - final BftConfigOptions config = fromConfigOptions(singletonMap("MessageQueueLimit", 100)); + final BftConfigOptions config = fromConfigOptions(singletonMap("messagequeuelimit", 100)); assertThat(config.getMessageQueueLimit()).isEqualTo(100); } @@ -126,7 +134,7 @@ public class JsonBftConfigOptionsTest { @Test public void shouldGetDuplicateMessageLimitFromConfig() { - final BftConfigOptions config = fromConfigOptions(singletonMap("DuplicateMessageLimit", 50)); + final BftConfigOptions config = fromConfigOptions(singletonMap("duplicatemessagelimit", 50)); assertThat(config.getDuplicateMessageLimit()).isEqualTo(50); } @@ -145,7 +153,7 @@ public class JsonBftConfigOptionsTest { @Test public void shouldGetFutureMessagesLimitFromConfig() { - final BftConfigOptions config = fromConfigOptions(singletonMap("FutureMessagesLimit", 50)); + final BftConfigOptions config = fromConfigOptions(singletonMap("futuremessageslimit", 50)); assertThat(config.getFutureMessagesLimit()).isEqualTo(50); } @@ -164,7 +172,7 @@ public class JsonBftConfigOptionsTest { @Test public void shouldGetFutureMessagesMaxDistanceFromConfig() { final BftConfigOptions config = - fromConfigOptions(singletonMap("FutureMessagesMaxDistance", 50)); + fromConfigOptions(singletonMap("futuremessagesmaxdistance", 50)); assertThat(config.getFutureMessagesMaxDistance()).isEqualTo(50); } diff --git a/config/src/test/java/org/hyperledger/besu/config/JsonUtilTest.java b/config/src/test/java/org/hyperledger/besu/config/JsonUtilTest.java index bf5708d59d..566100030f 100644 --- a/config/src/test/java/org/hyperledger/besu/config/JsonUtilTest.java +++ b/config/src/test/java/org/hyperledger/besu/config/JsonUtilTest.java @@ -514,6 +514,101 @@ public class JsonUtilTest { .hasMessageContaining("Expected boolean value but got number"); } + @Test + public void getPositiveInt_validValue() { + final ObjectNode node = mapper.createObjectNode(); + final int validValue = 2; + node.put("test", validValue); + final OptionalInt result = JsonUtil.getPositiveInt(node, "test"); + assertThat(result).hasValue(validValue); + } + + @Test + public void getPositiveInt_nonExistentKey() { + final ObjectNode node = mapper.createObjectNode(); + final OptionalInt result = JsonUtil.getPositiveInt(node, "test"); + assertThat(result).isEmpty(); + } + + @Test + public void getPositiveInt_decimalValue() { + final ObjectNode node = mapper.createObjectNode(); + final float decimalValue = Float.MAX_VALUE; + node.put("test", decimalValue); + assertThatThrownBy(() -> JsonUtil.getPositiveInt(node, "test")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Invalid property value, test should be a positive integer: " + decimalValue); + } + + @Test + public void getPositiveInt_nonPositiveValue() { + final ObjectNode node = mapper.createObjectNode(); + final int nonPositiveValue = 0; + node.put("test", nonPositiveValue); + assertThatThrownBy(() -> JsonUtil.getPositiveInt(node, "test")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage( + "Invalid property value, test should be a positive integer: " + nonPositiveValue); + } + + @Test + public void getPositiveInt_negativeValue() { + final ObjectNode node = mapper.createObjectNode(); + final int negativeValue = Integer.MIN_VALUE; + node.put("test", negativeValue); + assertThatThrownBy(() -> JsonUtil.getPositiveInt(node, "test")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Invalid property value, test should be a positive integer: " + negativeValue); + } + + @Test + public void getPositiveInt_validValue_withDefault() { + final ObjectNode node = mapper.createObjectNode(); + final int validValue = 2; + node.put("test", validValue); + final int result = JsonUtil.getPositiveInt(node, "test", 1); + assertThat(result).isEqualTo(validValue); + } + + @Test + public void getPositiveInt_nonExistentKey_withDefault() { + final ObjectNode node = mapper.createObjectNode(); + final int defaultValue = 1; + final int result = JsonUtil.getPositiveInt(node, "test", defaultValue); + assertThat(result).isEqualTo(defaultValue); + } + + @Test + public void getPositiveInt_decimalValue_withDefault() { + final ObjectNode node = mapper.createObjectNode(); + final float decimalValue = Float.MAX_VALUE; + node.put("test", decimalValue); + assertThatThrownBy(() -> JsonUtil.getPositiveInt(node, "test", 1)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Invalid property value, test should be a positive integer: " + decimalValue); + } + + @Test + public void getPositiveInt_nonPositiveValue_withDefault() { + final ObjectNode node = mapper.createObjectNode(); + final int nonPositiveValue = 0; + node.put("test", nonPositiveValue); + assertThatThrownBy(() -> JsonUtil.getPositiveInt(node, "test", 1)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage( + "Invalid property value, test should be a positive integer: " + nonPositiveValue); + } + + @Test + public void getPositiveInt_negativeValue_withDefault() { + final ObjectNode node = mapper.createObjectNode(); + final int negativeValue = Integer.MIN_VALUE; + node.put("test", negativeValue); + assertThatThrownBy(() -> JsonUtil.getPositiveInt(node, "test", 1)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Invalid property value, test should be a positive integer: " + negativeValue); + } + @Test public void objectNodeFromMap() { final Map map = new TreeMap<>(); diff --git a/util/src/test/java/org/hyperledger/besu/util/number/PositiveNumberTest.java b/util/src/test/java/org/hyperledger/besu/util/number/PositiveNumberTest.java new file mode 100644 index 0000000000..430e43ca6a --- /dev/null +++ b/util/src/test/java/org/hyperledger/besu/util/number/PositiveNumberTest.java @@ -0,0 +1,62 @@ +/* + * Copyright Hyperledger Besu Contributors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + * + * SPDX-License-Identifier: Apache-2.0 + */ +package org.hyperledger.besu.util.number; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import org.junit.Test; + +public class PositiveNumberTest { + + @Test + public void shouldBuildPositiveNumberFromString() { + final String positiveNumberString = "1"; + final int positiveNumberInt = 1; + PositiveNumber positiveNumber = PositiveNumber.fromString(positiveNumberString); + assertThat(positiveNumber).isNotNull(); + assertThat(positiveNumber.getValue()).isEqualTo(positiveNumberInt); + } + + @Test + public void shouldThrowOnDecimalValueFromString() { + assertThatThrownBy(() -> PositiveNumber.fromString("1.1")) + .isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void shouldThrowOnNonPositiveValueFromString() { + assertThatThrownBy(() -> PositiveNumber.fromString("0")) + .isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void shouldThrowOnNegativeValueFromString() { + assertThatThrownBy(() -> PositiveNumber.fromString("-1")) + .isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void shouldThrowOnEmptyValueFromString() { + assertThatThrownBy(() -> PositiveNumber.fromString("")) + .isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void shouldThrowOnNullValueFromString() { + assertThatThrownBy(() -> PositiveNumber.fromString(null)) + .isInstanceOf(IllegalArgumentException.class); + } +}