Add support for RFB protocol version 3.7 and 3.8 #2549
Conversation
Jesse-Bakker
force-pushed
the
vnc-upgrade
branch
from
40ce05c
to
69cb532
Compare
|
@Jesse-Bakker - thanks for this. It's been a busy week here, I'll get a proper review to you probably early next week now. |
There was a problem hiding this comment.
Thanks @Jesse-Bakker
There's a few comments below. Get back to me on anything I haven't explained well, or you disagree with.
| (v[n]) = '\0'; \ | ||
| (s)->p++; \ | ||
| } while (0) | ||
|
|
||
| /******************************************************************************/ |
There was a problem hiding this comment.
This isn't copying the string from the input buffer.
when the macro runs:-
(v) = (s)->p;setserr_reasonto be(s)->p(which points into(s)->data)(v[n]) = '\0';writes a null into the input buffer(s)->p++advances one character along the buffer. This isn't useful, but you're not seeing any bad effects as the macro is only used at the end of the input.
I don't think it's good form to overwrite the input buffer in this way - you should be copying the string. Here's a possibility (not tested):-
#define in_str(s, n, v, vlen) do \
{ \
int copylen = MIN((n), (vlen - 1)); \
in_uint8a((s), (v), copylen); \
(v)[copylen] = '\0'; \
if ((n) > copylen) \
{ \
in_uint8s((s), (n) - copylen); \
} \
} while (0)Use with :-
char err_reason[128];
. . .
in_str(s, i, err_reason, sizeof(err_reason));It's a pretty complex macro however.
| if (g_strncmp(version_str, "RFB 003.00", 10) != 0) | ||
| { | ||
| version_str[11] = '\0'; | ||
| LOG(LOG_LEVEL_ERROR, "Invalid server version string %s", version_str); | ||
| error = 1; | ||
| } | ||
| if (version_str[11] != '\n') |
There was a problem hiding this comment.
Need an else if here.
If the previous test succeeds (invalid version string), the newline will be overwritten with a NUL for error reporting, and then this test will trigger too!
| if (i <= 1024) | ||
| { | ||
| error = trans_force_read_s(v->trans, s, i); | ||
| } |
There was a problem hiding this comment.
Logic problem here. If this line returns 0, the string is read again at line 1796 below.
Also, i is intrather than unsigned int, so the test against 1024 is not sufficient.
Suggest the following, if in_str() is redefined:-
char err_str[128]; // replaces char *err_str;
. . .
// The client chooses the security type
error = trans_force_read_s(v->trans, s, 1);
if (error == 0)
{
in_uint8(s, n_sec_lvls);
if (n_sec_lvls > 0)
{
error = trans_force_read_s(v->trans, s, n_sec_lvls);
}
else
{
// Server has closed the connection
error = 1;
g_snprintf(err_reason, sizeof(err_reason),
"<reason unavailable>");
// Read size of reason
if (trans_force_read_s(v->trans, s, 4) == 0)
{
in_uint32_be(s, i);
if (i > 0 && i < sizeof(err_reason) &&
trans_force_read_s(v->trans, s, i) == 0)
{
in_str(s, i, err_reason, sizeof(err_reason));
}
}
LOG(LOG_LEVEL_ERROR, "Connection closed by server : %s",
err_reason);
}
}NB Lines need reformatting to keep width to 80 chars (see https://github.com/neutrinolabs/xrdp/wiki/Coding-Style)
| { | ||
| in_str(s, err_reason, i); | ||
| v->server_msg(v, err_reason, 0); | ||
| } |
There was a problem hiding this comment.
If we get to this point, the server has disconnected, and yet error is still 0 (it should probably be 2).
Also, i can be negative again.
Suggest something like:-
if (i != 0)
{
if (version < 8)
{
g_snprintf(text, sizeof(text), "VNC password failed");
}
else
{
g_snprintf(err_reason, sizeof(err_reason),
"<reason unavailable>");
if (trans_force_read_s(v->trans, s, 4) == 0)
{
in_uint32_be(s, i);
if (i > 0 && i < sizeof(err_reason) &&
trans_force_read_s(v->trans, s, i) == 0)
{
in_str(s, i, err_reason, sizeof(err_reason));
}
}
g_snprintf(text, sizeof(text), "VNC password failed : %s",
err_reason);
}
v->server_msg(v, text, 0);
error = 2;
}
else
{
v->server_msg(v, "VNC password ok", 0);
}
|
PS : Don't worry about the code check fails - we're looking at it. See #2567 |
No description provided.