cerg2010cerg2010 March 18, 2019 at 15:42

Checking FreeRDP with the PVS-Studio analyzer

FreeRDP is an open source implementation of Remote Desktop Protocol (RDP), a protocol that implements remote computer control developed by Microsoft. The project supports many platforms, including Windows, Linux, macOS, and even iOS with Android. This project was selected as the first in a series of articles devoted to checking RDP clients using the PVS-Studio static analyzer.

A bit of history

The FreeRDP project came after Microsoft unveiled the specifications of its proprietary RDP protocol. At that time, there was an rdesktop client, the implementation of which is based on the results of Reverse Engineering.

In the process of implementing the protocol, it became more difficult to add new functionality due to the then existing project architecture. Changes in it created a conflict between developers, which led to the creation of the rdesktop fork - FreeRDP. Further distribution of the product was limited by the GPLv2 license, as a result of which a decision was made to re-license to Apache License v2. However, not everyone agreed to change the license of their code, so the developers decided to rewrite the project, as a result of which we have a modern look of the code base.

More details about the history of the project can be found in the official blog note: “The history of the FreeRDP project”. PVS-Studio was

used as a tool for detecting errors and potential vulnerabilities in the code . It is a static code analyzer for C, C ++, C #, and Java, available on Windows, Linux, and macOS. The article presents only those errors that seemed to me the most interesting.

Memory leak

V773 The function was exited without releasing the 'cwd' pointer. A memory leak is possible. environment.c 84

DWORD GetCurrentDirectoryA(DWORD nBufferLength, LPSTR lpBuffer)
{
  char* cwd;
  ....
  cwd = getcwd(NULL, 0);
  ....
  if (lpBuffer == NULL)
  {
    free(cwd);
    return 0;
  }
  if ((length + 1) > nBufferLength)
  {
    free(cwd);
    return (DWORD) (length + 1);
  }
  memcpy(lpBuffer, cwd, length + 1);
  return length;
  ....
}

This fragment was taken from the winpr subsystem that implements the WINAPI wrapper for non-Windows systems, i.e. It is a lightweight counterpart to Wine. There is a leak here: the memory allocated by the getcwd function is freed up only during special cases. To fix the error, add a call to free after memcpy .

Going beyond the bounds of an array

V557 Array overrun is possible. The value of 'event-> EventHandlerCount' index could reach 32. PubSub.c 117

#define MAX_EVENT_HANDLERS  32
struct _wEventType
{
  ....
  int EventHandlerCount;
  pEventHandler EventHandlers[MAX_EVENT_HANDLERS];
};
int PubSub_Subscribe(wPubSub* pubSub, const char* EventName,
      pEventHandler EventHandler)
{
  ....
  if (event->EventHandlerCount <= MAX_EVENT_HANDLERS)
  {
    event->EventHandlers[event->EventHandlerCount] = EventHandler;
    event->EventHandlerCount++;
  }
  ....
}

In this example, a new item is added to the list, even if the number of items has reached the maximum. It is enough to replace the operator <= with < so as not to go beyond the boundaries of the array.

Another error of this type was found:

V557 Array overrun is possible. The value of 'iBitmapFormat' index could reach 8. orders.c 2623

Typos

Fragment 1

V547 Expression '! Pipe-> In' is always false. MessagePipe.c 63

wMessagePipe* MessagePipe_New()
{
  ....
  pipe->In = MessageQueue_New(NULL);
  if (!pipe->In)
    goto error_in;
  pipe->Out = MessageQueue_New(NULL);
  if (!pipe->In) // <=
    goto error_out;
  ....
}

Here we see the usual typo: in the second condition, the same variable is checked as in the first. Most likely, the error appeared as a result of unsuccessful copying of the code.

Fragment 2

V760 Two identical blocks of text were found. The second block begins from line 771. tsg.c 770

typedef struct _TSG_PACKET_VERSIONCAPS
{
  ....
  UINT16 majorVersion;
  UINT16 minorVersion;
  ....
} TSG_PACKET_VERSIONCAPS, *PTSG_PACKET_VERSIONCAPS;
static BOOL TsProxyCreateTunnelReadResponse(....)
{
  ....
  PTSG_PACKET_VERSIONCAPS versionCaps = NULL;
  ....
  /* MajorVersion (2 bytes) */
  Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
  /* MinorVersion (2 bytes) */
  Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
  ....
}

Another typo: a comment on the code implies that minorVersion should come from the stream , however, reading takes place in a variable named majorVersion . However, I am not familiar with the protocol, so this is just an assumption.

Fragment 3

V524 It is odd that the body of 'trio_index_last' function is fully equivalent to the body of 'trio_index' function. triostr.c 933

/**
   Find first occurrence of a character in a string.
   ....
 */
TRIO_PUBLIC_STRING char *
trio_index
TRIO_ARGS2((string, character),
     TRIO_CONST char *string,
     int character)
{
  assert(string);
  return strchr(string, character);
}
/**
   Find last occurrence of a character in a string.
   ....
 */
TRIO_PUBLIC_STRING char *
trio_index_last
TRIO_ARGS2((string, character),
     TRIO_CONST char *string,
     int character)
{
  assert(string);
  return strchr(string, character);
}

Judging by the comment, the trio_index function finds the first match of a character in a string when trio_index_last is the last. But the bodies of these functions are identical! Most likely, this is a typo, and in the trio_index_last function you need to use strrchr instead of strchr . Then the behavior will be expected.

Fragment 4

V769 The 'data' pointer in the expression equals nullptr. The resulting value of arithmetic operations on this pointer is senseless and it should not be used. nsc_encode.c 124

static BOOL nsc_encode_argb_to_aycocg(NSC_CONTEXT* context,
                                      const BYTE* data,
                                      UINT32 scanline)
{
  ....
  if (!context || data || (scanline == 0))
    return FALSE;
  ....
  src = data + (context->height - 1 - y) * scanline;
  ....
}

Looks like they accidentally missed the negation operator ! next to data . It is strange that this went unnoticed.

Fragment 5

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 213, 222. rdpei_common.c 213

BOOL rdpei_write_4byte_unsigned(wStream* s, UINT32 value)
{
  BYTE byte;
  if (value <= 0x3F)
  {
    ....
  }
  else if (value <= 0x3FFF)
  {
    ....
  }
  else if (value <= 0x3FFFFF)
  {
    byte = (value >> 16) & 0x3F;
    Stream_Write_UINT8(s, byte | 0x80);
    byte = (value >> 8) & 0xFF;
    Stream_Write_UINT8(s, byte);
    byte = (value & 0xFF);
    Stream_Write_UINT8(s, byte);
  }
  else if (value <= 0x3FFFFF)
  {
    byte = (value >> 24) & 0x3F;
    Stream_Write_UINT8(s, byte | 0xC0);
    byte = (value >> 16) & 0xFF;
    Stream_Write_UINT8(s, byte);
    byte = (value >> 8) & 0xFF;
    Stream_Write_UINT8(s, byte);
    byte = (value & 0xFF);
    Stream_Write_UINT8(s, byte);
  }
  ....
}

The last two conditions are the same: apparently, someone forgot to check them after copying. According to the code, it is noticeable that the last part works with four-byte values, so we can assume that the last condition should be value <= 0x3FFFFFFF .

Another error of this type was found:

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 169, 173. file.c 169

Input Validation

Fragment 1

V547 Expression 'strcat (target, source)! = NULL' is always true. triostr.c 425

TRIO_PUBLIC_STRING int
trio_append
TRIO_ARGS2((target, source),
     char *target,
     TRIO_CONST char *source)
{
  assert(target);
  assert(source);
  return (strcat(target, source) != NULL);
}

Checking the result of the function in this example is incorrect. The strcat function returns a pointer to the final version of the string, i.e. first parameter passed. In this case, it is target . However, if it is NULL , then it is too late to check, since in the strcat function it will be dereferenced.

Fragment 2

V547 Expression 'cache' is always true. glyph.c 730

typedef struct rdp_glyph_cache rdpGlyphCache;
struct rdp_glyph_cache
{
  ....
  GLYPH_CACHE glyphCache[10];
  ....
};
void glyph_cache_free(rdpGlyphCache* glyphCache)
{
  ....
  GLYPH_CACHE* cache = glyphCache->glyphCache;
  if (cache)
  {
    ....
  }
  ....
}

In this case, the cache variable is assigned the address of the static array glyphCache-> glyphCache . Thus, the if (cache) check can be omitted.

Resource Management Error

V1005 The resource was acquired using 'CreateFileA' function but was released using incompatible 'fclose' function. certificate.c 447

BOOL certificate_data_replace(rdpCertificateStore* certificate_store,
                              rdpCertificateData* certificate_data)
{
  HANDLE fp;
  ....
  fp = CreateFileA(certificate_store->file, GENERIC_READ | GENERIC_WRITE, 0,
                   NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
  ....
  if (size < 1)
  {
    CloseHandle(fp);
    return FALSE;
  }
  ....
  if (!data)
  {
    fclose(fp);
    return FALSE;
  }
  ....
}

The fp file descriptor created by calling the CreateFile function is mistakenly closed by the fclose function from the standard library, rather than CloseHandle .

Same conditions

V581 The conditional expressions of the 'if' statements located alongside each other are identical. Check lines: 269, 283. ndr_structure.c 283

void NdrComplexStructBufferSize(PMIDL_STUB_MESSAGE pStubMsg,
      unsigned char* pMemory, PFORMAT_STRING pFormat)
{
  ....
  if (conformant_array_description)
  {
    ULONG size;
    unsigned char array_type;
    array_type = conformant_array_description[0];
    size = NdrComplexStructMemberSize(pStubMsg, pFormat);
    WLog_ERR(TAG, "warning: NdrComplexStructBufferSize array_type: "
      "0x%02X unimplemented", array_type);
    NdrpComputeConformance(pStubMsg, pMemory + size,
      conformant_array_description);
    NdrpComputeVariance(pStubMsg, pMemory + size,
      conformant_array_description);
    MaxCount = pStubMsg->MaxCount;
    ActualCount = pStubMsg->ActualCount;
    Offset = pStubMsg->Offset;
  }
  if (conformant_array_description)
  {
    unsigned char array_type;
    array_type = conformant_array_description[0];
    pStubMsg->MaxCount = MaxCount;
    pStubMsg->ActualCount = ActualCount;
    pStubMsg->Offset = Offset;
    WLog_ERR(TAG, "warning: NdrComplexStructBufferSize array_type: "
      "0x%02X unimplemented", array_type);
  }
  ....
}

Perhaps this example is not a mistake. However, both conditions contain the same message, one of which, most likely, can be removed.

Clearing Null Pointers

V575 The null pointer is passed into 'free' function. Inspect the first argument. smartcard_pcsc.c 875

WINSCARDAPI LONG WINAPI PCSC_SCardListReadersW(
  SCARDCONTEXT hContext,
  LPCWSTR mszGroups,
  LPWSTR mszReaders,
  LPDWORD pcchReaders)
{
  LPSTR mszGroupsA = NULL;
  ....
  mszGroups = NULL; /* mszGroups is not supported by pcsc-lite */
  if (mszGroups)
    ConvertFromUnicode(CP_UTF8,0, mszGroups, -1, 
                       (char**) &mszGroupsA, 0,
                       NULL, NULL);
  status = PCSC_SCardListReaders_Internal(hContext, mszGroupsA,
                                          (LPSTR) &mszReadersA,
                                          pcchReaders);
  if (status == SCARD_S_SUCCESS)
  {
    ....
  }
  free(mszGroupsA);
  ....
}

You can pass a null pointer to the free function and the analyzer knows about this. But if a situation is revealed in which the pointer is always passed zero, as in this fragment, a warning will be issued.

The mszGroupsA pointer is initially NULL and is not initialized anywhere else. The only branch of code where the pointer could be initialized is unreachable.

There were other posts like this:

V575 The null pointer is passed into 'free' function. Inspect the first argument. license.c 790
V575 The null pointer is passed into 'free' function. Inspect the first argument. rdpsnd_alsa.c 575

Most likely, such forgotten variables arise during refactoring and can be simply deleted.

Possible overflow

V1028 Possible overflow. Consider casting operands, not the result. makecert.c 1087

// openssl/x509.h
ASN1_TIME *X509_gmtime_adj(ASN1_TIME *s, long adj);
struct _MAKECERT_CONTEXT
{
  ....
  int duration_years;
  int duration_months;
};
typedef struct _MAKECERT_CONTEXT MAKECERT_CONTEXT;
int makecert_context_process(MAKECERT_CONTEXT* context, ....)
{
  ....
  if (context->duration_months)
    X509_gmtime_adj(after, (long)(60 * 60 * 24 * 31 *
      context->duration_months));
  else if (context->duration_years)
    X509_gmtime_adj(after, (long)(60 * 60 * 24 * 365 *
      context->duration_years));
  ....
}

Bringing the result to long is not protection against overflow, since the calculation itself is performed using the int type .

Pointer dereferencing in initialization

V595 The 'context' pointer was utilized before it was verified against nullptr. Check lines: 746, 748. gfx.c 746

static UINT gdi_SurfaceCommand(RdpgfxClientContext* context,
                               const RDPGFX_SURFACE_COMMAND* cmd)
{
  ....
  rdpGdi* gdi = (rdpGdi*) context->custom;
  if (!context || !cmd)
    return ERROR_INVALID_PARAMETER;
  ....
}

Here, the context pointer is dereferenced in the initialization - before it is checked.

Other errors of this type were found:

V595 The 'ntlm' pointer was utilized before it was verified against nullptr. Check lines: 236, 255. ntlm.c 236
V595 The 'context' pointer was utilized before it was verified against nullptr. Check lines: 1003, 1007. rfx.c 1003
V595 The 'rdpei' pointer was utilized before it was verified against nullptr. Check lines: 176, 180. rdpei_main.c 176
V595 The 'gdi' pointer was utilized before it was verified against nullptr. Check lines: 121, 123. xf_gfx.c 121

Meaningless condition

V547 Expression 'rdp-> state> = CONNECTION_STATE_ACTIVE' is always true. connection.c 1489

int rdp_server_transition_to_state(rdpRdp* rdp, int state)
{
  ....
  switch (state)
  {
    ....
    case CONNECTION_STATE_ACTIVE:
      rdp->state = CONNECTION_STATE_ACTIVE;          // <=
      ....
      if (rdp->state >= CONNECTION_STATE_ACTIVE)     // <=
      {
        IFCALLRET(client->Activate, client->activated, client);
        if (!client->activated)
          return -1;
      }
    ....
  }
  ....
}

It is easy to see that the first condition does not make sense due to the assignment of the corresponding value earlier.

Incorrect line parsing

V576 Incorrect format. Consider checking the third actual argument of the 'sscanf' function. A pointer to the unsigned int type is expected. proxy.c 220

V560 A part of conditional expression is always true: (rc> = 0). proxy.c 222

static BOOL check_no_proxy(....)
{
  ....
  int sub;
  int rc = sscanf(range, "%u", &sub);
  if ((rc == 1) && (rc >= 0))
  {
    ....
  }
  ....
}

The analyzer for this fragment immediately gives 2 warnings. The % u specifier expects a variable of type unsigned int , but the variable sub is of type int . Next, we see a suspicious check: the condition on the right does not make sense, since in the beginning there is a comparison with unity. I don’t know what the author of this code had in mind, but there is clearly something wrong here.

Unordered Checks

V547 Expression 'status == 0x00090314' is always false. ntlm.c 299

BOOL ntlm_authenticate(rdpNtlm* ntlm, BOOL* pbContinueNeeded)
{
  ....
  if (status != SEC_E_OK)
  {
    ....
    return FALSE;
  }
  if (status == SEC_I_COMPLETE_NEEDED)            // <=
    status = SEC_E_OK;
  else if (status == SEC_I_COMPLETE_AND_CONTINUE) // <=
    status = SEC_I_CONTINUE_NEEDED;
  ....
}

The marked conditions will always be false, since the execution will reach the second condition only if status == SEC_E_OK . The correct code might look like this:

if (status == SEC_I_COMPLETE_NEEDED)
  status = SEC_E_OK;
else if (status == SEC_I_COMPLETE_AND_CONTINUE)
  status = SEC_I_CONTINUE_NEEDED;
else if (status != SEC_E_OK)
{
  ....
  return FALSE;
}

Conclusion

Thus, the verification of the project revealed many problems, but only the most interesting part was described in the article. Project developers can verify the project themselves by requesting a temporary license key on the PVS-Studio website . There were also false positives, work on which will help to improve the analyzer. Nevertheless, static analysis is important if you want to not only improve the quality of the code, but also reduce the time to search for errors, and PVS-Studio can help with this.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Larin. Checking FreeRDP with PVS-Studio

Tags:

﻿Checking FreeRDP with the PVS-Studio analyzer

A bit of history

Memory leak

Going beyond the bounds of an array

Typos

Fragment 1

Fragment 2

Fragment 3

Fragment 4

Fragment 5

Input Validation

Fragment 1

Fragment 2

Resource Management Error

Same conditions

Clearing Null Pointers

Possible overflow

Pointer dereferencing in initialization

Meaningless condition

Incorrect line parsing

Unordered Checks

Conclusion

Also popular now:

Checking FreeRDP with the PVS-Studio analyzer