Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 833087 - www-misc/shellinabox-2.20-r4 installs shell script that uses non-POSIX features (to run with dash)
Summary: www-misc/shellinabox-2.20-r4 installs shell script that uses non-POSIX featur...
Status: CONFIRMED
Alias: None
Product: Gentoo Linux
Classification: Unclassified
Component: Current packages (show other bugs)
Hardware: All Linux
: Normal normal (vote)
Assignee: Patrice Clement
URL:
Whiteboard:
Keywords: Inclusion
Depends on:
Blocks: nonbash
  Show dependency tree
 
Reported: 2022-02-10 20:08 UTC by Anna Vyalkova
Modified: 2022-11-11 10:06 UTC (History)
3 users (show)

See Also:
Package list:
Runtime testing required: ---


Attachments
shellinaboxd.init-r1 (shellinaboxd.init-r1,1.35 KB, text/plain)
2022-02-12 02:16 UTC, Kerin Millar
Details
shellinaboxd.init-r1 (with changes by Anna) (shellinaboxd.init-r1,1.11 KB, text/plain)
2022-02-12 13:48 UTC, Anna Vyalkova
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Anna Vyalkova 2022-02-10 20:08:58 UTC
$ dash -n /etc/init.d/shellinaboxd
/etc/init.d/shellinaboxd: 15: Syntax error: "(" unexpected (expecting "}")

> OpenRC are expected to be POSIX-compliant and must therefore avoid e.g. Bash-specific tests and constructs.
https://devmanual.gentoo.org/tasks-reference/init-scripts/index.html

Reproducible: Always

Steps to Reproduce:
1. eselect sh set dash
2. rc-service shellinaboxd start
Comment 1 Kerin Millar 2022-02-12 02:16:35 UTC
Created attachment 764868 [details]
shellinaboxd.init-r1

This is my rewrite of the runscript. It is fully POSIX conforming, with the exception that it continues to use the local keyword. I left local in because, despite its behaviour being undefined by POSIX, it is both useful and widely implemented. In the worst case, a user might symlink /bin/sh to a strictly conforming implementation, in which case an error would appear with an extremely low probability of impacting the overall behaviour of the script.

Various quality issues have been addressed. The definition of the label now takes into account all characters that could impact upon the grammar of the --user-css option. The command_args variable is no longer subject to an unquoted expansion but is instead handled by the criminally under-utilised xargs(1) utility. Thus, not only is it protected from unintentional word splitting and pathname expansion, but double quotes will be parsed helpfully; now even CSS pathnames that contain spaces will be handled without issue. Pathnames that contain double quotes still won't be, but that was already the case. Besides, xargs(1) can be expected to generate an error should that ever happen.

I'm prepared to generate a patch directly against the gentoo repo but it's going to require a revision bump and I don't see any point in doing that until the fact that it doesn't build successfully is addressed. I don't think there's an open bug for that, so I'll file one shortly.
Comment 2 Anna Vyalkova 2022-02-12 13:48:53 UTC
Created attachment 764923 [details]
shellinaboxd.init-r1 (with changes by Anna)

(In reply to Kerin Millar from comment #1)
> This is my rewrite of the runscript. It is fully POSIX conforming, with the
> exception that it continues to use the local keyword. I left local in
> because, despite its behaviour being undefined by POSIX, it is both useful
> and widely implemented. In the worst case, a user might symlink /bin/sh to a
> strictly conforming implementation, in which case an error would appear with
> an extremely low probability of impacting the overall behaviour of the
> script.

Thank you very much!
But I suppose "Signed-off-by" thing is needed to use this in Gentoo.

> The command_args variable is no longer subject to an
> unquoted expansion but is instead handled by the criminally under-utilised
> xargs(1) utility. Thus, not only is it protected from unintentional word
> splitting and pathname expansion, but double quotes will be parsed
> helpfully; now even CSS pathnames that contain spaces will be handled
> without issue. Pathnames that contain double quotes still won't be, but that
> was already the case. Besides, xargs(1) can be expected to generate an error
> should that ever happen.

I removed start()/stop() functions and used declarative config instead.
Comment 3 Kerin Millar 2022-02-12 21:11:20 UTC
(In reply to jan Anja from comment #2)

> Thank you very much!
> But I suppose "Signed-off-by" thing is needed to use this in Gentoo.

Indeed. I'll get to it. I'm still trying to figure out how to fix an OpenSSL-related build error.

> I removed start()/stop() functions and used declarative config instead.

I would have done the same thing, if it were not for the variable handling in OpenRC being of a fundamentally broken design. It appears to work as expected in most cases but this is only because its inputs tend to be trivial. In case you don't believe it, try going to the CSS directory and running the following.

# cp black-on-white.css '$(echo gotcha).css'

Now start shellinaboxd using your declarative version of the runscript. The following error will occur.

[config] Cannot access style sheet "/usr/share/shellinabox-resources/gotcha.css"!

Yes, that was a code injection - running as root. Imagine if the name were to contain a more destructive payload.

Stuffing argument lists into scalar variables already makes the problem domain difficult to contend with (sh doesn't grant us the luxury of using arrays). Still, the singularly low calibre of the start-stop-daemon and supervise-daemon helpers in the declarative context is truly remarkable. Essentially, all of the applicable variables are subject to unquoted expansion - rendering them vulnerable to word splitting and pathname expansion - with the results then being joined and fed to eval! What could possibly go wrong? A great deal, as it transpires.

I have a personal fork of OpenRC that aims to address these issues. It specifically uses xargs to process command_args. That is, after all, what it was designed for. I suppose that I ought to get around to proposing that my changes be included. In the meantime, I cannot personally endorse the use of the declarative style and my avoidance of it is deliberate. Sadly, most developers appear to lack the skills to employ even the non-declarative style in a safe fashion. The current state of the shellinaboxd runscript is merely one of a plethora of examples.
Comment 4 Agostino Sarubbo gentoo-dev 2022-08-07 05:39:33 UTC
lto_tinderbox has reproduced this issue with version 2.20-r4 - Updating summary.