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

Don’t allow other users to connect by default #28

Merged
merged 1 commit into from
Jun 16, 2020

Conversation

Demi-Marie
Copy link
Contributor

This is the most secure default configuration, and prevents local
privilege escalation vulnerabilities. On Unix, permissions are set to
0o600, while on Windows, the DACL of the process’s token is used.

This is the most secure default configuration, and prevents local
privilege escalation vulnerabilities. On Unix, permissions are set to
0o600, while on Windows, the DACL of the process’s token is used.
@dlon dlon mentioned this pull request Jun 15, 2020
@NikVolf
Copy link
Contributor

NikVolf commented Jun 15, 2020

Thanks @demimarie-parity

I am not sure this should be default.

Have you researched how other libraries are organised and what users expect to be default (deny or allow all)?

@Demi-Marie
Copy link
Contributor Author

Demi-Marie commented Jun 15, 2020

@NikVolf Personally, I expect the default to be to deny access. Furthermore, overly strict permissions will be caught during testing, while overly loose permissions will not unless explicitly checked for.

@Demi-Marie
Copy link
Contributor Author

@NikVolf I looked at permissions and found:

  • Connecting to an AF_UNIX socket requires write access to the socket. Since default umasks deny write access to other users, this is safe. It also means that using 0o600 permissions does not change anything in normal situations.
  • On Windows, default permissions for a named pipe give read access to everyone, and full control to the creator of the pipe, LocalSystem, and administrators. LocalSystem and Administrators can do anything, so that is fine. Read-only access is useless in most cases.

@NikVolf NikVolf merged commit 96c9bf6 into paritytech:master Jun 16, 2020
@Demi-Marie Demi-Marie deleted the strict-ipc-permissions branch June 22, 2020 00:25
@dlon
Copy link

dlon commented Sep 24, 2020

Could you release a new version including these changes on crates.io? It will allow us to get rid of some workarounds.

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.

3 participants