[PATCH] alias: aliascmd: refuse uninvokable aliases

[PATCH] alias: aliascmd: refuse uninvokable aliases

[наб]

[-- Attachment #1: Type: text/plain, Size: 2987 bytes --]

See standards quote within, but the fun bit is:
  alias "a'b=c" "ls&id=cd"; alias
outputs
  ls&id='cd'
  a'b='c'
neither of which is What You Want, and also you can't invoke them
because you need to escape the quote/&/whatever, which disables
alias processing. Forbid the minimum broken set.

For reference's sake, here's a test driver:
  #include <stdio.h>
  #include <stdlib.h>
  #include <string.h>
  #include <sys/wait.h>
  #include <unistd.h>
  int main(int _, char ** argv) {
  	putenv("LC_ALL=C");
  	char val[] = "alias 'aQb=echo a' && alias";
  	unsigned char * vp = strchr(val, 'Q');
  	for(unsigned i = 0; i <= 0xFF; ++i) {
  		*vp = i;
  		if(!vfork()) {
  			execl(argv[1], "sh", "-c", val, (char *)NULL);
  			_exit(-1);
  		}
  		int r;
  		wait(&r);
  		fprintf(stderr, "%02x: %d\n", i, WEXITSTATUS(r));
  	}
  }

zsh  refuses nothing
dash refuses 09 0a 20 22 24 26 27 28 29 3b 3c 3e 5c 60 7c
bash refuses dash + 2f
mksh refuses bash + 23 + 2a + 3f + 5e + <20 + >7c
ksh  refuses bash + 2a + 3f + 5b + 7b + 7d

Fixes: https://bugs.debian.org/758542
---
 src/alias.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/src/alias.c b/src/alias.c
index daeacbb..81de975 100644
--- a/src/alias.c
+++ b/src/alias.c
@@ -32,6 +32,7 @@
  * SUCH DAMAGE.
  */
 
+#include <stdbool.h>
 #include <stdlib.h>
 #include "shell.h"
 #include "input.h"
@@ -46,10 +47,38 @@
 
 struct alias *atab[ATABSIZE];
 
+STATIC bool validalias(const char *);
 STATIC void setalias(const char *, const char *);
 STATIC struct alias *freealias(struct alias *);
 STATIC struct alias **__lookupalias(const char *);
 
+/*
+ * POSIX Issue 7, XCU, 2.3.1 Alias Substitution:
+ *   the command name word of a simple command shall be examined
+ *   to determine whether it is an unquoted, valid alias name
+ * POSIX Issue 7, XCU, 2.2 Quoting:
+ *   The various quoting mechanisms are the escape character,
+ *   single-quotes, and double-quotes.
+ *
+ * Trivially, it's impossible to invoke an alias with whitespace inside
+ * (which has to be quoted, and therefore won't be an alias),
+ * with a backslash, or with quotes (likewise).
+ * The same applies to $, &, ), (, ;, >, <, `, and |.
+ *
+ * Additionally, rejecting quotes and other garbage means that we prevent
+ *   alias "a'b=c" "ls&id=cd"
+ * being output as
+ *   a'b='c'
+ *   ls&id='cd'
+ * which explodes and/or executes code when it's evaled back.
+ */
+STATIC
+bool
+validalias(const char *name)
+{
+	return !strpbrk(name, "\t\n \\\"'$&)(;><`|");
+}
+
 STATIC
 void
 setalias(const char *name, const char *val)
@@ -151,7 +180,11 @@ aliascmd(int argc, char **argv)
 				printalias(ap);
 		} else {
 			*v++ = '\0';
-			setalias(n, v);
+			if (!validalias(n)) {
+				outfmt(out2, "%s: %s: invalid name\n", "alias", n);
+				ret = 1;
+			} else
+				setalias(n, v);
 		}
 	}
 
-- 
2.30.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] alias: aliascmd: refuse uninvokable aliases

[Herbert Xu]

наб <nabijaczleweli@nabijaczleweli.xyz> wrote:
> [-- text/plain, encoding quoted-printable, charset: us-ascii, 109 lines --]
> 
> See standards quote within, but the fun bit is:
>  alias "a'b=c" "ls&id=cd"; alias
> outputs
>  ls&id='cd'
>  a'b='c'
> neither of which is What You Want, and also you can't invoke them
> because you need to escape the quote/&/whatever, which disables
> alias processing. Forbid the minimum broken set.
> 
> For reference's sake, here's a test driver:
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
>  #include <sys/wait.h>
>  #include <unistd.h>
>  int main(int _, char ** argv) {
>        putenv("LC_ALL=C");
>        char val[] = "alias 'aQb=echo a' && alias";
>        unsigned char * vp = strchr(val, 'Q');
>        for(unsigned i = 0; i <= 0xFF; ++i) {
>                *vp = i;
>                if(!vfork()) {
>                        execl(argv[1], "sh", "-c", val, (char *)NULL);
>                        _exit(-1);
>                }
>                int r;
>                wait(&r);
>                fprintf(stderr, "%02x: %d\n", i, WEXITSTATUS(r));
>        }
>  }
> 
> zsh  refuses nothing
> dash refuses 09 0a 20 22 24 26 27 28 29 3b 3c 3e 5c 60 7c
> bash refuses dash + 2f
> mksh refuses bash + 23 + 2a + 3f + 5e + <20 + >7c
> ksh  refuses bash + 2a + 3f + 5b + 7b + 7d
> 
> Fixes: https://bugs.debian.org/758542

The main objective of dash is to try to be minimal, so I'm not
going to take this patch.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] alias: aliascmd: refuse uninvokable aliases

[наб]

[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]

Hi!

On Sun, Dec 18, 2022 at 11:04:19AM +0800, Herbert Xu wrote:
> The main objective of dash is to try to be minimal, so I'm not
> going to take this patch.

In that case, how's about the scissor-patch below that fixes the main
questionable part of this (these dubious aliases executed as code when
re-evaled, which POSIX says should be fine)? It's roughly what zsh does.
-- >8 --
Subject: [PATCH] alias: printalias: quote the name, too

This ensures even something like
  alias 'a|b|c=d'
is output by alias as
  'a|b|c'='d'
instead of
  a|b|c='d'
which is both "suitable for reinput to the shell" per POSIX
and doesn't execute the aliases as code.
---
 src/alias.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/alias.c b/src/alias.c
index daeacbb..1375cdd 100644
--- a/src/alias.c
+++ b/src/alias.c
@@ -197,7 +197,7 @@ freealias(struct alias *ap) {
 
 void
 printalias(const struct alias *ap) {
-	out1fmt("%s=%s\n", ap->name, single_quote(ap->val));
+	out1fmt("%s=%s\n", single_quote(ap->name), single_quote(ap->val));
 }
 
 STATIC struct alias **
-- 
2.30.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] alias: aliascmd: refuse uninvokable aliases

[Herbert Xu]

On Sun, Dec 18, 2022 at 04:34:56AM +0100, наб wrote:
>
> In that case, how's about the scissor-patch below that fixes the main
> questionable part of this (these dubious aliases executed as code when
> re-evaled, which POSIX says should be fine)? It's roughly what zsh does.

Yes this looks simple enough.  I'll look into it.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] alias: aliascmd: refuse uninvokable aliases

[Herbert Xu]

наб <nabijaczleweli@nabijaczleweli.xyz> wrote:
>
> -- >8 --
> Subject: [PATCH] alias: printalias: quote the name, too

Please resend this with a new Subject line as otherwise it doesn't
get tracked properly by patchwork.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH RESEND] alias: printalias: quote the name, too

[наб]

[-- Attachment #1: Type: text/plain, Size: 675 bytes --]

This ensures even something like
  alias 'a|b|c=d'
is output by alias as
  'a|b|c'='d'
instead of
  a|b|c='d'
which is both "suitable for reinput to the shell" per POSIX
and doesn't execute the aliases as code.
---
 src/alias.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/alias.c b/src/alias.c
index daeacbb..1375cdd 100644
--- a/src/alias.c
+++ b/src/alias.c
@@ -197,7 +197,7 @@ freealias(struct alias *ap) {
 
 void
 printalias(const struct alias *ap) {
-	out1fmt("%s=%s\n", ap->name, single_quote(ap->val));
+	out1fmt("%s=%s\n", single_quote(ap->name), single_quote(ap->val));
 }
 
 STATIC struct alias **
-- 
2.30.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RESEND] alias: printalias: quote the name, too

[Herbert Xu]

On Thu, Jan 05, 2023 at 01:49:47PM +0100, наб wrote:
> This ensures even something like
>   alias 'a|b|c=d'
> is output by alias as
>   'a|b|c'='d'
> instead of
>   a|b|c='d'
> which is both "suitable for reinput to the shell" per POSIX
> and doesn't execute the aliases as code.
> ---
>  src/alias.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] alias: aliascmd: refuse uninvokable aliases

[Harald van Dijk]

On 17/12/2022 19:07, наб wrote:
> See standards quote within, but the fun bit is:
>    alias "a'b=c" "ls&id=cd"; alias
> outputs
>    ls&id='cd'
>    a'b='c'
> neither of which is What You Want, and also you can't invoke them
> because you need to escape the quote/&/whatever, which disables
> alias processing. Forbid the minimum broken set.

I didn't realise this at the time, and since it's not getting merged 
it's not very important, but for completeness: this patch also rejects 
some aliases that are invokable:

   alias $=echo
   $ hello world

This is because whether $ is special depends on the character that follows.

Cheers,
Harald van Dijk

Re: [PATCH RESEND] alias: printalias: quote the name, too

[Harald van Dijk]

On 08/01/2023 12:07, Herbert Xu wrote:
> On Thu, Jan 05, 2023 at 01:49:47PM +0100, наб wrote:
>> This ensures even something like
>>    alias 'a|b|c=d'
>> is output by alias as
>>    'a|b|c'='d'
>> instead of
>>    a|b|c='d'
>> which is both "suitable for reinput to the shell" per POSIX
>> and doesn't execute the aliases as code.
>> ---
>>   src/alias.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Patch applied.  Thanks.

Did either of you test this patch before submitting and pushing it? 
Actually running current dash git, I'm seeing

   $ src/dash -c 'alias foo=bar; alias foo'
   'bar'='bar'

single_quote() is written so that it writes to stackblock() without 
reserving the memory. The second single_quote() call is always going to 
clobber the first.

Cheers,
Harald van Dijk

[PATCH] alias: fix name quoting in printalias

[наб]

[-- Attachment #1: Type: text/plain, Size: 1144 bytes --]

single_quote() over-writes the stack string, so just output the name
separately first.

Fixes: commit 4ec545e8dc98a3f461cf56bed03adafa81c64aec ("alias: Quote
 name in printalias")
---
I coulda swore I tested it and that's where I got my commit message from;
I try to always use my history verbatim for the messages,
for this exact reason.
That said, I can reproduce the bug on trunk and my clean original branch,
so dunno how that got there. my b.

On trunk:
  $ src/dash -c 'alias foo=bar "a|b|c"=d; alias'
  'bar'='bar'
  'd'='d'
With patch:
  $ src/dash -c 'alias foo=bar "a|b|c"=d; alias'
  'foo'='bar'
  'a|b|c'='d'

 src/alias.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/alias.c b/src/alias.c
index 1375cdd..3cd589c 100644
--- a/src/alias.c
+++ b/src/alias.c
@@ -197,7 +197,8 @@ freealias(struct alias *ap) {
 
 void
 printalias(const struct alias *ap) {
-	out1fmt("%s=%s\n", single_quote(ap->name), single_quote(ap->val));
+	out1str(single_quote(ap->name));
+	out1fmt("=%s\n", single_quote(ap->name), single_quote(ap->val));
 }
 
 STATIC struct alias **
-- 
2.30.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2] alias: fix name quoting in printalias

[наб]

[-- Attachment #1: Type: text/plain, Size: 695 bytes --]

single_quote() over-writes the stack string, so just output the name
separately first.

Fixes: commit 4ec545e8dc98a3f461cf56bed03adafa81c64aec ("alias: Quote
 name in printalias")
---
really not my year sorry

 src/alias.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/alias.c b/src/alias.c
index 1375cdd..fcad43b 100644
--- a/src/alias.c
+++ b/src/alias.c
@@ -197,7 +197,8 @@ freealias(struct alias *ap) {
 
 void
 printalias(const struct alias *ap) {
-	out1fmt("%s=%s\n", single_quote(ap->name), single_quote(ap->val));
+	out1str(single_quote(ap->name));
+	out1fmt("=%s\n", single_quote(ap->val));
 }
 
 STATIC struct alias **
-- 
2.30.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] alias: fix name quoting in printalias

[Harald van Dijk]

On 09/01/2023 00:15, наб wrote:
> single_quote() over-writes the stack string, so just output the name
> separately first.
> 
> Fixes: commit 4ec545e8dc98a3f461cf56bed03adafa81c64aec ("alias: Quote
>   name in printalias")
> ---
> really not my year sorry

This one works, thanks.

I'm wondering if we can save on code size by storing aliases differently 
instead, allowing for a simpler fix here. If we store aliases in memory 
in name=value form in a single block of memory, we only need a single 
savestr() when creating the alias, we only need a single single_quote() 
call here, we only need a single free() when we're done with the alias, 
and may be able to share some code with src/var.c for checking the alias 
name and stopping at the '='.

That'd be a larger change though, and even if it is possible, I am not 
sure it's worth it.

Cheers,
Harald van Dijk

Re: [PATCH v2] alias: fix name quoting in printalias

[Herbert Xu]

On Mon, Jan 09, 2023 at 01:15:43AM +0100, наб wrote:
> single_quote() over-writes the stack string, so just output the name
> separately first.
> 
> Fixes: commit 4ec545e8dc98a3f461cf56bed03adafa81c64aec ("alias: Quote
>  name in printalias")
> ---
> really not my year sorry
> 
>  src/alias.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2] alias: fix name quoting in printalias

[Harald van Dijk]

On 09/01/2023 00:30, Harald van Dijk wrote:
> On 09/01/2023 00:15, наб wrote:
>> single_quote() over-writes the stack string, so just output the name
>> separately first.
>>
>> Fixes: commit 4ec545e8dc98a3f461cf56bed03adafa81c64aec ("alias: Quote
>>   name in printalias")
>> ---
>> really not my year sorry
> 
> This one works, thanks.
> 
> I'm wondering if we can save on code size by storing aliases differently 
> instead, allowing for a simpler fix here. If we store aliases in memory 
> in name=value form in a single block of memory, we only need a single 
> savestr() when creating the alias, we only need a single single_quote() 
> call here, we only need a single free() when we're done with the alias, 
> and may be able to share some code with src/var.c for checking the alias 
> name and stopping at the '='.
> 
> That'd be a larger change though, and even if it is possible, I am not 
> sure it's worth it.

Having implemented this, I am also seeing that this does achieve the 
result of reducing code size. The only visible change is that given e.g.

   alias foo=bar
   alias foo

we get

   'foo=bar'

rather than

   'foo'='bar'

which should be equally acceptable.

If there is interest in having this in dash, can adapt and submit it.

> Cheers,
> Harald van Dijk

Re: [PATCH v2] alias: fix name quoting in printalias

[Herbert Xu]

On Wed, Jan 11, 2023 at 12:06:04AM +0000, Harald van Dijk wrote:
>
> If there is interest in having this in dash, can adapt and submit it.

Yes please send it to the list.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt