Fix graffiti for Lighthouse

[pepawel]

Set graffiti for Lighthouse without double quotes because quoting in docker compose env file is not supported.

From Docker Compose docs:

"The value of VAL is used as is and not modified at all. For example if the value is surrounded by quotes (as is often the case of shell variables), the quotes are included in the value passed to Compose."

https://docs.docker.com/compose/compose-file/compose-file-v3/

[stefa2k]

I'd suggest to use | quote instead, like the ansible docs suggest: https://docs.ansible.com/ansible/latest/user_guide/playbooks_filters.html#manipulating-strings

wdyt?

[pepawel]

Hm, I'm not sure. Using quote would introduce - obviously - quotes which we are trying to avoid.
I've created example playbook to test it:

- hosts: localhost
  tasks:
  - set_fact:
      user_input: single'double"backslash\
  - fail: msg="before:{{ user_input | quote }}:after"

If you execute it you will notice it starts and ends with single quotes. Docker won't parse those quotes.

I can think of just a single character the quote may defend us against - new line (\n) and/or maybe carriage return (\r). User could paste following as graffiti:
harmless\nLD_PRELOAD=/tmp/my_malicious_library.so

As you can see this would allow user to insert arbitrary env variables. I don't think it is possible to set graffiti with newline inside using dialog interface, but may be possible using web interface (I haven't tested it). However, the user is already root, so he/she won't gain anything from expliting this vulnerability. But maybe somewhere in the future you decide to allow non-root users to set graffiti?

Here is PoC of vulnerability with fix implemented. Just run this playbook and compare secure.env and insecure.env files.

- hosts: localhost
  tasks:
  - shell: echo "GRAFFITI=old" > secure.env
  - shell: echo "GRAFFITI=old" > insecure.env
  - set_fact:
      user_input: |-
        harmless
        LD_PRELOAD=/tmp/malicious_lib.so
  - lineinfile:
      path: secure.env
      regexp: '^GRAFFITI='
      line: 'GRAFFITI={{ user_input.splitlines()[0] }}'
  - lineinfile:
      path: insecure.env
      regexp: '^GRAFFITI='
      line: 'GRAFFITI={{ user_input }}'

Basically the fix makes sure we use only the first line of graffiti text.

Do you have in mind other special characters we should quote?
If not, then I propose to use splitlines()[0].

[stefa2k]

Do you have in mind other special characters we should quote?

I think all special characters should be escaped, this includes \#$&"' and much more. We don't know which characters a user might want to use.

@gbayasgalan do you have some input? You had this issue before with something else afair.

[pepawel]

Why do you want to escape any of those characters? What will happen if they are not escaped?
From my understanding, they will be simply displayed as part of graffiti. Why not allow users to set graffiti they want?

[gbayasgalan]

Do you have in mind other special characters we should quote?

I think all special characters should be escaped, this includes \#$&"' and much more. We don't know which characters a user might want to use.

@gbayasgalan do you have some input? You had this issue before with something else afair.

Yes, I had issue with a input which is parameter of bash script. In my case single quote {{ ansible | quote }} worked, any character inside 'single quote' is taken as input
I think special characters could be ( ) [ ] { } < > ~ _ + = - , . % ^ : ; and more...

[pepawel]

In this case (setting graffiti for Lighthouse) the data flow looks like this:

1. User types "something" (without quotes) as graffiti in dialog menu or via web-ui.
2. It is passed to Ansible playbook.
3. Ansible playbook places it in config/lighthouse/validator.env as GRAFFITI=something.
4. Docker compose reads this file and passes it to the container as environment variables.
5. Docker entry point is executed (which is a bash script) receiving environment variables.
6. This bash script passes GRAFFITI variable contents - "something" (without the quotes) to lighthouse binary.

Current solution is to escape data at point 3. It is like escaping data before writing it to database.

From my experience, the widely accepted good practice is to write raw data to database, and take care escaping it later, when it is used. For example when it is displayed on html page as a text it should be escaped to not include html tags. When it becomes part of javascript string, html tags are ok, but quotes has to be escaped. You never know how data should be escaped, therefore it's better to store it in the raw form.

And .env used by Docker compose looks like a database, in which such a raw form could be stored. Of course with the exception of special handling of newlines.

I think the user input should be sanitized according to requirements of the channel it is being transmitted in.
In this case, it means escaping at point 2 (which currently has a problem described here: stereum-dev/ethereum2-control-center-cli#51), point 3 (remove newlines, nothing else IMO) and point 6 (I think the escaping there is implemented properly).

What do you think?

[stefa2k]

I agree and it makes sense to escape as late as possible. Interesting enough, some special characters already need attention in the .env file: https://superuser.com/questions/775173/how-to-escape-and-in-dockers-environment-varibles

To me the more time I look at it the more complex it gets.