Дефекты безопасности, которые устранила команда PVS-Studio на этой неделе: выпуск N3

Мы решили в меру своих сил регулярно искать и устранять потенциальные уязвимости и баги в различных проектах. Можно назвать это помощью open-source проектам. Можно — разновидностью рекламы или тестированием анализатора. Еще вариант — очередной способ привлечения внимания к вопросам качества и надёжности кода. На самом деле, не важно название, просто нам нравится это делать. Назовём это необычным хобби. Давайте посмотрим, что интересного было обнаружено в коде различных проектов на этой неделе. Мы нашли время сделать исправления и предлагаем вам ознакомиться с ними.

Для тех, кто ещё не знаком с инструментом PVS-Studio

PVS-Studio — это инструмент, который выявляет в коде многие разновидности ошибок и уязвимостей. PVS-Studio выполняет статический анализ кода и рекомендует программисту обратить внимание на участки программы, в которых с большой вероятностью содержатся ошибки. Наилучший эффект достигается тогда, когда статический анализ выполняется регулярно. Идеологически предупреждения анализатора подобны предупреждениям компилятора. Но в отличии от компиляторов, PVS-Studio выполняет более глубокий и разносторонний анализ кода. Это позволяет ему находить ошибки в том числе и в компиляторах: GCC; LLVM 1, 2, 3; Roslyn.

Поддерживается анализ кода на языках C, C++ и C#. Анализатор работает под управлением Windows и Linux. В Windows анализатор может интегрироваться как плагин в Visual Studio.

Для дальнейшего знакомства с анализатором, предлагаем изучить следующие материалы:

• Подробная презентация на сайте SlideShare. В формате видео она доступна на YouTube (47 минут).
• Статьи о проверенных открытых проектах.
• PVS-Studio: поиск дефектов безопасности.

Потенциальные уязвимости (weaknesses)

В этом разделе приведены дефекты, которые попадают под классификацию CWE и, по сути, являются потенциальными уязвимостями. Конечно, не в каждом проекте дефекты безопасности создают какую-то практическую угрозу, но хочется продемонстрировать, что мы умеем находить подобные ситуации.

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 %dn", 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: 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

Прочие ошибки

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

Заключение

Предлагаем скачать анализатор PVS-Studio и попробовать проверить ваш проект:

• Скачать PVS-Studio для Windows
• Скачать PVS-Studio для Linux

Для снятия ограничения демонстрационной версии, вы можете написать нам, и мы отправим вам временный ключ.

Для быстрого знакомства с анализатором, вы можете воспользоваться утилитами, отслеживающими запуски компилятора и собирающие для проверки всю необходимую информацию. См. описание утилиты CLMonitoring и pvs-studio-analyzer. Если вы работаете с классическим типом проекта в Visual Studio, то всё ещё проще: достаточно выбрать в меню PVS-Studio команду «Check Solution».