Server: Crash if unrecognized packet ID about node-minecraft-protocol HOT 29 CLOSED

👍

from node-minecraft-protocol.

Been looking at this, and that's not the only thing. All the asserts can cause a crash by a potentially malicious user. I suppose there are two ways to fix it :
rewrite asserts to ifs
enclose asserts in try blocks.

from node-minecraft-protocol.

There are also some places where a malicious user could cause bad things to happen, for example by specifying an insanely large record count in several of the packets.

A full audit of the packet parsing code would be the best bet for achieving relatively robust security.

Is this an ongoing blocker for you?

from node-minecraft-protocol.

Well, I'm working on a minecraft server written in node.js as a fun side-project. For now, it's nothing too serious, the goal is to reproduce a somewhat functional vanilla server. Obviously, I wouldn't want it to be crashing on errors lol.

Even if it isn't an ongoing blocker for me, I suppose providing a working library would be a good idea anyway...

from node-minecraft-protocol.

I agree - and I do intend to fix it. Just trying to figure out how to prioritize it. There is opportunity cost, after all.

from node-minecraft-protocol.

Yeah, I understand, and it is important to prioritize! I suppose your current priority is the client code, with mineflayer (which is a pretty cool project). I'll be fiddling with the server code to fix the asserts (and other potential complete crashes I may find) in a branch on my fork. I believe replacing them with ifs() is the best idea, as the try {} catch {} blocks are slower (from my understanding ? Ain't a pro at v8 or node, started not too long ago really ^^).

I'll also try to learn TDD to make tests with unexpected packet lengths and unknown packets against the server.

BTW, looking at the code architecture, I was pretty amazed at the way you reused the client class in your server class for connection, that's pretty clever ^^'.

from node-minecraft-protocol.

I don't know about the performance of try/catch in node, because the convention is usually to have an error argument in your callbacks (since everything should be asynchronous). For example:

doSomething(function(err, result) {
  if(err) handleError(err);
  else handleResult(result);
});

rather than

try {
  result = doSomething();
  handleResult(result);
} catch(err) {
  handleError(err);
}

from node-minecraft-protocol.

@roblabla

Thanks for the compliment.

I was thinking along the same lines as you - in the protocol, the parsing functions already have to return an object which includes size and value. It would be pretty easy to return an object with an error property if there was a problem, and then bubble that up into parsePacket. Then in client.js if parsePacket returns an error, you could gracefully emit an error and unhook the relevant event listeners.

If you like I can create a branch and skeleton this out, and then all you'd have to do is update the parse functions, returning an error instead of crashing.

from node-minecraft-protocol.

btw I don't do TDD. I haphazardly add tests in as an afterthought ;)

from node-minecraft-protocol.

Why not run a fuzzer against the server as a test? Would test a whole lot of cases that have to do with invalid data

from node-minecraft-protocol.

Haha, I was thinking about the same alternative while looking at the code after doing some updates for my node.js website. Well, I don't have much yet, but yes, I did realize your architecture for packet returns.
I suppose the best way would be to modify parsePacket as to return an "error" as part of the object in case an error happen anywhere (also, make sure to break out of the for loop if an error happen).
Then, in client.js, setSocket,

if(! parsed) break; // Why is this here btw ?
if(!parsed.error) end(parsed.error);

And concerning TDD, I suppose you could call them "regression tests" then ^^'... Although the way they are done, they look more like unit tests than regression tests.

from node-minecraft-protocol.

@contra I was thinking about that too, send all the packets with correct data to see if the server crashes, and then some incorrect data to check if the client gets kicked properly.

from node-minecraft-protocol.

Why is this here btw ?

Because the minecraft protocol does not prepend packet lengths to packets, we have no way of knowing whether the packet has been parsed until we try to parse it. So we try to parse it, and if there wasn't enough data to parse it, we wait until more data has been buffered before trying again.

from node-minecraft-protocol.

Aaah, I see now.

from node-minecraft-protocol.

Ok, so as ugly as it seems, here is what assert uses in node.js :

function ok(value, message) {
  if (!!!value) fail(value, true, message, '==', assert.ok);
}

To maximize compability, I am replacing assert.ok with if (!!!) checks.

from node-minecraft-protocol.

You don't need to use !!!. You can use !.

from node-minecraft-protocol.

I'm pretty confused about why it's written this way in assert.js... The javascript internals are always so obscure lol.

from node-minecraft-protocol.

Lol at duplicate code :

function get(packetId, toServer) {
  var packetInfo = packets[packetId];
  assert.ok(packetInfo, "unrecognized packet id: " + packetId);
  return Array.isArray(packetInfo) ?
    packetInfo :
    toServer ?
      packetInfo.toServer :
      packetInfo.toClient;
}
// ...
var packetInfo = get(packetId, isServer);
 assert.ok(packetInfo, "Unrecognized packetId: " + packetId);

Anyhow, should I make get return an object with a potential error ?

from node-minecraft-protocol.

can never have too many asserts ;)

Actually I think you can leave that code alone since it's only used by serialization code. It's ok (and in fact preferred) to crash if the developer uses the API incorrectly.

What you want to focus on is code that has to do with parsing.

from node-minecraft-protocol.

parsing uses this function though.

from node-minecraft-protocol.

function parsePacket(buffer, isServer) {
  if (buffer.length < 1) return null;
  var packetId = buffer.readUInt8(0);
  var size = 1;
  var results = { id: packetId };
  var packetInfo = get(packetId, isServer);
  var error;
  if (!packetInfo) {
    error = "Unrecognized packetId: " + packetId;
  }
//...

I suppose a simple way would be to change !packetInfo to an actual check and move it over the get function, like

function parsePacket(buffer, isServer) {
  if (buffer.length < 1) return null;
  var packetId = buffer.readUInt8(0);
  var size = 1;
  var results = { id: packetId };
  if (!packets[packetId]) {
    return {
      error: "Unrecognized packetId: " + packetId;
    }
  }
  var packetInfo = get(packetId, isServer);

from node-minecraft-protocol.

My bad, I missed that.

I think:

1. get should return null if no packet found
2. createPacketBuffer should have an assertion that packet, the result from get, is not null
3. parsePacket should return a value indicating an error (maybe null) if packetInfo, the result from get is null.

from node-minecraft-protocol.

Okay. Doing it that way.

from node-minecraft-protocol.

Okay, doing a first push with the main skeleton changes on my fork bugfix-serverAsserts. All that should be left (from what I understand (I may be wrong) is to add the errors for each parsing method.
And yes, it passes the tests.

from node-minecraft-protocol.

Yep, and that's fine. I think it won't break anything though - the behavior that will change is that instead of crashing with an assertion failure, it will emit an error on the client and disconnect.

from node-minecraft-protocol.

Well right now, the way I coded it, I modified the client.js, so if it is a client, it will end the client abruptly, if it is a server, it will send a kick packet with the reason. (hence why I deleted my comment, thinking "I'm an idiot").

I'm unsure if it should emit an error or no.

from node-minecraft-protocol.

I think we should avoid if statements in the Client class. One way to do this is that if there is a parsing error, emit an error. Then the server can respond to this error by kicking the client, and as a client you can respond to the error however you want (probably by disconnecting)

from node-minecraft-protocol.

Feel free to do a PR with what you have and I can illustrate my points by adding commit(s)

from node-minecraft-protocol.

New push with all the asserts from readers replaced with ifs and errors.

I think doing what Contra suggested is probably in order, it would allow both client and server to be tested adequately.

Okay, creating a PR.

from node-minecraft-protocol.