IBM / unitxt / 42387451526

78%

push

github

Security: Resolve metric names against the catalog (#cs-12, #cs-13, #cs-14, #cs-18) (#1997)

The 4 py/path-injection alerts (HIGH, CWE-22) reported in #1994 are still open
after #1995, and are reported again in #1996: cs-12, cs-13, cs-14 and cs-18.
CodeQL analysed the merge commit (603f1c5ce, analysis 2093723778) and still
found all four.

#1995 was wrong, and its own commit message overstated the guarantee. It claimed
the rebuilt name "shares nothing with the raw input". As dataflow that is false:

    segments = name.split(constants.catalog_hierarchy_sep)
    if not all(is_name_legal_for_catalog(segment) for segment in segments):
        raise IllegalCatalogNameError(identifier)
    return constants.catalog_hierarchy_sep.join(segments), args

split() and join() propagate taint, so the returned string is still derived from
the request. The re.match guard sits in a separate `if`, which CodeQL does not
treat as sanitizing the returned value -- and it is right not to: a value
rebuilt from the input is only ever as good as the checks applied to it. The
post-merge SARIF shows the flow running straight through the new helper and out
the other side; all four sinks route through the join at artifact.py:112:

  service/metrics/main.py:69,85   metric_name (FastAPI path parameter)
  artifact.py:70,97               identifier -> separate_inside_and_outside...
  artifact.py:108,112             segments -> SEP.join(segments)
  service/metrics/main.py:85,107  safe_metric_name
  operators.py:1193,1212          artifact_identifier
  artifact.py:612,628             artifact_rep
  artifact.py:303-306             open(path)            <- cs-12, cs-13, cs-14
  utils.py:221-222                open(path)            <- cs-18

The fix stops deriving the value from the request at all. sanitize_catalog_
identifier() now resolves the requested name against the names the local
catalogs publish and returns *the catalog's own string*, so what reaches a
loader originates in trusted... (continued)

2784 of 4026 branches covered (69.15%)

Branch coverage included in aggregate %.

10937 of 13580 relevant lines covered (80.54%)

0.81 hits per line