[PATCH 0/6] Harden includehandler against path traversals

[Felix Moessbauer]

While our security model clearly states, that all input needs to be
validated, it still makes sense to protect the user from configuration
issues like invalid cross-repo includes and includes outside of the
top repo (which are against kas design principle to fully describe
the build).

Thanks to Keyvan Hardani for reporting!

Best regards,
Felix Moessbauer
Siemens AG

Felix Moessbauer (6):
includehandler: ensure _source_dir is only set from main config file
test: check _source_dir rejection in additional config files
repos(git-clone): ensure path is not processed as option
includehandler: drop incorrect check for absolute path
includehandler: limit path traversals to repository
test: add check for path traversal guards of includehandler

docs/userguide/project-configuration.rst | 13 ++---
kas/includehandler.py | 70 ++++++++++++++----------
kas/repos.py | 6 +-
tests/test_includehandler.py | 39 +++++++++++++
4 files changed, 89 insertions(+), 39 deletions(-)

--
2.53.0

[Felix Moessbauer]

By that, we ensure the top level repo is not changed during parsing of
further config files.

Proposed-by: Keyvan Hardani <keyvan....@ieee.org>
Signed-off-by: Felix Moessbauer <felix.mo...@siemens.com>
---
kas/includehandler.py | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/kas/includehandler.py b/kas/includehandler.py
index 53f351856..8a8b05cfc 100644
--- a/kas/includehandler.py
+++ b/kas/includehandler.py
@@ -76,7 +76,8 @@ class ConfigFile():
self.is_lockfile = is_lockfile

@staticmethod
- def load(filename, is_external=False, is_lockfile=False):
+ def load(filename, is_external=False, is_lockfile=False,
+ is_main_file=True):
"""
Load the configuration file and test if version is supported.
"""
@@ -136,6 +137,10 @@ class ConfigFile():
'This has no effect and will be rejected soon.')

cf.src_dir = cf.config.get(SOURCE_DIR_OVERRIDE_KEY, None)
+ if cf.src_dir is not None and not is_main_file:
+ raise LoadConfigException(
+ f'{SOURCE_DIR_OVERRIDE_KEY!r} is only allowed in the '
+ 'top-level kas configuration', filename)
return cf


@@ -218,7 +223,8 @@ class IncludeHandler:
repos = repos or {}

def _internal_include_handler(filename, repo_path,
- is_external=False, is_lockfile=False):
+ is_external=False, is_lockfile=False,
+ is_main_file=False):
"""
Recursively loads include files and finds missing repos.

@@ -248,7 +254,8 @@ class IncludeHandler:
configs = []
try:
current_config = \
- ConfigFile.load(filename, is_external, is_lockfile)
+ ConfigFile.load(filename, is_external, is_lockfile,
+ is_main_file)
# if lockfile exists, inject it after current file
lockfile = self.get_lock_filename(filename)
if Path(lockfile).exists():
@@ -368,9 +375,10 @@ class IncludeHandler:
self.config_files = []
missing_repos = []
self.ensure_from_same_repo()
- for configfile in self.top_files:
+ for idx, configfile in enumerate(self.top_files):
cfgs, reps = _internal_include_handler(configfile,
- self.get_top_repo_path())
+ self.get_top_repo_path(),
+ is_main_file=idx == 0)
self.config_files.extend(cfgs)
for repo in reps:
if repo not in missing_repos:
--
2.53.0

[Felix Moessbauer]

This hardens the git clone command to ensure a provided path is not
processed as an option by git.

Proposed-by: Keyvan Hardani <keyvan....@ieee.org>
Signed-off-by: Felix Moessbauer <felix.mo...@siemens.com>
---

kas/repos.py | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kas/repos.py b/kas/repos.py
index 6d7586c71..adb59a96b 100644
--- a/kas/repos.py
+++ b/kas/repos.py
@@ -699,11 +699,11 @@ class GitRepo(RepoImpl):
self.remove_ref_prefix(self.branch)])

if createref:
- cmd.extend([self.effective_url, '--bare', srcdir])
+ cmd.extend(['--bare', '--', self.effective_url, srcdir])
elif srcdir:
- cmd.extend([srcdir, '--reference', srcdir, self.path])
+ cmd.extend(['--reference', srcdir, '--', srcdir, self.path])
else:
- cmd.extend([self.effective_url, self.path])
+ cmd.extend(['--', self.effective_url, self.path])
return cmd

def commit_cmd(self, env, author, msg, date):
--
2.53.0

[Felix Moessbauer]

Signed-off-by: Felix Moessbauer <felix.mo...@siemens.com>
---

tests/test_includehandler.py | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/tests/test_includehandler.py b/tests/test_includehandler.py
index 918b4ca60..15c856bdd 100644
--- a/tests/test_includehandler.py
+++ b/tests/test_includehandler.py
@@ -143,6 +143,18 @@ class TestLoadConfig:
with patch_open(includehandler, string='header: {version: "0.10"}'):
ConfigFile.load('x.yml')

+ def test_source_dir_rejected_in_non_main_file(self):
+ string = 'header: {version: 5}\n_source_dir: /some/path'
+ with patch_open(includehandler, string=string):
+ with pytest.raises(includehandler.LoadConfigException):
+ ConfigFile.load('x.yml', is_main_file=False)
+
+ def test_source_dir_allowed_in_main_file(self):
+ string = 'header: {version: 5}\n_source_dir: /some/path'
+ with patch_open(includehandler, string=string):
+ cf = ConfigFile.load('x.yml', is_main_file=True)
+ assert cf.src_dir == '/some/path'
+

class TestIncludes:
header = '''
--
2.53.0

[Felix Moessbauer]

The documentation states that includes with an absolute path are
supported. However, the check was implemented incorrectly as it assumed
an absolute path starts with ':' (os.path.pathsep) instead of '/'
(os.path.sep). As nobody ever complained about this, we just drop
support for including files via an absolute path.

Fixes: 34dd07e76 ("added initial implementation of a include handler")
Reported-by: Keyvan Hardani <keyvan....@ieee.org>

Signed-off-by: Felix Moessbauer <felix.mo...@siemens.com>
---

Note, that the diff is strangely mangled. The patch actually removes
the "if include.startswith(os.path.pathsep):" which always was False
and unindents the else block below.

docs/userguide/project-configuration.rst | 10 ++-----
kas/includehandler.py | 36 +++++++++++-------------
2 files changed, 19 insertions(+), 27 deletions(-)

diff --git a/docs/userguide/project-configuration.rst b/docs/userguide/project-configuration.rst
index bbb107d70..8a6162fbd 100644
--- a/docs/userguide/project-configuration.rst
+++ b/docs/userguide/project-configuration.rst
@@ -87,13 +87,9 @@ repository/layer like this:
- bsp.yml
- product.yml

-The paths to the files in the include list are either absolute, if they start
-with a `/`, or relative.
-
-If the path is relative and the configuration file is inside a repository,
-then path is relative to the repositories base directory. If the
-configuration file is not in a repository, then the path is relative to the
-parent directory of the file.
+If the configuration file is inside a repository, then path is relative to
+the repositories base directory. If the configuration file is not in a
+repository, then the path is relative to the parent directory of the file.

Including configuration files from other repos
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/kas/includehandler.py b/kas/includehandler.py
index 8a8b05cfc..804271d09 100644
--- a/kas/includehandler.py
+++ b/kas/includehandler.py
@@ -282,27 +282,23 @@ class IncludeHandler:

for include in header.get('includes', []):
if isinstance(include, str):
- includefile = ''
- if include.startswith(os.path.pathsep):
- includefile = include
- else:
- includefile = os.path.abspath(
- os.path.join(repo_path, include))
- if not os.path.exists(includefile):
- alternate = os.path.abspath(
- os.path.join(
- os.path.dirname(current_config.filename),
- include
- )
+ includefile = os.path.abspath(
+ os.path.join(repo_path, include))
+ if not os.path.exists(includefile):
+ alternate = os.path.abspath(
+ os.path.join(
+ os.path.dirname(current_config.filename),
+ include
)
- if os.path.exists(alternate):
- logging.warning(
- 'Falling back to file-relative addressing '
- 'of local include "%s"', include)
- logging.warning(
- 'Update your layer to repo-relative '
- 'addressing to avoid this warning')
- includefile = alternate
+ )
+ if os.path.exists(alternate):
+ logging.warning(
+ 'Falling back to file-relative addressing '
+ 'of local include "%s"', include)
+ logging.warning(
+ 'Update your layer to repo-relative '
+ 'addressing to avoid this warning')
+ includefile = alternate
(cfg, rep) = _internal_include_handler(
includefile,
repo_path,
--
2.53.0

[Felix Moessbauer]

Signed-off-by: Felix Moessbauer <felix.mo...@siemens.com>
---

tests/test_includehandler.py | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)

diff --git a/tests/test_includehandler.py b/tests/test_includehandler.py
index 15c856bdd..1f49f4465 100644
--- a/tests/test_includehandler.py
+++ b/tests/test_includehandler.py
@@ -485,3 +485,30 @@ v: {v3: z, v4: z}''')}
assert index['v2'] < index['v1']
assert index['v3'] < index['v1']
assert index['v5'] < index['v1']
+
+ def test_err_string_include_traversal(self, monkeypatch):
+ monkeypatch.setattr(includehandler, 'CONFIGSCHEMA', {})
+ header = self.__class__.header
+ fdict = {
+ 'x.yml': header.format(
+ ' includes: [{repo: rep, file: y.yml}]'),
+ '/top-repo/repos/rep/y.yml': header.format(
+ ' includes: ["../../other-repo/secret.yml"]'),
+ }
+ with patch_open(includehandler, dictionary=fdict):
+ ginc = includehandler.IncludeHandler(['x.yml'])
+ with pytest.raises(includehandler.IncludeException):
+ ginc.get_config(repos={'rep': '/top-repo/repos/rep'})
+
+ def test_err_repo_include_traversal(self, monkeypatch):
+ monkeypatch.setattr(includehandler, 'CONFIGSCHEMA', {})
+ header = self.__class__.header
+ fdict = {
+ 'x.yml': header.format(
+ ' includes: [{repo: rep, '
+ 'file: "../../other-repo/secret.yml"}]'),
+ }
+ with patch_open(includehandler, dictionary=fdict):
+ ginc = includehandler.IncludeHandler(['x.yml'])
+ with pytest.raises(includehandler.IncludeException):
+ ginc.get_config(repos={'rep': '/top-repo/repos/rep'})
--
2.53.0

[Felix Moessbauer]

When including files, kas currently allows to traverse to path outside
of the reference repository (and also outside of the top repository). As
a hardening measure against accidental misconfiguration (path-dependent
cross-repo references), we now enforce that a include file must be
within the referenced repository.

Reported-by: Reported-by: Keyvan Hardani <keyvan....@ieee.org>

Signed-off-by: Felix Moessbauer <felix.mo...@siemens.com>
---

docs/userguide/project-configuration.rst | 3 ++-
kas/includehandler.py | 30 ++++++++++++++++--------
2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/docs/userguide/project-configuration.rst b/docs/userguide/project-configuration.rst
index 8a6162fbd..37fecdbac 100644
--- a/docs/userguide/project-configuration.rst
+++ b/docs/userguide/project-configuration.rst
@@ -126,7 +126,8 @@ It's also possible to include configuration files from other repos like this:
# overwrite it by setting:
meta-yocto-bsp: excluded

-The files are addressed relative to the git repository path.
+The files are addressed relative to the git repository path and must not
+traverse outside of the referenced repo.

The include mechanism collects and merges the content from top to bottom and
depth first. That means that settings in one include file are overwritten
diff --git a/kas/includehandler.py b/kas/includehandler.py
index 804271d09..393f9b13e 100644
--- a/kas/includehandler.py
+++ b/kas/includehandler.py
@@ -208,6 +208,18 @@ class IncludeHandler:
'belong to the same repository or all '
'must be outside of versioning control')

+ def sanitize_include_path(self, base_path, include):
+ """
+ Ensure the created path is with the base_path.
+ Returns the resolved path of base_path / include.
+ """
+ repo = Path(base_path)
+ candidate = (repo / Path(include)).resolve()
+ if not candidate.is_relative_to(repo.resolve()):
+ raise IncludeException(
+ f'include {candidate} resolves outside repository {repo}')
+ return str(candidate)
+
def get_config(self, repos=None):
"""
Parameters:
@@ -282,15 +294,12 @@ class IncludeHandler:

for include in header.get('includes', []):
if isinstance(include, str):

- includefile = os.path.abspath(
- os.path.join(repo_path, include))
+ includefile = self.sanitize_include_path(
+ repo_path, include)

if not os.path.exists(includefile):
- alternate = os.path.abspath(
- os.path.join(
- os.path.dirname(current_config.filename),
- include
- )

- )
+ alternate = self.sanitize_include_path(
+ os.path.dirname(current_config.filename),
+ include)
if os.path.exists(alternate):
logging.warning(

'Falling back to file-relative addressing '

@@ -316,9 +325,10 @@ class IncludeHandler:
except KeyError:
raise IncludeException(
f'"file" is not specified: {include}')
- abs_includedir = os.path.abspath(includedir)
+ abs_includedir = Path(includedir).absolute()
(cfg, rep) = _internal_include_handler(
- os.path.join(abs_includedir, includefile),
+ self.sanitize_include_path(abs_includedir,
+ includefile),
abs_includedir, is_external=incexternal)
configs.extend(cfg)
missing_repos.extend(rep)
--
2.53.0

[Jan Kiszka]

To improve readability, I changed it like this:

is_main_file=(idx == 0)

> self.config_files.extend(cfgs)
> for repo in reps:
> if repo not in missing_repos:

Jan

--
Siemens AG, Foundational Technologies
Linux Expert Center

[Jan Kiszka]

On 29.04.26 17:08, Felix Moessbauer wrote:

> The documentation states that includes with an absolute path are
> supported. However, the check was implemented incorrectly as it assumed
> an absolute path starts with ':' (os.path.pathsep) instead of '/'
> (os.path.sep). As nobody ever complained about this, we just drop
> support for including files via an absolute path.
>
> Fixes: 34dd07e76 ("added initial implementation of a include handler")
> Reported-by: Keyvan Hardani <keyvan....@ieee.org>
> Signed-off-by: Felix Moessbauer <felix.mo...@siemens.com>
> ---
> Note, that the diff is strangely mangled. The patch actually removes
> the "if include.startswith(os.path.pathsep):" which always was False
> and unindents the else block below.
>

Indeed:

$ git diff -b HEAD^ kas/includehandler.py
diff --git a/kas/includehandler.py b/kas/includehandler.py
index ca968df..ae2e9ed 100644
--- a/kas/includehandler.py
+++ b/kas/includehandler.py
@@ -282,10 +282,6 @@ class IncludeHandler:

for include in header.get('includes', []):
if isinstance(include, str):
- includefile = ''
- if include.startswith(os.path.pathsep):
- includefile = include
- else:

includefile = os.path.abspath(
os.path.join(repo_path, include))
if not os.path.exists(includefile):

[Jan Kiszka]

On 29.04.26 17:08, Felix Moessbauer wrote:

Thanks, applied.