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

Rewrite in favor of ProtocolV2 #70

Closed
wants to merge 11 commits into from
Closed

Conversation

coelho
Copy link

@coelho coelho commented Mar 31, 2014

The current protocol (v1) is insecure. To fix this, we'll introduce ProtocolV2.

The first change is that the server will now be sending a challenge with it's version payload sent initially to the client. The payload will look like so:

VOTIFIER 2.0 4blpmp48dh27thonajemi2qte2

ProtocolV2 uses JSON for messages. An example message is below:

{
    "service": "MinecraftServerList", // name of the service
    "signature": "YmFzZTY0ZW5jb2RlZHNpZ25hdHVyZQ==", // sha256withrsa signature
    "payload": "{
        \"host\": \"myservername.com\", // the host that we are connecting to (useful for vhosting)
        \"username\": \"etc\", // username of the originating voter
        \"address\": \"123.123.123.123\", // ip address of the originating voter
        \"challenge\": \"4blpmp48dh27thonajemi2qte2\", // random string initially sent by the server
        \"timestamp\": 1396257969 // unix time
    }"
}

Every voting website will have an RSA key pair. (php generator @ https://gist.github.com/coelho/9887580) It will allow users to add it to their config.yml as such:

host: 0.0.0.0
port: 8192
debug: false
listener_folder: plugins/Votifier/listeners
enable_old_protocol: true
websites:
  test: https://example.com/votifier/public.key

This RSA key pair will be used to perform SHA256withRSA signature signing on the payload. The Votifier server can then validate the signature against the payload and public key (retrieved from the website in the config.yml with the matching service name).

The server is responsible for sending a challenge at the beginning of the connection that the client must send back in the signed payload. This resolves issues with replay attacks.

If the public key is not loaded over SSL, there will be a warning in console stating that it is insecure.
Usage of the old protocol after this has become the standard will result in a warning in console that it is insecure.

Pros:

  1. This is secure.
  2. We provide compatibility towards V1 and V2 at the same time (with the option to disable V1 for security reasons).
  3. This is much easier for server owners to setup in the long run.
  4. This protocol is so much more beautiful (I mean JSON oh my god).
  5. We no longer block while serving clients on the acceptor.

Cons:

  1. We can't do this on our own. Every server list owner must see the importance in this and implement it. We can help them by creating some useful libraries for Ruby and PHP.
  2. The new setup process may be confusing at first for server owners (because it is different).

Known Issues:

  1. If "Server" trusts "Voting Website", and "Attacker" submits "Server"'s Votifier to "Voting Website", he can submit votes to that banner that were not intended for "Server". The solution to this issue is to implement unique IDs, where the "Voting Website" will send a unique ID to the client that the client must cache and forever keep. If he ever receives a different unique ID, he shall reject the request.

Thanks @blakeman8192 for helping me out with this 👍

@coelho
Copy link
Author

coelho commented Apr 1, 2014

Generate a key pair in PHP (outputs a file "private.config.php" and "public.key")
https://gist.github.com/coelho/9887580

Example PHP Implementation of the VotifierV2 Protocol
https://gist.github.com/coelho/9907277

@dalbothek
Copy link

I'm happy you took the time to revisit the protocol, I was not a big fan of the original version. That said, there are a few things I'd like to discuss:

Nested JSON

The huge advantage of formats like JSON or YAML is that they're not only readable by machines but also humans. Nesting a JSON object into a string of another one kind of defeats that purpose. I do realize that the signing a string is a bit easier, but by simply defining how exactly the JSON object has to be serialized when signing would solve that issue. It would also allow for a flatter structure (i.e. just one object containing all keys).

Timestamp

I'm not entirely sure what the benefit of this timestamp is, it might be a source of unnecessary issues.

Random number

This is a pretty solid idea, but you could prevent even more attacks by requiring server lists to use the same random number each time they're trying to send the same vote. Otherwise a MITM could simply make the server list believe that sending a vote failed and it will try again and again, each time registering a vote since the random number is different. I'd even call it "Vote ID".

Replay attack to other servers

When receiving a vote you have to make sure the host actually matches your server, otherwise it is easily possible to replay a vote to other servers.

JSON for all communication

Is there a reason votifier doesn't use JSON messages to communicate its version?

Feedback after successful vote

Currently votifier gives absolutely no feedback to the server list on whether a vote was received successfully. I'd appreciate some kind of success/error message with a description of what went wrong. This could be especially useful for server owners trying to figure out why their setup doesn't work.

Default server lists

To make things easier I would include a few of the biggest server lists in the default config, possibly commented out, so server owner can get started right away.

@coelho
Copy link
Author

coelho commented Apr 4, 2014

Nested JSON

I think it is best to avoid issues in server lists implementations by doing this. It makes the formatting more apparent so we can sign it more consistently. Otherwise, we need to make another standard for how to properly sign the JSON object (the correct formatting), and we may have more issues.

Timestamp

The timestamp is to prevent against replay attacks.

Random number

The logic behind this is we rely on the timestamp to expire the message within 10 minutes. This way, there is only a 10 minute window for the message to be vulnerable to a replay attack. To prevent against this further, we take the 'service name + "\0" + timestamp + "\0" + random number', and put it in a HashSet. Then, when a vote comes in, we make sure that this data is not already in the HashSet, and if it is, it is a replay attack. This makes a replay attack almost impossible.

Replay attack to other servers

The replay attack patches are meant for a MITM scenario. Although this does show a potential vulnerability in the protocol that needs to be fixed. Let me think of a good solution.

JSON for all communication

The reason the version (being sent from the server -> client) is still not in JSON is for cross compatibility.

Feedback after successful vote

This can be implemented fairly easily. Maybe a JSON payload like so:

{
    "status": "err",
    "errno": 1,
    "errstr": "This is an error str"
}

and on success:

{
    "status": "ok"
}

Default server lists

👍 We'd need server lists to comment and tell us where their public key will be on their website however.

@blakeman8192
Copy link
Member

Vote IDs and Replay Attacks
I agree with changing the random number field to a unique voteId field. This would allow us to prevent replay attacks and would eliminate the need of the timestamp field and handling of expiry. Votifier will just ignore multiple packets with the same vote ID.

Server-to-server Relay Attacks
Votifier can also resolve the host to an IP address and ensure that the vote packet was received from that address. This could cause potential problems with DNS though, but it seems to be the only way to do this unless we introduce a server field that identifies the server it was intended for.

Networking and Performance
Do you think the networking system should be upgraded to handle multiple connections simultaneously in a scalable way? Most servers will not benefit from this, but many large servers that are listed on multiple server list websites that get thousands of votes per day could possibly have have issues with vote notifications timing out as the server is too busy handling notifications from other (possibly slow communicating) server lists. If deemed necessary, I am willing to rewrite the networking system.

@coelho
Copy link
Author

coelho commented Apr 5, 2014

Vote IDs and Replay Attacks

What is needed to prevent replay attacks is a field, within the payload, that must certainly be unique to the server. Something like a server ID. The Votifier server would then need to cache this ID in a datastore and ensure it always receives the same ID.

Then, we can just use a challenge to prevent against all other replay attack vectors, and remove the "random" and "timestamp" fields.

Networking and Performance

This commit rewrites the networking system to be multithreaded 👍

@blakeman8192
Copy link
Member

Regarding the multi-threaded networking, have you taken any measures to prevent DDoS attacks?

@coelho
Copy link
Author

coelho commented Apr 5, 2014

The only thing I believe to be exploitable is the fact that we spawn a thread per client (as OIO normally does anyway). This is not an issue inherently (minecraft until 1.7 did the same approach), but it is something to be noted.

@blakeman8192
Copy link
Member

You can write a program in 10 lines of code that will send thousands of connections per second to a server. Unless connection limits are imposed, one can exhaust all available heap space and crash the server within seconds or minutes by simply spamming connections on the Votifier port. With a 64-bit Java installation running on the Linux JVM implementation, threads are allocated with a 1024kb stack size.

I recommend checking the host of an accepted Socket before spawning a thread to handle I/O, and if there is already a connection from the same host, close the new one immediately without spawning a thread. It may be desirable to allow 2-3 connections from one server list at a time.

I also recommend imposing a maximum connection/thread count for Votifier. The above solution will halt a denial of service attack, but not a distributed denial of service attack. The first versions of Votifier didn't have this problem - it handled only one connection at a time - but now that we will be handling multiple connections simultaneously we need to take measures to protect the Minecraft server from DDoS attacks. A maximum thread/connection count can be enforced inherently by implementing a fixed-size (perhaps 10 threads) thread pool for I/O handling, and that is the approach I would take to solve the issue.

@dalbothek
Copy link

Why would you even create a new thread for each connection? Minecraft already uses netty for its own networking and I don't see a reason why you shouldn't do the same thing.

@coelho
Copy link
Author

coelho commented Apr 7, 2014

@sadimusi

We're then required to add netty to our dependencies and then pack it in our jar (an extra 2MB). We can not use net.minecraft.server's netty because it is unstable.

To be noted:

  • OIO is to be used in net as thread per connection.
  • Only 1.7 uses Netty.
  • Votifier is made to support all Bukkit versions.
  • Votifier originally used OIO for the reasons above. Thread per connection was added in this commit to prevent attacks against the Votifier server. (It is trivial to take down a Votifier server in existing builds)

Although the new thread per connection is only in theory exploitable, while in practice being immune, the choice has already been made to port this to netty.

I'm just finding some time to do it.

@blakeman8192
Copy link
Member

Votifier originally used blocking I/O because it kept the codebase small and simple. It was never conceived that Votifier would need to handle multiple connections simultaneously, thus it was built to only handle one at a time. This had the added benefit of a small memory footprint and CPU usage, and protected the rest of the JVM from a DDoS attack on the Votifier server.

Your modifications allow literally anyone with a computer to spawn limitless threads on the target JVM. This leaves the entire Minecraft server susceptible to attack. While modern Java is fast, it is still extremely expensive to spawn and destroy thousands of native threads per second, without taking into account memory usage for stack allocations. It is possible to exploit this in the real world, not just theory - especially with a botnet.

Even with Netty, I see absolutely no reason for the Votifier server needing to accept hundreds or thousands of connections. Care must be taken to protect the Minecraft server, nobody will use a plugin that makes it easy to take their server offline.

@coelho
Copy link
Author

coelho commented Apr 8, 2014

@blakeman8192

Might I note that until 1.7 Minecraft spawned up to 5 threads per connection, and not a single Minecraft server I knew (with connection-throttle set to zero) was ever hit by an attack taking advantage of this. Although it was theoretically possible, in practice it never happened.

You will hit the TCP port limit before you kill the Java process. In fact, you will hit kernel limits. This is just beyond the definition of unlikely and you are blowing it out of proportion. If you have a botnet of this capability, you might as well throw UDP at it and achieve the same result. It will go down.

If you want to use OIO, this is the way to implement it. Otherwise I will change it to Netty like you asked (although I believe it is entirely unnecessary and this is an issue blown out of proportion). A connection throttle is not the solution. It would band-aid for our otherwise broken implementation.

Is unavailability of the Votifier service not an issue for you? Do you realize the plugin is completely 100% broken if someone can render it useless with a single connection? We need to get our priorities straight here. If you have a solution, be my guest, but don't put the blame on me when you don't.

@blakeman8192
Copy link
Member

At the very minimum, an attack will cause CPU usage to spike as native threads are continually spawned and destroyed. Context switching between thousands of threads (even if the file descriptor ulimit is reached) will also cause significant overhead at that point. These operations are expensive and will very likely result in lagging the game significantly if not making it unplayable.

Furthermore, allowing Votifier to reach the file descriptor ulimit will also prevent new players from logging into the server. Nobody wants to use a plugin that leaves their server vulnerable like this. I have been developing and running game servers (both emulators and original titles) for the greater half of a decade now and I have seen attacks happen. I would rather have the Votifier service be easy to take down as opposed to leaving the entire Minecraft server wide open to unthrottled attacks.

Networking stability and security is a basic requirement of a server program. It shouldn't have needed to be brought up in the first place, as this problem should've been solved in your commit when you switched it to thread-per-client. You simply cannot give everyone the power to spawn and destroy thousands of native threads per second in your process without limits. I specialize in server engineering and I will not approve a pull request for public production software unless it meets my standards.

@coelho
Copy link
Author

coelho commented Apr 8, 2014

All I see in your post is blaming me for the issue and reiterating your points. You do not give a solution. You do not even suggest a solution.

I am a server engineer as well and I manage servers in a production environment that receive over 20,000 concurrent connections and 10gbit/s throughput per machine on a daily basis. I am very aware of the caveats within using an OIO model, however as previously stated: it is not a huge issue.

Hitting the file descriptor ulimit is not an application issue. This can happen with any application of any type very easily, including even the most well-made like NGINX.

I would also like to note that Apache until lately used a process per connection model (similar to thread per connection). Through this, there was an attack called Slowloris, which was avoided by applying a timeout to the duration of the request instead of a solid read timeout. Apache is used still on more than 50% of the internet, and it wouldn't surprise me if it was something even more absurd like 75%.

I would much rather work on production proven concepts like the one stated above. This is not an issue, and the same fix Apache had above can be applied to our existing codebase without rewriting to netty. If need be, however, as I've already said I will rewrite to netty. You do not need to continue a baseless argument.

Might I also add that there are huge gaping vulnerabilities in regards to this commit's security yet instead of addressing those (they aren't important anyway, there was no issue with ProtocolV1 which was completely 100% insecure, and had a single thread for accepting and handling connections that could be exploited to render the Votifier server useless), you choose to discuss a single networking issue which I previously addressed a fix for, multiple times.

@dalbothek
Copy link

I'd just like to note that Minecraft's model of spawning one or more threads per connection did indeed cause some issues. Mojang addressed this by adding a 5 second timeout for new connections coming from the same IP address, which didn't solve the problem completely but at least made it harder to exploit.

That being said, using async I/O doesn't provide a huge (if any) performance gain for a small number of connections and I therefore wouldn't object to a thread based model. Since the service isn't public facing you can easily enforce a maximum connection count by rejecting superfluous connections. The server lists should retry later in that case.

I think we should be focusing more on the protocol itself rather than the way it's implemented here, as this affects a broader audience and can't easily be changed later.

@coelho
Copy link
Author

coelho commented Apr 29, 2014

I'll have more time to work on this soon. Sorry for the delay :)

@socram8888
Copy link

I had also commented about this one to an admin of these pages where you can vote. Here's the message I sent him:

Hello.

I've been using Votifier for a month or so, and I've notified that it causes a severe slowdown when it has to decode the RSA-encrypted message. Therefore, I've been thinking in programming another voting protocol.

Rather than using a RSA, it will use a plain unencrypted protocol, that will be, nevertheless, more secure, and much faster to create and parse, than the old Votifier. How? Well, signing packets using HMAC-SHA1 instead.

The voting page will first connect to the server, and it will return 4 random bytes. The voting page will append those use 4 bytes to another 4 bytes that the user will have previously entered in the voting page (a pre-shared key (PSK), generated using a cryptographycally-strong RNG, the same as the current RSA public key but smaller and faster to create), and will use it to sign a packet which will contain the voting page ID (a 4 bytes random "password" that will be part of the PSK, which the server can use to use a per-page keying) and the user who voted.

This schema offers higher security: a MitM hacker can see the packet's contents, but he will not be able to replicate or to modify it because of its signature. Also, even if a voting page is hacked, because of the per-server keying, it will not have impact in any other web site, and the admin will not have to change all the keys.

Using HMAC-SHA1 means faster processing both for you and for me (HMAC is much faster than asynchronus encryption), and also you would have to store smaller keys.

If you're interested in implementing this voting system in your website, I'll code the plugin, and I will release it under a open-source MIT license, compatible with current Votifier listeners.

Answer:

Hello,

sorry about my late answer. August was a very busy month.

Your project seems interesting. Have you already started working on it?

Before deciding if I will implement this voting system on my servers list, I need to test it. Can you please provide me a working example in PHP?

Code:

<?php

function send_vote($serverIP, $serverPort, $serverKey, $voterName) {
    $socket = fsockopen($serverIP, $serverPort, $errno, $errstr, 5);
    if ($socket === false) {
        die("Unable to connect: " . $errstr);
    };

    // Read 4 bytes nonce. This is a random number generated by the server to prevent duplicated voted.
    $keyNonce = fread($socket, 4);
    // If the connection died before reading the nonce, quit
    if (strlen($keyNonce) != 4) {
        @fclose($socket);
        die("Unable to read nonce");
    };

    // Build packet. A packet is formed by a variable number of TLV fields:
    // 4 bytes identifying the type, an unsigned byte with the field size, and then the payload.
    // PAGE field: first 4 bytes of server key, used to identify the voting page and use the appropiate MAC key.
    $packet = "PAGE\x04" . substr($serverKey, 0, 4);
    // USER field: who voted, in ASCII or UTF8 (UTF8 is mostly backwards-compatible, and as Minecraft nicks
    // only use the basic ASCII charset, you can send it as you please
    $packet .= "USER" . chr(strlen($voterName)) . $voterName;

    // You can add custom fields like TIME which will could be retrieved by plugins in the server
    $packet .= "TIME\x04" . pack("N", time_utc());

    // End of TLV fields (0x00 0x00 0x00 0x00)
    $packet .= "\x00\x00\x00\x00";

    // Append a Message Authentication Code: SHA1 hash of last 16 bytes of server key + nonce + raw packet
    $packet .= sha1(substr($serverKey, 4, 16) . $keyNonce . $packet, true);

    // Send packet
    if (!fwrite($socket, $packet)) {
        @fclose($socket);
        die("Unable to send packet");
    };
    @fclose($socket);
};

function time_utc() {
    $tz = date_default_timezone_get();
    date_default_timezone_set("UTC");
    $time = time();
    date_default_timezone_set($tz);
    return $time;
};

$serverkey = base64_decode("7S4GL5wmQfHITqsgL8sTIycw9D94gq")
send_vote("minecraft.example.com", 25566, $serverkey, "socram8888");

?>

Example packet with the code given above and nonce "1234" (0x31323334):

0000000: 5041 4745 04ed 2e06 2f55 5345 520a 736f  PAGE..../USER.so
0000010: 6372 616d 3838 3838 5449 4d45 0452 205b  cram8888TIME.R [
0000020: af00 0000 00d9 a535 1c55 34a3 6cf0 ae1b  .......5.U4.l...
0000030: f154 5827 64e1 288a 07                   .TX'd.(..

In order to break this protocol, the attacker would have to sniff the connection to grab a packet, and then bruteforce the server key.

@coelho
Copy link
Author

coelho commented May 6, 2014

I have removed the "random" token and "timestamp" 10 minute invalidation in favor of a "challenge" token sent by the server at the start of the connection.

The last known issue in the protocol is in the "Known Issues" section.

@socram8888
I see a few issues with your proposed change:

  1. It is not backwards compatible with the old Votifier protocol
  2. It is not humanly readable (this could have been your intention while making it, but it's not really necessary to make this a binary protocol when so little data is being sent)
  3. The weight is on the server owner and the voting website to protect the secret keys. The implementation above only expects the owner of the voting website to be responsible for a single key. If this key is compromised, he can swap it out and continue operations instantly.

RSA is slow!

I also see your reasons that this protocol may be impractical due to RSA being slow, but that is actually incorrect. RSA encryption is fast (0.2ms) while decryption is slow (6ms). The same goes for the fact RSA verification is fast (0.2ms) while signing is slow (6ms).

This would mean that it is impossible to exploit the fact of RSA being slow in this implementation scenario and thus it is a non-issue.

The implementation above is insecure!

Although there are some security issues in it (shown in the "Known Issues") section, they are actually not very bad as a server list owner can protect against them. Even so, there are proposed changes to make it impossible to exploit in the first place.

The idea of changing from RSA asymmetric encryption to HMAC symmetric

The way the above implementation was planned out, it was made to work perfectly with RSA. If we were to change to HMAC, it would be insecure, and the entire concept would have to be rethought. We would likely run into the same issues your implementation has.

I have a little bit more time lately so I'm going to finish up the protocol and then think about switching this codebase over to Netty.

Cheers.

@socram8888
Copy link

  1. I don't think fetching the host's running version of Votifier and based on that send something or something else is being backwards compatible. There is no reason my suggestion could not be made backwards compatible if you add that mechanism too. I've left it out for simplicity.

  2. I don't see the point of using a verbose language description like JSON. This is not the kind of data an admin would be handling, nor the data the average plugin programmer would need to manage either. This is going to be generated by machines, and processed by machines. And machines work better with binary strings.

  3. Sharing a key, private or public, with several web sites is not a good idea by any means. If a website gets hacked, your server is totally vulnerable to fake votes, and you will be totally clueless on which site was hacked. You could of course roll new keys, but how do you know the hacker is not going to get them again? By using a per-server keying mechanism you can easily see the key ID and reject it, without having to roll new keys everywhere.

  4. RSA is slower than a HMAC. That's a fact. 6ms in highly optimised assembly like OpenSSL aren't 6ms in a crowded Java-based single-threaded server like Minecraft. Even web browsers attempt to avoid the RSA phase of SSL as much as possible by reusing shared keys.

@coelho
Copy link
Author

coelho commented May 9, 2014

@socram8888

  1. Even so, it is important to decide how backwards compatibility is to be done if you wish to propose an alternative.

  2. You can say that about any API over HTTP, but it doesn't mean it is true. JSON allows developers to more easily hook onto the protocol from different programming languages consistently. If you want the protocol to be optimized in terms of data size, we can switch from JSON to MsgPack (but JSON is more ubiquitous, so that'd be pointless). On top of this, machines handle strings the exact same as any other data, and the performance difference is hardly worth mentioning.

  3. With the current Votifier, if a voting website is compromised, the keys associated with all users are compromised. With your method, if a voting website is compromised, the secret keys associated with all users are compromsied. With the proposed method above, if a voting website is compromised, the voting website's private key is compromised, and he can simply swap it out and continue operations immediately. All servers using this voting website will automatically update their public key with no intervention by the server owner.

  4. RSA signature signing along with a temporary key is used in browsers to accomplish "perfect forward secrecy," and in fact is not an attempt to avoid RSA. Although some people have decided ECC is better than RSA, there is no reason behind it as it is all theoretical. ECC is not a good algorithm to use in this as it's verification times are slow. Please read up on cryptography as you show a lack of knowledge with the points you bring.

I would also like to note, that we are running all of Votifiers operations on different threads than the main thread in the Minecraft server. So, your argument about Minecraft being single threaded is invalid, and it even further proves why RSA's speed is irrelevant.

It seems you have not even taken the opportunity to glance at the proposed protocol above from what you are saying. Please take the time to do so.

@socram8888
Copy link

  1. I don't get the point of implementing a feature of I am not sure the protocol is going to be actually used. Programming the implementation, then argueuing about is effectiveness is IMHO quite inefficient. I'd rather discuss the protocol and then implement it after it has been accepted. It's how most RFCs are made.

  2. About the secret key: when a website gets compromised all the server keys are compromised, yes. But they are only used on a single server, so you can view the log and see that server key A is being used to forge rogue votes, so you can revoke the key. I've not seen the mechanism of changing keys that you've mentioned. How are you going to validate it? Using the same private key that had been compromised?

  3. I know perfectly what I am talking about. When you connect to a website, protected by ssl, the server gives it a transaction ID, along with a private key (much like my proposed protocol). Web browsers reuse these from a connection pool in order to avoid the costly computation of RSA signatures used in the handshake process: http://vincent.bernat.im/en/blog/2011-ssl-session-reuse-rfc5077.html

If you still want to go the asymmetric way, you should switch to another like Curve25519, used by another well-known project (DNSCurve), which is faster (at least by an order of 4 in my tests, with an average of 1.07ms using reference implementation with no optimizations vs 4.31ms using OpenSSL for 1024 bit keys - which are considered insecure by the way)

/**
* The service.
*/
public String service;

Choose a reason for hiding this comment

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

These should be private

Copy link
Author

Choose a reason for hiding this comment

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

I do not believe GSON will be able to access the fields if it is private.

Choose a reason for hiding this comment

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

It uses reflection. It can access them perfectly.

Copy link
Author

Choose a reason for hiding this comment

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

Just because it uses reflection doesn't mean it doesn't respect the privacy of variables. Have you tested to ensure that it works?

EDIT: Seems it does. I'll push a commit to fix this in a bit.

Choose a reason for hiding this comment

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

AFAIK the only variables it doesn't serialize are the one marked with "volatile", much like Java's own serialization mechanism.

@coelho
Copy link
Author

coelho commented May 11, 2014

@socram8888

  1. Fair enough.

  2. If you look at the proposed example above, it is the minecraft server's responsibility to retrieve the public key from the voting website over an HTTPS connection.

  3. To a certain extent, but the main reason for this is perfect forward secrecy, the idea where in the future if the main RSA key pair is compromised, the AES shared secret can not be compromised.

We are using RSA keys where the key length is defined by the owner of the voting website. The default in the generation script I created is 2048-bit. To be noted: VotifierV1 uses 2048-bit keys as well, so I don't understand where the idea of 1024-bit keys came from.

@socram8888
Copy link

  1. Good idea. That's definately not possible with an HMAC.

I was just noting that 1024-bit keys are already slower than the full-fledged Curve25519.

@coelho
Copy link
Author

coelho commented May 11, 2014

@socram8888

It might be a good idea to look into Curve25519, but we would require another dependency I believe (BouncyCastle) unless it is built into Java and I'm wrong. The verification times of RSA should be extremely fast (less than 1ms) while the signature signing will be done by the voting website (if PHP, using OpenSSL for less than 6ms).

This normally shouldn't be an issue, but it might become one.

@socram8888
Copy link

I was just suggesting the usage of Curve25519 because CurveDNS was using it also, as it is faster and lighter than RSA, while retaining an equivalent level of security. Most smartcards are phasing out RSA in favor of ECC for the same reasons. Minecraft uses BouncyCastle but apparently it is private.

Apparently there is a single-file implementation of Curve25519 in https://code.google.com/p/curve25519-java/, but I have not personally tested it. Also the largest perfomance hit would be in the voting server, which are mostly based on PHP whose OpenSSL does not support yet custom ECC curves, so it would have to be made in pure PHP (ugh)

PD: I don't know why I said the single-threading of Minecraft would affect the perfomance. I even submitted a PR some months ago which set the listener thread priority lower (#60)...

@dalbothek
Copy link

I'd advise against using anything other than RSA or HMAC for the simple reason that these two are already built into virtually every language, making it easier to implement your protocol. I also feel like you're trying to optimize something which will have little to no impact on the overall server performance as even the biggest minecraft server networks get less than one vote every five seconds.

@dariusc93
Copy link

How stable is this commit @coelho ? I have you did any testing yourself to insure its stability and usage?

@blakeman8192
Copy link
Member

@dariusc93 These changes still leave the Votifier server open to DoS and DDoS attacks. This is the sole reason why I have not merged it into the main repository. No server lists (that I know of) currently support the new protocol and I do not recommend that you use it until the vulnerability is fixed.

@dariusc93
Copy link

@blakeman8192 Thanks for letting me know.

@@ -20,7 +20,7 @@
<!-- Bukkit Dependency -->
<dependency>
<groupId>org.bukkit</groupId>
<artifactId>bukkit</artifactId>
<artifactId>craftbukkit</artifactId>

Choose a reason for hiding this comment

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

You should bundle your own IOUtils rather than relying on CraftBukkit.

@dalbothek
Copy link

@blakeman8192 I run one of the biggest Minecraft server lists out there and would very much welcome a change in protocol. So far @coelho's protocol proposition looks pretty good to me, however I have not looked through his implementation thoroughly enough to judge if it's ready to be merged. I can assure you that I will integrate the new protocol into our service as soon as possible after it's been released.

@coelho To me it looks like the protocol we discussed in this thread and your implementation are not entirely consistent. For one, I can't find the success message anywhere in your code.

@coelho
Copy link
Author

coelho commented Jun 26, 2014

@sadimusi

Unfortunately I never had the chance to fully complete my implementation of the new protocol. The known issues that need fixing are currently in the OP.

@mibby
Copy link

mibby commented Jul 3, 2014

So I compiled your branch @coelho for testing of course, and noticed this error in console.
http://paste.ubuntu.com/7743993/

In the config.yml;

enable_old_protocol: true

Anything to worry about?

@coelho
Copy link
Author

coelho commented Jul 3, 2014

It may be that the vote for ProtocolV2 that you are sending to the server is not valid JSON.

@mibby
Copy link

mibby commented Jul 4, 2014

I haven't switched anything over to the V2 protocol yet though. Still using the v1 legacy settings, only have the updated plugin installed.

@coelho
Copy link
Author

coelho commented Jul 4, 2014

There may be a very rare condition where the first character of the RSA payload in the old protocol is a "{", thus making it think that it is the new protocol.

I believe this can be fixed or rather better handled by searching for the first two characters instead, which should be "{""

@socram8888
Copy link

IMHO this should be catched using a try-catch: instead of looking at any characters, just attempt to parse it as an V2 packet. Should it fail, then attempt to parse it as an V1 packet.

@coelho
Copy link
Author

coelho commented Jul 4, 2014

Sounds good.

@mibby
Copy link

mibby commented Jul 6, 2014

Going to downgrade back to official repository branch because of the errors. Best of luck with development!

@blakeman8192
Copy link
Member

I'm going to close this as we've seen no signs of development over the past year. Submit a new pull request when the requested changes have been made and you want new protocol support! Thank you for your efforts. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants