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.

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

Jump to bottom

fix: current-cycle-hash resp type issue #35

Merged
merged 6 commits into from
Jun 17, 2024
Merged

fix: current-cycle-hash resp type issue #35

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
05b4d75
baked in newPOQReceipt = false
afostr Jun 16, 2024
e4e0405
3.4.20
afostr Jun 16, 2024
a270ead
Added the before/after account hash exist check in the appliedReceipt
jairajdev Jun 17, 2024
b5333f9
Additional hardening for old POQ
jairajdev Jun 17, 2024
00e6712
fix: current-cycle-hash resp type issue
tanuj-shardeum Jun 17, 2024
13cfc2e
3.4.21
afostr Jun 17, 2024
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
4 changes: 2 additions & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@shardus/archiver",
"version": "3.4.19",
"version": "3.4.20",
"engines": {
"node": "18.16.1"
},
Expand Down
2 changes: 1 addition & 1 deletion src/Config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ let config: Config = {
cycleRecordsCache: {
enabled: false,
},
newPOQReceipt: true,
newPOQReceipt: false,
}
// Override default config params from config file, env vars, and cli args
export async function overrideDefaultConfig(file: string): Promise<void> {
Expand Down
27 changes: 17 additions & 10 deletions src/Data/Collector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@
if (config.VERBOSE) Logger.mainLogger.debug('robustQuery', receipt.tx.txId, robustQuery)
if (!robustQuery || !robustQuery.value || !(robustQuery.value as any).receipt) {
Logger.mainLogger.error(
`❌ 'null' response from all nodes in receipt-validation for txId: ${receipt.tx.txId} , ${receipt.cycle}, ${receipt.tx.timestamp})
`❌ 'null' response from all nodes in receipt-validation for txId: ${receipt.tx.txId} , ${receipt.cycle}, ${receipt.tx.timestamp}
}`
)
if (nestedCountersInstance)
Expand Down Expand Up @@ -235,7 +235,10 @@
Logger.mainLogger.error(
`The receipt validation failed from robustQuery nodes ${receipt.tx.txId} , ${receipt.cycle}, ${receipt.tx.timestamp}`
)
Logger.mainLogger.error(StringUtils.safeStringify(robustQueryReceipt), StringUtils.safeStringify(fullReceipt))
Logger.mainLogger.error(
StringUtils.safeStringify(robustQueryReceipt),
StringUtils.safeStringify(fullReceipt)
)
}
}
Logger.mainLogger.error(
Expand Down Expand Up @@ -384,7 +387,7 @@

export const verifyReceiptData = async (
receipt: Receipt.ArchiverReceipt,
checkReceiptRobust: boolean = true
checkReceiptRobust = true
): Promise<{ success: boolean; newReceipt?: Receipt.ArchiverReceipt }> => {
if (config.newPOQReceipt === false) return { success: true }
const result = { success: false }
Expand Down Expand Up @@ -641,6 +644,7 @@
txDataList.push({ txId, timestamp })
// If the receipt is a challenge, then skip updating its accounts data or transaction data
if (
config.newPOQReceipt === true &&
appliedReceipt &&
appliedReceipt.confirmOrChallenge &&
appliedReceipt.confirmOrChallenge.message === 'challenge'
Expand Down Expand Up @@ -796,7 +800,8 @@
cycleMarker: cycleRecord.marker,
cycleRecord,
}
if (config.dataLogWrite && CycleLogWriter) CycleLogWriter.writeToLog(`${StringUtils.safeStringify(cycleObj)}\n`)
if (config.dataLogWrite && CycleLogWriter)
CycleLogWriter.writeToLog(`${StringUtils.safeStringify(cycleObj)}\n`)
const cycleExist = await queryCycleByMarker(cycleObj.cycleMarker)
if (cycleExist) {
if (StringUtils.safeStringify(cycleObj) !== StringUtils.safeStringify(cycleExist))
Expand Down Expand Up @@ -1247,9 +1252,10 @@
savedReceiptsCount++
}
}
Logger.mainLogger.debug(
`Clean ${savedReceiptsCount} old receipts from the processed receipts cache on cycle ${getCurrentCycleCounter()}`
)
if (savedReceiptsCount > 0)
Logger.mainLogger.debug(
`Clean ${savedReceiptsCount} old receipts from the processed receipts cache on cycle ${getCurrentCycleCounter()}`

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
Loading
.

Copilot Autofix AI 5 months ago

The problem with the code is that it logs user-provided data without sanitizing it first. This can lead to log injection attacks where an attacker can manipulate the log entries by providing malicious input.

To fix this issue, we need to sanitize the user-provided data before logging it. In this case, we can use the String.prototype.replace method to remove any newline characters from the user-provided data. This will prevent the attacker from creating new log entries by injecting newline characters into the input.

In the file src/API.ts, we need to sanitize the gossipPayload variable before passing it to the Collector.validateGossipData and Collector.processGossipData methods. We can do this by replacing all newline characters in the gossipPayload string with an empty string.

In the file src/Data/Collector.ts, we need to sanitize the getCurrentCycleCounter() function call before logging it. We can do this by converting the result of the function call to a string and then replacing all newline characters with an empty string.

Suggested changeset 2
src/Data/Collector.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/Data/Collector.ts b/src/Data/Collector.ts
--- a/src/Data/Collector.ts
+++ b/src/Data/Collector.ts
@@ -1254,6 +1254,9 @@
   }
-  if (savedReceiptsCount > 0)
+  if (savedReceiptsCount > 0) {
+    // Sanitize getCurrentCycleCounter() by replacing newline characters
+    const sanitizedCycleCounter = String(getCurrentCycleCounter()).replace(/\n|\r/g, "")
     Logger.mainLogger.debug(
-      `Clean ${savedReceiptsCount} old receipts from the processed receipts cache on cycle ${getCurrentCycleCounter()}`
+      `Clean ${savedReceiptsCount} old receipts from the processed receipts cache on cycle ${sanitizedCycleCounter}`
     )
+  }
 }
EOF
@@ -1254,6 +1254,9 @@
}
if (savedReceiptsCount > 0)
if (savedReceiptsCount > 0) {
// Sanitize getCurrentCycleCounter() by replacing newline characters
const sanitizedCycleCounter = String(getCurrentCycleCounter()).replace(/\n|\r/g, "")
Logger.mainLogger.debug(
`Clean ${savedReceiptsCount} old receipts from the processed receipts cache on cycle ${getCurrentCycleCounter()}`
`Clean ${savedReceiptsCount} old receipts from the processed receipts cache on cycle ${sanitizedCycleCounter}`
)
}
}
src/API.ts
Outside changed files

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
@@ -838,3 +838,5 @@
   server.post('/gossip-data', async (_request: GossipDataRequest, reply) => {
-    const gossipPayload = _request.body
+    let gossipPayload = _request.body
+    // Sanitize gossipPayload by replacing newline characters
+    gossipPayload = JSON.parse(JSON.stringify(gossipPayload).replace(/\\n|\\r/g, ""))
     if (config.VERBOSE)
EOF
@@ -838,3 +838,5 @@
server.post('/gossip-data', async (_request: GossipDataRequest, reply) => {
const gossipPayload = _request.body
let gossipPayload = _request.body
// Sanitize gossipPayload by replacing newline characters
gossipPayload = JSON.parse(JSON.stringify(gossipPayload).replace(/\\n|\\r/g, ""))
if (config.VERBOSE)
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
)
}

export function cleanOldOriginalTxsMap(timestamp: number): void {
Expand All @@ -1262,9 +1268,10 @@
savedOriginalTxsCount++
}
}
Logger.mainLogger.debug(
`Clean ${savedOriginalTxsCount} old originalTxsData from the processed originalTxsData cache on cycle ${getCurrentCycleCounter()}`
)
if (savedOriginalTxsCount > 0)
Logger.mainLogger.debug(
`Clean ${savedOriginalTxsCount} old originalTxsData from the processed originalTxsData cache on cycle ${getCurrentCycleCounter()}`

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
Loading
.

Copilot Autofix AI 5 months ago

The problem with the code is that it logs user-provided data without sanitizing it first. This can lead to log injection attacks where a malicious user can manipulate the log entries by providing input with special characters that are interpreted when the log output is displayed.

To fix this issue, we need to sanitize the user-provided data before logging it. In this case, we can use the String.prototype.replace method to remove any newline characters from the user-provided data. This will prevent the user from injecting new log entries by providing input with newline characters.

In the file src/API.ts, we need to sanitize the gossipPayload variable before logging it. We can do this by replacing all newline characters in the gossipPayload variable with an empty string.

In the file src/Data/Collector.ts, we need to sanitize the getCurrentCycleCounter() function before logging it. We can do this by converting the function's return value to a string and then replacing all newline characters in the string with an empty string.

Suggested changeset 2
src/Data/Collector.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/Data/Collector.ts b/src/Data/Collector.ts
--- a/src/Data/Collector.ts
+++ b/src/Data/Collector.ts
@@ -1272,3 +1272,3 @@
     Logger.mainLogger.debug(
-      `Clean ${savedOriginalTxsCount} old originalTxsData from the processed originalTxsData cache on cycle ${getCurrentCycleCounter()}`
+      `Clean ${savedOriginalTxsCount} old originalTxsData from the processed originalTxsData cache on cycle ${getCurrentCycleCounter().toString().replace(/\n|\r/g, "")}`
     )
EOF
@@ -1272,3 +1272,3 @@
Logger.mainLogger.debug(
`Clean ${savedOriginalTxsCount} old originalTxsData from the processed originalTxsData cache on cycle ${getCurrentCycleCounter()}`
`Clean ${savedOriginalTxsCount} old originalTxsData from the processed originalTxsData cache on cycle ${getCurrentCycleCounter().toString().replace(/\n|\r/g, "")}`
)
src/API.ts
Outside changed files

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
@@ -840,3 +840,3 @@
     if (config.VERBOSE)
-      Logger.mainLogger.debug('Gossip Data received', StringUtils.safeStringify(gossipPayload))
+      Logger.mainLogger.debug('Gossip Data received', StringUtils.safeStringify(gossipPayload).replace(/\n|\r/g, ""))
     const result = Collector.validateGossipData(gossipPayload)
EOF
@@ -840,3 +840,3 @@
if (config.VERBOSE)
Logger.mainLogger.debug('Gossip Data received', StringUtils.safeStringify(gossipPayload))
Logger.mainLogger.debug('Gossip Data received', StringUtils.safeStringify(gossipPayload).replace(/\n|\r/g, ""))
const result = Collector.validateGossipData(gossipPayload)
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
)
}

export const scheduleMissingTxsDataQuery = (): void => {
Expand Down
9 changes: 9 additions & 0 deletions src/shardeum/calculateAccountHash.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,15 @@ export const verifyAccountHash = (receipt: ArchiverReceipt): boolean => {
)
return false
}
if (
receipt.appliedReceipt.appliedVote.account_state_hash_before.length !==
receipt.appliedReceipt.appliedVote.account_state_hash_after.length
) {
Logger.mainLogger.error(
`Account state hash before and after count does not match! ${receipt.tx.txId} , ${receipt.cycle} , ${receipt.tx.timestamp}`
)
return false
}
for (const account of receipt.accounts) {
if (account.data.accountType === AccountType.Account) {
fixAccountUint8Arrays(account.data.account)
Expand Down
11 changes: 11 additions & 0 deletions src/shardeum/verifyAppReceiptData.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,16 @@ export const verifyAppReceiptData = async (
appliedReceipt.appliedVote.account_state_hash_after.length > 0
) {
for (let i = 0; i < appliedReceipt.appliedVote.account_id.length; i++) {
if (
// eslint-disable-next-line security/detect-object-injection
!appliedReceipt.appliedVote.account_state_hash_before[i] ||
// eslint-disable-next-line security/detect-object-injection
!appliedReceipt.appliedVote.account_state_hash_after[i]
) {
Logger.mainLogger.error(
`The account state hash before or after is missing in the receipt! ${receipt.tx.txId} , ${receipt.cycle} , ${receipt.tx.timestamp}`
)
}
if (
// eslint-disable-next-line security/detect-object-injection
appliedReceipt.appliedVote.account_state_hash_before[i] !==
Expand All @@ -44,6 +54,7 @@ export const verifyAppReceiptData = async (
// If the existing receipt is challenged and the new receipt is confirmed, overwrite the existing receipt
let skipAppReceiptCheck = false
if (
config.newPOQReceipt === true &&
existingReceipt.appliedReceipt &&
existingReceipt.appliedReceipt.confirmOrChallenge &&
receipt.appliedReceipt &&
Expand Down
8 changes: 4 additions & 4 deletions src/sync-v2/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,11 +192,11 @@ function syncLatestCycleRecordAndMarker(
activeNodes: P2PTypes.SyncTypes.ActiveNode[]
): ResultAsync<[P2PTypes.CycleCreatorTypes.CycleData, hexstring], Error> {
// run a robust query for the latest cycle record hash
return robustQueryForCycleRecordHash(activeNodes).andThen(({ value: cycleRecordHash, winningNodes }) =>
return robustQueryForCycleRecordHash(activeNodes).andThen(({ value, winningNodes }) =>
// get current cycle record from node
getCurrentCycleDataFromNode(winningNodes[0], cycleRecordHash).andThen((cycleRecord) =>
verifyCycleRecord(cycleRecord, cycleRecordHash).map(
() => [cycleRecord, cycleRecordHash] as [P2PTypes.CycleCreatorTypes.CycleData, hexstring]
getCurrentCycleDataFromNode(winningNodes[0], value.currentCycleHash).andThen((cycleRecord) =>
verifyCycleRecord(cycleRecord, value.currentCycleHash).map(
() => [cycleRecord, value.currentCycleHash] as [P2PTypes.CycleCreatorTypes.CycleData, hexstring]
)
)
)
Expand Down
2 changes: 1 addition & 1 deletion src/sync-v2/queries.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ function attemptSimpleFetch<T>(
}

/** Executes a robust query to retrieve the cycle marker from the network. */
export function robustQueryForCycleRecordHash(nodes: ActiveNode[]): RobustQueryResultAsync<hexstring> {
export function robustQueryForCycleRecordHash(nodes: ActiveNode[]): RobustQueryResultAsync<{ currentCycleHash: hexstring }> {
return makeRobustQueryCall(nodes, 'current-cycle-hash')
}

Expand Down
Loading