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

Vncpassfile #497

Closed
wants to merge 7 commits into from

Conversation

Projects
None yet
5 participants
@jsorg71
Copy link
Contributor

commented Nov 19, 2016

This fixes an old VNC security issue were the VNC password file is based on the user password.
Now it is derived from a random GUID that is generated for each session.
This GUID can be used in the future for session reconnect and other things.

@jsorg71

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2016

Let me manually merge this if it all looks good.

@@ -416,6 +416,21 @@ scp_session_set_addr(struct SCP_SESSION *s, int type, const void *addr)
}

/*******************************************************************/
int
scp_session_set_guid(struct SCP_SESSION *s, const tui8 *guid)

This comment has been minimized.

Copy link
@proski

proski Nov 19, 2016

Contributor

If tui8 a useful type? uint8_t is a standard type that is also unsigned 8-bit integer.

This comment has been minimized.

Copy link
@jsorg71

jsorg71 Nov 20, 2016

Author Contributor

Currently we are not using the _t types, only the t types in arch.h

This comment has been minimized.

Copy link
@proski

proski Nov 20, 2016

Contributor

OK, we can change it after the release.

int
scp_session_set_guid(struct SCP_SESSION *s, const tui8 *guid)
{
if (0 == guid)

This comment has been minimized.

Copy link
@proski

proski Nov 19, 2016

Contributor

Let's use NULL for pointers, it's more readable.

This comment has been minimized.

Copy link
@jsorg71

jsorg71 Nov 20, 2016

Author Contributor

I know what you mean here, the NULL means it's a pointer. Currently, zeros are used all over the project. I've never liked that NULL is defined differently sometimes.

This comment has been minimized.

Copy link
@proski

proski Nov 20, 2016

Contributor

NULL is a special value for every variable that can be compared to it (i.e. every pointer). But 0 is not always a special value for a variable that can be compared to it. A variable can be 0 and be used, say, as an index. When I see comparison to 0, I need to examine it more closely. It can be a pointer check, or it can be something completely different.

@@ -61,7 +61,7 @@ scp_v0s_accept(struct SCP_CONNECTION* c, struct SCP_SESSION** s, int skipVchk);
*
*/
enum SCP_SERVER_STATES_E
scp_v0s_allow_connection(struct SCP_CONNECTION* c, SCP_DISPLAY d);
scp_v0s_allow_connection(struct SCP_CONNECTION* c, SCP_DISPLAY d, tui8 *guid);

This comment has been minimized.

Copy link
@proski

proski Nov 19, 2016

Contributor

I believe it should be a constant pointer.

This comment has been minimized.

Copy link
@jsorg71

jsorg71 Nov 20, 2016

Author Contributor

I agree.

@@ -99,6 +99,11 @@ scp_v0_process(struct SCP_CONNECTION *c, struct SCP_SESSION *s)

if (1 == access_login_allowed(s->username))
{
tui8 guid[16];

g_random((char*)guid, 16);

This comment has been minimized.

Copy link
@proski

proski Nov 19, 2016

Contributor

Please use correct spacing.

This comment has been minimized.

Copy link
@jsorg71

jsorg71 Nov 20, 2016

Author Contributor

ok

@@ -410,7 +410,8 @@ wait_for_xserver(int display)
static int APP_CC
session_start_fork(int width, int height, int bpp, char *username,
char *password, tbus data, tui8 type, char *domain,
char *program, char *directory, char *client_ip)
char *program, char *directory, char *client_ip,
const tui8 *guid)

This comment has been minimized.

Copy link
@proski

proski Nov 19, 2016

Contributor

Would not it be easier to send the session pointer instead of so many fields?

This comment has been minimized.

Copy link
@jsorg71

jsorg71 Nov 20, 2016

Author Contributor

I was thinking that too, I'll take a look at it.

vnc/vnc.c Outdated
}

/******************************************************************************/
/* taken from vncauth.c */

This comment has been minimized.

Copy link
@proski

proski Nov 19, 2016

Contributor

Function comments should primarily describe what the function does, not where the code came from.

This comment has been minimized.

Copy link
@jsorg71

jsorg71 Nov 20, 2016

Author Contributor

ok

vnc/vnc.c Outdated
/******************************************************************************/
/* taken from vncauth.c */
void DEFAULT_CC
rfbHashEncryptBytes(char *bytes, char *passwd)

This comment has been minimized.

Copy link
@proski

proski Nov 19, 2016

Contributor

Do both arguments need to be non-constant?

This comment has been minimized.

Copy link
@jsorg71

jsorg71 Nov 20, 2016

Author Contributor

ok

This comment has been minimized.

Copy link
@proski

proski Nov 20, 2016

Contributor

Ouch! This function needs a real comment.

@@ -1195,11 +1199,18 @@ xrdp_mm_process_login_response(struct xrdp_mm *self, struct stream *s)
int rv;
char ip[256];
char port[256];
tui8 guid[16];
tui8* pguid;

This comment has been minimized.

Copy link
@proski

proski Nov 19, 2016

Contributor

Formatting - * sticks to the variable, not to the type.

This comment has been minimized.

Copy link
@jsorg71

jsorg71 Nov 20, 2016

Author Contributor

yup

@@ -1422,6 +1447,11 @@ lib_mod_set_param(struct vnc *v, const char *name, char *value)
{
v->delay_ms = g_atoi(value);
}
else if (g_strcasecmp(name, "guid") == 0)

This comment has been minimized.

Copy link
@proski

proski Nov 19, 2016

Contributor

Why is a user configuration needed? Should it be documented?

This comment has been minimized.

Copy link
@jsorg71

jsorg71 Nov 20, 2016

Author Contributor

This is the only API to give the modules general data. We don't give the modules access to the entire xrdp internals.
It was not meant to be only user data.

This comment has been minimized.

Copy link
@proski

proski Nov 20, 2016

Contributor

I don't understand. Should users configure it? If not, who should configure it? If nobody should configure it, why is it read from the config file?

This comment has been minimized.

Copy link
@speidy

speidy Nov 20, 2016

Contributor

lib_mod_set_param is not only for passing params from config file,
please look @ https://github.com/jsorg71/xrdp/blob/ed862ea361e1a364f153da2bd22c402f529b547e/xrdp/xrdp_mm.c#L568

This is the api for passing params to modules also for internal usages.

This comment has been minimized.

Copy link
@proski

proski Nov 20, 2016

Contributor

Oh, I see it now.

@proski

proski approved these changes Nov 20, 2016

@@ -1422,6 +1447,11 @@ lib_mod_set_param(struct vnc *v, const char *name, char *value)
{
v->delay_ms = g_atoi(value);
}
else if (g_strcasecmp(name, "guid") == 0)

This comment has been minimized.

Copy link
@proski

proski Nov 20, 2016

Contributor

Oh, I see it now.

@metalefty metalefty added this to the v0.9.1 milestone Nov 21, 2016

@proski

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2016

#523 has been merged. I assume this PR can be closed.

@metalefty metalefty closed this Dec 9, 2016

@carnil

This comment has been minimized.

Copy link

commented Dec 11, 2016

Additional note for this pull request.

Originally reported to by Dag-Erling Smørgrav and Petter Reinholdtsen:

When successfully logging in using RDP into a xrdp session, the file
~/.vnc/sesman_${username}_passwd is created. Its content is the
equivalent of the users clear text password, DES encrypted with a known
key.

This issue got assigned back then CVE-2013-1430

@jsorg71 jsorg71 deleted the jsorg71:vncpassfile branch Jul 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.