Andrey Karpov

Mar 24 2017

Tags:

#Cpp #Security

Weaknesses detected by PVS-Studio this week: episode N3

• For those who are not familiar with PVS-Studio tool
• Weaknesses
• Miscellaneous errors
• Conclusion

We decided to search and fix potential vulnerabilities in various projects. You can call this as you wish - some kind of help to open source projects; a method of promotion or testing of the analyzer. Another way to see it as a way to attract attention to the reliability and quality of the code. In fact, the way to name these posts does not really matter - we just like doing it. This is our little hobby. So, let us have a look at our findings in the code of various projects this week - we had some time to make fixes and suggest looking at them.

For those who are not familiar with PVS-Studio tool

PVS-Studio is a tool that detects a large number of types of vulnerabilities and errors in the code. It performs static analysis and points to code fragments that are likely to contain errors. The best effect is achieved when the static analysis is performed regularly. Ideologically, the analyzer warnings are similar to the compiler warnings. However, unlike compilers, PVS-Studio can perform deeper and more versatile code analysis. This enables it to detect errors, even in compilers: GCC; LLVM 1, 2, 3; Roslyn.

The tool supports the analysis of C, C++ and C#; works under Windows and Linux. The analyzer can be integrated as a Visual Studio plug-in.

We suggest the following materials for further investigation of the tool:

• A detailed presentation on SlideShare. It is available on YouTube (47 min.) in the video format.
• Articles about checked open source projects.
• PVS-Studio: searching software weaknesses.

Weaknesses

In this section we show those defects that fall under the CWE classification and are potential vulnerabilities in their core. Of course, not all weaknesses are really threatening for a project, but we wanted to show that our tool is able to detect them.

1. MSBuild. CWE-476 (NULL Pointer Dereference)

• V3095 The 'searchLocation' object was used before it was verified against null. Check lines: 170, 178. Microsoft.Build.Tasks Resolver.cs 170
• V3095 The 'searchLocation' object was used before it was verified against null. Check lines: 249, 264. Microsoft.Build.Tasks Resolver.cs 249
• V3095 The 'assemblyName' object was used before it was verified against null. Check lines: 176, 194. Microsoft.Build.Tasks Resolver.cs 176

protected bool FileMatchesAssemblyName
(
  AssemblyNameExtension assemblyName,
  ....
  ResolutionSearchLocation searchLocation
)
{
  searchLocation.FileNameAttempted =  // <=
    pathToCandidateAssembly;
  ....
  if (String.Compare(assemblyName.Name, ....) != 0)  // <=
  {
    ....
  }
  ....
  if (searchLocation != null)
  {
    ....
  }
  ....
  bool isSimpleAssemblyName = assemblyName == null ? 
    false : assemblyName.IsSimpleName;
  ....
  searchLocation.Reason =  // <=
    NoMatchReason.ProcessorArchitectureDoesNotMatch;
  ....
  if (searchLocation != null)
  {
    ....
  }
  ....
}

Report: https://github.com/Microsoft/msbuild/pull/1891

2. MSBuild. CWE-476 (NULL Pointer Dereference)

V3095 The 'e' object was used before it was verified against null. Check lines: 165, 170. MSBuild InitializationException.cs 165

internal static void Throw(string messageResourceName,
  string invalidSwitch, Exception e, bool showStackTrace)
{
  ....
  if (showStackTrace)
  {
    errorMessage += Environment.NewLine + e.ToString();  // <=
  }
  else
  {
    errorMessage = ResourceUtilities.FormatString(errorMessage, 
      ((e == null) ? String.Empty : e.Message));
  }
  ....
}

Report: https://github.com/Microsoft/msbuild/pull/1891

3. Entity Framework. CWE-670 (Always-Incorrect Control Flow Implementation)

V3014 It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'i'. EFCore ExpressionEqualityComparer.cs 214

V3015 It is likely that a wrong variable is being compared inside the 'for' operator. Consider reviewing 'i' EFCore ExpressionEqualityComparer.cs 214

var memberInitExpression = (MemberInitExpression)obj;
....
for (var i = 0; i < memberInitExpression.Bindings.Count; i++)
{
  var memberBinding = memberInitExpression.Bindings[i];
  .... 
  switch (memberBinding.BindingType)
  {
    case ....
    case MemberBindingType.ListBinding:
      var memberListBinding = (MemberListBinding)memberBinding;
      for(var j=0; 
              i < memberListBinding.Initializers.Count;    // <=
              i++)                                         // <=
      {
        hashCode += (hashCode * 397) ^
        GetHashCode(memberListBinding.Initializers[j].Arguments);
      }
      break;
    ....
   }
}

Report: https://github.com/aspnet/EntityFramework/pull/7909

4. Entity Framework. CWE-670 (Always-Incorrect Control Flow Implementation)

V3081 The 'j' counter is not used inside a nested loop. Consider inspecting usage of 'i' counter. EFCore.Specification.Tests ComplexNavigationsQueryTestBase.cs 2393

for (var i = 0; i < result.Count; i++)
{
  var expectedElement = expected
    .Single(e => e.Name == result[i].Name);
    
  var expectedInnerNames = expectedElement
    .OneToMany_Optional.Select(e => e.Name)
    .ToList();
    
  for (var j = 0; j < expectedInnerNames.Count; j++)    // <=
  {
    Assert.True
    (
      result[i]
      .OneToMany_Optional.Select(e => e.Name)
      .Contains(expectedInnerNames[i])                  // <=
    );
  }
}

Report: https://github.com/aspnet/EntityFramework/pull/7909

5. CoreCLR. CWE-188 (Reliance on Data/Memory Layout)

V557 Array overrun is possible. The value of 'dwCode - 1' index could reach 8. cordbdi rsmain.cpp 67

const char * GetDebugCodeName(DWORD dwCode)
{
  if (dwCode < 1 || dwCode > 9)
  {
    return "!Invalid Debug Event Code!";
  }

  static const char * const szNames[] = {
    "(1) EXCEPTION_DEBUG_EVENT",
    "(2) CREATE_THREAD_DEBUG_EVENT",
    ....
    "(8) OUTPUT_DEBUG_STRING_EVENT"         // <=
    "(9) RIP_EVENT",
  };

  return szNames[dwCode - 1];
}

Report: https://github.com/dotnet/coreclr/pull/10417

6. FreeBSD. CWE-561 (Unreachable code detected)

V779 Unreachable code detected. It is possible that an error is present. mps.c 1306

static int
mps_alloc_requests(struct mps_softc *sc)
{
  ....
    else {
      panic("failed to allocate command %d\n", i);
      sc->num_reqs = i;
      break;
    }
  ....
}

Report: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=218002

7. FreeBSD. CWE-561 (Unreachable code detected)

V779 Unreachable code detected. It is possible that an error is present. efx_mcdi.c 910

void
efx_mcdi_ev_death(
  __in    efx_nic_t *enp,
  __in    int rc)
{
  ....
  efx_mcdi_raise_exception(enp, emrp, rc);

  if (emrp != NULL && ev_cpl)
   emtp->emt_ev_cpl(emtp->emt_context);
}

Report: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=218004

8. FreeBSD. CWE-561 (Unreachable code detected)

V779 Unreachable code detected. It is possible that an error is present. sctp_pcb.c 183

struct sctp_vrf *
sctp_allocate_vrf(int vrf_id)
{
  ....
  if (vrf->vrf_addr_hash == NULL) {
    /* No memory */
#ifdef INVARIANTS
    panic("No memory for VRF:%d", vrf_id);
#endif
    SCTP_FREE(vrf, SCTP_M_VRF);
    return (NULL);
  }
  ....
}

Report: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=218005

10. FreeBSD. CWE-570 (Expression is Always False)

V547 Expression 'value < 0' is always false. Unsigned type value is never < 0. ar9300_xmit.c 450

HAL_BOOL
ar9300_reset_tx_queue(struct ath_hal *ah, u_int q)
{
  u_int32_t cw_min, chan_cw_min, value;
  ....
  value = (ahp->ah_beaconInterval * 50 / 100)
    - ah->ah_config.ah_additional_swba_backoff
    - ah->ah_config.ah_sw_beacon_response_time
    + ah->ah_config.ah_dma_beacon_response_time;
  if (value < 10)
    value = 10;
  if (value < 0)
    value = 10;
  ....
}

Report: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=218007

11. FreeBSD. CWE-571 (Expression is Always True)

V617 Consider inspecting the condition. The '0x00000080' argument of the '|' bitwise operation contains a non-zero value. mac_bsdextended.c 128

#define  MBO_TYPE_DEFINED 0x00000080

static int
ugidfw_rule_valid(struct mac_bsdextended_rule *rule)
{
  ....
  if ((rule->mbr_object.mbo_neg | MBO_TYPE_DEFINED) &&      // <=
      (rule->mbr_object.mbo_type | MBO_ALL_TYPE) != MBO_ALL_TYPE)
    return (EINVAL);
  if ((rule->mbr_mode | MBI_ALLPERM) != MBI_ALLPERM)
    return (EINVAL);
  return (0);
}

Report: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=218039

Miscellaneous errors

1. FreeBSD

V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. if_em.c 1944

static int
em_if_msix_intr_assign(if_ctx_t ctx, int msix) 
{
  ....
  if (adapter->hw.mac.type < igb_mac_min) {
    tx_que->eims = 1 << (22 + i);
    adapter->ims |= tx_que->eims;
    adapter->ivars |= (8 | tx_que->msix) << (8 + (i * 4));
  } if (adapter->hw.mac.type == e1000_82575)                // <=
    tx_que->eims =
      E1000_EICR_TX_QUEUE0 << (i %  adapter->tx_num_queues);
  else
    tx_que->eims = 1 << (i %  adapter->tx_num_queues);
  ....
}

Report: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=218041

2. CoreCLR

V534 It is likely that a wrong variable is being compared inside the 'for' operator. Consider reviewing 'i'. ildasm mdinfo.cpp 1421

void MDInfo::DisplayFields(mdTypeDef inTypeDef,
                           COR_FIELD_OFFSET *rFieldOffset,
                           ULONG cFieldOffset)
 {
  ....
  for (ULONG i = 0; i < count; i++, totalCount++)
  {
    ....
    for (ULONG iLayout = 0; i < cFieldOffset; ++iLayout)  // <=
    {
      if (RidFromToken(rFieldOffset[iLayout].ridOfField) ==
          RidFromToken(fields[i]))
      {
        ....
      }
    }
  }
  ....
}

Report: https://github.com/dotnet/coreclr/pull/10414

Conclusion

We suggest downloading PVS-Studio analyzer and trying to check your project:

• Download PVS-Studio for Windows.
• Download PVS-Studio for Linux.

To remove the restrictions of a demo version, you can contact us and we will provide a temporary license key for you.

For a quick introduction to the analyzer, you can use the tools, tracking the runs of the compiler and collect all the necessary information for the analysis. See the description of the utilities CLMonitoring and pvs-studio-analyzer. If you are working with a classic type of project in Visual Studio, everything is much simpler: you should just choose in PVS-Studio menu a command "Check Solution".

#Cpp #Security

0

0

0

0

SHARE

Tags:

#Cpp #Security

We can email you a selection of our best articles once a month