Bug 91390 - weston rdp-backend crash when "rdp4-key" option is used
Summary: weston rdp-backend crash when "rdp4-key" option is used
Status: RESOLVED FIXED
Alias: None
Product: Wayland
Classification: Unclassified
Component: weston (show other bugs)
Version: unspecified
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Wayland bug list
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-19 05:51 UTC by Dawid Gajownik
Modified: 2015-08-06 16:29 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
weston output (4.64 KB, text/plain)
2015-07-19 05:51 UTC, Dawid Gajownik
Details
weston output when incorrect file is specified (5.19 KB, text/plain)
2015-07-20 15:19 UTC, Dawid Gajownik
Details
small fixes (1.35 KB, patch)
2015-07-25 00:30 UTC, Dawid Gajownik
Details | Splinter Review

Description Dawid Gajownik 2015-07-19 05:51:11 UTC
Created attachment 117240 [details]
weston output

Hi,

if you try to run weston with rdp-backend and rdp4-key option, program will crash after client initializing a connection. For example execute 

weston --backend=rdp-backend.so --rdp4-key=test

and connect with any RDP client.

In src/compositor-rdp.c:1093 in rdp_peer_init() there's:

        client->Initialize(client);

        settings = client->settings;
        /* configure security settings */
        if (b->rdp_key)
                settings->RdpKeyFile = strdup(b->rdp_key);

client->Initialize(client) executes libfreerdp/core/peer.c:freerdp_peer_initialize() from FreeRDP. It populates settings->RdpServerRsaKey if settings->RdpKeyFile is set. As you can see above rdp_peer_init() initializes settings->RdpKeyFile too late.

In the end settings->RdpServerRsaKey is not set and weston crashes when libfreerdp/core/gcc.c:gcc_write_server_security_data() tries to access
    keyLen = settings->RdpServerRsaKey->ModulusLength;

Thanks,
     Dawid

weston and freerdp from today's git head.
Comment 1 Hardening 2015-07-20 11:56:49 UTC
My fault I have done changes there recently. Sending a patch for this.
Comment 2 Hardening 2015-07-20 12:40:28 UTC
The patch here should fix the problem for you:

http://lists.freedesktop.org/archives/wayland-devel/2015-July/023520.html
Comment 3 Dawid Gajownik 2015-07-20 15:18:15 UTC
Thanks for the quick reply. After applying a patch it's a bit better now, although weston will still crash if you provide a key that does not exist.

libfreerdp just prints information that it can't open the key but it still tries to access settings->RdpServerRsaKey->ModulusLength although settings->RdpServerRsaKey is not initialized

[12:09:49:168] [1766:1766] [ERROR][com.freerdp.core] - unable to open RSA key file test: No such file or directory.
[12:09:49:168] [1766:1766] [ERROR][com.freerdp.core.peer] - invalid RDP key file test

Is it a bug in weston or freerdp?
Comment 4 Dawid Gajownik 2015-07-20 15:19:30 UTC
Created attachment 117259 [details]
weston output when incorrect file is specified
Comment 5 Dawid Gajownik 2015-07-20 15:59:08 UTC
Maybe we should check the return value of "client->Initialize(client)"? I noticed in FreeRDP/server/Windows/wf_peer.c something like this:


        if (!client->Initialize(client))
                goto fail_client_initialize;

[snip]

fail_client_initialize:
        freerdp_peer_context_free(client);

It's just the example how this could be done. I'm not the programmer :-)
Comment 6 Hardening 2015-07-24 20:57:36 UTC
You're right for the Initialize callback, anyway I must check if it has always returned a BOOL, or if it's a quite new change.
Comment 7 Dawid Gajownik 2015-07-24 23:08:27 UTC
Hmm, I did not thought about it. It looks that at least since commit 19028a27b0e2aaaa68f699f9814e183309376696 (version 1.0.1) it always returned a BOOL
https://github.com/FreeRDP/FreeRDP/blob/19028a27b0e2aaaa68f699f9814e183309376696/libfreerdp/core/peer.c
https://github.com/FreeRDP/FreeRDP/blob/19028a27b0e2aaaa68f699f9814e183309376696/CMakeLists.txt

weston requires FreeRDP >= 1.1.0 so it should not be a problem. I also found out that since ac7507ab8dcf6891c73f245f6481e5473eef574d (1.2.0-beta1) it started to return FALSE should something go wrong
https://github.com/FreeRDP/FreeRDP/commit/ac7507ab8dcf6891c73f245f6481e5473eef574d#diff-4b3f37096be4d2d45210500986824518
Comment 8 Dawid Gajownik 2015-07-25 00:30:10 UTC
Created attachment 117362 [details] [review]
small fixes

BTW while reading the code I spotted few small things that could be changed. Most of them are self explanatory. I modified the help output because "--env-socket" is of type WESTON_OPTION_BOOLEAN.

I also noticed that recently freerdp_peer_context_new() changed the return value from void to BOOL so it would be possible later to check its output.

I just learn to code in C so take it with a grain of salt ;-)
Comment 9 Hardening 2015-08-01 22:38:34 UTC
http://lists.freedesktop.org/archives/wayland-devel/2015-August/023767.html should allow to close this bug.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.