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

Fixed username and domain separation in getACL #26738

Closed
wants to merge 2 commits into from
Closed

Fixed username and domain separation in getACL #26738

wants to merge 2 commits into from

Conversation

Snafu
Copy link

@Snafu Snafu commented Apr 24, 2021

Inspired by anikrooz commit
1f95f9b

Addresses issue mentioned here: #26457 (comment)

Signed-off-by: Christopher Gabriel stuff@c-gabriel.at

Inspired by anikrooz commit
1f95f9b

Signed-off-by: Christopher Gabriel <stuff@c-gabriel.at>
@Snafu
Copy link
Author

Snafu commented Apr 24, 2021

I want to point out the comment by anikrooz in the commit that inspired my fix and for some reason I did not pay enough attention to:
"Returning a plain username eg. ['', $user] could be a security risk or at least would be incorrect".
I'm not an expert regarding ACL security, if the above statement is true, then the original fix should be implemented, returning an empty username.

Edit: If that is a security issue it could be possible that the following reolves any security concerns:
...
[$workgroup, $user] = $this->splitUser($user); // strip domain
if ($user === $this->server->getAuth()->getUsername() && $workgroup == $this->server->getAuth()->getWorkgroup()) {
...

Edit2: On my server I get the following ACLs from $file->getAcls() on a file in a share I use for tvheadend recordings. I had to add a domain to the external share in nextcloud otherwise I could not mount the share at all, I used the server name 'server'.
I added posixacls on a file for my user. As you can see my user 'snafu' is explicitly stated due to the ACL but the getAcls() call returned the domain/workgroup as uppercase, whereas the IAuth object returns it as lowercase. A case insensitive check on the domain/workgroup should be fine (DNS standard says case insensitive) but on the username I would stick to case sensitive (POSIX standard).

...
[$workgroup, $user] = $this->splitUser($user); // strip domain
if ($user === $this->server->getAuth()->getUsername() && strcasecmp($workgroup, $this->server->getAuth()->getWorkgroup()) === 0) {
...

NextcloudDebug[16501]: ACLS: array (
NextcloudDebug[16501]: 'SERVER\snafu' =>
NextcloudDebug[16501]: Icewind\SMB\ACL::__set_state(array(
NextcloudDebug[16501]: 'type' => 0,
NextcloudDebug[16501]: 'flags' => 0,
NextcloudDebug[16501]: 'mask' => 2032127,
NextcloudDebug[16501]: )),
NextcloudDebug[16501]: 'Unix Group\video' =>
NextcloudDebug[16501]: Icewind\SMB\ACL::__set_state(array(
NextcloudDebug[16501]: 'type' => 0,
NextcloudDebug[16501]: 'flags' => 0,
NextcloudDebug[16501]: 'mask' => 1179817,
NextcloudDebug[16501]: )),
NextcloudDebug[16501]: '\Everyone' =>
NextcloudDebug[16501]: Icewind\SMB\ACL::__set_state(array(
NextcloudDebug[16501]: 'type' => 0,
NextcloudDebug[16501]: 'flags' => 0,
NextcloudDebug[16501]: 'mask' => 1179817,
NextcloudDebug[16501]: )),
NextcloudDebug[16501]: )

@@ -142,7 +142,7 @@ public function __construct($params) {
private function splitUser($user) {
if (strpos($user, '/')) {
Copy link
Author

Choose a reason for hiding this comment

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

I'm not familiar with this ACL user notation containing a slash. Maybe the strpos() check here also has to be changed to strpos($user, '/') !== false

@Snafu Snafu changed the title Fixed getACL to return the username without domain Fixed username and domain separation in getACL Apr 24, 2021
Signed-off-by: Christopher Gabriel <stuff@c-gabriel.at>
@csware
Copy link
Contributor

csware commented Aug 15, 2021

Cross-link: #25540

@skjnldsv skjnldsv added the 2. developing Work in progress label Feb 21, 2024
@skjnldsv
Copy link
Member

@Snafu would you mind rebasing your PR? :)
Then I should be able to review and ask other coworkers to have a look

@skjnldsv skjnldsv added bug stale Ticket or PR with no recent activity labels Feb 27, 2024
@skjnldsv skjnldsv marked this pull request as draft February 27, 2024 17:41
@susnux susnux added this to the Nextcloud 30 milestone Apr 18, 2024
@skjnldsv
Copy link
Member

skjnldsv commented May 2, 2024

Closing this pull request due to lack of recent activity and updates. We appreciate your contribution and encourage you to reopen or provide further updates if necessary. ☺️
Our aim is to keep the project moving forward with active collaboration. Thank you for your understanding and continued support! 🙏

@skjnldsv skjnldsv closed this May 2, 2024
@Snafu
Copy link
Author

Snafu commented May 2, 2024

Hi,
this pull request was no longer relevant because the strpos() calls were refactored to str_contains() in the following commit.

Author: Faraz Samapoor f.samapoor@gmail.com
Date: 02.06.2023 18:24:49
Commit hash: cfb921b

Refactors "strpos" calls in /apps/files_external to improve code readability.

Signed-off-by: Faraz Samapoor f.samapoor@gmail.com

@Snafu Snafu deleted the fix-smb-getacl branch May 2, 2024 15:09
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress bug stale Ticket or PR with no recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants