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.

Sign up for GitHub

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

Jump to bottom

Add support for RFB protocol version 3.7 and 3.8 #2549

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

Add support for RFB protocol version 3.7 and 3.8 #2549

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
69cb532
Add support for RFB protocol version 3.7 and 3.8
Jesse-Bakker Feb 13, 2023
5192640
Review fixes
Jesse-Bakker Feb 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions common/parse.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,15 @@ parser_stream_overflow_check(const struct stream *s, int n, int is_out,
(s)->p += (n); \
} while (0)

/******************************************************************************/
#define in_str(s, v, n) do \
{ \
S_CHECK_REM((s), (n + 1)); \
(v) = (s)->p; \
(v[n]) = '\0'; \
(s)->p++; \
} while (0)

/******************************************************************************/
Copy link
Member

Choose a reason for hiding this comment

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

This isn't copying the string from the input buffer.

when the macro runs:-

  • (v) = (s)->p; sets err_reason to 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.

#define in_uint8a(s, v, n) do \
{ \
Expand Down
211 changes: 174 additions & 37 deletions vnc/vnc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1643,10 +1643,16 @@ lib_mod_connect(struct vnc *v)
char cursor_mask[32 * (32 / 8)];
char con_port[256];
char text[256];
char version_str[12];
char *err_reason;
char j;
struct stream *s;
struct stream *pixel_format;
int error;
int i;
int sec_lvl = 0;
int version;
int n_sec_lvls;
int check_sec_result;

v->server_msg(v, "VNC started connecting", 0);
Expand Down Expand Up @@ -1710,64 +1716,167 @@ lib_mod_connect(struct vnc *v)
error = trans_force_read_s(v->trans, s, 12);
if (error == 0)
{
s->p = s->data;
out_uint8a(s, "RFB 003.003\n", 12);
s_mark_end(s);
error = trans_force_write_s(v->trans, s);
in_uint8a(s, version_str, 12);
if (g_strncmp(version_str, "RFB 003.00", 10) != 0)
Jesse-Bakker marked this conversation as resolved.
Show resolved Hide resolved
{
version_str[11] = '\0';
LOG(LOG_LEVEL_ERROR, "Invalid server version string %s", version_str);
error = 1;
}
if (version_str[11] != '\n')
Copy link
Member

Choose a reason for hiding this comment

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

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!

{
LOG(LOG_LEVEL_ERROR, "Invalid server version string (missing trailing newline)");
error = 1;
}
}
if (error == 0)
{
version = version_str[10] - '0';
if (version != 3 && version != 7 && version != 8)
{
LOG(LOG_LEVEL_ERROR, "Unsupported VNC version 3.%d", version);
error = 1;
}
}

/* sec type */
if (error == 0)
{
init_stream(s, 8192);
error = trans_force_read_s(v->trans, s, 4);
out_uint8p(s, version_str, 12);
s_mark_end(s);
error = trans_force_write_s(v->trans, s);
}

/* sec type */
if (error == 0)
{
in_uint32_be(s, i);
g_sprintf(text, "VNC security level is %d (1 = none, 2 = standard)", i);
v->server_msg(v, text, 0);

if (i == 1) /* none */
init_stream(s, 8192);
if (version == 3)
{
check_sec_result = 0;
// The server chooses the security type
error = trans_force_read_s(v->trans, s, 4);
if (error == 0)
{
in_uint32_be(s, sec_lvl);
}
}
else if (i == 2) /* dec the password and the server random */
else if (version >= 7)
{
init_stream(s, 8192);
error = trans_force_read_s(v->trans, s, 16);

// The client chooses the security type
error = trans_force_read_s(v->trans, s, 1);
if (error == 0)
{
init_stream(s, 8192);
if (guid_is_set(&v->guid))
in_uint8(s, n_sec_lvls);
if (n_sec_lvls > 0)
{
char guid_str[GUID_STR_SIZE];
guid_to_str(&v->guid, guid_str);
rfbHashEncryptBytes(s->data, guid_str);
error = trans_force_read_s(v->trans, s, n_sec_lvls);
}
else
{
rfbEncryptBytes(s->data, v->password);
// Read size of reason
error = trans_force_read_s(v->trans, s, 4);
if (error == 0)
{
in_uint32_be(s, i);
if (i <= 1024)
{
error = trans_force_read_s(v->trans, s, i);
}
Copy link
Member

Choose a reason for hiding this comment

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

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)

else
{
LOG(LOG_LEVEL_ERROR, "Connection closed by server. Reason string exceeds size limit (%d > 1024 bytes)", i);
error = 1;
}
}
else
{
i = 0;
}
if (error == 0)
{
error = trans_force_read_s(v->trans, s, i);
Jesse-Bakker marked this conversation as resolved.
Show resolved Hide resolved
}
if (error == 0)
{
in_str(s, err_reason, i);
LOG(LOG_LEVEL_ERROR, "Connection closed by server with reason: %s", err_reason);
error = 1;
}
}
}
if (error == 0)
{
for (; n_sec_lvls > 0; --n_sec_lvls)
{
in_uint8(s, j);
// Choose the highest security level that we support
if (j > sec_lvl && j <= 2)
{
sec_lvl = j;
}
}
if (sec_lvl != 0)
{
init_stream(s, 8192);
out_uint8(s, sec_lvl);
s_mark_end(s);

error = trans_force_write_s(v->trans, s);
}
s->p += 16;
s_mark_end(s);
error = trans_force_write_s(v->trans, s);
check_sec_result = 1; // not needed
}
}
else if (i == 0)
}
}

if (error == 0)
{
g_sprintf(text, "VNC security level is %d (1 = none, 2 = standard)", sec_lvl);
v->server_msg(v, text, 0);

if (sec_lvl == 1) /* none */
{
if (version >= 8)
{
LOG(LOG_LEVEL_ERROR, "VNC Server will disconnect");
error = 1;
check_sec_result = 1;
}
else
{
LOG(LOG_LEVEL_ERROR, "VNC unsupported security level %d", i);
error = 1;
check_sec_result = 0;
}
}
else if (sec_lvl == 2) /* dec the password and the server random */
{
init_stream(s, 8192);
error = trans_force_read_s(v->trans, s, 16);

if (error == 0)
{
init_stream(s, 8192);
if (guid_is_set(&v->guid))
{
char guid_str[GUID_STR_SIZE];
guid_to_str(&v->guid, guid_str);
rfbHashEncryptBytes(s->data, guid_str);
}
else
{
rfbEncryptBytes(s->data, v->password);
}
s->p += 16;
s_mark_end(s);
error = trans_force_write_s(v->trans, s);
check_sec_result = 1; // not needed
}
}
else if (sec_lvl == 0)
{
LOG(LOG_LEVEL_ERROR, "VNC Server will disconnect");
error = 1;
}
else
{
LOG(LOG_LEVEL_ERROR, "VNC unsupported security level %d", sec_lvl);
error = 1;
}
}

if (error != 0)
Expand All @@ -1788,8 +1897,37 @@ lib_mod_connect(struct vnc *v)

if (i != 0)
{
v->server_msg(v, "VNC password failed", 0);
error = 2;
if (version >= 8)
{
error = trans_force_read_s(v->trans, s, 4);
if (error == 0)
{
in_uint32_be(s, i);
if (i <= 1024)
{
error = trans_force_read_s(v->trans, s, i);
}
else
{
LOG(LOG_LEVEL_ERROR, "VNC security failure reason exceeds size limit (%d > 1024 bytes)", i);
error = 1;
}
}
else
{
i = 0;
}
if (error == 0)
{
in_str(s, err_reason, i);
v->server_msg(v, err_reason, 0);
}
Copy link
Member

Choose a reason for hiding this comment

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

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);
            }

}
if (version < 8 || error != 0)
{
v->server_msg(v, "VNC password failed", 0);
error = 2;
}
}
else
{
Expand All @@ -1802,8 +1940,7 @@ lib_mod_connect(struct vnc *v)
{
v->server_msg(v, "VNC sending share flag", 0);
init_stream(s, 8192);
s->data[0] = 1;
s->p++;
out_uint8(s, 1);
s_mark_end(s);
Jesse-Bakker marked this conversation as resolved.
Show resolved Hide resolved
error = trans_force_write_s(v->trans, s); /* share flag */
}
Expand Down Expand Up @@ -2073,7 +2210,7 @@ init_client_layout(struct vnc_screen_layout *layout,
for (i = 0 ; i < client_info->display_sizes.monitorCount ; ++i)
{
/* Use minfo_wm, as this is normalised for a top-left of (0,0)
* as required by RFC6143 */
* as required by RFC6143 */
layout->s[i].id = i;
layout->s[i].x = client_info->display_sizes.minfo_wm[i].left;
layout->s[i].y = client_info->display_sizes.minfo_wm[i].top;
Expand Down