Skip to content
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.

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

Add version check for firstnode in restore network #62

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
65 changes: 58 additions & 7 deletions src/API.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,18 @@
* CZ adds AZ's join reqeuest to cycle zero and sets AZ as cycleRecipient
*/
type NodeListRequest = FastifyRequest<{
Body: P2P.FirstNodeInfo & Crypto.SignedMessage
Body: {
joinRequest: {
appJoinData: {
shardeumVersion: string
minVersion: string
activeVersion: string
latestVersion: string
operatorCLIVersion: string
operatorGUIVersion: string
}
} & P2P.FirstNodeInfo
} & Crypto.SignedMessage
}>

server.get('/myip', function (request, reply) {
Expand All @@ -74,21 +85,26 @@
reply.send({ ip })
})

server.post('/nodelist', (request: NodeListRequest, reply) => {
profilerInstance.profileSectionStart('POST_nodelist')
nestedCountersInstance.countEvent('consensor', 'POST_nodelist', 1)
const signedFirstNodeInfo = request.body
const requestBody = request.body
// eslint-disable-next-line no-constant-condition
if(config.VERBOSE) {
Logger.mainLogger.debug('POST /nodelist requestBody:', requestBody)
Dismissed Show dismissed Hide dismissed
}

if (State.isFirst && NodeList.isEmpty() && !NodeList.foundFirstNode) {
try {
const isSignatureValid = Crypto.verify(signedFirstNodeInfo)
const isSignatureValid = Crypto.verify(requestBody)
if (!isSignatureValid) {
Logger.mainLogger.error('Invalid signature', signedFirstNodeInfo)
Logger.mainLogger.error('Invalid signature', requestBody)

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.

Copilot Autofix AI about 2 months ago

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.

Suggested changeset 1
src/API.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/API.ts b/src/API.ts
--- a/src/API.ts
+++ b/src/API.ts
@@ -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
EOF
@@ -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
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

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

Please select one or more of the options
reply.send({ success: false, error: 'Invalid signature' })
return
}
} catch (e) {
Logger.mainLogger.error(e)
Logger.mainLogger.error('Signature verification failed', requestBody)

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.

Copilot Autofix AI 3 months ago

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.

Suggested changeset 1
src/API.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/API.ts b/src/API.ts
--- a/src/API.ts
+++ b/src/API.ts
@@ -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)
EOF
@@ -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)
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

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

Please select one or more of the options
reply.send({ success: false, error: 'Signature verification failed' })
return
}
Expand All @@ -98,16 +114,51 @@
return
}
NodeList.toggleFirstNode()
const ip = signedFirstNodeInfo.nodeInfo.externalIp
const port = signedFirstNodeInfo.nodeInfo.externalPort
const publicKey = signedFirstNodeInfo.nodeInfo.publicKey

const signedFirstNodeInfo = requestBody.joinRequest.nodeInfo
const appJoinData = requestBody.joinRequest.appJoinData

const ip = signedFirstNodeInfo.externalIp
const port = signedFirstNodeInfo.externalPort
const publicKey = signedFirstNodeInfo.publicKey

const firstNode: NodeList.ConsensusNodeInfo = {
ip,
port,
publicKey,
}

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

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.

Copilot Autofix AI about 2 months ago

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.

Suggested changeset 1
src/API.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/API.ts b/src/API.ts
--- a/src/API.ts
+++ b/src/API.ts
@@ -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)
       }
EOF
@@ -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)
}
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

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

Please select one or more of the options
}


const networkAccount: AccountDB.AccountsCopy | string = getGlobalNetworkAccount(false)
Logger.mainLogger.debug('networkAccount', networkAccount)

if (networkAccount) {
if (
Copy link
Contributor

Choose a reason for hiding this comment

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

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

typeof networkAccount != 'string' &&
!NodeList.isValidVersion(
networkAccount?.data?.current?.minVersion,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.

Copilot Autofix AI 3 months ago

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.

Suggested changeset 1
src/API.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/API.ts b/src/API.ts
--- a/src/API.ts
+++ b/src/API.ts
@@ -150,3 +150,3 @@
           Logger.mainLogger.debug(
-            `Invalid version of the node: ${appJoinData.shardeumVersion}, required minVersion: ${networkAccount?.data?.current?.minVersion}, required latestVersion: ${networkAccount?.data?.current?.latestVersion}`
+            `Invalid version of the node: ${appJoinData.shardeumVersion.replace(/\n|\r/g, '')}, required minVersion: ${networkAccount?.data?.current?.minVersion}, required latestVersion: ${networkAccount?.data?.current?.latestVersion}`
           )
EOF
@@ -150,3 +150,3 @@
Logger.mainLogger.debug(
`Invalid version of the node: ${appJoinData.shardeumVersion}, required minVersion: ${networkAccount?.data?.current?.minVersion}, required latestVersion: ${networkAccount?.data?.current?.latestVersion}`
`Invalid version of the node: ${appJoinData.shardeumVersion.replace(/\n|\r/g, '')}, required minVersion: ${networkAccount?.data?.current?.minVersion}, required latestVersion: ${networkAccount?.data?.current?.latestVersion}`
)
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

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

Please select one or more of the options
)
reply.send({ success: false, error: 'Invalid version' })
return
} else {
Logger.mainLogger.debug('POST /nodelist network account is a string or version is valid: ', typeof networkAccount)
}
} else {
Logger.mainLogger.debug('POST /nodelist network account does not exist')
}

Data.initSocketClient(firstNode)

// Add first node to NodeList
Expand Down
2 changes: 1 addition & 1 deletion src/GlobalAccount.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export interface GlobalAccountsHashAndTimestamp {
export const globalAccountsMap = new Map<string, GlobalAccountsHashAndTimestamp>()
const appliedConfigChanges = new Set<string>()

export function getGlobalNetworkAccount(hash: boolean): object | string {
export function getGlobalNetworkAccount(hash: boolean): AccountDB.AccountsCopy | string {
if (hash) {
return cachedGlobalNetworkAccountHash
}
Expand Down
32 changes: 32 additions & 0 deletions src/NodeList.ts
Original file line number Diff line number Diff line change
Expand Up @@ -416,3 +416,35 @@
foundFirstNode = !foundFirstNode
Logger.mainLogger.debug('foundFirstNode', foundFirstNode)
}

export function isEqualOrNewerVersion(minimumVersion: string, nodeVersion: string): boolean {
if (minimumVersion === nodeVersion) {
return true
}

const minVerParts = minimumVersion.split('.')
const nodeVerParts = nodeVersion.split('.')

/* eslint-disable security/detect-object-injection */
for (let i = 0; i < nodeVerParts.length; i++) {
const nodeV = ~~nodeVerParts[i] // parse int
const minV = ~~minVerParts[i] // parse int
if (nodeV > minV) return true
if (nodeV < minV) return false
}
/* eslint-enable security/detect-object-injection */
return false
}

export function isEqualOrOlderVersion(maximumVersion: string, nodeVersion: string): boolean {
return isEqualOrNewerVersion(nodeVersion, maximumVersion)
}

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

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.

Copilot Autofix AI about 2 months ago

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.

Suggested changeset 1
src/NodeList.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/NodeList.ts b/src/NodeList.ts
--- a/src/NodeList.ts
+++ b/src/NodeList.ts
@@ -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)
   
EOF
@@ -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)

Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

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

Please select one or more of the options

const equalOrNewer = isEqualOrNewerVersion(minimumVersion, nodeVersion)
const equalOrOlder = isEqualOrOlderVersion(latestVersion, nodeVersion)

return equalOrNewer && equalOrOlder
}
Loading