New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

missing value validation for `hosts` plugin #1719

Open
BernhardDenner opened this Issue Dec 9, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@BernhardDenner
Contributor

BernhardDenner commented Dec 9, 2017

The hosts plugin misses some value validation checks, which may lead to an syntactically incorrect hosts file

  • check for whitespace in key names: If a key name (here a host or host alias) contains a white space char it is written as is to the config file. A subsequent read of that hosts file results in a different behaviour, since spaces or tabs are used as separators between two different host or alias names. A new line char leads to a syntactically incorrect hosts file.
  • host and alias names are not checked for maximum length: RFC1123 defines it be 255 chars at max.
  • comments meta key values: adding a newline char breaks the hosts syntax

Steps to Reproduce the Problem

All steps were done in Docker with Ubuntu 16.04 based image with elektra current master (f6f7952)

cat >/etc/hosts.new <<EOF
#localhost
127.0.0.1	localhost

# my one
10.10.10.10	myone myalias

::1	localhost ip6-localhost ip6-loopback
fe00::0	ip6-localnet
ff00::0	ip6-mcastprefix
ff02::1	ip6-allnodes
ff02::2	ip6-allrouters
172.17.0.2	cce3deb79e38
EOF

kdb mount /etc/hosts.new system/hosts hosts

kdb ls system/hosts
#> system/hosts
#> system/hosts/ipv4/cce3deb79e38
#> system/hosts/ipv4/localhost
#> system/hosts/ipv4/myone
#> system/hosts/ipv4/myone/myalias
#> system/hosts/ipv6/ip6-allnodes
#> system/hosts/ipv6/ip6-allrouters
#> system/hosts/ipv6/ip6-localnet
#> system/hosts/ipv6/ip6-mcastprefix
#> system/hosts/ipv6/localhost
#> system/hosts/ipv6/localhost/ip6-localhost
#> system/hosts/ipv6/localhost/ip6-loopback

kdb set 'system/hosts/ipv4/myone/my xalias' ''
#> Create a new key system/hosts/ipv4/myone/my xalias with string ""

kdb ls system/hosts
#> system/hosts
#> system/hosts/ipv4/cce3deb79e38
#> system/hosts/ipv4/localhost
#> system/hosts/ipv4/myone
#> system/hosts/ipv4/myone/my
#> system/hosts/ipv4/myone/myalias
#> system/hosts/ipv4/myone/xalias
#> system/hosts/ipv6/ip6-allnodes
#> system/hosts/ipv6/ip6-allrouters
#> system/hosts/ipv6/ip6-localnet
#> system/hosts/ipv6/ip6-mcastprefix
#> system/hosts/ipv6/localhost
#> system/hosts/ipv6/localhost/ip6-localhost
#> system/hosts/ipv6/localhost/ip6-loopback

kdb set 'system/hosts/ipv4/myone/your           
> yalias' ''
#> Create a new key system/hosts/ipv4/myone/your
#> yalias with string ""

cat /etc/hosts.new 
#> #localhost
#> 127.0.0.1	localhost
#> 
#> # my one
#> 10.10.10.10	myone my myalias xalias your
#> yalias
#> 
#> 
#> ::1	localhost ip6-localhost ip6-loopback
#> fe00::0	ip6-localnet
#> ff00::0	ip6-mcastprefix
#> ff02::1	ip6-allnodes
#> ff02::2	ip6-allrouters
#> 172.17.0.2	cce3deb79e38

kdb lsmeta system/hosts/ipv4/localhost         
#> comment/#0
#> comment/#1
#> comment/#1/space
#> comment/#1/start
#> order

kdb getmeta system/hosts/ipv4/localhost comment/#1
#> localhost

kdb setmeta system/hosts/ipv4/localhost comment/#1 'localhost 
> adding a new line'

cat /etc/hosts.new 
#> #localhost 
#> adding a new line
#> 127.0.0.1	localhost
#> 
#> # my one
#> 10.10.10.10	myone my myalias xalias your
#> fe00::0	ip6-localnet
#> ff00::0	ip6-mcastprefix
#> ff02::1	ip6-allnodes
#> ff02::2	ip6-allrouters
#> 172.17.0.2	cce3deb79e38

@BernhardDenner BernhardDenner added the bug label Dec 9, 2017

@BernhardDenner BernhardDenner referenced this issue Dec 9, 2017

Open

bugs in storage plugins #1718

0 of 2 tasks complete
@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Dec 9, 2017

Contributor

Thank you for reporting! The new line/space problems are definitely serious bugs.

But I have doubts about the 255 character requirement. Quoting from RFC1123:

"Host software MUST handle host names of up to 63 characters and
SHOULD handle host names of up to 255 characters."

Defines 63 and 255 as minimum, not as maximum?

"The DNS defines domain name syntax very generally -- a
string of labels each containing up to 63 8-bit octets,
separated by dots, and with a maximum total of 255 octets."

Refers to DNS and not to entries in hosts file?

So I am not against limitations of the length (to protect applications that cannot handle names longer than 63 or 255 characters) but I doubt that we should enforce it within the plugin (or even recommend such enforcement within hosts' contract).

In a quick check I had no troubles pinging a host with >500 character length. So the limitation might be obsolete in practice?

Nevertheless, good to be aware that such limitations might exist. We definitely should have a plugin enforcing a certain length of strings. (To support enforcements on legacy systems with applications that cannot handle host names with >255 character length.)

Contributor

markus2330 commented Dec 9, 2017

Thank you for reporting! The new line/space problems are definitely serious bugs.

But I have doubts about the 255 character requirement. Quoting from RFC1123:

"Host software MUST handle host names of up to 63 characters and
SHOULD handle host names of up to 255 characters."

Defines 63 and 255 as minimum, not as maximum?

"The DNS defines domain name syntax very generally -- a
string of labels each containing up to 63 8-bit octets,
separated by dots, and with a maximum total of 255 octets."

Refers to DNS and not to entries in hosts file?

So I am not against limitations of the length (to protect applications that cannot handle names longer than 63 or 255 characters) but I doubt that we should enforce it within the plugin (or even recommend such enforcement within hosts' contract).

In a quick check I had no troubles pinging a host with >500 character length. So the limitation might be obsolete in practice?

Nevertheless, good to be aware that such limitations might exist. We definitely should have a plugin enforcing a certain length of strings. (To support enforcements on legacy systems with applications that cannot handle host names with >255 character length.)

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Jun 3, 2018

Contributor

With the new comments metakeys it is even easier to break syntax. Simply forgetting to add comment start symbols already produces invalid hosts files.

That the IP address is copied into every alias also seems to be not very useful.

Contributor

markus2330 commented Jun 3, 2018

With the new comments metakeys it is even easier to break syntax. Simply forgetting to add comment start symbols already produces invalid hosts files.

That the IP address is copied into every alias also seems to be not very useful.

markus2330 added a commit that referenced this issue Jun 5, 2018

doc: improve text about comments
to catch problems like #1719

@markus2330 markus2330 added this to the 0.8.24 milestone Jun 5, 2018

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Jun 5, 2018

Contributor

In 51727e3 I added explanations about problems that implementers of storage plugins need to be aware of.

@sanssecours Do you have any suggestions for improvements of the comments meta-data?

Contributor

markus2330 commented Jun 5, 2018

In 51727e3 I added explanations about problems that implementers of storage plugins need to be aware of.

@sanssecours Do you have any suggestions for improvements of the comments meta-data?

@sanssecours

This comment has been minimized.

Show comment
Hide comment
@sanssecours

sanssecours Jun 5, 2018

Contributor

Do you have any suggestions for improvements of the comments meta-data?

Not in the moment. That might change, if I have to write a storage plugin that supports the meta key comment though.

Contributor

sanssecours commented Jun 5, 2018

Do you have any suggestions for improvements of the comments meta-data?

Not in the moment. That might change, if I have to write a storage plugin that supports the meta key comment though.

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Jun 6, 2018

Contributor

Thank you. Please make suggestions if you find any possible improvements.

Contributor

markus2330 commented Jun 6, 2018

Thank you. Please make suggestions if you find any possible improvements.

omnidan added a commit to omnidan/libelektra that referenced this issue Jun 18, 2018

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Aug 18, 2018

Contributor

@tom-wa Do you already have a fix here?

Contributor

markus2330 commented Aug 18, 2018

@tom-wa Do you already have a fix here?

@markus2330 markus2330 modified the milestones: 0.8.24, 0.8.25 Aug 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment