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

Cookie data #229

Open
wants to merge 35 commits into
base: stable
Choose a base branch
from
Open

Cookie data #229

wants to merge 35 commits into from

Conversation

ismaileke
Copy link

Im not completely sure of the code i wrote. Please add/subtract and report my mistakes. We need to update this

@ismaileke ismaileke changed the title Cookie issue Cookie data Jul 28, 2024
@ismaileke
Copy link
Author

ismaileke commented Jul 30, 2024

I think an Exception should be added to Cookie.php line 46 and i want to add the serverHasSecurity variable to pocketmine.yml or server.properties

@ismaileke
Copy link
Author

What to do in Cookie.php line 51

Copy link
Member

@ShockedPlot7560 ShockedPlot7560 left a comment

Choose a reason for hiding this comment

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

All cookie storage and verification logic must be placed in the server instance in order to eliminate all static access.

In addition, the lib must include a way for implementation to enable or disable all cookie logic.

src/protocol/OpenConnectionReply1.php Outdated Show resolved Hide resolved
src/protocol/OpenConnectionRequest2.php Outdated Show resolved Hide resolved
src/protocol/OpenConnectionRequest2.php Outdated Show resolved Hide resolved
@SOF3
Copy link
Member

SOF3 commented Aug 7, 2024

also this seems to be a memory leak if you never clean up the data. but this should be handled by the server instead; RakLib implements the protocol, not the server to handle the protocol.

src/utils/Cookie.php Outdated Show resolved Hide resolved
Co-authored-by: Jonathan Chan Kwan Yin <sofe2038@gmail.com>
@ismaileke
Copy link
Author

ismaileke commented Aug 10, 2024

image

They say ServerAddress is 7 bytes but look at this

MessageIdentifiers.php
image

remoteBindingAddress(6 bytes)

Co-authored-by: Jonathan Chan Kwan Yin <sofe2038@gmail.com>
@ShockedPlot7560
Copy link
Member

To avoid having to store all the cookies, I was wondering if it wasn't possible to use a predictable string based on the address and a server secret generated at start-up?

Smth like a hash but for int ?

@@ -46,4 +46,4 @@ protected function writeMagic(BinaryStream $out){
public function isValid() : bool{
return $this->magic === self::MAGIC;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated change

Copy link
Member

Choose a reason for hiding this comment

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

fwiw, by linux and git conventions, text files should have a trailing newline.

Comment on lines 106 to 107
// If you don't want to use security on the server, just delete this line.
$this->cookie = new Cookie();
Copy link
Member

Choose a reason for hiding this comment

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

Users of the library don't want to change the library itself to change a behavior, a switch in the constructor need to be created

src/utils/Cookie.php Outdated Show resolved Hide resolved
@dktapps
Copy link
Member

dktapps commented Aug 10, 2024

To avoid having to store all the cookies, I was wondering if it wasn't possible to use a predictable string based on the address and a server secret generated at start-up?

Smth like a hash but for int ?

I previously considered this. I don't think a static secret for the whole server runtime is wise (attackers could collect cookies and reuse them), but we could have the secret periodically rotated (similar to how GS4 Query operates in PM).

A mechanism like that might be problematic for proxies, though, since they don't see the real IP of the client.

src/utils/Cookie.php Outdated Show resolved Hide resolved
@ismaileke ismaileke closed this Aug 13, 2024
@SOF3
Copy link
Member

SOF3 commented Aug 14, 2024

@dktapps why would proxies be a problem? unless some packets are not sent through the proxy

@dktapps
Copy link
Member

dktapps commented Aug 14, 2024

because proxy sees only 1 IP address for all clients

@dktapps why would proxies be a problem? unless some packets are not sent through the proxy

@dktapps
Copy link
Member

dktapps commented Aug 14, 2024

Why is this PR closed?

@SOF3
Copy link
Member

SOF3 commented Aug 14, 2024

because proxy sees only 1 IP address for all clients

@dktapps why would proxies be a problem? unless some packets are not sent through the proxy

I'd say it's the responsibility of the proxy to override the cookie field.

@ismaileke
Copy link
Author

Why is this PR closed?

I couldn't find any solution.

@ismaileke ismaileke reopened this Sep 20, 2024
@ismaileke
Copy link
Author

it is better for security that the cookie feature is always on

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.

4 participants