[-- Attachment #1.1: Type: text/plain, Size: 478 bytes --] Hello, I think I found a bug in Functions/Misc/regexp-replace Steps to reproduce: zsh -f str='x :=bad' autoload regexp-replace regexp-replace str 'a' 'z' && echo $str Actual Output: (eval):1: bzd not found Expected: x :=bzd Root cause: Line 41: eval ${1}=${(q)5} It appears the ${(q)5} is not escaping = so the =command is executed after : Instead of 'x\ :\=bad', you get '= x\ :=bad' One way to fix: 41: eval ${1}=${(qqq)5} Patch file is attached. Thanks Jacob Menke [-- Attachment #1.2: Type: text/html, Size: 941 bytes --] [-- Attachment #2: regexp-replace.patch --] [-- Type: application/x-patch, Size: 328 bytes --]
On Thu, Apr 29, 2021 at 07:53:52PM -0400, Jacob Menke wrote: > Hello, > > I think I found a bug in Functions/Misc/regexp-replace > One way to fix: > 41: eval ${1}=${(qqq)5} > > Patch file is attached. Thanks for the report, minimal reproducer, and patch. I think it would make sense to swap the eval for ${(P) ::= here as below. diff --git a/Functions/Misc/regexp-replace b/Functions/Misc/regexp-replace index dec105524..ff82c39b6 100644 --- a/Functions/Misc/regexp-replace +++ b/Functions/Misc/regexp-replace @@ -38,6 +38,6 @@ while [[ -n $4 ]]; do done 5+=$4 -eval ${1}=${(q)5} +: ${(P)1::=$5} # status 0 if we did something, else 1. [[ -n $6 ]]
2021-04-29 19:40:41 -0500, Matthew Martin: > On Thu, Apr 29, 2021 at 07:53:52PM -0400, Jacob Menke wrote: > > Hello, > > > > I think I found a bug in Functions/Misc/regexp-replace > > > One way to fix: > > 41: eval ${1}=${(qqq)5} > > > > Patch file is attached. > > Thanks for the report, minimal reproducer, and patch. I think it would > make sense to swap the eval for ${(P) ::= here as below. [...] I had a patch fixing that and some other issues in that function in https://www.zsh.org/mla/workers/2020/msg00009.html but it seems it wasn't applied. -- Stephane
2021-04-29 19:53:52 -0400, Jacob Menke: [...] > regexp-replace str 'a' 'z' && echo $str > > Actual Output: > (eval):1: bzd not found > > Expected: > x :=bzd [...] One might argue there's a problem with the (q) parameter expansion flag, it escapes leading =s but not the =s that follow : even though they're special there in assignments. $ echo a=x:=y a=x:=y $ a=x:=y zsh: y not found BTW, zsh is the only shell where ~ is expanded in: $ zsh -c 'a=a\:~; echo $a' a:/home/chazelas [...] > One way to fix: > 41: eval ${1}=${(qqq)5} The safest quoting operator is the (qq) one. I wouldn't use any other for things to be reinput to the shell. See https://unix.stackexchange.com/questions/379181/escape-a-variable-for-use-as-content-of-another-script/600214#600214 for details on that. In particular qqq uses double quotes inside which \ and ` are still special and those characters also appear in the encoding of some other characters in some locales. But here, the best thing to do is to not expose the parser to the contents of $5 by doing: eval "$1=\$5" (which tells the shell to evaluate varname=$5) You need to expand $1 here which contains the variable name. Note that as already noted at https://www.zsh.org/mla/workers/2019/msg01113.html whether you use that or : ${(P)1::="$5"} You'll still have a command injection vulnerability if $1 is not guaranteed to be a variable name. -- Stephane
2021-04-30 07:51:23 +0100, Stephane Chazelas: [...] > BTW, zsh is the only shell where ~ is expanded in: > > $ zsh -c 'a=a\:~; echo $a' > a:/home/chazelas [...] That's a POSIX non-conformance btw: https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/utilities/V3_chap02.html#tag_18_06_01 Tilde expansion is only meant to be done after unquoted : per POSIX. Though I can't imagine being a problem in practice. If people don't want tilde expansion, they'd quote the ~, not the :. Also, there's a lot of variation between shells in how tilde expansion is done and the specification is quite vague there. See also ~$user or ~"user" or var=foo$COLON~ or var=foo${-+:}~ ... See also https://www.austingroupbugs.net/view.php?id=1172 (I had completely forgotten I'd raised that). -- Stephane
On Fri, Apr 30, 2021 at 1:18 AM Stephane Chazelas <stephane@chazelas.org> wrote:
>
> Tilde expansion is only meant to be done after unquoted : per
> POSIX. Though I can't imagine being a problem in practice.
I was expecting this to mean that colons in e.g. $PATH could be
escaped with a backslash, but that's not the case.
Therefore I suppose it's so that one can do
a=${this}\:${that}
without having to worry whether $that begins with a tilde, while
simultaneously allowing ${that} to be whitespace-split or to itself
contain unquoted colons. Although again that seems a pretty weird
case since the only reason to care whether those colons are quoted is
if they are followed by more tildes and I can't imagine what splitting
makes sense in that context.
[-- Attachment #1: Type: text/plain, Size: 1673 bytes --] Awesome, thanks for the expert response! I vote for eval "$1=\$5" Jacob On Fri, 30 Apr 2021 at 02:51, Stephane Chazelas <stephane@chazelas.org> wrote: > 2021-04-29 19:53:52 -0400, Jacob Menke: > [...] > > regexp-replace str 'a' 'z' && echo $str > > > > Actual Output: > > (eval):1: bzd not found > > > > Expected: > > x :=bzd > [...] > > One might argue there's a problem with the (q) parameter > expansion flag, it escapes leading =s but not the =s that follow > : even though they're special there in assignments. > > $ echo a=x:=y > a=x:=y > $ a=x:=y > zsh: y not found > > BTW, zsh is the only shell where ~ is expanded in: > > $ zsh -c 'a=a\:~; echo $a' > a:/home/chazelas > > [...] > > One way to fix: > > 41: eval ${1}=${(qqq)5} > > The safest quoting operator is the (qq) one. I wouldn't use any > other for things to be reinput to the shell. > > See > > https://unix.stackexchange.com/questions/379181/escape-a-variable-for-use-as-content-of-another-script/600214#600214 > for details on that. > > In particular qqq uses double quotes inside which \ and ` are > still special and those characters also appear in the encoding > of some other characters in some locales. > > But here, the best thing to do is to not expose the parser to > the contents of $5 by doing: > > eval "$1=\$5" > > (which tells the shell to evaluate varname=$5) > > You need to expand $1 here which contains the variable name. > > Note that as already noted at > https://www.zsh.org/mla/workers/2019/msg01113.html > whether you use that or > > : ${(P)1::="$5"} > > You'll still have a command injection vulnerability if $1 is not > guaranteed to be a variable name. > > -- > Stephane > [-- Attachment #2: Type: text/html, Size: 2596 bytes --]
On Thu, Apr 29, 2021 at 11:51 PM Stephane Chazelas
<stephane@chazelas.org> wrote:
>
> One might argue there's a problem with the (q) parameter
> expansion flag, it escapes leading =s but not the =s that follow
> : even though they're special there in assignments.
Arguably (q) should either quote all =s or none of them. It has no
idea what appears outside the parameter reference, that is, there's no
reason for it to treat a leading = as being the start of a word in the
surrounding context (or to believe the context is assignment). I
can't think of any situation where I've depended upon the
quotes-leading-equal behavior.
On the other hand the check for a leading equal means that (q-) would
work here (and is frequently better than just (q) for other reasons).
On Fri, Apr 30, 2021 at 1:14 PM Jacob Menke <linux.dev25@gmail.com> wrote:
>
> I vote for
>
> eval "$1=\$5"
I'd actually go for
typeset -g $1=$5
and avoid the eval entirely.