holepunchto / dht-rpc

Make RPC calls over a Kademlia based DHT.
MIT License
196 stars 37 forks source link

Crash when key length mismatches #17

Closed KrishnaPG closed 3 years ago

KrishnaPG commented 4 years ago

Giving Infohash (from torrent) of length 20bytes crashes this package. While infohashes may not be compatible, just crashing the whole app on differences over the length of the key may not be the best idea, since the data / key could be coming from any third party and handling the error is more better solution. Below is crash stack:

\test\node_modules\xor-distance\index.js:4
  if (a.length !== b.length) throw new Error('Inputs should have the same length')
                             ^

Error: Inputs should have the same length
    at dist (\test\node_modules\xor-distance\index.js:4:36)      
    at QueryTable.addUnverified (\test\node_modules\dht-rpc\lib\query-table.js:15:21)
    at QueryStream._onresponse (\test\node_modules\dht-rpc\lib\query-stream.js:80:20)
    at IO._finish (\test\node_modules\dht-rpc\lib\io.js:132:9)   
    at IO._onmessage (\test\node_modules\dht-rpc\lib\io.js:104:14
)
    at Socket.emit (events.js:209:13)
    at UDP.onMessage [as onmessage] (dgram.js:853:8)

Handling the exception here to ignore it could make the app more robust: https://github.com/mafintosh/dht-rpc/blob/a7752605bb18bf622a5e152bc4762b49f6525cda/lib/query-table.js#L15

make-github-pseudonymous-again commented 4 years ago

@KrishnaPG Can you explain what is your use case here? What is the code that would trigger this error?

KrishnaPG commented 4 years ago

When node IDs are creating using different length strings (obtained from third party database), the whole app crashes when lookup is performed. https://github.com/mafintosh/dht-rpc/blob/b6daae9f8979ff4a6949dc92991685b355ad1b3c/lib/query-stream.js#L81

Since the lookup code has no control over the thrown error, it cannot be handled at the app level.

Ignore the node, or just not adding the peer to the discovered list of items is a more suited solution.

mafintosh commented 4 years ago

That line only gets 32 byte ids. I think we are guarding the ids from any untrusted source, do you have test case where that isn’t the case?

mafintosh commented 3 years ago

v5 has a lot of checks in addition to my above comment, feel free to reopen with a test case