[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openbsd-tech
Subject:    Re: vmd 3/5: add size checks for control imsg
From:       Gilles Chehade <gilles () poolp ! org>
Date:       2017-02-27 10:12:26
Message-ID: 20170227101226.GA62499 () primary ! online ! net
[Download RAW message or body]

On Mon, Feb 27, 2017 at 10:52:14AM +0100, Reyk Floeter wrote:
> Reminder: using IMSG_SIZE_CHECK() in user-facing imsg handlers is a
> bad thing as an invalid imsg would kill the daemon (via fatal).
> 
> OK?
> 
>     Add size checks for imsg received over the control socket.
>     
>     Additionally, make sure that vmd never fatal()s when receiving an
>     invalid imsg from an arbitrary user over the control socket.
> 

ok gilles@


> diff --git usr.sbin/vmd/control.c usr.sbin/vmd/control.c
> index 5e0141f..cda7df9 100644
> --- usr.sbin/vmd/control.c
> +++ usr.sbin/vmd/control.c
> @@ -286,11 +286,13 @@ control_close(int fd, struct control_sock *cs)
>  void
>  control_dispatch_imsg(int fd, short event, void *arg)
>  {
> -	struct control_sock	*cs = arg;
> -	struct privsep		*ps = cs->cs_env;
> -	struct ctl_conn		*c;
> -	struct imsg		 imsg;
> -	int			 n, v, ret = 0;
> +	struct control_sock		*cs = arg;
> +	struct privsep			*ps = cs->cs_env;
> +	struct ctl_conn			*c;
> +	struct imsg			 imsg;
> +	struct vmop_create_params	 vmc;
> +	struct vmop_id			 vid;
> +	int				 n, v, ret = 0;
>  
>  	if ((c = control_connbyfd(fd)) == NULL) {
>  		log_warn("%s: fd %d: not found", __func__, fd);
> @@ -347,7 +349,8 @@ control_dispatch_imsg(int fd, short event, void *arg)
>  			c->flags |= CTL_CONN_NOTIFY;
>  			break;
>  		case IMSG_CTL_VERBOSE:
> -			IMSG_SIZE_CHECK(&imsg, &v);
> +			if (IMSG_DATA_SIZE(&imsg) < sizeof(v))
> +				goto fail;
>  
>  			memcpy(&v, imsg.data, sizeof(v));
>  			log_setverbose(v);
> @@ -355,17 +358,34 @@ control_dispatch_imsg(int fd, short event, void *arg)
>  			proc_forward_imsg(ps, &imsg, PROC_PARENT, -1);
>  			break;
>  		case IMSG_VMDOP_START_VM_REQUEST:
> +			if (IMSG_DATA_SIZE(&imsg) < sizeof(vmc))
> +				goto fail;
> +			if (proc_compose_imsg(ps, PROC_PARENT, -1,
> +			    imsg.hdr.type, fd, -1,
> +			    imsg.data, IMSG_DATA_SIZE(&imsg)) == -1) {
> +				control_close(fd, cs);
> +				return;
> +			}
> +			break;
>  		case IMSG_VMDOP_TERMINATE_VM_REQUEST:
> -		case IMSG_VMDOP_GET_INFO_VM_REQUEST:
> -			imsg.hdr.peerid = fd;
> -
> +			if (IMSG_DATA_SIZE(&imsg) < sizeof(vid))
> +				goto fail;
>  			if (proc_compose_imsg(ps, PROC_PARENT, -1,
> -			    imsg.hdr.type, imsg.hdr.peerid, -1,
> +			    imsg.hdr.type, fd, -1,
>  			    imsg.data, IMSG_DATA_SIZE(&imsg)) == -1) {
>  				control_close(fd, cs);
>  				return;
>  			}
>  			break;
> +		case IMSG_VMDOP_GET_INFO_VM_REQUEST:
> +			if (IMSG_DATA_SIZE(&imsg) != 0)
> +				goto fail;
> +			if (proc_compose_imsg(ps, PROC_PARENT, -1,
> +			    imsg.hdr.type, fd, -1, NULL, 0) == -1) {
> +				control_close(fd, cs);
> +				return;
> +			}
> +			break;
>  		case IMSG_VMDOP_LOAD:
>  		case IMSG_VMDOP_RELOAD:
>  		case IMSG_CTL_RESET:
> @@ -384,6 +404,8 @@ control_dispatch_imsg(int fd, short event, void *arg)
>  	return;
>  
>   fail:
> +	if (ret == 0)
> +		ret = EINVAL;
>  	imsg_compose_event(&c->iev, IMSG_CTL_FAIL,
>  	    0, 0, -1, &ret, sizeof(ret));
>  	imsg_flush(&c->iev.ibuf);
> 

-- 
Gilles Chehade

https://www.poolp.org                                          @poolpOrg

[prev in list] [next in list] [prev in thread] [next in thread]