Summary: | [Flatpak SDK] Flatpak 1.10 environment variable issues | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Lauro Moura <lmoura> | ||||||||
Component: | Tools / Tests | Assignee: | Philippe Normand <pnormand> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aperez, pnormand, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=221068 https://bugs.webkit.org/show_bug.cgi?id=221070 https://bugs.webkit.org/show_bug.cgi?id=221711 |
||||||||||
Attachments: |
|
Description
Lauro Moura
2021-01-20 15:19:46 PST
I'm afraid we'll need to fix bug 213878 very soon indeed. They're backporting this CVE fix in older flatpak releases... Created attachment 418118 [details]
Patch
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? (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. 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 ...`). Created attachment 418395 [details]
Patch
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. 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 =) I'm afraid this option will be added in older versions. Created attachment 418438 [details]
Patch
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? 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.
Committed r271938: <https://trac.webkit.org/changeset/271938> |