modif: Escape control characters of the command line

[layderv]

Names and commands can contain control characters:

firejail --name="$(echo -e '\e[31mRed\n\b\b\bText\e[0m')" sleep 10s

results in "Text" printed in red.

Prevent commands like --tree to control the terminal.

I've tested some control characters and I can't do worse than making jails disappear from --tree (by controlling the cursor) and possibly output anything to console (hijacking a console output). Reading various documentations and forums, I didn't find interesting control characters like "writing to file" or "execute command", that resulted in CVEs in the past.

• Which other functions should be fixed? print_elem is the one used in --tree.
• Which other control characters should be escaped?
• In if (iscntrl(*c)) {, if the *ccharacter is not one in the switch, the character is discarded. Is this the behavior you want, or do you prefer to allow that character?

Before submitting this PR, I contacted @netblue30.

[kmk3]

Style/grammar suggestions; not sure about the overall idea.

[layderv]

Style/grammar suggestions; not sure about the overall idea.

See @netblue30 in layderv@ab4bd9c#commitcomment-97589396

If you are talking about the implementation or my questions, I'll wait for netblue30 to answer. Thanks for your review

[kmk3]

@layderv on Jan 24:

Style/grammar suggestions; not sure about the overall idea.

See @netblue30 in layderv@ab4bd9c#commitcomment-97589396

If you are talking about the implementation or my questions, I'll wait for
netblue30 to answer. Thanks for your review

Thanks for the link.

To clarify, I meant that that the review was just about style/grammar
suggestions and that I did not look extensively into the idea/implementation
details.

[netblue30]

I'll merge it in this week. @layderv, @kmk3 let me know when you are done. Thanks!

[layderv]

I'm waiting for answers to the questions at the top before making code changes

Which other functions should be fixed? print_elem is the one used in --tree.
Which other control characters should be escaped?
In if (iscntrl(*c)) {, if the *ccharacter is not one in the switch, the character is discarded. Is this the behavior you want, or do you prefer to allow that character?

[kmk3]

Sorry for the delay; I wasn't sure how exactly to articulate this but here
goes:

The code by itself looks fine to me, but on a higher level, I see that there
are already multiple functions related to dealing with control characters on
common.c:

char *do_replace_cntrl_chars(char *str, char c);
char *replace_cntrl_chars(const char *str, char c);
int has_cntrl_chars(const char *str);
void reject_cntrl_chars(const char *fname);
void reject_meta_chars(const char *fname, int globbing);

Currently these functions (more specifically, replace_cntrl_chars and
do_replace_cntrl_chars) only seem to be used in ls.c for replacing control
characters with ? when printing paths (in the print_file_or_dir and ls
functions) and when printing file contents (in the cat function).

And I agree that it makes sense to replace/escape control characters in
--tree.

But other than when printing things that exist independently of firejail (and
whose contents are therefore not controlled by firejail), wouldn't it make more
sense to always error in the presence of control characters?

For example, in what scenario would it be desirable to allow control characters
in --name? If they are always escaped, wouldn't the result look like a bunch
of escape sequences anyway?

Wouldn't it be better to just exit with an error?

[layderv]

But other than when printing things that exist independently of firejail (and
whose contents are therefore not controlled by firejail), wouldn't it make more
sense to always error in the presence of control characters?
For example, in what scenario would it be desirable to allow control characters
in --name? If they are always escaped, wouldn't the result look like a bunch
of escape sequences anyway?
Wouldn't it be better to just exit with an error?

It depends on what behavior you want your users to see. This does not happen only with --name; this happens with commands too. firejail bash -c "$(echo -e ...)" has the same effect.

If you exit with an error, when do you exit?

• When creating the jail? Some legitimate commands won't work
• When printing the tree? It prevents the root user and the current user from printing the tree

I can forbid non-printable in --name. I don't think you want to prevent most characters in the command string.

Another issue is that --name can contain newlines, and those are written to the name file in /run/firejail/name, and the file is read back up until the first newline in some parts of the code. I haven't read all the ways that file is read back, and I don't know what will happen in the future, but this is another possible problem relevant to our discussion.

[kmk3]

@layderv on Feb 7:

But other than when printing things that exist independently of firejail
(and whose contents are therefore not controlled by firejail), wouldn't it
make more sense to always error in the presence of control characters? For
example, in what scenario would it be desirable to allow control characters
in --name? If they are always escaped, wouldn't the result look like a
bunch of escape sequences anyway? Wouldn't it be better to just exit with
an error?

It depends on what behavior you want your users to see. This does not happen
only with --name; this happens with commands too. firejail bash -c "$(echo -e ...)" has the same effect.

If you exit with an error, when do you exit?

To be clear, I'm talking about when a user attempts to use control characters
in firejail-specific arguments and what not (such as in --name=), not
whenever firejail sees any control character at all.

• When creating the jail? Some legitimate commands won't work

Suggestion: When parsing --name in main.c/profile.c, exit if it contains a
control character.

I was only thinking about --name specifically in the previous comment, but
the following would be other potential candidates for aborting on parse:

• --hostname=name
• --output=logfile
• --output-stderr=logfile
• --trace=filename

• When printing the tree? It prevents the root user and the current user from
  printing the tree

As I said, I agree with escaping/replacing the control characters (rather than
aborting) when printing paths and the jailtree. IIRC ls(1) also does this.

I can forbid non-printable in --name. I don't think you want to prevent
most characters in the command string.

No idea about the command string (that is, in cases like bash -c "echo -e");
hopefully firejail doesn't try to do anything too fancy with that before
executing it.

Another issue is that --name can contain newlines, and those are written to
the name file in /run/firejail/name, and the file is read back up until the
first newline in some parts of the code. I haven't read all the ways that
file is read back, and I don't know what will happen in the future, but this
is another possible problem relevant to our discussion.

Yes, this is one excellent example of issues that weird characters could cause
in firejail-controlled variables and paths.

Forbidding it on parse would allow firejail to "fail early", which should help
mitigate such scenarios.

[layderv]

hopefully firejail doesn't try to do anything too fancy with that before
executing it

I don't know, I've only seen the behavior with the output from ls.

If I understand correctly:

• hostname, output, output-stderr, trace, name: error if there are control characters in these options
• escape control characters in the command string when it is printed with ls

[kmk3]

@layderv on Feb 8:

hopefully firejail doesn't try to do anything too fancy with that before
executing it

I don't know, I've only seen the behavior with the output from ls.

If I understand correctly:

• hostname, output, output-stderr, trace, name: error if there are control
  characters in these options

Yes.

• escape control characters in the command string when it is printed with ls

By "ls" do you mean --tree?

Good idea; I didn't think about the command string in that case.

I was wondering what similar tools do, so I did some testing:

ps replaces some control characters with ? and pstree apparently
replaces each one with an octal escape sequence:

test.sh:

#!/bin/sh

sleep 1000

Test commands (in bash):

$ ./test.sh HELLO $'\a' $'\n' $'\177' WORLD &
# [...]
$ ps -ao command | grep HELLO | grep -v grep
/bin/sh ./test.sh HELLO ?   ? WORLD
$ pstree -aAT | grep HELLO | grep -v grep | sed 's/.*-test.sh /test.sh /'
test.sh ./test.sh HELLO \007 \012 \177 WORLD

During the tests, I noticed something: escape_cntrl_chars() currently only
deals with 8 control characters plus a few other characters.

While ASCII has 33 control characters in total: 0x00 to 0x1f and 0x7f:

$ pacman -Qo ascii
/opt/plan9/bin/ascii is owned by 9base 6-8
$ ascii
|00 nul|01 soh|02 stx|03 etx|04 eot|05 enq|06 ack|07 bel|
|08 bs |09 ht |0a nl |0b vt |0c np |0d cr |0e so |0f si |
|10 dle|11 dc1|12 dc2|13 dc3|14 dc4|15 nak|16 syn|17 etb|
|18 can|19 em |1a sub|1b esc|1c fs |1d gs |1e rs |1f us |
|20 sp |21  ! |22  " |23  # |24  $ |25  % |26  & |27  ' |
|28  ( |29  ) |2a  * |2b  + |2c  , |2d  - |2e  . |2f  / |
|30  0 |31  1 |32  2 |33  3 |34  4 |35  5 |36  6 |37  7 |
|38  8 |39  9 |3a  : |3b  ; |3c  < |3d  = |3e  > |3f  ? |
|40  @ |41  A |42  B |43  C |44  D |45  E |46  F |47  G |
|48  H |49  I |4a  J |4b  K |4c  L |4d  M |4e  N |4f  O |
|50  P |51  Q |52  R |53  S |54  T |55  U |56  V |57  W |
|58  X |59  Y |5a  Z |5b  [ |5c  \ |5d  ] |5e  ^ |5f  _ |
|60  ` |61  a |62  b |63  c |64  d |65  e |66  f |67  g |
|68  h |69  i |6a  j |6b  k |6c  l |6d  m |6e  n |6f  o |
|70  p |71  q |72  r |73  s |74  t |75  u |76  v |77  w |
|78  x |79  y |7a  z |7b  { |7c  | |7d  } |7e  ~ |7f del|

And depending on the locale, iscntrl(3) could match more/different
characters.

So wouldn't it be safer to escape all control characters like pstree does?

In this case, I think that replacing each control character with a numeric
escape sequence (in octal, decimal or hexadecimal) might be simpler, even if
maybe it wouldn't look as good in the output compared to letter-based escape
sequences.

[layderv]

By "ls" do you mean --tree?

Yes sorry

Good idea; I didn't think about the command string in that case.
So wouldn't it be safer to escape all control characters like pstree does?

Both of these were in the first opening comment :) #5613 (comment)

I can make it that the common ones (already in the PR) are escaped nicely, the others are replaced with their hex equivalent. Is that ok?

[kmk3]

@layderv on Feb 9:

Good idea; I didn't think about the command string in that case.
So wouldn't it be safer to escape all control characters like pstree does?

Both of these were in the first opening comment :) #5613 (comment)

To clarify, I did see both being mentioned, but I didn't make the connection
between them. That is, I didn't think about the command string in --tree,
only the paths. I originally understood the first comment to mean that just
running a command with control characters in firejail could cause issues.

I can make it that the common ones (already in the PR) are escaped nicely,
the others are replaced with their hex equivalent. Is that ok?

Sure, that also seems like it could work.

What would the hex escape look like?

Considering the one from printf(1p):

$ printf '\x7f' | xxd
00000000: 7f                                       .

I imagine that the escaped command string in the previous example would look
something like this:

test.sh ./test.sh HELLO \a \n \x7f WORLD

[netblue30]

Thanks @layderv. I will move the esc cleanup code for --name from pid.c into main.c (line 2178), so we end up with a clean name to begin with.