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

Fix permission issues with sesman. #16

Closed
wants to merge 1 commit into from
Closed

Fix permission issues with sesman. #16

wants to merge 1 commit into from

Conversation

Natureshadow
Copy link

Accompanied by a patch to xrdp.

Accompanied by a patch to xrdp.
@@ -1127,7 +1127,7 @@ rdpClientConInit(rdpPtr dev)
return 0;
}
}
g_chmod_hex("/tmp/.xrdp", 0x1777);
g_chmod_hex("/tmp/.xrdp", 0x3777);
Copy link
Contributor

Choose a reason for hiding this comment

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

Anything that touches suid and sgid bits should be documented. Also, it's it highly desirable that such behavior can be configured using configuration files. Creating a directory with a predictable name in a world writable directory was bad already. Creating a sgid directory makes it even worse. What if an attacker replaces /tmp/.xrdp with a link to an executable and exploits this code to get sgid permissions? What if the process group is root or wheel?

@@ -1145,7 +1145,13 @@ rdpClientConInit(rdpPtr dev)
LLOGLN(0, ("rdpClientConInit: g_tcp_local_bind failed"));
return 1;
}
g_sck_listen(dev->listen_sck);
if (g_sck_listen(dev->listen_sck) != 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like an unrelated change. Please submit unrelated changes as separate commits, even if they are in the same pull request.

@metalefty
Copy link
Member

Do not just submit downstream patches to upstream. If a downstream patch contains fixes for more than one issue, please separate the patch and create PRs for each patch.

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.

None yet

3 participants