docker / docker-bench-security

The Docker Bench for Security is a script that checks for dozens of common best-practices around deploying Docker containers in production.
Apache License 2.0
9.18k stars 1.02k forks source link

when jq output is equal "null", is not handled well and checks return a wrong PASS. Also, when jq is not available, cat does not handle well complex values like for example 'default-ulimits' #537

Closed halfluke closed 1 year ago

halfluke commented 1 year ago
  "default-ulimits": {
    "nofile": {
      "Hard": 64000,
      "Name": "nofile",
      "Soft": 64000
    }

this is an example of default-ulimits in daemon.json - jq in the helper function does not seem to handle it well Additionally, jq gives "none" when cannot find a string, and "none" is a PASS for checks 2.8 and 2.12, as it's not an empty string.

As I am not a jq expert, I've paused the use of jq (and simplified the cat command to handle the default-ulimits json structure) in get_docker_configuration_file_args() in helper_lib.sh.

Tested a bit but I wouldn't bet it's fool proof for all the possible configurations of the daemon.json file

line 125-129 functions/of helper_lib.sh

   #if "$HAVE_JQ"; then
   # jq --monochrome-output --raw-output ".[\"${OPTION}\"]" "$CONFIG_FILE"
  #else
  cat "$CONFIG_FILE" |  grep "$OPTION" 
 #fi

I do not see jq used anywhere else, it makes sense to use it to parse the json but the current output is problematic for an entry such as default-ulimits, and more importantly is not handled well by the checks 2.8 and 2.12 (maybe other checks as well?) when the output of jq is "none"

konstruktoid commented 1 year ago

Hi @halfluke and thanks for opening this issue, I'll have a look as soon as possible. Using https://docs.docker.com/engine/reference/commandline/dockerd/#on-linux as an example, do you have any settings that's not default so to say, just to test using jq?

halfluke commented 1 year ago

no, what I am actually saying is that - when using jq - if /etc/docker/daemon.json does not contain any entry for authorization-plugins or default-ulimits (check 2.8 and 2.12) , and those parameters have not been passed via the command line when starting dockerd either, you get a PASS in the test: [PASS] 2.8 - Ensure the default ulimit is configured appropriately (Manual) null ................. null [PASS] 2.12 - Ensure that authorization for Docker client commands is enabled (Scored)

(I am not sure where the "null" comes from but I think it has to do with the jq output?)

halfluke commented 1 year ago

a daemon.json with this, also passes both 2.8 and 2.12 for example:

{ "default-ulimits": {} }

halfluke commented 1 year ago
  akus="$(get_docker_configuration_file_args 'default-ulimits' | grep -v '{}')"
  if [ "$akus" != "null" ] && [ "$akus" != "" ]; then
  pass -c "$check"
  logcheckresult "PASS"
  return
  fi

and

  akus2="$(get_docker_configuration_file_args 'authorization-plugins' | grep -v '\[]')"
  if [ "$akus2" != "null" ] && [ "$akus2" != "" ]; then
  pass -s "$check"
  logcheckresult "PASS"
  return
  fi

seem to work with jq. They take care of both the absence of the entry in the config file, as well as the presence of the entry with [] or {} only. For cat, we need this simpliefied version: cat "$CONFIG_FILE" | grep "$OPTION" | tr -d '" ', Also notice default-ulimits (plural) in the config file check

halfluke commented 1 year ago

It looks like it's common for other checks as well, for the script to return PASS if the element is simply absent in the daemon.json file. For instance, I have just noticed the same for the "log level" check. Is this behaviour intentional?

halfluke commented 1 year ago

You may want to change each line where the daemon.json config file is checked, with an expression like this (for each check) - to handle jq output:

if [[ $(get_docker_configuration_file_args 'default-ulimits' | grep -v '{}') ]] && [[ $(get_docker_configuration_file_args 'default-ulimits' | grep -v '{}') != "null" ]] ; then
  pass -c "$check"
  logcheckresult "PASS"
  return
  fi

and NOT remove '{' in the helper function to account for settings like default-ulimits that use '{' - when jq is not used (unless there are side effects to this). cat "$CONFIG_FILE" | tr , '\n' | grep "$OPTION" | sed 's/.*://g' | tr -d '" ',

konstruktoid commented 1 year ago

sounds like a good idea, want to submit a PR?

halfluke commented 1 year ago

yes, if you give me some time to test the 7-8 'get_docker_configuration_file_args' checks against the sample daemon.json file :-)

By the way, even the sample daemon.json has a typo: "proxies": { "http-proxy": "http://proxy.example.com:80", "https-proxy": "https://proxy.example.com:443", "no-proxy": "*.test.example.com,.example.org", <- this last comma should not be there and may break a parser },

konstruktoid commented 1 year ago

no stress at all