You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by GitBox <gi...@apache.org> on 2022/12/07 15:12:33 UTC
[GitHub] [mina-sshd] rmischke-dlr opened a new issue, #282: The code for restricting Windows host key file ACLs can result in empty permissions
rmischke-dlr opened a new issue, #282:
URL: https://github.com/apache/mina-sshd/issues/282
### Version
2.9.2
### Bug description
For SSHD 2.9.2, new code was added to restrict the host key file's permissions to the current user. The relevant code for Windows compares existing ACLs to the new file's owner, and only keeps them if the check in https://github.com/apache/mina-sshd/blob/1ccde6cdfe72adf13ef9dd49138434a74aabd784/sshd-common/src/main/java/org/apache/sshd/server/keyprovider/AbstractGeneratorHostKeyProvider.java#L304 is `true`.
The expectation seems to be that the new file's UserPrincipal ("owner") always has a matching `ALLOW` ACL, so the owner would be the only one left with ALLOW permissions. However, we have encountered systems where this check is always `false`, resulting in a host key file that can not be written to, and only deleted with admin permissions.
We have written a small test program to compare the ACLs present on these systems; output will be attached below.
### Actual behavior
The code results in a host key file that has no permissions when viewed in Windows Explorer's file properties, and can only be deleted with admin permissions.
### Expected behavior
The code results in a host key file that is read/writable for the current user.
### Relevant log output
```Shell
Example of two FAILING Windows 10 systems' ACLs; user names are placeholders, and the boolean output is the result of owner.equals(acl.principal()):
---
Owner: DLR\user_a (User)
ACL:
Principal: VORDEFINIERT\Administratoren (Alias)
Type: ALLOW
Permissions: [EXECUTE, WRITE_ACL, READ_ATTRIBUTES, SYNCHRONIZE, WRITE_DATA, READ_ACL, WRITE_ATTRIBUTES, READ_NAMED_ATTRS, DELETE, WRITE_NAMED_ATTRS, DELETE_CHILD, APPEND_DATA, READ_DATA, WRITE_OWNER]
Owner~Principal: false
ACL:
Principal: NT-AUTORITÄT\SYSTEM (Well-known group)
Type: ALLOW
Permissions: [EXECUTE, WRITE_ACL, READ_ATTRIBUTES, SYNCHRONIZE, WRITE_DATA, READ_ACL, WRITE_ATTRIBUTES, READ_NAMED_ATTRS, DELETE, WRITE_NAMED_ATTRS, DELETE_CHILD, APPEND_DATA, READ_DATA, WRITE_OWNER]
Owner~Principal: false
ACL:
Principal: VORDEFINIERT\Benutzer (Alias)
Type: ALLOW
Permissions: [EXECUTE, READ_ATTRIBUTES, SYNCHRONIZE, READ_ACL, READ_NAMED_ATTRS, READ_DATA]
Owner~Principal: false
ACL:
Principal: NT-AUTORITÄT\Authentifizierte Benutzer (Well-known group)
Type: ALLOW
Permissions: [WRITE_NAMED_ATTRS, EXECUTE, READ_ATTRIBUTES, SYNCHRONIZE, APPEND_DATA, WRITE_DATA, READ_ACL, WRITE_ATTRIBUTES, READ_NAMED_ATTRS, READ_DATA, DELETE]
Owner~Principal: false
---
Example of a SUCCEEDING Windows 10 system:
---
Owner: DLR\user_b (User)
ACL:
Principal: NT-AUTORITÄT\SYSTEM (Well-known group)
Type: ALLOW
Permissions: [WRITE_NAMED_ATTRS, READ_DATA, SYNCHRONIZE, READ_ATTRIBUTES, WRITE_OWNER, DELETE_CHILD, APPEND_DATA, WRITE_ACL, WRITE_ATTRIBUTES, DELETE, WRITE_DATA, READ_NAMED_ATTRS, EXECUTE, READ_ACL]
Owner~Principal: false
ACL:
Principal: VORDEFINIERT\Administratoren (Alias)
Type: ALLOW
Permissions: [WRITE_NAMED_ATTRS, READ_DATA, SYNCHRONIZE, READ_ATTRIBUTES, WRITE_OWNER, DELETE_CHILD, APPEND_DATA, WRITE_ACL, WRITE_ATTRIBUTES, DELETE, WRITE_DATA, READ_NAMED_ATTRS, EXECUTE, READ_ACL]
Owner~Principal: false
ACL:
Principal: DLR\user_b (User)
Type: ALLOW
Permissions: [WRITE_NAMED_ATTRS, READ_DATA, SYNCHRONIZE, READ_ATTRIBUTES, WRITE_OWNER, DELETE_CHILD, APPEND_DATA, WRITE_ACL, WRITE_ATTRIBUTES, DELETE, WRITE_DATA, READ_NAMED_ATTRS, EXECUTE, READ_ACL]
Owner~Principal: true
---
Example of a SUCCEEDING Windows Server system, running as administrator; here, the ownership was assigned to a group, which is also what passes the check, so the whole group would retain access; is this the indended result?
---
Owner: VORDEFINIERT\Administratoren (Alias)
ACL:
Principal: NT-AUTORITÄT\SYSTEM (Well-known group)
Type: ALLOW
Permissions: [DELETE_CHILD, WRITE_DATA, READ_ATTRIBUTES, EXECUTE, READ_ACL, READ_DATA, DELETE, WRITE_OWNER, WRITE_ATTRIBUTES, READ_NAMED_ATTRS, APPEND_DATA, SYNCHRONIZE, WRITE_NAMED_ATTRS, WRITE_ACL]
Owner~Principal: false
ACL:
Principal: VORDEFINIERT\Administratoren (Alias)
Type: ALLOW
Permissions: [DELETE_CHILD, WRITE_DATA, READ_ATTRIBUTES, EXECUTE, READ_ACL, READ_DATA, DELETE, WRITE_OWNER, WRITE_ATTRIBUTES, READ_NAMED_ATTRS, APPEND_DATA, SYNCHRONIZE, WRITE_NAMED_ATTRS, WRITE_ACL]
Owner~Principal: true
ACL:
Principal: WIN-redacted\Administrator (User)
Type: ALLOW
Permissions: [DELETE_CHILD, WRITE_DATA, READ_ATTRIBUTES, EXECUTE, READ_ACL, READ_DATA, DELETE, WRITE_OWNER, WRITE_ATTRIBUTES, READ_NAMED_ATTRS, APPEND_DATA, SYNCHRONIZE, WRITE_NAMED_ATTRS, WRITE_ACL]
Owner~Principal: false
---
```
### Other information
_No response_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf closed issue #282: The code for restricting Windows host key file ACLs can result in empty permissions
Posted by GitBox <gi...@apache.org>.
tomaswolf closed issue #282: The code for restricting Windows host key file ACLs can result in empty permissions
URL: https://github.com/apache/mina-sshd/issues/282
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on issue #282: The code for restricting Windows host key file ACLs can result in empty permissions
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on issue #282:
URL: https://github.com/apache/mina-sshd/issues/282#issuecomment-1345274336
Thanks for reporting back.
I missed that LIST_DIRECTORY == READ_DATA. (Only noticed ADD_FILE and ADD_SUBDIRECTORY.) I don't know if DELETE_CHILD can apply to a file at all; but safest would be to just keep these two in the set.
The idea of setting empty permissions for non-owners instead of just removing them is that I don't know what happens with inherited ACLs (i.e., set on the directory or a parent, and then inherited.) By explicitly setting an empty set that should not matter, and the file really should only be accessible by the owner.
Good idea about making this configurable; I'll add such a setter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on issue #282: The code for restricting Windows host key file ACLs can result in empty permissions
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on issue #282:
URL: https://github.com/apache/mina-sshd/issues/282#issuecomment-1343137681
@rmischke-dlr : how about this:
```
// Remove all access rights from non-owners.
List<AclEntry> restricted = new ArrayList<>();
for (AclEntry acl : view.getAcl()) {
if (owner.equals(acl.principal())) {
// We explicitly give the owner full access permissions below.
continue;
}
if (AclEntryType.ALLOW.equals(acl.type())) {
// We can't use DENY access: if the owner is member of a group and we deny the group
// access, the owner won't be able to perform the access. Instead of denying permissions
// simply allow nothing.
restricted.add(AclEntry.newBuilder()
.setType(AclEntryType.ALLOW)
.setPrincipal(acl.principal())
.setPermissions(Collections.emptySet())
.build());
} else {
// DENY, AUDIT, and ALARM: keep them. The owner has successfully created the file, so any
// existing DENY entries are assumed not to have any influence on reading from or writing to
// this file by the owner.
restricted.add(acl);
}
}
// Ensure the owner has all the necessary rights to work with this file.
Set<AclEntryPermission> neededPermissions = EnumSet.allOf(AclEntryPermission.class);
neededPermissions.remove(AclEntryPermission.DELETE_CHILD);
neededPermissions.remove(AclEntryPermission.LIST_DIRECTORY);
restricted.add(AclEntry.newBuilder()
.setType(AclEntryType.ALLOW)
.setPrincipal(owner)
.setPermissions(neededPermissions)
.build());
view.setAcl(restricted);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] rmischke-dlr commented on issue #282: The code for restricting Windows host key file ACLs can result in empty permissions
Posted by GitBox <gi...@apache.org>.
rmischke-dlr commented on issue #282:
URL: https://github.com/apache/mina-sshd/issues/282#issuecomment-1341118430
This is the test program for reproducing the above output:
```
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.attribute.AclEntry;
import java.nio.file.attribute.AclFileAttributeView;
import java.nio.file.attribute.UserPrincipal;
import java.util.Objects;
public class ACLTest {
public static void main(String[] args) {
Path path = Paths.get("dummy.dat");
if (Files.exists(path)) {
System.err.println("Test file " + path + " already exists - please delete it");
System.exit(1);
}
try {
Files.newOutputStream(path).close();
} catch (IOException e) {
e.printStackTrace();
System.exit(1);
}
try {
AclFileAttributeView view = Files.getFileAttributeView(path, AclFileAttributeView.class);
UserPrincipal owner = Files.getOwner(path);
Objects.requireNonNull(view, "Null AclFileAttributeView");
Objects.requireNonNull(owner, "Null UserPrincipal");
System.out.println("Owner: " + owner);
for (AclEntry acl : view.getAcl()) {
System.out.println("ACL:");
System.out.println(" Principal: " + acl.principal());
System.out.println(" Type: " + acl.type());
System.out.println(" Permissions: " + acl.permissions());
System.out.println(" Owner~Principal: " + owner.equals(acl.principal()));
}
} catch (IOException | SecurityException e) {
e.printStackTrace();
} finally {
try {
Files.delete(path);
} catch (IOException e) {
System.err.println("Failed to delete test file " + path + ": " + e.toString());
System.exit(1);
}
}
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on issue #282: The code for restricting Windows host key file ACLs can result in empty permissions
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on issue #282:
URL: https://github.com/apache/mina-sshd/issues/282#issuecomment-1341351838
The intent was to get the logically same behavior as in Linux: only the owner should have read (and especially write) access.
The failing Win 10 output is a surprise. Surely the safest way to resolve these problems would be to just remove that ACL code again. Apparently these ACLs behave much more strangely than I imagined. (I also didn't encounter this in my testing.) Or perhaps the code should explicitly add an allow entry for the owner if there was none.
The Windows Server example is also surprising. I would not have expected Windows to assign a group there. But I guess one could say that this behavior at least is expected.
You seem to be in a much better position than me to fix this. (At least you seem to have access to different Windows versions and setups.) If you could provide a PR that would be great.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] rmischke-dlr commented on issue #282: The code for restricting Windows host key file ACLs can result in empty permissions
Posted by GitBox <gi...@apache.org>.
rmischke-dlr commented on issue #282:
URL: https://github.com/apache/mina-sshd/issues/282#issuecomment-1345269439
I put that code into the test program, and this is the result after running it on one of the Win10 machines where it previously failed:
```
ACL:
Principal: VORDEFINIERT\Administratoren (Alias)
Type: ALLOW
Permissions: []
Owner~Principal: false
ACL:
Principal: NT-AUTORITÄT\SYSTEM (Well-known group)
Type: ALLOW
Permissions: []
Owner~Principal: false
ACL:
Principal: VORDEFINIERT\Benutzer (Alias)
Type: ALLOW
Permissions: []
Owner~Principal: false
ACL:
Principal: NT-AUTORITÄT\Authentifizierte Benutzer (Well-known group)
Type: ALLOW
Permissions: []
Owner~Principal: false
ACL:
Principal: DLR\user_a (User)
Type: ALLOW
Permissions: [EXECUTE, WRITE_ACL, READ_ATTRIBUTES, SYNCHRONIZE, WRITE_DATA, READ_ACL, WRITE_ATTRIBUTES, READ_NAMED_ATTRS, DELETE, WRITE_NAMED_ATTRS, APPEND_DATA, WRITE_OWNER]
Owner~Principal: true
```
The test program could also delete the file afterwards. But when I disabled the deletion and inspected the file manually, I noticed something odd -- Windows Explorer shows only write permissions for the user, and none for reading:
Apparently, READ_DATA is missing, but from the code, that was unexpected. So I added `neededPermissions.add(AclEntryPermission.READ_DATA);` as a test, and then it worked. The reason for this behavior seems to be the declaration of LIST_DIRECTORY: `Permission to list the entries of a directory (equal to {@link #READ_DATA})`.
I'm not very familiar with Windows ACLs, so I can't say what the ideal set of permissions is. But maybe `LIST_DIRECTORY` is only relevant for directories? Also, is the removed `DELETE_CHILD` relevant for a file at all?
Not a functional problem, but does it make sense to generate empty ALLOW ACLs for all non-matching users/groups in the first place? Why not simply drop/remove them altogether, only keeping the other types and the new single ALLOW rule?
Another thought - maybe it would also be a good idea to add an API-level flag to enable/disable these permission modifications completely. For default use cases, restricting these permissions seems like a good idea, but depending on the application's setup, it may not fit. Something like `.setRestrictHostKeyFilePermissions(bool)`, with a default of "true", maybe?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on issue #282: The code for restricting Windows host key file ACLs can result in empty permissions
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on issue #282:
URL: https://github.com/apache/mina-sshd/issues/282#issuecomment-1345283861
> Why not simply drop/remove them altogether, only keeping the other types and the new single ALLOW rule?
Apparently that also works, even in the presence of inherited permissions. Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org