[SEC] beacon_node CLI - deposits - segfault and AssertionError when importing keystore

[tintinweb]

Description

nbc crashes when importing invalid keystore json files.

1) vendor/NimYAML/test/yaml-test-suite/Q5MG/in.json

contents:

{}

crash on import:

⇒  build/beacon_node deposits import vendor/NimYAML/test/yaml-test-suite 
Traceback (most recent call last)
/Users/tintin/workspace/nim/nim-beacon-chain-round3/vendor/nim-testutils/testutils/moduletests.nim(21) beacon_node
/Users/tintin/workspace/nim/nim-beacon-chain-round3/beacon_chain/beacon_node.nim(1258) main
/Users/tintin/workspace/nim/nim-beacon-chain-round3/beacon_chain/keystore_management.nim(268) importKeystoresFromDir
/Users/tintin/workspace/nim/nim-beacon-chain-round3/beacon_chain/spec/keystore.nim(469) decryptCryptoField
/Users/tintin/workspace/nim/nim-beacon-chain-round3/beacon_chain/spec/keystore.nim(353) shaChecksum
/Users/tintin/workspace/nim/nim-beacon-chain-round3/vendor/nimcrypto/nimcrypto/utils.nim(417) update
SIGSEGV: Illegal storage access. (Attempt to read from nil?)

2) ./.vscode/tasks.json

contents:

{
  // See https://go.microsoft.com/fwlink/?LinkId=733558
  // for the documentation about the tasks.json format
  "version": "2.0.0",
  "tasks": [
    {
      "label": "nim-beacon-chain build",
      "type": "shell",
      "command": "make",
      "group": {
        "kind": "build",
        "isDefault": true
      },
      "problemMatcher": [
        "$gcc"
      ]
    },
    {
      "label": "nim-beacon-chain test",
      "type": "shell",
      "command": "make test",
      "group": {
        "kind": "build",
        "isDefault": true
      },
      "problemMatcher": [
        "$gcc"
      ]
    }
  ]
}

crash on import:

/Users/tintin/workspace/nim/nim-beacon-chain-round3/vendor/nim-testutils/testutils/moduletests.nim(21) beacon_node
/Users/tintin/workspace/nim/nim-beacon-chain-round3/beacon_chain/beacon_node.nim(1258) main
/Users/tintin/workspace/nim/nim-beacon-chain-round3/vendor/nim-serialization/serialization.nim(96) importKeystoresFromDir
/Users/tintin/workspace/nim/nim-beacon-chain-round3/vendor/nim-serialization/serialization.nim(46) readValue
/Users/tintin/workspace/nim/nim-beacon-chain-round3/vendor/nim-json-serialization/json_serialization/reader.nim(492) readValue
/Users/tintin/workspace/nim/nim-beacon-chain-round3/vendor/nim-json-serialization/json_serialization/reader.nim(149) skipToken
/Users/tintin/workspace/nim/nim-beacon-chain-round3/vendor/nim-json-serialization/json_serialization/lexer.nim(357) next
/Users/tintin/workspace/nim/nim-beacon-chain-round3/vendor/nim-json-serialization/json_serialization/lexer.nim(229) skipWhitespace
/Users/tintin/workspace/nim/nim-beacon-chain-round3/vendor/nim-faststreams/faststreams/inputs.nim(668) advance
/Users/tintin/workspace/nim/nim-beacon-chain-round3/vendor/nim-faststreams/faststreams/inputs.nim(214) getNewSpan
/Users/tintin/workspace/nim/nim-beacon-chain-round3/vendor/nimbus-build-system/vendor/Nim/lib/system/assertions.nim(29) failedAssertImpl
/Users/tintin/workspace/nim/nim-beacon-chain-round3/vendor/nimbus-build-system/vendor/Nim/lib/system/assertions.nim(22) raiseAssert
/Users/tintin/workspace/nim/nim-beacon-chain-round3/vendor/nimbus-build-system/vendor/Nim/lib/system/fatal.nim(49) sysFatal
Error: unhandled exception: /Users/tintin/workspace/nim/nim-beacon-chain-round3/vendor/nim-faststreams/faststreams/inputs.nim(214, 12) `buffers != nil`  [AssertionError]

Exploit Scenario

This sink should only be reachable by trusted entities, however, someone might provide a victim with a modified keystore file which they will end up crashing with. Severity depends on whether the segfault can be exploited in some way (so far it looks like an invalid memory read which suggests it is not an exploitable crash)

Mitigation Recommendation

• concrete mitigation requires an investigation. Looks like two distinct bugs, one being an out of bounds access and the other is a failing assertion that should not occur.
• fuzz-testing; unit-testing; there are likely more scenarios where this might break so we highly recommend to fuzztest the interface.

[tintinweb]

(review commit: a0a4526c8347b12a672d3b1d333505f4572119c0)

• ✓ raises an InvalidKeystore exception instead of assertion or segfault
• ✗ ask's for a password on empty json even though there is no data to decrypt --> vendor/NimYAML/test/yaml-test-suite/Q5MG/in.json : {}

Please enter the password for decrypting 'vendor/NimYAML/test/yaml-test-suite/Q5MG/in.json' or press ENTER to skip importing this keystore
Password: 

• duplicate testcase in https://github.com/status-im/nimbus-eth2/blob/faadc3cbeb047bec4f92a776b22de473ad9cdf3b/tests/test_keystore.nim#L279-L283

[zah]

I've re-addressed this in the fix-1687 branch

[tintinweb]

1687 LGTM

• consider fuzz-testing and improving on unit-tests