dev-sec / windows-baseline

DevSec Windows Baseline - InSpec Profile
https://dev-sec.io/baselines/windows/
Apache License 2.0
220 stars 67 forks source link

windows-011 array order failure #63

Open spencer-cdw opened 1 year ago

spencer-cdw commented 1 year ago

Description

Window-011 check is failing due to an array ordering problem

Screenshot 2022-11-15 at 12 57 17 PM

It is looking for expected: ["S-1-5-9", "S-1-5-32-544"] but is finding got: ["S-1-5-32-544", "S-1-5-9"]

Reproduction steps

This is with a stock windows 2016 build, using the stock inspec defaults

https://github.com/dev-sec/windows-baseline/blob/master/inspec.yml#L34-L38

Current Behavior

expected: ["S-1-5-9", "S-1-5-32-544"]
got: ["S-1-5-32-544", "S-1-5-9"]

Expected Behavior

The array order should not change. If the array order does change, it should not be considered a failure as long as the contents are equivalent.

OS / Environment

windows 2016

Inspec Version

5.18.14

Baseline Version

name: .
title: InSpec Profile
maintainer: The Authors
copyright: The Authors
copyright_email: you@example.com
license: Apache-2.0
summary: An InSpec Compliance Profile
version: 0.1.0
supports:
  platform-family: windows
depends:
  - name: windows-baseline
    git: https://github.com/dev-sec/windows-baseline
    tag: 2.1.9

Additional information

No response

spencer-cdw commented 1 year ago

A quick workaround may be to just override the default ordering in the inspec.yml file

name: .
title: InSpec Profile
maintainer: The Authors
copyright: The Authors
copyright_email: you@example.com
license: Apache-2.0
summary: An InSpec Compliance Profile
version: 0.1.0
supports:
  platform-family: windows
depends:
  - name: windows-baseline
    git: https://github.com/dev-sec/windows-baseline
    tag: 2.1.9
inputs:
  - name: se_network_logon_right
    value: ['S-1-5-32-544', 'S-1-5-9']
aaronlippold commented 1 year ago

You could also just used to be_in matcher witch don’t care about element order.

On Tue, Nov 15, 2022 at 15:09 Spencer @.***> wrote:

A quick workaround may be to just override the default ordering in the inspec.yml file

name: . title: InSpec Profile maintainer: The Authors copyright: The Authors copyright_email: @.*** license: Apache-2.0 summary: An InSpec Compliance Profile version: 0.1.0 supports: platform-family: windows depends:

— Reply to this email directly, view it on GitHub https://github.com/dev-sec/windows-baseline/issues/63#issuecomment-1315809135, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALK42CEOGHYDET7MZCZVITWIPUWXANCNFSM6AAAAAASBJ2GFE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

--

Aaron Lippold

@.***

260-255-4779

twitter/aim/yahoo,etc. 'aaronlippold'

aaronlippold commented 1 year ago

We don’t recommend modifying the inspec.yml file to override defaults. If you want to update an inputs value, you should do it either via the command line CLI flag or providing an input file.

On Thu, Nov 17, 2022 at 21:14 Aaron Lippold @.***> wrote:

You could also just used to be_in matcher witch don’t care about element order.

On Tue, Nov 15, 2022 at 15:09 Spencer @.***> wrote:

A quick workaround may be to just override the default ordering in the inspec.yml file

name: . title: InSpec Profile maintainer: The Authors copyright: The Authors copyright_email: @.*** license: Apache-2.0 summary: An InSpec Compliance Profile version: 0.1.0 supports: platform-family: windows depends:

— Reply to this email directly, view it on GitHub < https://github.com/dev-sec/windows-baseline/issues/63#issuecomment-1315809135 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AALK42CEOGHYDET7MZCZVITWIPUWXANCNFSM6AAAAAASBJ2GFE

. You are receiving this because you are subscribed to this thread.Message ID: @.***>

--

Aaron Lippold

@.***

260-255-4779

twitter/aim/yahoo,etc. 'aaronlippold'

— Reply to this email directly, view it on GitHub https://github.com/dev-sec/windows-baseline/issues/63#issuecomment-1319459178, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALK42HDSLRPHXFX7KLG7PTWI3RBTANCNFSM6AAAAAASBJ2GFE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

--

Aaron Lippold

@.***

260-255-4779

twitter/aim/yahoo,etc. 'aaronlippold'

spencer-cdw commented 1 year ago

Thanks @aaronlippold Does be_in support comparing 2 arrays?

Here is a POC leveraging be_in that loops over the first array and checks each value in the second array

inspec shell

foo=['42','9000']
bar=['9000','42']
foo.each do |f|
  describe f do
    it {should be_in bar}
  end
end
Profile:   inspec-shell
Version:   (not specified)
Target ID: 

  42
     ✔  is expected to be in "9000" and "42"
  9000
     ✔  is expected to be in "9000" and "42"

Test Summary: 2 successful, 0 failures, 0 skipped
aaronlippold commented 1 year ago

The being matcher will compare a list to an array and make sure it is a proper subset. So if I had an array, which was ABC, I would expect that a would be in ABC or that a C or CA would be in ABC. It doesn't care about order. I built this specifically to solve the list of Cyphers problem.

aaronlippold commented 1 year ago

So you don't really need to do a loop

schurzi commented 1 year ago

As I understand the be_in matcher returns true even for subsets, but we want to check for equality. Since the order of the elements is of no consequence, why don't we simply sort the input? :)

This should do the trick:

  describe security_policy do
    its('SeNetworkLogonRight.sort') { should eq input('se_network_logon_right') }
  end
spencer-cdw commented 1 year ago

I've tested .sort with no luck.

Screenshot 2022-11-23 at 12 26 47 PM Screenshot 2022-11-23 at 12 28 27 PM


I setup an additional test for the .sort method in inspec-shell. (Using the ini resource instead of the security_policy for simplicity)

touch foo.ini
echo "foo=['42','9000']" > foo.ini
bar=['9000','42']
describe ini('foo.ini') do
  its('foo.sort') {should eq bar}
end

Which returned error undefined method 'sort'

  INI foo.ini
     ×  foo.sort 
     undefined method `sort' for "['42','9000']":String

Looking at the Rspec documentation, I'm not finding a .sort method that could be used. https://rubydoc.info/gems/rspec-core/RSpec/Core/ExampleGroup

However there are examples of the azurerm_virtual_machines resource using os_disks.sort

its('os_disks.sort') { should eq [['MyDisk1'], ['MyDisk2']] }

https://docs.chef.io/inspec/resources/azurerm_virtual_machines/#os_disks

https://github.com/inspec/inspec-azure/blob/98d93642bdb5afb856a95edd9ac4e56b3e1fe1b6/test/integration/verify/controls/azurerm_virtual_machines.rb#L3

Looking at the code for security_policy I see the following methods

But I'm not seeing a .sort method 🤔

schurzi commented 1 year ago

This is very stranke. I also did some tests on a temporary system. My cinc-auditor shell behaves a bit different.

I'm using a default Windows 10 with the current version of cinc-auditor

PS C:\cinc-project\cinc-auditor\bin> .\cinc-auditor shell
Welcome to the interactive InSpec Shell
To find out how to use it, type: help

You are currently running on:

    Name:      windows_10_enterprise
    Families:  windows, os
    Release:   10.0.19044
    Arch:      x86_64

The security policy is set and is an array

inspec> security_policy.SeNetworkLogonRight
=> ["S-1-1-0", "S-1-5-32-544", "S-1-5-32-545", "S-1-5-32-551"]
inspec> security_policy.SeNetworkLogonRight.instance_of? Array
=> true

So I took the reported values and reversed them ["S-1-5-32-551", "S-1-5-32-545", "S-1-5-32-544", "S-1-1-0"]

inspec> describe security_policy do
inspec>   its('SeNetworkLogonRight.sort') { should eq ["S-1-5-32-551", "S-1-5-32-545", "S-1-5-32-544", "S-1-1-0"] }
inspec> end

Profile: inspec-shell
Version: (not specified)

  Security Policy
     [FAIL]  SeNetworkLogonRight.sort should eq ["S-1-5-32-551", "S-1-5-32-545", "S-1-5-32-544", "S-1-1-0"]

     expected: ["S-1-5-32-551", "S-1-5-32-545", "S-1-5-32-544", "S-1-1-0"]
          got: ["S-1-1-0", "S-1-5-32-544", "S-1-5-32-545", "S-1-5-32-551"]

     (compared using ==)

Test Summary: 0 successful, 1 failure, 0 skipped
inspec> describe security_policy do
inspec>   its('SeNetworkLogonRight.reverse') { should eq ["S-1-5-32-551", "S-1-5-32-545", "S-1-5-32-544", "S-1-1-0"] }
inspec> end

Profile: inspec-shell
Version: (not specified)

  Security Policy
     [PASS]  SeNetworkLogonRight.reverse should eq ["S-1-5-32-551", "S-1-5-32-545", "S-1-5-32-544", "S-1-1-0"]

Test Summary: 1 successful, 0 failures, 0 skipped

So the basic idea seems to work. I have no clue, why it does not work for your environment.

spencer-cdw commented 1 year ago

@schurzi Thank you for testing that. Your test looks very concise and logical. It appears that .sort and .reverse are valid methods on security_policy.

My tests were done through a layer of abstraction so I trust your results over mine.

Changing the order should fix it for me, but there is a possibility it will break the benchmark for other users if they have different ordering.

Would sorting both sides of the comparator be safer?

e.g.

  describe security_policy do
    its('SeNetworkLogonRight.sort') { should eq input('se_network_logon_right').sort }
  end
schurzi commented 1 year ago

Changing the order should fix it for me, but there is a possibility it will break the benchmark for other users if they have different ordering.

very good point. Are you up to generate a PR for this and do some testing in your environment?