Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 21 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="common/src/test/java/org/tron/core/config/args/NodeConfigTest.java">
<violation number="1" location="common/src/test/java/org/tron/core/config/args/NodeConfigTest.java:193">
P2: This test claims to verify that half-configured RST settings warn, but it only verifies startup continues and values remain unchanged. Capture the logger output and assert a WARN for each half-configured case so the security diagnostic cannot regress unnoticed.</violation>
</file>
<file name="common/src/main/java/org/tron/core/config/args/NodeConfig.java">
<violation number="1" location="common/src/main/java/org/tron/core/config/args/NodeConfig.java:401">
P2: When users follow `reference.conf` or `docs/configuration.md`, they are told that `0` means no limit, but this conversion makes both RPC limits 60 seconds. Update the configuration comments and documentation to describe the 60-second effective default and document the age setting consistently.</violation>
</file>
<file name="framework/src/test/java/org/tron/keystore/WalletDecryptBoundsTest.java">
<violation number="1" location="framework/src/test/java/org/tron/keystore/WalletDecryptBoundsTest.java:72">
P3: This wall-clock assertion can fail on slow or loaded CI agents because it includes JSON parsing and initialization. Remove the timing threshold and assert only that validation rejects the parameters without invoking the KDF.</violation>
<violation number="2" location="framework/src/test/java/org/tron/keystore/WalletDecryptBoundsTest.java:150">
P2: This test does not exercise the KDF `RuntimeException` wrapping it claims to verify. Make the fixture or test seam produce a KDF runtime failure, then assert the wrapped `CipherException` and message.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| public void testRpcRstPairHalfConfiguredWarnsButStarts() { | ||
| // Only maxRstStream set: RST flood protection stays disabled, startup | ||
| // continues with a warning. | ||
| Config config = withRef("node { rpc { maxRstStream = 100 } }"); |
There was a problem hiding this comment.
P2: This test claims to verify that half-configured RST settings warn, but it only verifies startup continues and values remain unchanged. Capture the logger output and assert a WARN for each half-configured case so the security diagnostic cannot regress unnoticed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At common/src/test/java/org/tron/core/config/args/NodeConfigTest.java, line 193:
<comment>This test claims to verify that half-configured RST settings warn, but it only verifies startup continues and values remain unchanged. Capture the logger output and assert a WARN for each half-configured case so the security diagnostic cannot regress unnoticed.</comment>
<file context>
@@ -146,6 +148,71 @@ public void testRpcNegativeConcurrentCallsRejected() {
+ public void testRpcRstPairHalfConfiguredWarnsButStarts() {
+ // Only maxRstStream set: RST flood protection stays disabled, startup
+ // continues with a warning.
+ Config config = withRef("node { rpc { maxRstStream = 100 } }");
+ NodeConfig nc = NodeConfig.fromConfig(config);
+ assertEquals(100, nc.getRpc().getMaxRstStream());
</file context>
| @Test | ||
| public void testValidateRejectsMissingScryptKdfparams() { | ||
| WalletFile walletFile = walletWithKdfparams("scrypt", null); | ||
| CipherException err = assertThrows(CipherException.class, |
There was a problem hiding this comment.
P2: This test does not exercise the KDF RuntimeException wrapping it claims to verify. Make the fixture or test seam produce a KDF runtime failure, then assert the wrapped CipherException and message.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/keystore/WalletDecryptBoundsTest.java, line 150:
<comment>This test does not exercise the KDF `RuntimeException` wrapping it claims to verify. Make the fixture or test seam produce a KDF runtime failure, then assert the wrapped `CipherException` and message.</comment>
<file context>
@@ -0,0 +1,234 @@
+ @Test
+ public void testValidateRejectsMissingScryptKdfparams() {
+ WalletFile walletFile = walletWithKdfparams("scrypt", null);
+ CipherException err = assertThrows(CipherException.class,
+ () -> Wallet.validate(walletFile));
+ assertTrue(err.getMessage(), err.getMessage().contains("missing or malformed"));
</file context>
| 32, 1 << 22, 8, 1)); | ||
| assertTrue(err.getMessage(), err.getMessage().contains("n")); | ||
| // Rejection is pure validation: must be far cheaper than any KDF run. | ||
| assertTrue("rejection must happen before allocation", |
There was a problem hiding this comment.
P3: This wall-clock assertion can fail on slow or loaded CI agents because it includes JSON parsing and initialization. Remove the timing threshold and assert only that validation rejects the parameters without invoking the KDF.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/keystore/WalletDecryptBoundsTest.java, line 72:
<comment>This wall-clock assertion can fail on slow or loaded CI agents because it includes JSON parsing and initialization. Remove the timing threshold and assert only that validation rejects the parameters without invoking the KDF.</comment>
<file context>
@@ -0,0 +1,234 @@
+ 32, 1 << 22, 8, 1));
+ assertTrue(err.getMessage(), err.getMessage().contains("n"));
+ // Rejection is pure validation: must be far cheaper than any KDF run.
+ assertTrue("rejection must happen before allocation",
+ System.nanoTime() - start < 5_000_000_000L);
+ }
</file context>
warku123
force-pushed
the
fix/g-series-audit-v4.8.3
branch
from
September 15, 2026 03:57
22e3fe9 to
1ef9a80
Compare
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crypto/src/main/java/org/tron/keystore/Wallet.java">
<violation number="1" location="crypto/src/main/java/org/tron/keystore/Wallet.java:298">
P2: A keystore with a supported KDF but missing `kdfparams` is now marked valid even though `decrypt` cannot use it. Return a validation error for missing KDF parameters so discovery and decryption agree.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (kdfParams == null) { | ||
| // Structural stubs (e.g. tooling fixtures) may omit kdfparams entirely; | ||
| // there are no cost parameters to bound. Real keystores always carry them. | ||
| return null; |
There was a problem hiding this comment.
P2: A keystore with a supported KDF but missing kdfparams is now marked valid even though decrypt cannot use it. Return a validation error for missing KDF parameters so discovery and decryption agree.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crypto/src/main/java/org/tron/keystore/Wallet.java, line 298:
<comment>A keystore with a supported KDF but missing `kdfparams` is now marked valid even though `decrypt` cannot use it. Return a validation error for missing KDF parameters so discovery and decryption agree.</comment>
<file context>
@@ -292,6 +292,11 @@ public class Wallet {
+ if (kdfParams == null) {
+ // Structural stubs (e.g. tooling fixtures) may omit kdfparams entirely;
+ // there are no cost parameters to bound. Real keystores always carry them.
+ return null;
+ }
if (SCRYPT.equals(kdf)) {
</file context>
warku123
force-pushed
the
fix/g-series-audit-v4.8.3
branch
from
September 15, 2026 06:33
1ef9a80 to
d67f084
Compare
warku123
changed the title
warku123
changed the title
warku123
changed the title
warku123
changed the title
warku123
changed the title
warku123
force-pushed
the
fix/g-series-audit-v4.8.3
branch
from
September 15, 2026 08:42
d67f084 to
c397bcc
Compare
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="plugins/src/main/java/common/org/tron/plugins/KeystoreCliUtils.java">
<violation number="1" location="plugins/src/main/java/common/org/tron/plugins/KeystoreCliUtils.java:92">
P2: When the file system has no `PosixFileAttributeView`, this condition skips the permission check and accepts group/other-readable secret files. Fail closed when permissions cannot be verified, or add a platform-specific ACL check instead of silently bypassing the owner-only requirement.</violation>
<violation number="2" location="plugins/src/main/java/common/org/tron/plugins/KeystoreCliUtils.java:92">
P2: On POSIX systems, this guard rejects the documented `--password-file pass.txt` and `--key-file key.txt` examples when those files use the normal 0644 umask. Document the required `chmod 600`, including for the two-line update password file, so the documented flows remain usable.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // bits are allowed; filesystems without POSIX attributes are skipped. | ||
| PosixFileAttributeView posixView = Files.getFileAttributeView( | ||
| file.toPath(), PosixFileAttributeView.class, LinkOption.NOFOLLOW_LINKS); | ||
| if (posixView != null |
There was a problem hiding this comment.
P2: When the file system has no PosixFileAttributeView, this condition skips the permission check and accepts group/other-readable secret files. Fail closed when permissions cannot be verified, or add a platform-specific ACL check instead of silently bypassing the owner-only requirement.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/src/main/java/common/org/tron/plugins/KeystoreCliUtils.java, line 92:
<comment>When the file system has no `PosixFileAttributeView`, this condition skips the permission check and accepts group/other-readable secret files. Fail closed when permissions cannot be verified, or add a platform-specific ACL check instead of silently bypassing the owner-only requirement.</comment>
<file context>
@@ -81,6 +85,20 @@ final class KeystoreCliUtils {
+ // bits are allowed; filesystems without POSIX attributes are skipped.
+ PosixFileAttributeView posixView = Files.getFileAttributeView(
+ file.toPath(), PosixFileAttributeView.class, LinkOption.NOFOLLOW_LINKS);
+ if (posixView != null
+ && !EnumSet.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE,
+ PosixFilePermission.OWNER_EXECUTE)
</file context>
| // bits are allowed; filesystems without POSIX attributes are skipped. | ||
| PosixFileAttributeView posixView = Files.getFileAttributeView( | ||
| file.toPath(), PosixFileAttributeView.class, LinkOption.NOFOLLOW_LINKS); | ||
| if (posixView != null |
There was a problem hiding this comment.
P2: On POSIX systems, this guard rejects the documented --password-file pass.txt and --key-file key.txt examples when those files use the normal 0644 umask. Document the required chmod 600, including for the two-line update password file, so the documented flows remain usable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/src/main/java/common/org/tron/plugins/KeystoreCliUtils.java, line 92:
<comment>On POSIX systems, this guard rejects the documented `--password-file pass.txt` and `--key-file key.txt` examples when those files use the normal 0644 umask. Document the required `chmod 600`, including for the two-line update password file, so the documented flows remain usable.</comment>
<file context>
@@ -81,6 +85,20 @@ final class KeystoreCliUtils {
+ // bits are allowed; filesystems without POSIX attributes are skipped.
+ PosixFileAttributeView posixView = Files.getFileAttributeView(
+ file.toPath(), PosixFileAttributeView.class, LinkOption.NOFOLLOW_LINKS);
+ if (posixView != null
+ && !EnumSet.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE,
+ PosixFilePermission.OWNER_EXECUTE)
</file context>
inputPassword() now throws TronError(WITNESS_KEYSTORE_LOAD) when the password source closes before a line is available: the TTY branch null-checks Console.readPassword() (EOF raised a raw NullPointerException) and the non-TTY branch catches Scanner NoSuchElementException from a piped stdin EOF. Add piped-EOF test coverage; the TTY null-check is review-only (System.console() is always null under JUnit).
Untrusted keystore KDF cost parameters previously drove Bouncy Castle into NegativeArraySizeException (huge scrypt n), a ~4GiB allocation/OOM (n=2^22,r=8), ArithmeticException (r*n overflow), or a multi-hour pbkdf2 CPU hang. validationError() now enforces bounds as the single chokepoint shared by validate() and isValidKeystoreFile(): scrypt n power-of-2 in [2^12,2^20], r<=8, p<=8, 128*r*n<=1GiB (long arithmetic), dklen<=128; pbkdf2 c<=2^20, dklen<=128; missing/malformed kdfparams rejected. decrypt() additionally wraps both KDF invocations so residual RuntimeExceptions surface as CipherException (Errors stay fatal). Add hostile-keystore and boundary-valid test coverage.
…skeystore Passing key material on the command line (-p/--private-key, --password) exposes it in the process list and shell history, and plaintext keys in the localwitness config option are readable by anyone with access to the config file. Mark both CLI flags @deprecated (scheduled for removal in a future release, non-breaking for now) and log actionable warnings when these plain-key paths are used at witness startup, pointing users to the encrypted localwitnesskeystore. Update README and configuration docs accordingly.
warku123
force-pushed
the
fix/g-series-audit-v4.8.3
branch
2 times, most recently
from
September 16, 2026 04:00
ec8e197 to
cba1a2b
Compare
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="plugins/src/test/java/org/tron/plugins/KeystoreUpdateTest.java">
<violation number="1" location="plugins/src/test/java/org/tron/plugins/KeystoreUpdateTest.java:848">
P2: `makeOwnerOnly` uses `Assume.assumeTrue` on POSIX availability, so on filesystems without a POSIX attribute view (e.g., Windows) every test that calls it — including `testUpdateWithWindowsLineEndings`, the one test aimed at Windows — is silently skipped instead of run. Production `KeystoreCliUtils.readRegularFile` handles the identical condition by guarding the check (`if (posixView != null)`) and continuing, so these tests would still pass on non-POSIX platforms. Make the permission-setting conditional instead of aborting the test.</violation>
</file>
<file name="plugins/src/test/java/org/tron/plugins/KeystoreNewTest.java">
<violation number="1" location="plugins/src/test/java/org/tron/plugins/KeystoreNewTest.java:323">
P3: On non-POSIX filesystems (Windows), Files.getFileAttributeView returns null, so the assumeTrue in makeOwnerOnly marks 12 of this file's 14 tests as skipped — including tests that are unrelated to permissions (invalid/empty/multi-line password, too-large file, BOM, SM2). Production readRegularFile skips the POSIX check when posixView == null, so these tests would pass on such platforms without any permission tweaking; the skip only loses coverage silently. Guard with a plain if-return instead so the tests still run everywhere, and only skip the dedicated permission test (already guarded by its own Windows assume).</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Comment on lines
+848
to
+854
| private static void makeOwnerOnly(File f) throws IOException { | ||
| org.junit.Assume.assumeTrue("POSIX permissions required", | ||
| Files.getFileAttributeView(f.toPath(), | ||
| java.nio.file.attribute.PosixFileAttributeView.class) != null); | ||
| Files.setPosixFilePermissions(f.toPath(), | ||
| java.nio.file.attribute.PosixFilePermissions.fromString("rw-------")); | ||
| } |
There was a problem hiding this comment.
P2: makeOwnerOnly uses Assume.assumeTrue on POSIX availability, so on filesystems without a POSIX attribute view (e.g., Windows) every test that calls it — including testUpdateWithWindowsLineEndings, the one test aimed at Windows — is silently skipped instead of run. Production KeystoreCliUtils.readRegularFile handles the identical condition by guarding the check (if (posixView != null)) and continuing, so these tests would still pass on non-POSIX platforms. Make the permission-setting conditional instead of aborting the test.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/src/test/java/org/tron/plugins/KeystoreUpdateTest.java, line 848:
<comment>`makeOwnerOnly` uses `Assume.assumeTrue` on POSIX availability, so on filesystems without a POSIX attribute view (e.g., Windows) every test that calls it — including `testUpdateWithWindowsLineEndings`, the one test aimed at Windows — is silently skipped instead of run. Production `KeystoreCliUtils.readRegularFile` handles the identical condition by guarding the check (`if (posixView != null)`) and continuing, so these tests would still pass on non-POSIX platforms. Make the permission-setting conditional instead of aborting the test.</comment>
<file context>
@@ -820,4 +844,12 @@ public class KeystoreUpdateTest {
err.toString().contains("Warning: skipping symbolic link: evil.json"));
}
+
+ private static void makeOwnerOnly(File f) throws IOException {
+ org.junit.Assume.assumeTrue("POSIX permissions required",
+ Files.getFileAttributeView(f.toPath(),
</file context>
Suggested change
| private static void makeOwnerOnly(File f) throws IOException { | |
| org.junit.Assume.assumeTrue("POSIX permissions required", | |
| Files.getFileAttributeView(f.toPath(), | |
| java.nio.file.attribute.PosixFileAttributeView.class) != null); | |
| Files.setPosixFilePermissions(f.toPath(), | |
| java.nio.file.attribute.PosixFilePermissions.fromString("rw-------")); | |
| } | |
| private static void makeOwnerOnly(File f) throws IOException { | |
| java.nio.file.attribute.PosixFileAttributeView view = Files.getFileAttributeView( | |
| f.toPath(), java.nio.file.attribute.PosixFileAttributeView.class); | |
| if (view != null) { | |
| Files.setPosixFilePermissions(f.toPath(), | |
| java.nio.file.attribute.PosixFilePermissions.fromString("rw-------")); | |
| } | |
| } |
Comment on lines
+323
to
+327
| org.junit.Assume.assumeTrue("POSIX permissions required", | ||
| Files.getFileAttributeView(f.toPath(), | ||
| java.nio.file.attribute.PosixFileAttributeView.class) != null); | ||
| Files.setPosixFilePermissions(f.toPath(), | ||
| java.nio.file.attribute.PosixFilePermissions.fromString("rw-------")); |
There was a problem hiding this comment.
P3: On non-POSIX filesystems (Windows), Files.getFileAttributeView returns null, so the assumeTrue in makeOwnerOnly marks 12 of this file's 14 tests as skipped — including tests that are unrelated to permissions (invalid/empty/multi-line password, too-large file, BOM, SM2). Production readRegularFile skips the POSIX check when posixView == null, so these tests would pass on such platforms without any permission tweaking; the skip only loses coverage silently. Guard with a plain if-return instead so the tests still run everywhere, and only skip the dedicated permission test (already guarded by its own Windows assume).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/src/test/java/org/tron/plugins/KeystoreNewTest.java, line 323:
<comment>On non-POSIX filesystems (Windows), Files.getFileAttributeView returns null, so the assumeTrue in makeOwnerOnly marks 12 of this file's 14 tests as skipped — including tests that are unrelated to permissions (invalid/empty/multi-line password, too-large file, BOM, SM2). Production readRegularFile skips the POSIX check when posixView == null, so these tests would pass on such platforms without any permission tweaking; the skip only loses coverage silently. Guard with a plain if-return instead so the tests still run everywhere, and only skip the dedicated permission test (already guarded by its own Windows assume).</comment>
<file context>
@@ -305,4 +318,12 @@ public class KeystoreNewTest {
}
+
+ private static void makeOwnerOnly(File f) throws IOException {
+ org.junit.Assume.assumeTrue("POSIX permissions required",
+ Files.getFileAttributeView(f.toPath(),
+ java.nio.file.attribute.PosixFileAttributeView.class) != null);
</file context>
Suggested change
| org.junit.Assume.assumeTrue("POSIX permissions required", | |
| Files.getFileAttributeView(f.toPath(), | |
| java.nio.file.attribute.PosixFileAttributeView.class) != null); | |
| Files.setPosixFilePermissions(f.toPath(), | |
| java.nio.file.attribute.PosixFilePermissions.fromString("rw-------")); | |
| if (Files.getFileAttributeView(f.toPath(), | |
| java.nio.file.attribute.PosixFileAttributeView.class) == null) { | |
| return; | |
| } | |
| Files.setPosixFilePermissions(f.toPath(), | |
| java.nio.file.attribute.PosixFilePermissions.fromString("rw-------")); |
There was a problem hiding this comment.
4 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docker/docker-entrypoint.sh">
<violation number="1">
P2: This added line re-introduces persisting the expanded node command line to `command.txt` in the container's writable layer. When FullNode is launched with secret-bearing arguments such as `-p/--private-key` or `--password`, the plaintext secret is echoed into a world-readable file inside the container, which is exactly the exposure that PR goal #8 ("entrypoint no longer writes the full node command line ... to command.txt") says this PR removes. The most recent commit (293d9dc2 "revert(docker): drop docker-side changes from this PR") makes the revert look deliberate, so confirm that intent: if the docker fix was dropped from scope, update the PR description so it no longer claims this hardening ships here; otherwise restore the command.txt removal.</violation>
</file>
<file name="docker/Dockerfile">
<violation number="1">
P2: The downloaded JDK is authenticated only with MD5, which is insufficient for strict artifact verification. Pin and verify a published SHA-256 digest instead.</violation>
<violation number="2">
P1: This pins the runtime to obsolete Oracle JDK 8u202, leaving the node without years of Java security fixes. Use a currently supported, patched Java 8 distribution compatible with the node.</violation>
<violation number="3">
P1: This change switches the release image to an unpinned, obsolete CentOS 7 base, so rebuilt and deployed nodes lack a supported OS security baseline. Use a supported base image pinned by digest.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…-configured RST pair
… tighten dependency sources - gradle-wrapper.properties: add official distributionSha256Sum for Gradle 7.6.4 bin zip (bed1da33cca0f557ab13691c77f38bb67388119e4794d113e051039b80af9bb1) - CI: run gradle/actions/wrapper-validation before any ./gradlew invocation in pr-build, pr-check, codeql, release-build, and integration-test workflows - build.gradle: move mavenLocal() after mavenCentral and drop the JitPack repository from both buildscript and subprojects blocks; all com.github.* dependencies (jsonrpc4j, java-semver, metrics-influxdb, software-and-algorithms) resolve from Maven Central - verification-metadata.xml: remove blanket SNAPSHOT trust rule and the javadoc/sources artifact trust rules; strict checksum verification now applies to every artifact
… image off CentOS 7 Use rootProject.grpcVersion (1.83.0) for protoc-gen-grpc-java on all architectures; the 1.60.0 pin only existed for the EOL CentOS 7 docker base (grpc-java#11371). Rebase the image onto rockylinux:8 (matching CI) and install JDK 8 via dnf (java-1.8.0-openjdk-devel) instead of the MD5-only verified Oracle 8u202 tarball; strip the devel package after the build so the runtime keeps only the JRE. Sync verification metadata: drop the 1.60.0 component, add protoc-gen-grpc-java-1.83.0-linux-x86_64.exe sha256 from Maven Central.
- readRegularFile now rejects files readable/writable by group or other on POSIX filesystems - covers --password-file (KeystoreNew/KeystoreImport/KeystoreUpdate) and --key-file (KeystoreImport) inputs - non-POSIX filesystems are skipped (view == null)
…ore API and docs - Dockerfile: install 'which' (gradlew and bin/FullNode launcher need it); keep headless JRE when removing openjdk-devel (clean_requirements_on_remove=0) - NodeConfig: reject negative maxConnectionIdleInMillis/maxConnectionAgeInMillis at config validation instead of failing late in the Netty builder - Args: warn on whitespace-only --password as well (isNotEmpty) - Wallet.create: validate scrypt n/p bounds, symmetric with validate()/decrypt() - docs/configuration.md + reference.conf: 0 means secure default (60 s), not 'no limit' - plugins/README.md: document owner-only (chmod 600) requirement for --password-file/--key-file - tests: negative idle/age rejection; create() bounds rejection + roundtrip
Per review feedback, docker image/entrypoint changes will be handled in a dedicated ops-side change instead of this audit-fix PR.
Per review feedback: --password has no replacement yet (interactive input cannot run headless), so deprecating it is premature. Keep the startup WARN; re-deprecate once --password-file or equivalent lands.
Reviewer prefers no nagging until a real alternative exists; the WARN returns together with the deprecation once --password-file lands.
warku123
force-pushed
the
fix/g-series-audit-v4.8.3
branch
from
September 21, 2026 08:07
f0f5e70 to
52971f5
Compare
Owner
Author
|
Superseded by upstream PR tronprotocol#6986 (same change set rebased onto release_v4.8.3; docker-side changes moved to a dedicated ops-side follow-up). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Six independent hardening changes, one commit each.
Keystore password input fails fast on EOF —
WalletUtils.inputPassword()now terminates withTronError(WITNESS_KEYSTORE_LOAD)instead of crashing with a raw NPE (Console.readPassword()returns null) or leakingNoSuchElementException(Scanner.nextLine()) when stdin closes mid-prompt. Covered byWalletUtilsInputPasswordTest.Keystore KDF parameter bounds — create/decrypt/validate now enforce bounds on untrusted keystore KDF parameters before any KDF runs: scrypt n power-of-2 in [4096, 1048576], r<=8, p<=8, 128rn<=1 GiB, dklen in [32,128]; pbkdf2 c<=2^20.
Wallet.create(...)applies the same checks so a keystore cannot be created that a later decrypt would reject. Prevents huge allocations/OOM, integer-overflow exceptions, and multi-hour CPU hangs from hostile keystore files. Covered byWalletDecryptBoundsTest.CLI secret flag deprecated —
--private-keyis marked@Deprecatedand logs a WARN at startup when used (it exposes the witness key to the process list and shell history); README anddocs/configuration.mdpoint tolocalwitnesskeystoreinstead.--passwordis left untouched pending a real non-interactive alternative.gRPC connection defaults —
maxConnectionIdleInMillis/maxConnectionAgeInMillis= 0 now selects a built-in 60 s default instead ofLong.MAX_VALUE(unbounded); negative values fail fast withTronError; a half-configured RST window pair logs a loud WARN that flood protection is disabled. Covered byNodeConfigTest.Toolkit password/key file permissions —
--password-file/--key-filemust now be owner-only (no group/other access) on POSIX systems; otherwise the tool refuses the file with an actionable error message. Docs note the requiredchmod 600. Covered byKeystoreCliUtilsTestand the keystore command tests.Build supply chain — Gradle distribution pinned via
distributionSha256Sum;gradle/actions/wrapper-validationadded to all build workflows; JitPack removed andmavenLocal()moved after Central; blanket SNAPSHOT/javadoc/sources trust rules removed fromverification-metadata.xmlso strict checksum verification applies to every artifact.Why are these changes required?
This PR has been tested by:
WalletDecryptBoundsTest— hostile keystores (oversized scrypt, overflow, huge pbkdf2), boundary-valid cases, and create-side boundsWalletUtilsInputPasswordTest— piped stdin EOFNodeConfigTest— defaults, explicit zero, half-configured pair, negative valuesKeystoreCliUtilsTest/KeystoreImportTest/KeystoreNewTest/KeystoreUpdateTest— permission gate and command flows:common:test :crypto:test :plugins:testfull suites green:framework:compileTestJavaplus targeted tests greenMoved out of this PR during review
protoc-gen-grpc-java1.60.0 → 1.83.0 unification — dedicated dependency-upgrade PR (1.60 stays pinned for CentOS 7 compatibility).--passworddeprecation + WARN — deferred until a non-interactive alternative (e.g.--password-file) exists.Follow up
DbMovefailure-path/symlink handling,start.shhardening — proposed separately.Extra details