fix: wrap base64-encoded PEM with 64-char line boundary #1292
Conversation
According to [RFC7468](https://datatracker.ietf.org/doc/html/rfc7468) > Generators MUST wrap the base64-encoded lines so that each line consists of exactly 64 characters except for the final line, which will encode the remainder of the data (within the 64-character line boundary), and they MUST NOT emit extraneous whitespace. Parsers can avoid branching and prevent timing sidechannel attacks. Ref https://arxiv.org/pdf/2108.04600.pdf Fixes compatibility with Deno as it enforces stricter handling of PEM.
|
how did you check this improve something? |
|
Hi @littledivy if nobody can verify that fix solve some issues, I think we should close this PR |
|
What example were you able to run with deno? |
|
It may be the standard length, but for Minecraft we'd likely want to match the encoding behavior to be like vanilla Minecraft otherwise it's one of the areas that can be easily flagged by server software (like anticheat, antibot, etc). I'm looking into the vanilla behavior here. |
There was a problem hiding this comment.
Ok per https://github.com/extremeheat/extracted_minecraft_data/blob/client1.20.5/client/net/minecraft/util/Crypt.java#L41 it seems minecraft is using 76-char width.
| @@ -223,7 +223,7 @@ module.exports = function (client, server, options) { | |||
| function mcPubKeyToPem (mcPubKeyBuffer) { | |||
| let pem = '-----BEGIN RSA PUBLIC KEY-----\n' | |||
| let base64PubKey = mcPubKeyBuffer.toString('base64') | |||
| const maxLineLength = 76 | |||
| const maxLineLength = 64 | |||
There was a problem hiding this comment.
For server/ we can keep it at 76 (but it probably won't matter whatever the width is as vanilla client doesn't care). It's not going to break Deno or anything as this call result isn't passed to anything except being sent to clients.
| @@ -79,7 +79,7 @@ module.exports = function (client, options) { | |||
| function mcPubKeyToPem (mcPubKeyBuffer) { | |||
| let pem = '-----BEGIN PUBLIC KEY-----\n' | |||
| let base64PubKey = mcPubKeyBuffer.toString('base64') | |||
| const maxLineLength = 65 | |||
| const maxLineLength = 64 | |||
There was a problem hiding this comment.
For the client/ width seems to not matter at all as it's not sent to server or anything but just as an intermediate variable that we pass to encrypt method -
|
I see. Honestly, even though I didn't make this PR, I think it should be closed. The main purpose of this PR was to enable Deno support, but since changing these widths seems to go against the actual Minecraft protocol, and since more work beyond changing these widths would probably need to be done to make it compatible with Deno, it doesn't seem worth it. A full on proper port of this package is probably necessary for anyone wanting to use it with Deno (or better Deno-Node compatibility) which is definitely not in the scope of this project. Thanks @extremeheat for helping to investigate this though. |
|
No, what I said was that only the client PEM encoder method needs to be changed, and that changing it here would have no effect on anything (beyond maybe fixing Deno). The width doesn't matter to nmp as long as it can be read by the crypto call. On the server code there is nothing to change |
According to RFC7468:
Parsers can avoid branching and prevent timing sidechannel attacks. Ref https://arxiv.org/pdf/2108.04600.pdf
Fixes compatibility with Deno as it enforces stricter handling of PEM.