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.
According to the Microsoft RDP specification, T.128 flow control PDUs
should be ignored when reading Share Control headers.
(http://msdn.microsoft.com/en-us/library/cc240576.aspx). This patch
checks if we got a flow control PDU (length = 0x8000) and advances the
stream to ignore the PDU.
The fOpRedundant field of the GlyphIndex primary drawing order
(MS-RDPEGDI, chapter 2.2.2.2.1.1.2.13) was neglected which resulted in some
severe text rendering errors.
* According to MS-RDPBCGR 2.2.7.1.5 the pointerCacheSize is optional
and its absence or a zero value indicates missing client support for
the New Pointer Update.
* Added and fixed some comments regarding the meaning of the KBDFLAGS_DOWN
keyboard flag and how it is currently used in the code.
"Fixed" the slow path keyboard input to generate the same keyboard flags
as the corresponding fast path code.
* Some arbitrary value was used for the ConnectPDULength in the GCC
Conference Create Response. According to MS-RDPBCGR 4.1.4 this value must
be ignored by the client so we encode a zero value instead.
