- handle WAIT_TIMEOUT result as error in async transport thread
if an INFINITE timeout was specified in WaitForMultipleObjects
- fix mfreerdp's async transport handling to not use
freerdp_get_event_handles/freerdp_check_event_handles if async
transport is activated
1)
Added missing checks for CreateEvent which also required the
following related changes:
- changed freerdp_context_new API to BOOL
- changed freerdp_peer_context_new API to BOOL
- changed pRdpClientNew callback to BOOL
- changed pContextNew callback to BOOL
- changed psPeerAccepted callback to BOOL
- changed psPeerContextNew callback to BOOL
2)
Fixed lots of missing alloc and error checks in the
changed code's neighbourhood.
3)
Check freerdp_client_codecs_prepare result to avoid segfaults
caused by using non-initialized codecs.
4)
Fixed deadlocks in x11 caused by missing xf_unlock_x11() calls
in some error handlers
5)
Some fixes in thread pool:
- DEFAULT_POOL assignment did not match TP_POOL definition
- don't free the pool pointer if it points to the static DEFAULT_POOL
- added error handling and cleanup in InitializeThreadpool
* top level GDI functions return 0 on error and != 0 otherwise but the
low level functions (16bpp.c, 8bpp.c 32bpp.c) which are called did it
exactly the other way around. Those were adapted.
* change gdi_InvalidateRegion to BOOL and check calls where appropriate
* integrate comments from pull request
Now using nCount as in and out argument.
When called, set nCount to the number of available handles.
This value is checked and an error returned, if not enough
handles are available.
* Though not frequent, it's possible to get TsProxySetupReceivePipe
data of stublength 4 that is actual data. This happens when
header->common.call_id == rpc->PipeCallId &&
!(header->common.pfc_flags & PFC_LAST_FRAG).
This should address GW disconnects that manifest as SSL read errors.
Change the return type of Stream_Ensure*Capacity from void to BOOL to be
able to detect realloc problems easily. Otherwise the only way to detect
this was to check if the capacity after the call was >= the required
size.
In case Stream_Ensure*Capacity fails the old memory is still available
and need to freed outside.
This commit also adds checks to most calls of Stream_Ensure*Capacity to
check if the call was successful.
The X.224 Connection Request PDU might contain an optional cookie or
routing token before the optional RDP Negotiation Request (rdpNegReq).
If present, both of these fields must be terminated by a 0x0D0A
two-byte sequence. It seems that until now FreeRDP has incorrectly
assumed that a token or cookie must always be present.
If that was not the case, FreeRDP was searching for 0x0D0A until it
arrived at the end of the stream which prevented the remaining data
(RDP Negotiation Request, RDP Correlation Info) from being parsed.
Small cleanup of passing around decorations flag.
Limit PercentScreen to single monitor vs. entire desktop. IMO - this is better behavior in a multimonitor environment.
Handle fullscreen windows better:
1. Ensure that size hints are set to allow resizing before setting a window to fullscreen as some window managers do not behave properly.
2. Handle fullscreen toggles without destroying and recreating window.
3. Use NET_WM_STATE_FULLSCREEN Extended Window Manager Hint for fullscreen functionality
4. Use the NET_WM_FULLSCREEN_MONITORS Extended Window Manager Hint when appropriate
5. When a single monitor fullscreen is requested - use the current monitor(as determined from mouse location)
6. Handle cases where there is no local monitor at coordinate 0,0. The Windows server expect there to be a monitor at this location, so we maintain offset if necessary between our local primary monitor and the server side primary monitor located at 0,0.
Situation: we have fragmented TPKT PDU without two last bytes
(or one last byte - for fast-path) in network stack.
First call to transport_read_pdu() works normally, read
available bytes and exit with status 0 - no whole PDU readed.
Before second call this missed bytes arrive.
Optionally with next PDU.
In second call header parsing code unconditionally read this
two bytes(one byte) despite this is not header bytes.
And increase stream position, so stream now contains whole PDU.
This cause (pduLength - Stream_GetPosition(s)) calculation to be 0.
So transport_read_layer_bytes()-->transport_read_layer() return 0
and transport_read_pdu() exits with "not enough data is available"
status.
If next PDU isn't available next calls to transport_read_pdu()
give same result.
If next PDU arrive - (pduLength - Stream_GetPosition(s)) will be
less than 0. Stream position will grow, grow and grow on each call.
And transport_read_pdu() never signals that PDU is readed.
Caught on Android FreeRDP client with high RDP traffic (several MBytes/s).
Keepalive settings are usually (depending on the implementation) only
used if the TCP connection is idle.
If the network is interrupted/disconnected/... click or keyboard input
generates outgoing traffic therefore the connection isn't idle
anymore and keepalives might not be used causing the connection to
stay open and the client to stall.
Linux 2.6.36 added a TCP_USER_TIMEOUT TCP socket option that lets a
program specify the maximum time transmitted data may remain
unacknowledged before TCP will close the corresponding connection with
ETIMEDOUT.
Setting TCP_USER_TIMEOUT allows us to detect a network problem (like
cable disconnect) even if the connection isn't idle.
refreshRectSupport and suppressOutputSupport of the General
Capability Set (MS-RDPBCGR 2.2.7.1.1) are server-only flags
that indicate whether the Refresh Rect or Suppress Output
PDUs are supported by the server.
Therefore in rdp_read_general_capability_set() we must only
change the respective settings if we are not in server mode.
WTSStartRemoteControlSession doesn't allow to specify additional flags
therefore add a new extended version WTSStartRemoteControlSessionEx
with an additional "flags" parameter.
The following flags are defined:
REMOTECONTROL_FLAG_DISABLE_KEYBOARD - disable keyboard input
REMOTECONTROL_FLAG_DISABLE_MOUSE - disable mouse input
REMOTECONTROL_FLAG_DISABLE_INPUT - disable input (keyboard and mouse)
This patch fixes a bug with mstsc connecting to a RDP security only FreeRDP server.
It seems like the mstsc shipped with Windows Seven considers packets after the nego_failure
packet as an error. So after trying to do TLS, depending on the timing, mstsc can print an
error message instead of retrying to connect with RDP security. With this patch, we
don't send the MCS disconnect message when the negociation has failed.
Before this patch, RDP security was (wrongly) the fallback when negociating a
security protocol between the client and the server. For example when a client
was claiming TLS-only when connecting to a FreeRDP based-server with RDP security only,
the result of the negociation was that the server started to do RDP security.
The expected behaviour is to send a nego failure packet with error code
SSL_NOT_ALLOWED_BY_SERVER. This patch fixes this.
We also try to handle all cases of failed negociation and return the corresponding
error code.
Commit 0357a38e31 modified the function
fastpath_send_update_pdu() to check if the desired update is possible
by checking the payload size against the computed maxLength and the
clients's advertised max request size.
If the check failed that commit added a workaround which simply
copied the payload to a slow path updade.
This workaround is totally flawed and causes protocol errors:
- the fast path update code is not checked and required data format
conversions are missing
- depending on the fast path update code rdp_send_data_pdu() would
have to be called with differend data pdu type values but the
workaround always uses DATA_PDU_TYPE_UPDATE
- the workaround does not check if the total size would exceed
the maximum possible size for a slow path update
The check if a fast path output is actually possible with the
passed parameters is basically a good idea.
However, if that check fails it would only indicate an error in
the server implementation who must not generate updates that
exceed the client's max request size.
Even though a slow-path conversion would be possible there is
much more involved than simply copying the payload stream.
In addition it is highly doubtful if there is a benefit at all.
Even the oldest rdesktop and windows ce clients do support fast
path and although some lack the multi-fragment update capability
we cannot really send larger updates using slow-path outputs.
For the reasons elucidated above, I have removed the workaround
but kept a modified version of the check if a fast-path output
is possible at all.
Commit 0357a38e31 has added some code
without any effect.
That commit added code to rdp_read_capability_sets() to check if
CAPSET_TYPE_MULTI_FRAGMENT_UPDATE was not received which caused
settings->MultifragMaxRequestSize to be set to 0.
- this was done in the wrong place because we do these kind
of checks in rdp_recv_confirm_active() by consulting the
variable settings->ReceivedCapabilities[]
- the code had no effect at all because MultifragMaxRequestSize gets
set to FASTPATH_FRAGMENT_SAFE_SIZE in rdp_recv_confirm_active()
if the CAPSET_TYPE_MULTI_FRAGMENT_UPDATE was not received.
Extend rdp_pointer with function SetPosition. Can then be used by
clients support setting pointer by server which might be used in
shadowing scenarios.
When a client disconnects from a server and its channel structures are removed, the global hash g_OpenHandles should not be destroyed. Only freed channels must be removed from the hash.
Skipping channels already loaded in freerdp_channels_client_load
This prevents channels already loaded in a context to be added a
second time to the channel list.
The input->FocusInEvent callback implementations (normal and fast-path) have
always sent the mouse position even if the pointer was outside of the freerdp
client area. In addition xfreerdp used the wrong pointer coordinates which
were relative to the root window instead of its own.
On focus-in the pointer position must only be sent if the pointer is
currently within the program's client area. However, the clients had no way
to pass that information to input->FocusInEvent which required an API change.
- removed mouse pointer x, y parameters from input interface's FocusInEvent
- clients are responsible to call input->MouseEvent on focus-in if necessary
- fixed xfreerdp and wfreerdp accordingly
Since commit a228952 FreeRDP generates corrupt licensing packets if the rdp
security layer is used and the peer did not indicate that it is capable of
processing encrypted licensing packets:
That commit changed rdp->sec_flags after the rdp stream was already initialized
with encryption enabled which placed the PDU payload at an incorrect offset.
Instead of directly modifying the rdp->sec_flags this patch temporarily
disables rdp->do_crypt during rdp stream initialization if the client has not
advertised support for encrypted licensing packets.
Some buggy server(s) send data for channels that weren't announced or
negotiated. When processing this data FreeRDP had a problem and always
used the last channel in the channels list even if it wasn't responsible
for the data. Depending on how the channel handled the data this could
lead to different kind of problems and also segmentation faults.
Now data for unknown channels is ignored and not processed further.
[MS-RDPBCGR] Section 5.3 describes the encryption level and method values for
standard RDP security.
Looking at the current usage of these values in the FreeRDP code gives me
reason to believe that there is a certain lack of understanding of how these
values should be handled.
The encryption level is only configured on the server side in the "Encryption
Level" setting found in the Remote Desktop Session Host Configuration RDP-Tcp
properties dialog and this value is never transferred from the client to the
server over the wire.
The possible options are "None", "Low", "Client Compatible", "High" and
"FIPS Compliant". The client receices this value in the Server Security Data
block (TS_UD_SC_SEC1), probably only for informational purposes and maybe to
give the client the possibility to verify if the server's decision for the
encryption method confirms to the server's encryption level.
The possible encryption methods are "NONE", "40BIT", "56BIT", "128BIT" and
"FIPS" and the RDP client advertises the ones it supports to the server in the
Client Security Data block (TS_UD_CS_SEC).
The server's configured encryption level value restricts the possible final
encryption method.
Something that I was not able to find in the documentation is the priority
level of the individual encryption methods based on which the server makes its
final method decision if there are several options.
My analysis with Windows Servers reveiled that the order is 128, 56, 40, FIPS.
The server only chooses FIPS if the level is "FIPS Comliant" or if it is the
only method advertised by the client.
Bottom line:
* FreeRDP's client side does not need to set settings->EncryptionLevel
(which was done quite frequently).
* FreeRDP's server side does not have to set the supported encryption methods
list in settings->EncryptionMethods
Changes in this commit:
Removed unnecessary/confusing changes of EncryptionLevel/Methods settings
Refactor settings->DisableEncryption
* This value actually means "Advanced RDP Encryption (NLA/TLS) is NOT used"
* The old name caused lots of confusion among developers
* Renamed it to "UseRdpSecurityLayer" (the compare logic stays untouched)
Any client's setting of settings->EncryptionMethods were annihilated
* All clients "want" to set all supported methods
* Some clients forgot 56bit because 56bit was not supported at the time the
code was written
* settings->EncryptionMethods was overwritten anyways in nego_connect()
* Removed all client side settings of settings->EncryptionMethods
The default is "None" (0)
* Changed nego_connect() to advertise all supported methods if
settings->EncryptionMethods is 0 (None)
* Added a commandline option /encryption-methods:comma separated list of the
values "40", "56", "128", "FIPS". E.g. /encryption-methods:56,128
* Print warning if server chooses non-advertised method
Verify received level and method in client's gcc_read_server_security_data
* Only accept valid/known encryption methods
* Verify encryption level/method combinations according to MS-RDPBCGR 5.3.2
Server implementations can now set settings->EncryptionLevel
* The default for settings->EncryptionLevel is 0 (None)
* nego_send_negotiation_response() changes it to ClientCompatible in that case
* default to ClientCompatible if the server implementation set an invalid level
Fix server's gcc_write_server_security_data
* Verify server encryption level value set by server implementations
* Choose rdp encryption method based on level and supported client methods
* Moved FIPS to the lowest priority (only used if other methods are possible)
Updated sample server
* Support RDP Security (RdpKeyFile was not set)
* Added commented sample code for setting the security level
When "detect" is used as gateway usage method (which is the default)
it is tried to by-pass gateway connection for local hosts.
The detection might take some time therefore print a message that people
are aware that a detection is tried.
Fixes#2171
Client side code always tells the server that it is capable of processing
encrypted licensing packages (SEC_LICENSE_ENCRYPT_SC) but didn't set
the recently added flag to indicate that.
Fixes#2196
rpc_send_enqueue_pdu returns -1 on error but the type of error isn't
distinguishable. Therefore make sure that the buffer gets always freed.
The only exception to this is when the pdu was already queued. Then the
dequeuing function should take care of freeing the buffer when
processing the pdu.
