-
-
Notifications
You must be signed in to change notification settings - Fork 239
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 -
const pubKey = mcPubKeyToPem(packet.publicKey) |
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 |
As of the release of Deno 2.0, Deno now has full backwards compatibility with npm packages. I've verified that this package (and thus the packages that depend on it like mineflayer) does indeed work with Deno 2.0. So I think we should finally close this PR now. |
As of Deno 2.0.4, the issue arises yet again: $ deno --version
deno 2.0.4 (stable, release, x86_64-pc-windows-msvc)
v8 12.9.202.13-rusty
typescript 5.6.2
$ deno task dev
Task dev deno run -A --watch src/main.ts
Watcher Process started.
error: Uncaught Error: ASN.1 error: PEM error: PEM Base64 error: invalid Base64 encoding Update: I downgraded to previous Mineflayer and Minecraft versions (down to 1.12.2!), and they all showed the same error. I believe this PR should be reopened. |
@interrrp can you send your Deno code? I just tried it with Deno 2.0.4 on Minecraft 1.20.4 and it was able to join my local server just fine. Here is my code: import { createBot } from 'npm:mineflayer'
const bot = createBot({
host: "localhost",
port: 25565,
username: 'TestBot',
});
bot.on('chat', (username, message) => {
if (username === bot.username) return
bot.chat(message)
}) |
It's a bit complicated, mine was loading from an external file. However, I tried your code, and it still did not work under the same Deno and Minecraft versions. |
Yeah honestly not sure what the problem is. I tried running it as a .ts file, using watcher mode instead of running directly, and it still works for me. All I can think of is that you aren't on Deno 2.0 but you apparently are. Well, in any case, I think your PR is a good change and would probably fix this. |
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.