The release notes for 5.2 state the following:- "There is a new shell option, `patsub_replacement'. When enabled, a `&' in the replacement string of the pattern substitution expansion is replaced by the portion of the string that matched the pattern. Backslash will escape the `&' and insert a literal `&'." This is a backwards-incompatible change, because the patsub_replacement shopt defaults to being enabled. $ echo "$BASH_VERSION" 5.1.16(1)-release $ val='foo - bar'; echo "${var//-/‐}" foo ‐ bar $ echo "$BASH_VERSION" 5.2.9(1)-release $ val='foo - bar'; echo "${var//-/‐}" foo - dash;bar What's more, the behaviour is not affected by BASH_COMPAT. The only way to impede this new behaviour is to disable the option with `shopt -u patsub_replacement`. Otherwise, any affected code will need to be adjusted, such that replacement ampersands intended as being literal are quoted. As such, the easiest way to defend against this change would be to disable the option for all of the existing EAPIs. In any case, I intend to perform a rudimentary audit, so as to get a sense of the degree to which it affects Gentoo.
Sorry, the sample commands didn't quite match the output shown, owing to a copy-paste mishap on my part. Here are the examples again, as they were originally intended. $ echo "$BASH_VERSION" 5.1.16(1)-release $ var='foo - bar'; echo "${var//-/‐}" foo ‐ bar $ echo "$BASH_VERSION" 5.2.9(1)-release $ var='foo - bar'; echo "${var//-/‐}" foo -dash; bar Also, if the substitution contains a parameter expansion, said expansion must be quoted to protect against patsub_replacement. $ var='foo - bar' replace='‐' $ echo "${var//-/"$replace"}" # safe in both 5.1 and 5.2 foo ‐ bar
I have searched for string replacing parameter expansions incorporating literal ampersands in: - the gentoo repo - the repo for portage itself Obviously, this is not a comprehensive audit, but it's better than nothing. I found no matches, which is good. Next, I will be checking for parameter expansions that incorporate unquoted parameter expansions. There is no question that this will produce many matches. I shall make an attempt to identify those that may be contending with untrustworthy inputs.
This has come up in the wild once already(!) From #gentoo-haskell: """ [16:57:12] <@hololeap> sam_, do you possibly know why there would be this discrepancy on his system? https://github.com/gentoo-haskell/gentoo-haskell/issues/1363#issuecomment-1328213696 [18:32:43] <@hololeap> It looks like it's an issue when patsub_replacement is enabled using shopt [18:33:45] <@hololeap> Should we add a line to escape '&' characters, or make sure this shopt is disabled somehow in the eclass? [18:34:21] <@hololeap> (Or use sed instead of bash pattern substitution) [19:23:14] <@hololeap> I went with the sed option so we know the behavior is consistent """
The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/proj/portage.git/commit/?id=69cac73ba0a7bcf2e2cff88c60d389895a550623 commit 69cac73ba0a7bcf2e2cff88c60d389895a550623 Author: Sam James <sam@gentoo.org> AuthorDate: 2022-11-30 01:09:12 +0000 Commit: Sam James <sam@gentoo.org> CommitDate: 2022-11-30 01:22:35 +0000 ebuild.sh: disable patsub_replacement in Bash 5.2 patsub_replacement is a new option in bash-5.2 that is also default-on in that release. The default value is not gated by BASH_COMPAT (see bug #881383), hence we need to disable it for older Bashes to avoid behaviour changes in ebuilds and eclasses. Thanks to Kerin for both raising this & being persistent with trying to get Bash 5.2 to be suitable for use in Gentoo. Bug: https://bugs.gentoo.org/881383 Thanks-to: Kerin Millar <kfm@plushkava.net> Signed-off-by: Sam James <sam@gentoo.org> NEWS | 5 +++++ bin/ebuild.sh | 14 ++++++++++++++ 2 files changed, 19 insertions(+)
(In reply to Sam James from comment #3) > This has come up in the wild once already(!) So soon :( I regret not finishing my audit yet as that one would have been easily spotted on my part. Meanwhile, do we have a version of portage (in ~arch, at least) that disables patsub_replacement yet? If not, the sooner the better, I think.
Also, a reminder to all that using sed will seldom be an appropriate workaround. As I made clear in comment 1, double quoting the variable expansions that are contained by the replacement string should suffice in most cases. Please take it as an opportunity to write more robust code. I'm concerned that ebuild developers will begin reaching for sed and, in some cases, perform sed code injection rather than simply fix their parameter expansions. To think of the replacement string as being literal is a mistake and always has been, even prior to the introduction of patsub_replacement. For the benefit of those that do not have bash >=5.2 installed yet, here is the relevant section of the manual: https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html. Taking the ${parameter/pattern/string} form as an example, it notes that "string undergoes tilde expansion, parameter and variable expansion, arithmetic expansion, command and process substitution, and quote removal". However, it goes on to say that "quoting any part of string inhibits replacement in the expansion of the quoted portion, including replacement strings stored in shell variables." Essentially, developers should apply what they (hopefully) already know about shell quoting to the so-called string.
The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=abc758f4cb662024ad88c17fefb68767785f271e commit abc758f4cb662024ad88c17fefb68767785f271e Author: Sam James <sam@gentoo.org> AuthorDate: 2022-11-30 07:05:38 +0000 Commit: Sam James <sam@gentoo.org> CommitDate: 2022-11-30 07:05:38 +0000 sys-apps/portage: backport patsub_replacement disablement for Bash 5.2 Quoting Portage's NEWS: """ * ebuild: Handle Bash 5.2's change in behavior which enables the shopt 'patsub_replacement' by default. This is needed to avoid breaking existing working ebuilds. Future EAPIs will need to adjust the logic added by this change. See bug #881383. """ Bug: https://bugs.gentoo.org/881383 Signed-off-by: Sam James <sam@gentoo.org> ...ortage-3.0.39-bash-5.2-patsub_replacement.patch | 49 ++++++++++++++++++++++ ...e-3.0.39-r2.ebuild => portage-3.0.39-r3.ebuild} | 1 + 2 files changed, 50 insertions(+)
Okay, backport done to ~arch, thanks for the prod. Leaving open for: - audititing repositories like ::haskell, ::guru (any others?) - note in PMS - making sure we don't forget to stable a new Portage before eventually Bash 5.2 gets to stable (no hurry there)
The bug has been closed via the following commit(s): https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=457170222d9a1133406b90955269580e4db2ee2b commit 457170222d9a1133406b90955269580e4db2ee2b Author: Sam James <sam@gentoo.org> AuthorDate: 2022-12-01 05:16:52 +0000 Commit: Sam James <sam@gentoo.org> CommitDate: 2022-12-01 05:16:52 +0000 sys-apps/portage: add 3.0.40 Closes: https://bugs.gentoo.org/814380 Closes: https://bugs.gentoo.org/850127 Closes: https://bugs.gentoo.org/881383 Closes: https://bugs.gentoo.org/883071 Closes: https://bugs.gentoo.org/883307 Closes: https://bugs.gentoo.org/883437 Signed-off-by: Sam James <sam@gentoo.org> sys-apps/portage/Manifest | 1 + sys-apps/portage/portage-3.0.40.ebuild | 283 +++++++++++++++++++++++++++++++++ 2 files changed, 284 insertions(+)
I began the task of reviewing the ::gentoo repo in all earnestness only to encounter an obstacle that limits the scope of the procedure. To put this obstacle into perspective, allow me to briefly reflect upon how string-replacing parameter expansions ought to be written. In principle, these are all examples of bad code. ${var/$search_string_var/$replacement_string_var} ${var/$search_string_var/$(replacement_string_printer)} ${var/$(search_string_printer)/$replacement_string_var} The correct way to write them would be as follows. ${var/"$search_string_var"/"$replacement_string_var"} ${var/"$search_string_var"/"$(replacement_string_printer)"} ${var/"$(search_string_printer)"/"$replacement_string_var"} Why do I use the word, would? Well, it has been the correct way to go about it since the release of bash-4.3 in Feburary 2014. Alas, portage sets the BASH_COMPAT level to 42 for EAPIs 6 and 7. Below is a quote from "The Ultimate Guide to EAPI 7" [*]. "One of the suggestions for EAPI 7 was to require bash-4.3. However, this was rejected as it was determined that it does not add any ‘killer features’ that could benefit ebuilds." Notwithstanding that hindsight is everything, this turns out to have been a bad decision. The quote removal behaviour was changed in bash-4.3, for the better. Because portage demands that the compatibility level be set in accordance with the minimum supported bash version that a given EAPI level specifies, we are thus forced to account for the antiquated quote removal behaviour until such time as EAPI 7 becomes obsolete. But what exactly was the nature of this change? Such can be easily explained with an example. $ declare -p BASH_VERSION declare -- BASH_VERSION="5.1.16(1)-release" $ x='f*'; y='baz'; var='f*bar'; printf '%s\n' "${var/"$x"/"$y"}" bazbar Here, we observe an ideal behaviour. That is, the value of x is taken to be literal (rather than a globbing pattern), and the value of y is taken to be literal (thus impeding patsub replacement and such). In other words, the use of double quotes renders this code robust, with the quotes, themselves, undergoing removal. Now let's see what happens in the 42 compat mode. $ BASH_COMPAT=42 $ printf '%s\n' "${var/"$x"/"$y"}" "baz"bar Well, the good news is that the quotes still prevent the value of x from being treated as a globbing pattern. The bad news is that the quotes are not removed from the replacement string! So, what does this mean in practice? Quite simply, we are prevented from writing such parameter expansions appropriately other than within ebuilds that declare EAPI=8. If we go about it in the 'right' way within an ebuild that declares EAPI=7 or earlier, spurious quotes will be incorporated by the replacement string. Yet, if we go about it in the 'wrong' way, the code will remain vulnerable to the effects of patsub_replacement in some future EAPI! Nor can we fix any affected code in an eclass because, in most cases, ebuilds are free to import arbitrary eclasses, irrespective of the targeted EAPI level. All in all, it is a frustrating situation. For now, the scope of any corrective work is limited to the following:- 1) judiciously using quotes as a part of the search string, where needed 2) judiciously using quotes as a part of the replacement string, where needed, only in ebuilds (not eclasses!) targeting EAPI=8 The reason that #2 is acceptable is because the compat level is set to 50 for EAPI 8. Any further work is stymied until the use of EAPI <=7 is eliminated, at which point eclasses may also be considered. Given all of the above, here is how I propose that we deal with the situation. Firstly, I shall continue my review process but will be limiting its scope to ebuilds that target EAPI 8. Secondly, while it will take a long time to bring this bug to closure, it need no longer block bug 881379. Thirdly, the sooner all ebuilds targeting EAPI <=7 are migrated to EAPI 8, the better, and I would encourage the broader community to do so at their earliest convenience. After all, that is the only avenue by which any affected code can be re-written in the proper way. Fourthly, on every occasion that an ebuild is migrated to EAPI 8, it can be - and should be - reviewed to that effect. For anyone else interested, this is the method by which I have been identifying improperly written parameter expansions, using the_silver_searcher. $ ag '\$\{[_[:alpha:]][_[:alnum:]]*(/[^/]|//)[^}]*/[^}]*(?<!")\$[^}]*\}' Now, that regular expression is by no means perfect. Nevertheless, it is capable of identifying the overwhelming majority of eligible cases. [*] https://mgorny.pl/articles/the-ultimate-guide-to-eapi-7.html#minimal-bash-version-at-4-3
Review process completed for ::gentoo (EAPI 8 only). Five commits in total. See https://codeberg.org/kerframil/gentoo/commits/branch/bug-881383-fixes.
Review process completed for ::haskell. No changes needed.
Review process completed for ::guru (EAPI 8 only). No changes needed.
Created attachment 839631 [details, diff] 0001-bin-etc-update-Apply-patsub_replacement-defences.patch I reviewed portage itself and found that the bin/etc-update utility falls within the purview of this bug. The patch is trivial so I'm attaching it here.
The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/proj/portage.git/commit/?id=2294cb62b65431df06ded745ded8fcfb6a4d5865 commit 2294cb62b65431df06ded745ded8fcfb6a4d5865 Author: Kerin Millar <kfm@plushkava.net> AuthorDate: 2022-12-04 11:14:34 +0000 Commit: Sam James <sam@gentoo.org> CommitDate: 2022-12-05 04:04:02 +0000 bin/etc-update: Apply patsub_replacement defences Per bug #881383, string replacing forms of parameter expansion must take care to quote - or appropriately escape - any nested parameter expansions, assuming that their values are intended to be taken literally (as is almost invariably the case). This has long been the case, but the introduction of the new patsub_replacement option in bash >=5.2 has brought the issue to the fore. This commit addresses two instances in which the etc-update script could unintentionally induce patsub replacement. There are many other quality issues that affect this script but this is enough to address the aforementioned bug. Bug: https://bugs.gentoo.org/881383 Signed-off-by: Kerin Millar <kfm@plushkava.net> Signed-off-by: Sam James <sam@gentoo.org> NEWS | 3 ++- bin/etc-update | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-)
The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=6a02df6f8f73f9d69ec9524f33adf761717118d4 commit 6a02df6f8f73f9d69ec9524f33adf761717118d4 Author: Kerin Millar <kfm@plushkava.net> AuthorDate: 2022-12-04 10:42:25 +0000 Commit: Sam James <sam@gentoo.org> CommitDate: 2022-12-06 09:11:28 +0000 app-editors/jupp: Apply patsub_replacement defences Per bug #881383, string replacing forms of parameter expansion must take care to quote - or appropriately escape - any nested parameter expansions, assuming that their values are intended to be taken literally (as is almost invariably the case). This has long been the case, but the introduction of the new patsub_replacement option in bash >=5.2 has brought the issue to the fore. In the case of the app-editors/jupp package, the improper quoting is not yet causing any issues. Still, it is better to write the code properly to begin with, especially considering the demonstrative value of robust code. Signed-off-by: Kerin Millar <kfm@plushkava.net> Bug: https://bugs.gentoo.org/881383 Signed-off-by: Sam James <sam@gentoo.org> app-editors/jupp/jupp-3.1_p41.ebuild | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=82e42fc71fbb1be48d44533ad636a144c4e451a1 commit 82e42fc71fbb1be48d44533ad636a144c4e451a1 Author: Kerin Millar <kfm@plushkava.net> AuthorDate: 2022-12-04 10:36:08 +0000 Commit: Sam James <sam@gentoo.org> CommitDate: 2022-12-06 09:11:27 +0000 app-dicts/*: Apply patsub_replacement defences Per bug #881383, string replacing forms of parameter expansion must take care to quote - or appropriately escape - any nested parameter expansions, assuming that their values are intended to be taken literally (as is almost invariably the case). This has long been the case, but the introduction of the new patsub_replacement option in bash >=5.2 has brought the issue to the fore. In the case of the app-dicts package category, the improper quoting is not yet causing any issues. Still, it is better to write the code properly to begin with, especially considering the demonstrative value of robust code. Signed-off-by: Kerin Millar <kfm@plushkava.net> Bug: https://bugs.gentoo.org/881383 Signed-off-by: Sam James <sam@gentoo.org> app-dicts/aspell-ast/aspell-ast-0.01-r1.ebuild | 2 +- app-dicts/aspell-be/aspell-be-0.01-r2.ebuild | 2 +- app-dicts/aspell-ca/aspell-ca-2.5.0-r1.ebuild | 2 +- app-dicts/aspell-de/aspell-de-20161207.7.0-r1.ebuild | 2 +- app-dicts/aspell-eo/aspell-eo-2.1.20000225.2-r1.ebuild | 2 +- app-dicts/aspell-ga/aspell-ga-5.1.0-r1.ebuild | 2 +- app-dicts/aspell-gl/aspell-gl-0.5.2-r1.ebuild | 2 +- app-dicts/aspell-it/aspell-it-2.4.20070901.0-r1.ebuild | 2 +- app-dicts/aspell-lt/aspell-lt-1.3.2-r1.ebuild | 2 +- app-dicts/aspell-ml/aspell-ml-0.04.1-r1.ebuild | 2 +- app-dicts/aspell-pt/aspell-pt-20220621.ebuild | 4 ++-- app-dicts/aspell-ro/aspell-ro-3.3.10-r1.ebuild | 2 +- app-dicts/aspell-ru/aspell-ru-0.99.1-r2.ebuild | 2 +- app-dicts/aspell-sc/aspell-sc-1.0-r1.ebuild | 2 +- app-dicts/aspell-sk/aspell-sk-2.02.0-r1.ebuild | 2 +- app-dicts/aspell-sr/aspell-sr-0.02-r1.ebuild | 2 +- app-dicts/aspell-tet/aspell-tet-0.1.1-r1.ebuild | 2 +- app-dicts/aspell-tl/aspell-tl-0.4.0-r1.ebuild | 2 +- 18 files changed, 19 insertions(+), 19 deletions(-) https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=68a42036991752020ece9bc09cb06f82e7440a0a commit 68a42036991752020ece9bc09cb06f82e7440a0a Author: Kerin Millar <kfm@plushkava.net> AuthorDate: 2022-12-04 04:41:09 +0000 Commit: Sam James <sam@gentoo.org> CommitDate: 2022-12-06 09:11:27 +0000 net-vpn/networkmanager-strongswan: Apply patsub_replacement defences Per bug #881383, string replacing forms of parameter expansion must take care to quote - or appropriately escape - any nested parameter expansions, assuming that their values are intended to be taken literally (as is almost invariably the case). This has long been the case, but the introduction of the new patsub_replacement option in bash >=5.2 has brought the issue to the fore. In the case of the net-vpn/networkmanager-strongswan package, the improper quoting is not yet causing any issues. Still, it is better to write the code properly to begin with, especially considering the demonstrative value of robust code. Signed-off-by: Kerin Millar <kfm@plushkava.net> Bug: https://bugs.gentoo.org/881383 Signed-off-by: Sam James <sam@gentoo.org> .../networkmanager-strongswan/networkmanager-strongswan-1.6.0.ebuild | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=9666ed882a52f3280c9078bf6107fffa1a9da134 commit 9666ed882a52f3280c9078bf6107fffa1a9da134 Author: Kerin Millar <kfm@plushkava.net> AuthorDate: 2022-12-04 04:38:26 +0000 Commit: Sam James <sam@gentoo.org> CommitDate: 2022-12-06 09:11:26 +0000 net-misc/asterisk: Apply patsub_replacement defences Per bug #881383, string replacing forms of parameter expansion must take care to quote - or appropriately escape - any nested parameter expansions, assuming that their values are intended to be taken literally (as is almost invariably the case). This has long been the case, but the introduction of the new patsub_replacement option in bash >=5.2 has brought the issue to the fore. Note that, in this instance, it's actually a command substitution injected into the replacement 'string' that merits quoting. Signed-off-by: Kerin Millar <kfm@plushkava.net> Bug: https://bugs.gentoo.org/881383 Signed-off-by: Sam James <sam@gentoo.org> net-misc/asterisk/files/asterisk_wrapper-16.26.1-18.12.1 | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=980c4cf6f69ef98d6b2795b3cda4a7cb4ccbf1b7 commit 980c4cf6f69ef98d6b2795b3cda4a7cb4ccbf1b7 Author: Kerin Millar <kfm@plushkava.net> AuthorDate: 2022-12-04 04:25:46 +0000 Commit: Sam James <sam@gentoo.org> CommitDate: 2022-12-06 09:11:26 +0000 games-util/game-device-udev-rules: Apply patsub_replacement defences Per bug #881383, string replacing forms of parameter expansion must take care to quote - or appropriately escape - any nested parameter expansions, assuming that their values are intended to be taken literally (as is almost invariably the case). This has long been the case, but the introduction of the new patsub_replacement option in bash >=5.2 has brought the issue to the fore. In the case of the games-util/game-device-udev-rules package, the improper quoting is not yet causing any issues. Still, it is better to write the code properly to begin with, especially considering the demonstrative value of robust code. Signed-off-by: Kerin Millar <kfm@plushkava.net> Bug: https://bugs.gentoo.org/881383 Signed-off-by: Sam James <sam@gentoo.org> .../game-device-udev-rules/game-device-udev-rules-20220311.ebuild | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=84b50e99241623bf5557cea8e00a8178b9f01e14 commit 84b50e99241623bf5557cea8e00a8178b9f01e14 Author: Sam James <sam@gentoo.org> AuthorDate: 2022-12-26 07:16:56 +0000 Commit: Sam James <sam@gentoo.org> CommitDate: 2022-12-26 07:17:16 +0000 sys-apps/portage: add 3.0.42 Bug: https://bugs.gentoo.org/881383 Bug: https://bugs.gentoo.org/882797 Closes: https://bugs.gentoo.org/884397 Closes: https://bugs.gentoo.org/884135 Closes: https://bugs.gentoo.org/884285 Closes: https://bugs.gentoo.org/887025 Signed-off-by: Sam James <sam@gentoo.org> sys-apps/portage/Manifest | 1 + sys-apps/portage/portage-3.0.42.ebuild | 283 +++++++++++++++++++++++++++++++++ 2 files changed, 284 insertions(+)
Tom Briden reported a regression at the bug-bash mailing list, pertaining to patsub_replacement [1]. $ declare -p BASH_VERSION declare -- BASH_VERSION="5.2.15(1)-release" $ a='backslash \' $ printf "%s\n" "${a//\\/\\\\}" backslash \\ $ BASH_COMPAT=42 $ printf "%s\n" "${a//\\/\\\\}" backslash \ Note that the actual 4.2 release does not behave this way! Here's the same test, performed in 5.1. $ declare -p BASH_VERSION declare -- BASH_VERSION="5.1.16(1)-release" $ a='backslash \' $ printf "%s\n" "${a//\\/\\\\}" backslash \\ $ BASH_COMPAT=42 backslash \\ So far, the only advice that has been given is to disable patsub_replacement, which is what portage is presently doing. [1] https://lists.gnu.org/archive/html/bug-bash/2023-02/msg00058.html
