-
Notifications
You must be signed in to change notification settings - Fork 114
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
Conversation
Generate a key pair in PHP (outputs a file "private.config.php" and "public.key") Example PHP Implementation of the VotifierV2 Protocol |
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 JSONThe 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). TimestampI'm not entirely sure what the benefit of this timestamp is, it might be a source of unnecessary issues. Random numberThis 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 serversWhen 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 communicationIs there a reason votifier doesn't use JSON messages to communicate its version? Feedback after successful voteCurrently 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 listsTo 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. |
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:
and on success:
Default server lists 👍 We'd need server lists to comment and tell us where their public key will be on their website however. |
Vote IDs and Replay Attacks Server-to-server Relay Attacks Networking and Performance |
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 👍 |
Regarding the multi-threaded networking, have you taken any measures to prevent DDoS attacks? |
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. |
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 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. |
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. |
@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:
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. |
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. |
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. |
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. |
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. |
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. |
I'll have more time to work on this soon. Sorry for the delay :) |
I had also commented about this one to an admin of these pages where you can vote. Here's the message I sent him:
Answer:
Code:
Example packet with the code given above and nonce "1234" (0x31323334):
In order to break this protocol, the attacker would have to sniff the connection to grab a packet, and then bruteforce the server key. |
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
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. |
|
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. |
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be private
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
I was just noting that 1024-bit keys are already slower than the full-fledged Curve25519. |
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. |
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)... |
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. |
How stable is this commit @coelho ? I have you did any testing yourself to insure its stability and usage? |
@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. |
@blakeman8192 Thanks for letting me know. |
@@ -20,7 +20,7 @@ | |||
<!-- Bukkit Dependency --> | |||
<dependency> | |||
<groupId>org.bukkit</groupId> | |||
<artifactId>bukkit</artifactId> | |||
<artifactId>craftbukkit</artifactId> |
There was a problem hiding this comment.
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.
@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. |
@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. |
So I compiled your branch @coelho for testing of course, and noticed this error in console. In the config.yml;
Anything to worry about? |
It may be that the vote for ProtocolV2 that you are sending to the server is not valid JSON. |
I haven't switched anything over to the V2 protocol yet though. Still using the v1 legacy settings, only have the updated plugin installed. |
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 "{"" |
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. |
Sounds good. |
Going to downgrade back to official repository branch because of the errors. Best of luck with development! |
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. :) |
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:
ProtocolV2 uses JSON for messages. An example message is below:
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:
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:
Cons:
Known Issues:
Thanks @blakeman8192 for helping me out with this 👍