Skip to content

Commit

Permalink
Make user/vendor data sensitive and remove log permissions (#2144)
Browse files Browse the repository at this point in the history
Because user data and vendor data may contain sensitive information,
this commit ensures that any user data or vendor data written to
instance-data.json gets redacted and is only available to root user.

Also, modify the permissions of cloud-init.log to be 640, so that
sensitive data leaked to the log isn't world readable.
Additionally, remove the logging of user data and vendor data to
cloud-init.log from the Vultr datasource.

LP: #2013967
CVE: CVE-2023-1786
  • Loading branch information
TheRealFalcon committed Apr 26, 2023
1 parent c1b4722 commit a378b7e
Show file tree
Hide file tree
Showing 6 changed files with 77 additions and 23 deletions.
9 changes: 6 additions & 3 deletions cloudinit/sources/DataSourceLXD.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
import time
from enum import Flag, auto
from json.decoder import JSONDecodeError
from typing import Any, Dict, List, Optional, Union, cast
from typing import Any, Dict, List, Optional, Tuple, Union, cast

import requests
from requests.adapters import HTTPAdapter
Expand Down Expand Up @@ -168,11 +168,14 @@ class DataSourceLXD(sources.DataSource):
_network_config: Union[Dict, str] = sources.UNSET
_crawled_metadata: Union[Dict, str] = sources.UNSET

sensitive_metadata_keys = (
"merged_cfg",
sensitive_metadata_keys: Tuple[
str, ...
] = sources.DataSource.sensitive_metadata_keys + (
"user.meta-data",
"user.vendor-data",
"user.user-data",
"cloud-init.user-data",
"cloud-init.vendor-data",
)

skip_hotplug_detect = True
Expand Down
14 changes: 6 additions & 8 deletions cloudinit/sources/DataSourceVultr.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
# Vultr Metadata API:
# https://www.vultr.com/metadata/

from typing import Tuple

import cloudinit.sources.helpers.vultr as vultr
from cloudinit import log as log
from cloudinit import sources, stages, util, version
Expand All @@ -28,6 +30,10 @@ class DataSourceVultr(sources.DataSource):

dsname = "Vultr"

sensitive_metadata_keys: Tuple[
str, ...
] = sources.DataSource.sensitive_metadata_keys + ("startup-script",)

def __init__(self, sys_cfg, distro, paths):
super(DataSourceVultr, self).__init__(sys_cfg, distro, paths)
self.ds_cfg = util.mergemanydict(
Expand All @@ -54,13 +60,8 @@ def _get_data(self):
self.get_datasource_data(self.metadata)

# Dump some data so diagnosing failures is manageable
LOG.debug("Vultr Vendor Config:")
LOG.debug(util.json_dumps(self.metadata["vendor-data"]))
LOG.debug("SUBID: %s", self.metadata["instance-id"])
LOG.debug("Hostname: %s", self.metadata["local-hostname"])
if self.userdata_raw is not None:
LOG.debug("User-Data:")
LOG.debug(self.userdata_raw)

return True

Expand Down Expand Up @@ -155,6 +156,3 @@ def get_datasource_list(depends):
)
config = md["vendor-data"]
sysinfo = vultr.get_sysinfo()

print(util.json_dumps(sysinfo))
print(util.json_dumps(config))
28 changes: 25 additions & 3 deletions cloudinit/sources/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,10 @@ def process_instance_metadata(metadata, key_path="", sensitive_keys=()):
sub_key_path = key_path + "/" + key
else:
sub_key_path = key
if key in sensitive_keys or sub_key_path in sensitive_keys:
if (
key.lower() in sensitive_keys
or sub_key_path.lower() in sensitive_keys
):
sens_keys.append(sub_key_path)
if isinstance(val, str) and val.startswith("ci-b64:"):
base64_encoded_keys.append(sub_key_path)
Expand All @@ -133,16 +136,27 @@ def redact_sensitive_keys(metadata, redact_value=REDACT_SENSITIVE_VALUE):
Replace any keys values listed in 'sensitive_keys' with redact_value.
"""
# While 'sensitive_keys' should already sanitized to only include what
# is in metadata, it is possible keys will overlap. For example, if
# "merged_cfg" and "merged_cfg/ds/userdata" both match, it's possible that
# "merged_cfg" will get replaced first, meaning "merged_cfg/ds/userdata"
# no longer represents a valid key.
# Thus, we still need to do membership checks in this function.
if not metadata.get("sensitive_keys", []):
return metadata
md_copy = copy.deepcopy(metadata)
for key_path in metadata.get("sensitive_keys"):
path_parts = key_path.split("/")
obj = md_copy
for path in path_parts:
if isinstance(obj[path], dict) and path != path_parts[-1]:
if (
path in obj
and isinstance(obj[path], dict)
and path != path_parts[-1]
):
obj = obj[path]
obj[path] = redact_value
if path in obj:
obj[path] = redact_value
return md_copy


Expand Down Expand Up @@ -250,6 +264,14 @@ class DataSource(CloudInitPickleMixin, metaclass=abc.ABCMeta):
sensitive_metadata_keys: Tuple[str, ...] = (
"merged_cfg",
"security-credentials",
"userdata",
"user-data",
"user_data",
"vendordata",
"vendor-data",
# Provide ds/vendor_data to avoid redacting top-level
# "vendor_data": {enabled: True}
"ds/vendor_data",
)

# True on datasources that may not see hotplugged devices reflected
Expand Down
4 changes: 3 additions & 1 deletion cloudinit/stages.py
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,9 @@ def _initialize_filesystem(self):
util.ensure_dirs(self._initial_subdirs())
log_file = util.get_cfg_option_str(self.cfg, "def_log_file")
if log_file:
util.ensure_file(log_file, mode=0o640, preserve_mode=True)
# At this point the log file should have already been created
# in the setupLogging function of log.py
util.ensure_file(log_file, mode=0o640, preserve_mode=False)
perms = self.cfg.get("syslog_fix_perms")
if not perms:
perms = {}
Expand Down
27 changes: 26 additions & 1 deletion tests/unittests/sources/test_init.py
Original file line number Diff line number Diff line change
Expand Up @@ -458,12 +458,24 @@ def test_get_data_writes_redacted_public_json_instance_data(self):
"cred2": "othersekret",
}
},
"someother": {
"nested": {
"userData": "HIDE ME",
}
},
"VENDOR-DAta": "HIDE ME TOO",
},
)
self.assertCountEqual(
(
"merged_cfg",
"security-credentials",
"userdata",
"user-data",
"user_data",
"vendordata",
"vendor-data",
"ds/vendor_data",
),
datasource.sensitive_metadata_keys,
)
Expand All @@ -490,7 +502,9 @@ def test_get_data_writes_redacted_public_json_instance_data(self):
"base64_encoded_keys": [],
"merged_cfg": REDACT_SENSITIVE_VALUE,
"sensitive_keys": [
"ds/meta_data/VENDOR-DAta",
"ds/meta_data/some/security-credentials",
"ds/meta_data/someother/nested/userData",
"merged_cfg",
],
"sys_info": sys_info,
Expand All @@ -500,6 +514,7 @@ def test_get_data_writes_redacted_public_json_instance_data(self):
"availability_zone": "myaz",
"cloud-name": "subclasscloudname",
"cloud_name": "subclasscloudname",
"cloud_id": "subclasscloudname",
"distro": "ubuntu",
"distro_release": "focal",
"distro_version": "20.04",
Expand All @@ -522,14 +537,18 @@ def test_get_data_writes_redacted_public_json_instance_data(self):
"ds": {
"_doc": EXPERIMENTAL_TEXT,
"meta_data": {
"VENDOR-DAta": REDACT_SENSITIVE_VALUE,
"availability_zone": "myaz",
"local-hostname": "test-subclass-hostname",
"region": "myregion",
"some": {"security-credentials": REDACT_SENSITIVE_VALUE},
"someother": {
"nested": {"userData": REDACT_SENSITIVE_VALUE}
},
},
},
}
self.assertCountEqual(expected, redacted)
self.assertEqual(expected, redacted)
file_stat = os.stat(json_file)
self.assertEqual(0o644, stat.S_IMODE(file_stat.st_mode))

Expand Down Expand Up @@ -574,6 +593,12 @@ def test_get_data_writes_json_instance_data_sensitive(self):
(
"merged_cfg",
"security-credentials",
"userdata",
"user-data",
"user_data",
"vendordata",
"vendor-data",
"ds/vendor_data",
),
datasource.sensitive_metadata_keys,
)
Expand Down
18 changes: 11 additions & 7 deletions tests/unittests/test_stages.py
Original file line number Diff line number Diff line change
Expand Up @@ -606,19 +606,23 @@ def test_log_files_existence_is_ensured_if_configured(self, init, tmpdir):
# Assert we create it 0o640 by default if it doesn't already exist
assert 0o640 == stat.S_IMODE(log_file.stat().mode)

def test_existing_file_permissions_are_not_modified(self, init, tmpdir):
"""If the log file already exists, we should not modify its permissions
def test_existing_file_permissions(self, init, tmpdir):
"""Test file permissions are set as expected.
CIS Hardening requires 640 permissions. These permissions are
currently hardcoded on every boot, but if there's ever a reason
to change this, we need to then ensure that they
are *not* set every boot.
See https://bugs.launchpad.net/cloud-init/+bug/1900837.
"""
# Use a mode that will never be made the default so this test will
# always be valid
mode = 0o606
log_file = tmpdir.join("cloud-init.log")
log_file.ensure()
log_file.chmod(mode)
# Use a mode that will never be made the default so this test will
# always be valid
log_file.chmod(0o606)
init._cfg = {"def_log_file": str(log_file)}

init._initialize_filesystem()

assert mode == stat.S_IMODE(log_file.stat().mode)
assert 0o640 == stat.S_IMODE(log_file.stat().mode)

0 comments on commit a378b7e

Please sign in to comment.