Bug 220781 – [Flatpak SDK] Flatpak 1.10 environment variable issues

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Lauro Moura]

Flatpak 1.10 changed how environment variables are passed to bwrap, making our current scripts fail. For example, WEBKIT_BUILD_DIR_BIND_MOUNT and the LC_* variables. These are two cases where webkit-bwrap (our python wrapper around bwrap) does not find them in the os.environ dictionary and fails to process.

In the case of WEBKIT_BUILD_DIR_BIND_MOUNT, this is causing the bind-mount from `WebKitBuild/<PORT>/<CONFIG>` (host) to `/app/webkit/WebKitBuild/<CONFIG>` (sandbox) to not be enabled, leading to always building in the `WebKitBuild/<CONFIG>` dir.

In the case of LC_*, this brought back the locale issue fixed by r263397.

Bisecting flatpak, the commit https://github.com/flatpak/flatpak/commit/6d1773d2a54dde9b099043f07a2094a4f1c2f486 (run: Convert all environment variables into bwrap arguments) is the first failure.

Inspecting webkit-bwrap in a call to bash (Tools/Scripts/webkit-flatpak --verbose -c bash + some print's), webkit-bwrap is inkoved three times:

* ldconfig, with a normal os.environ
* flatpak-dbux-proxy, with the augmented os.environ with the extra info from webkit scripts
* bash (actual command). Here things start to differ.

Before the mentioned commit, the last invocation has the same environment as the second one, with all extra information.
But after the commit, now just LC_CTYPE is forwarded to webkit-bwrap.

Trying to use flatpak's `--env=....` did not expose the variable to bwrap, but only to the sandboxed process itself.

Meanwhile, falling back to 1.6.5 (Focal's) seems to be working fine. But as the change comes from a security advisory, there's a chance the changes will be picked by distros in the LTS packages.

[Philippe Normand]

I'm afraid we'll need to fix bug 213878 very soon indeed. They're backporting this CVE fix in older flatpak releases...

[Philippe Normand]

Created attachment 418118 [details]
Patch

[Adrian Perez]

Comment on attachment 418118 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418118&action=review

> Tools/flatpak/flatpakutils.py:854
> +            if envvar.startswith("LC"):

It would be safer to list the local-related LC_* variable names only:

  if envvar in ("LC_CTYPE=", "LC_NUMERIC", "LC_TIME", "LC_COLLATE",
                "LC_MONETARY", "LC_MESSAGES", "LC_PAPER", "LC_NAME",
                "LC_ADDRESS", "LC_TELEPHONE", "LC_MEASUREMENT",
                "LC_IDENTIFICATION", "LC_ALL"):

If the above feels like too much of a mouthful (though I think it's fine),
at least I would make this check “.startswith("LC_")” — to make sure some
other variable like “LCFOO” is not accidentally picked by the loop.

Also: This check does not match the “LANG” variable, which can also be
used to setup locale attributes. Is that on purpose, or maybe missing a
check here?

[Lauro Moura]

(In reply to Adrian Perez from comment #3)
> Comment on attachment 418118 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418118&action=review
> 
> > Tools/flatpak/flatpakutils.py:854
> > +            if envvar.startswith("LC"):
> 
> It would be safer to list the local-related LC_* variable names only:
> 
>   if envvar in ("LC_CTYPE=", "LC_NUMERIC", "LC_TIME", "LC_COLLATE",
>                 "LC_MONETARY", "LC_MESSAGES", "LC_PAPER", "LC_NAME",
>                 "LC_ADDRESS", "LC_TELEPHONE", "LC_MEASUREMENT",
>                 "LC_IDENTIFICATION", "LC_ALL"):
> 
> If the above feels like too much of a mouthful (though I think it's fine),
> at least I would make this check “.startswith("LC_")” — to make sure some
> other variable like “LCFOO” is not accidentally picked by the loop.
> 

Indeed `.startswith("LC")` is too greedy. I'm fine with listing them explicitly.

But one issue with this I just found was that `--unset-env` was added in flatpak 1.10 (as part of the overall CVE work that triggered this change). 

The patch as-is failed here with flatpak 1.6.5 from Ubuntu 20.04 LTS due to this. At the time I had tested it only with 1.10.0, sorry.

This could lead to duplicate information (and risking the old "nothing is more permanent than a temporary fix"), but what if we end up cleaning up the LC_ vars both in flatpakutils.py (through unset-env is flatpak >=1.10) and webkit-bwrap (for older flatpak versions) while the proper solution in bug213878 is developed?

> Also: This check does not match the “LANG” variable, which can also be
> used to setup locale attributes. Is that on purpose, or maybe missing a
> check here?

Currently LANG is always overridden by webkit-bwrap to en_US.UTF-8, but LANGUAGE is also cleared and needs to be added to the list.

[Philippe Normand]

I'm not sure bug 213878 is the way forward anymore, because it would imply a rather large NIH, reinventing what both `flatpak run` and the whole instance concept already implemented there (`flatpak ps`, `flatpak enter ...`).

[Philippe Normand]

Created attachment 418395 [details]
Patch

[Philippe Normand]

Comment on attachment 418395 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418395&action=review

> Tools/flatpak/flatpakutils.py:856
> +                if self.flatpak_version >= (1, 10, 0):

A version check is not necessarily right, would be better to check the existence of the option instead. I'll revisit this part.

[Adrian Perez]

Comment on attachment 418395 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418395&action=review

>> Tools/flatpak/flatpakutils.py:856
>> +                if self.flatpak_version >= (1, 10, 0):
> 
> A version check is not necessarily right, would be better to check the existence of the option instead. I'll revisit this part.

A version check is quite likely safer and more robust than trying to parse the
output of “flatpak --help” — I think it's okay to leave it like thi =)

[Philippe Normand]

I'm afraid this option will be added in older versions.

[Philippe Normand]

Created attachment 418438 [details]
Patch

[Lauro Moura]

Comment on attachment 418438 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418438&action=review

Tested here and it's working fine. Just two minor comments.

> Tools/flatpak/flatpakutils.py:176
> +    return output.find(option) != -1

What about `return option in output`, as it does not need the index?

> Tools/flatpak/flatpakutils.py:864
> +                if self.flatpak_version >= (1, 10, 0) or self.flatpak_run_has_unsetenv_option:

Does it need an explicit 1.10 check? Shouldn't `self.flatpak_has_unsetenv_option` always be true for it?

[Philippe Normand]

Comment on attachment 418395 [details]
Patch

I got confused thinking that new option was part of the CVE fix, but it's not the case. So it's simpler to land this version.

[Philippe Normand]

Committed r271938: <https://trac.webkit.org/changeset/271938>

[Radar WebKit Bug Importer]

<rdar://problem/73653233>