Add version check for firstnode in restore network #62

tanuj-shardeum · 2024-08-12T07:17:37Z

No description provided.

github-actions · 2024-08-12T07:18:27Z

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review Error Handling The error handling for the version check in 'registerRoutes' does not account for the scenario where 'networkAccount' is a string. This could lead to misleading error messages or incorrect error responses. Security Concern The use of '~~' for parsing integers in 'isEqualOrNewerVersion' is unconventional and may lead to unexpected behavior if not properly validated.

src/API.ts

        if (!isSignatureValid) {
-          Logger.mainLogger.error('Invalid signature', signedFirstNodeInfo)
+          Logger.mainLogger.error('Invalid signature', requestBody)


To fix the log injection issue, we need to sanitize the user-provided input before logging it. Specifically, we should remove any newline characters from the requestBody to prevent log injection. This can be done using String.prototype.replace to ensure no line endings are present in the user input.

src/API.ts

          reply.send({ success: false, error: 'Invalid signature' })
          return
        }
      } catch (e) {
        Logger.mainLogger.error(e)
+        Logger.mainLogger.error('Signature verification failed', requestBody)


To fix the log injection issue, we need to sanitize the user-provided input before logging it. Specifically, we should remove any newline characters from the requestBody to prevent log injection. This can be done using String.prototype.replace to ensure no line endings are present in the user input.

src/API.ts

+      // eslint-disable-next-line no-constant-condition
+      if (true) {
+        Logger.mainLogger.debug('POST /nodelist firstNode:', firstNode)
+        Logger.mainLogger.debug('POST /nodelist appJoinData:', appJoinData)


To fix the log injection issue, we need to sanitize the appJoinData before logging it. Specifically, we should remove any newline characters from the user input to prevent log injection. This can be done using the String.prototype.replace method to replace newline characters with an empty string.

src/API.ts

src/primary-process/index.ts

+      }, based on ${receiptLoadTraker} receipts received.`
+    )
+    receiptLoadTraker = 0 // Reset the count
+  }, config.receiptLoadTrakerInterval)


To fix the problem, we need to validate the receiptLoadTrakerInterval value before it is used in the setInterval function. This can be done by adding a check to ensure the interval is within an acceptable range. If the value is outside this range, we should set it to a default safe value.

Add validation for receiptLoadTrakerInterval in src/API.ts when updating the configuration.

Ensure that the validated value is used in src/primary-process/index.ts.

src/txDigestAPIserver.ts

+      host: '0.0.0.0',
+    },
+    (err) => {
+      Logger.mainLogger.debug('TXDigestAPI Listening', config.txDigest.apiServerPort)


To fix the log injection issue, we need to sanitize the config.txDigest.apiServerPort value before logging it. This can be done by ensuring that any potentially harmful characters are removed or escaped. Specifically, we should remove any newline characters from the user input before logging it.

Identify the line where the logging occurs: Logger.mainLogger.debug('TXDigestAPI Listening', config.txDigest.apiServerPort).

Sanitize the config.txDigest.apiServerPort value by removing newline characters before logging it.

Ensure that the sanitization does not alter the functionality of the code.

src/txDigester.ts

+  cron.schedule(config.txDigest.txCronSchedule, async () => {
+    console.log('Running cron task....')
+    console.log('Checking archiver status....')
+    const archiverStatusResp = await axios.get(ARCHIVER_STATUS_CHECK_URL)


To fix the SSRF vulnerability, we need to ensure that the ARCHIVER_IP and ARCHIVER_PORT values are validated and restricted to a safe set of values. This can be achieved by implementing an allow-list of acceptable IP addresses and ports. Additionally, we should ensure that the URL is constructed safely.

Implement an allow-list for ARCHIVER_IP and ARCHIVER_PORT.

Validate the user input against the allow-list before updating the configuration.

Update the /set-config endpoint to include this validation.

src/worker-process/index.ts

+      console.log(`Worker ${process.pid} is idle for more than 1 minute`)
+      process.send({ type: 'child_close' })
+    }
+  }, config.lastActivityCheckInterval)


To fix the problem, we need to ensure that the lastActivityCheckInterval value is validated and restricted to a safe range before it is used in the setInterval function. This can be done by adding a validation check in the src/API.ts file where the configuration is updated.

Add a validation check for lastActivityCheckInterval in the src/API.ts file to ensure it is within an acceptable range.

If the value is outside the acceptable range, return an error response and do not update the configuration.

src/API.ts

+          )
+        ) {
+          Logger.mainLogger.debug(
+            `Invalid version of the node: ${appJoinData.shardeumVersion}, required minVersion: ${networkAccount?.data?.current?.minVersion}, required latestVersion: ${networkAccount?.data?.current?.latestVersion}`


To fix the log injection issue, we need to sanitize the user input before logging it. Specifically, we should remove any newline characters from the appJoinData.shardeumVersion value to prevent log injection. This can be done using the String.prototype.replace method to strip out newline characters.

src/NodeList.ts

+}
+
+export function isValidVersion(minimumVersion: string, latestVersion: string, nodeVersion: string): boolean {
+  if(config.VERBOSE) Logger.mainLogger.debug('isValidVersion: ', minimumVersion, latestVersion, nodeVersion)


To fix the log injection issue, we need to sanitize the nodeVersion before logging it. Specifically, we should remove any newline characters from the nodeVersion string to prevent log injection attacks. This can be done using String.prototype.replace to remove newline characters.

thantsintoe · 2024-09-25T13:25:29Z

src/API.ts

+        if (
+          typeof networkAccount != 'string' &&
+          !NodeList.isValidVersion(
+            networkAccount?.data?.current?.minVersion,


How will the archiver knows the data type and format of the global/network account from the Shardeum? For instance, if archiver is used other app layers (other than Shardeum) that use different format for network account, this will fail.

thantsintoe · 2024-09-25T13:26:21Z

src/API.ts

+      Logger.mainLogger.debug('networkAccount', networkAccount)
+
+      if (networkAccount) {
+        if (


It would be good if this if check can be simplified for better readability

github-actions bot added the Review effort [1-5]: 3 label Aug 12, 2024

github-advanced-security bot found potential problems Aug 12, 2024

View reviewed changes

src/API.ts Fixed Show fixed Hide fixed

tanuj-shardeum force-pushed the BLUE-167 branch from b613218 to e1259ab Compare August 13, 2024 10:59

github-advanced-security bot found potential problems Aug 13, 2024

View reviewed changes

src/API.ts Fixed Show fixed Hide fixed

tanuj-shardeum force-pushed the BLUE-167 branch from e1259ab to 790aa50 Compare August 29, 2024 08:40

github-advanced-security bot found potential problems Aug 29, 2024

View reviewed changes

tanuj-shardeum requested a review from jairajdev August 29, 2024 08:44

mhanson-github force-pushed the BLUE-167 branch from 790aa50 to 7bccf0e Compare September 2, 2024 04:40

tanuj-shardeum force-pushed the BLUE-167 branch from 7bccf0e to a856dfe Compare September 6, 2024 12:17

Add version check for firstnode in restore network

87ba1c7

tanuj-shardeum force-pushed the BLUE-167 branch from a856dfe to 87ba1c7 Compare September 11, 2024 13:48

github-advanced-security bot found potential problems Sep 11, 2024

View reviewed changes

tanuj-shardeum marked this pull request as ready for review September 24, 2024 13:23

thantsintoe suggested changes Sep 25, 2024

View reviewed changes

@@ -90,3 +90,5 @@
                 nestedCountersInstance.countEvent('consensor', 'POST_nodelist', 1)
-                const requestBody = request.body
+                let requestBody = request.body
+                // Sanitize requestBody to remove newline characters
+                requestBody = JSON.parse(JSON.stringify(requestBody).replace(/\n|\r/g, ""))
                 // eslint-disable-next-line no-constant-condition

@@ -90,3 +90,4 @@
                 nestedCountersInstance.countEvent('consensor', 'POST_nodelist', 1)
-                const requestBody = request.body
+                let requestBody = request.body
+                requestBody = JSON.parse(JSON.stringify(requestBody).replace(/\n|\r/g, ""))
                 // eslint-disable-next-line no-constant-condition
@@ -105,3 +106,3 @@
                   } catch (e) {
-                    Logger.mainLogger.error(e)
+                    Logger.mainLogger.error(e.message.replace(/\n|\r/g, ""))
                     Logger.mainLogger.error('Signature verification failed', requestBody)

@@ -133,3 +133,4 @@
                     Logger.mainLogger.debug('POST /nodelist firstNode:', firstNode)
-                    Logger.mainLogger.debug('POST /nodelist appJoinData:', appJoinData)
+                    const sanitizedAppJoinData = JSON.parse(JSON.stringify(appJoinData).replace(/\\n|\\r/g, ""))
+                    Logger.mainLogger.debug('POST /nodelist appJoinData:', sanitizedAppJoinData)
                   }

@@ -1005,2 +1005,9 @@
                     const { sign, ...newConfig } = _request.body
+                    if (newConfig.receiptLoadTrakerInterval !== undefined) {
+                      const interval = parseInt(newConfig.receiptLoadTrakerInterval, 10);
+                      if (isNaN(interval) || interval < 1000 || interval > 60000) { // Set acceptable range between 1 second and 1 minute
+                        throw new Error('Invalid receiptLoadTrakerInterval value. Must be between 1000 and 60000 milliseconds.');
+                      }
+                      newConfig.receiptLoadTrakerInterval = interval;
+                    }
                     const validKeys = new Set(Object.keys(config))

@@ -79,3 +79,4 @@
                 (err) => {
-                  Logger.mainLogger.debug('TXDigestAPI Listening', config.txDigest.apiServerPort)
+                  const sanitizedPort = config.txDigest.apiServerPort.toString().replace(/\n|\r/g, "");
+                  Logger.mainLogger.debug('TXDigestAPI Listening', sanitizedPort)
                   if (err) {

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add version check for firstnode in restore network #62

Add version check for firstnode in restore network #62

tanuj-shardeum commented Aug 12, 2024

github-actions bot commented Aug 12, 2024

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

thantsintoe Sep 25, 2024

thantsintoe Sep 25, 2024

@@ -1014,2 +1014,12 @@
+                    // Validate ARCHIVER_IP and ARCHIVER_PORT
+                    const allowedIPs = ['127.0.0.1', '192.168.1.1'] // Example allow-list
+                    const allowedPorts = [8080, 9090] // Example allow-list
+                    if (newConfig.ARCHIVER_IP && !allowedIPs.includes(newConfig.ARCHIVER_IP)) {
+                      throw new Error(`Invalid ARCHIVER_IP provided: ${newConfig.ARCHIVER_IP}`)
+                    }
+                    if (newConfig.ARCHIVER_PORT && !allowedPorts.includes(newConfig.ARCHIVER_PORT)) {
+                      throw new Error(`Invalid ARCHIVER_PORT provided: ${newConfig.ARCHIVER_PORT}`)
+                    }
                     if (config.VERBOSE)

@@ -1014,2 +1014,6 @@
+                    if (newConfig.lastActivityCheckInterval && (newConfig.lastActivityCheckInterval < 1000 || newConfig.lastActivityCheckInterval > 60000)) {
+                      throw new Error('Invalid value for lastActivityCheckInterval. It must be between 1000 and 60000 milliseconds.')
+                    }
                     if (config.VERBOSE)

@@ -443,3 +443,4 @@
             export function isValidVersion(minimumVersion: string, latestVersion: string, nodeVersion: string): boolean {
-              if(config.VERBOSE) Logger.mainLogger.debug('isValidVersion: ', minimumVersion, latestVersion, nodeVersion)
+              const sanitizedNodeVersion = nodeVersion.replace(/\n|\r/g, "");
+              if(config.VERBOSE) Logger.mainLogger.debug('isValidVersion: ', minimumVersion, latestVersion, sanitizedNodeVersion)

Add version check for firstnode in restore network #62

Are you sure you want to change the base?

Add version check for firstnode in restore network #62

Conversation

tanuj-shardeum commented Aug 12, 2024

github-actions bot commented Aug 12, 2024

PR Reviewer Guide 🔍

thantsintoe Sep 25, 2024

Choose a reason for hiding this comment

thantsintoe Sep 25, 2024

Choose a reason for hiding this comment