net/sysctl: zero-init stable_secret strings

Submitted by Pavel Tikhomirov on May 20, 2016, 3:41 p.m.

Details

Message ID 1463758887-22558-1-git-send-email-ptikhomirov@virtuozzo.com
State Accepted
Series "net/sysctl: zero-init stable_secret strings"
Commit 395d55eedb04275dc65a4dcb3850dc03d25eabaf
Headers show

Commit Message

Pavel Tikhomirov May 20, 2016, 3:41 p.m.
Sting field sysctl_entry.sarg has wrong length in protobuf encoding in
netdev-9 image, according to
https://developers.google.com/protocol-buffers/docs/encoding :

In netdev-9.img binary representation of sarg field is: "1a 2c 32 36 30
37 3a 66 30 64 30 3a 31 30 30 32 3a 30 30 35 31 3a 30 30 30 30 3a 30 30
30 30 3a 30 30 30 30 3a 30 30 30 34 0a c0 f4 a7 01"
Field key is 0x1a - means type 2(Length-delimited field), field 3 - that
is sarg field.
Field len is 0x2c which is 44, first 40-byte
"2607:f0d0:1002:0051:0000:0000:0000:0004\0" and then last 4 - "c0 f4 a7
01" where 0xc0 is not utf-8, and that makes crit fail.

In sysctl_op we just read() from sysctl so no '\0' is added in the end
of the string. So we can zero-init arrays and that will fix the issue.

https://github.com/xemul/criu/issues/161

*Alternatively or additionaly we can put '\0' in the end of string in
sysctl_read_char to make it harder to make such a mistake in future.

Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 criu/net.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/net.c b/criu/net.c
index d3b659a..d57e0aa 100644
--- a/criu/net.c
+++ b/criu/net.c
@@ -331,7 +331,7 @@  static int dump_one_netdev(int type, struct ifinfomsg *ifi,
 	int size4 = ARRAY_SIZE(devconfs4);
 	SysctlEntry *confs6 = NULL;
 	int size6 = ARRAY_SIZE(devconfs6);
-	char stable_secret[MAX_STR_CONF_LEN + 1];
+	char stable_secret[MAX_STR_CONF_LEN + 1] = {};
 
 	if (!tb[IFLA_IFNAME]) {
 		pr_err("No name for link %d\n", ifi->ifi_index);
@@ -1132,8 +1132,8 @@  static int dump_netns_conf(struct cr_imgset *fds)
 	int size4 = ARRAY_SIZE(devconfs4);
 	SysctlEntry *def_confs6 = NULL, *all_confs6 = NULL;
 	int size6 = ARRAY_SIZE(devconfs6);
-	char def_stable_secret[MAX_STR_CONF_LEN + 1];
-	char all_stable_secret[MAX_STR_CONF_LEN + 1];
+	char def_stable_secret[MAX_STR_CONF_LEN + 1] = {};
+	char all_stable_secret[MAX_STR_CONF_LEN + 1] = {};
 
 	netns.n_def_conf4 = size4;
 	netns.n_all_conf4 = size4;

Comments

Andrey Vagin May 20, 2016, 8:28 p.m.
On Fri, May 20, 2016 at 06:41:27PM +0300, Pavel Tikhomirov wrote:
> Sting field sysctl_entry.sarg has wrong length in protobuf encoding in
> netdev-9 image, according to
> https://developers.google.com/protocol-buffers/docs/encoding :
> 
> In netdev-9.img binary representation of sarg field is: "1a 2c 32 36 30
> 37 3a 66 30 64 30 3a 31 30 30 32 3a 30 30 35 31 3a 30 30 30 30 3a 30 30
> 30 30 3a 30 30 30 30 3a 30 30 30 34 0a c0 f4 a7 01"
> Field key is 0x1a - means type 2(Length-delimited field), field 3 - that
> is sarg field.
> Field len is 0x2c which is 44, first 40-byte
> "2607:f0d0:1002:0051:0000:0000:0000:0004\0" and then last 4 - "c0 f4 a7
> 01" where 0xc0 is not utf-8, and that makes crit fail.
> 
> In sysctl_op we just read() from sysctl so no '\0' is added in the end
> of the string. So we can zero-init arrays and that will fix the issue.
> 
> https://github.com/xemul/criu/issues/161
> 
> *Alternatively or additionaly we can put '\0' in the end of string in
> sysctl_read_char to make it harder to make such a mistake in future.
> 

Acked-by: Andrew Vagin <avagin@virtuozzo.com>

> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> ---
>  criu/net.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/criu/net.c b/criu/net.c
> index d3b659a..d57e0aa 100644
> --- a/criu/net.c
> +++ b/criu/net.c
> @@ -331,7 +331,7 @@ static int dump_one_netdev(int type, struct ifinfomsg *ifi,
>  	int size4 = ARRAY_SIZE(devconfs4);
>  	SysctlEntry *confs6 = NULL;
>  	int size6 = ARRAY_SIZE(devconfs6);
> -	char stable_secret[MAX_STR_CONF_LEN + 1];
> +	char stable_secret[MAX_STR_CONF_LEN + 1] = {};
>  
>  	if (!tb[IFLA_IFNAME]) {
>  		pr_err("No name for link %d\n", ifi->ifi_index);
> @@ -1132,8 +1132,8 @@ static int dump_netns_conf(struct cr_imgset *fds)
>  	int size4 = ARRAY_SIZE(devconfs4);
>  	SysctlEntry *def_confs6 = NULL, *all_confs6 = NULL;
>  	int size6 = ARRAY_SIZE(devconfs6);
> -	char def_stable_secret[MAX_STR_CONF_LEN + 1];
> -	char all_stable_secret[MAX_STR_CONF_LEN + 1];
> +	char def_stable_secret[MAX_STR_CONF_LEN + 1] = {};
> +	char all_stable_secret[MAX_STR_CONF_LEN + 1] = {};
>  
>  	netns.n_def_conf4 = size4;
>  	netns.n_all_conf4 = size4;
> -- 
> 2.5.5
>
Pavel Emelianov May 23, 2016, 5:03 p.m.
Applied