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 comparison of IPv6 addresses in session_get_bydata() #1187

Open
wants to merge 4 commits into
base: devel
Choose a base branch
from

Conversation

s-k2
Copy link

@s-k2 s-k2 commented Aug 5, 2018

I encountered a bug in sesman when UBI-policy is used and xrdp was compiled with IPv6 enabled (Debian does this for the xrdp-package). In this case sesman does not compare the IP-addresses correctly and reuses sessions from different addresses.

The problem can be located in sesman/session.c, line 144 in master. The client's address and the session-item's address are only compared up to the first colon using g_strncmp_d() to ignore the port-number. This approach works well for IPv4 but not for IPv6.

I attached a quick-and-dirty-hack to solve that problem which compares the addresses up to the last colon. It works with either IPv4 or IPv6 but assumes that a colon followed by a port-number is part of the client_ip-string.

I am not quite sure if session.c is the right place for the two new functions I added, maybe you have a better idea where to put them (or know of a better approach)

@metalefty
Copy link
Member

Good catch.

sesman/session.c Outdated
@@ -89,6 +89,37 @@ dumpItemsToString(struct list *self, char *outstr, int len)
return outstr ;
}

/* find the last occurrence of ch in str limited by n */
static int
last_index_of(const char *str, char ch, int n)
Copy link
Member

Choose a reason for hiding this comment

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

This function should be placed common/ because it has nothing sesman-specific.
It looks general helper function handling string.

@s-k2
Copy link
Author

s-k2 commented Aug 11, 2018

Would os_calls.c be the right place for it?

int last_colon_ip1 = g_last_ch_pos(ip1, ':', n);
int last_colon_ip2 = g_last_ch_pos(ip2, ':', n);

if (last_colon_ip1 != last_colon_ip2)
Copy link
Member

Choose a reason for hiding this comment

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

please always use brackets.

sesman/session.c Outdated
if (last_colon_ip1 != last_colon_ip2)
return 0;

if (last_colon_ip1 == -1)
Copy link
Member

Choose a reason for hiding this comment

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

same here

sesman/session.c Outdated
@@ -89,6 +89,21 @@ dumpItemsToString(struct list *self, char *outstr, int len)
return outstr ;
}

/* compare two strings containing ip-addresses and ports but ignore the latter */
static int
is_equal_ip_ignoring_port(const char *ip1, const char *ip2, size_t n)
Copy link
Member

Choose a reason for hiding this comment

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

const size_t n ?

Copy link
Author

@s-k2 s-k2 Aug 11, 2018

Choose a reason for hiding this comment

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

Ouch, I accidentally used size_t as I always do. But in xrdp's codebase it is hardly used. Should I change it to an signed const int here, too?

@@ -2822,6 +2822,22 @@ g_pos(const char *str, const char *to_find)
return (pp - str);
}

/*****************************************************************************/
int
g_last_ch_pos(const char *str, char ch_to_find, int n)
Copy link
Member

@speidy speidy Aug 11, 2018

Choose a reason for hiding this comment

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

you want strrchr from string.h

then do pointer arithmetics:

p = strrchr(string, ':');
index = (int)(p - string);

Copy link
Author

Choose a reason for hiding this comment

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

No I can't do this. I would not like to trust on the input beeing correctly terminated with \0. Just like the the code I'd like to patch.
Unfortunately the C standard does not know of strnrchr() so I had to write my own...

@metalefty
Copy link
Member

os_calls.c is a possible candidate to put it on. but please don't hesitate to create a new file if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants