NetBSD Problem Report #39559

From www@NetBSD.org  Tue Sep 16 04:22:37 2008
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by narn.NetBSD.org (Postfix) with ESMTP id C344063B8E3
	for <gnats-bugs@gnats.netbsd.org>; Tue, 16 Sep 2008 04:22:37 +0000 (UTC)
Message-Id: <20080916042237.8A08163B842@narn.NetBSD.org>
Date: Tue, 16 Sep 2008 04:22:37 +0000 (UTC)
From: xtraeme@gmail.com
Reply-To: xtraeme@gmail.com
To: gnats-bugs@NetBSD.org
Subject: veriexec(4): too easy to cause a NULL dereference through it in kernel
X-Send-Pr-Version: www-1.0

>Number:         39559
>Category:       kern
>Synopsis:       veriexec(4): too easy to cause a NULL dereference through it in kernel
>Confidential:   no
>Severity:       critical
>Priority:       high
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Sep 16 06:50:00 +0000 2008
>Closed-Date:    Sun Dec 14 23:21:35 +0000 2008
>Last-Modified:  Thu Dec 18 01:00:03 +0000 2008
>Originator:     Juan RP
>Release:        Latest
>Organization:
>Environment:
>Description:
While looking at other stuff in kernel, I've found some problems in
the veriexec(4) code.

1) The ioctl handler doesn't check if the fd is writable.
2) Root privilege and a simple small code is enough to cause a NULL
   dereference pointer though the ioctls VERIEXEC_LOAD/QUERY/DELETE.

Will send the example code in a few minutes.



>How-To-Repeat:

>Fix:
1) Check if fd is writable.
2) Check that prop_string_cstring_nocopy returns a valid string before
   passing it to namei() in the ioctl handler.

>Release-Note:

>Audit-Trail:
From: Juan RP <xtraeme@gmail.com>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/39559: veriexec(4): too easy to cause a NULL dereference
 through it in kernel
Date: Tue, 16 Sep 2008 13:35:35 +0200

 Here's the example code:

 ---- Makefile -----
 SRCS= verycrash.c
 PROG= verycrash

 LDADD+= -lprop
 DPADD+= ${LIBPROP}
 NOMAN=
 WARNS=	4

 .include <bsd.prog.mk>
 ---- END ----

 ---- verycrash ----
 #include <sys/verified_exec.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <strings.h>
 #include <err.h>
 #include <fcntl.h>
 #include <prop/proplib.h>

 static void
 usage(void)
 {
 	printf("%s: load | query | delete\n", getprogname());
 	exit(EXIT_FAILURE);
 }

 int
 main(int argc, char **argv)
 {
 	prop_dictionary_t dict;
 	int action = 0, fd, error;

 	if (argc != 2)
 		usage();

 	/*
 	 * All these ioctls will cause a NULL
 	 * pointer dereference.
 	 */
 	if (strcasecmp(argv[1], "load") == 0)
 		action = VERIEXEC_LOAD;
 	else if (strcasecmp(argv[1], "query") == 0)
 		action = VERIEXEC_QUERY;
 	else if (strcasecmp(argv[1], "delete") == 0)
 		action = VERIEXEC_DELETE;
 	else
 		usage();

 	/*
 	 * O_RDONLY works as well, hmmm.
 	 */
 	if ((fd = open("/dev/veriexec", O_RDONLY, 0)) == -1)
 		err(EXIT_FAILURE, "open");

 	/*
 	 * Empty dictionary or any with no keys required
 	 * will do the same effect.
 	 */
 	dict = prop_dictionary_create();
 	error = prop_dictionary_send_ioctl(dict, fd, action);
 	/*
 	 * There's no need to check return value, there wlll
 	 * a NULL pointer dereference in kernel.
 	 */

 	return EXIT_SUCCESS;
 }
 ---- END ----

From: Juan RP <xtraeme@gmail.com>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/39559: veriexec(4): too easy to cause a NULL dereference
 through it in kernel
Date: Tue, 16 Sep 2008 15:33:06 +0200

 The following patch fixes all the problems reported here:

 Index: kern/kern_verifiedexec.c
 ===================================================================
 RCS file: /cvsroot/src/sys/kern/kern_verifiedexec.c,v
 retrieving revision 1.110
 diff -b -u -p -r1.110 kern_verifiedexec.c
 --- kern/kern_verifiedexec.c	10 Sep 2008 16:36:54 -0000	1.110
 +++ kern/kern_verifiedexec.c	16 Sep 2008 13:30:37 -0000
 @@ -1185,7 +1185,8 @@ veriexec_file_add(struct lwp *l, prop_di
  	const char *file, *fp_type;
  	int error;

 -	file = prop_string_cstring_nocopy(prop_dictionary_get(dict, "file"));
 +	if (!prop_dictionary_get_cstring_nocopy(dict, "file", &file))
 +		return EINVAL;

  	NDINIT(&nid, LOOKUP, FOLLOW, UIO_SYSSPACE, file);
  	error = namei(&nid);
 Index: dev/verified_exec.c
 ===================================================================
 RCS file: /cvsroot/src/sys/dev/verified_exec.c,v
 retrieving revision 1.63
 diff -b -u -p -r1.63 verified_exec.c
 --- dev/verified_exec.c	11 Dec 2007 12:16:14 -0000	1.63
 +++ dev/verified_exec.c	16 Sep 2008 13:30:37 -0000
 @@ -128,10 +128,13 @@ static int
  veriexec_delete(prop_dictionary_t dict, struct lwp *l)
  {
  	struct nameidata nid;
 +	const char *str;
  	int error;

 -	NDINIT(&nid, LOOKUP, FOLLOW, UIO_SYSSPACE,
 -	    prop_string_cstring_nocopy(prop_dictionary_get(dict, "file")));
 +	if (!prop_dictionary_get_cstring_nocopy(dict, "file", &str))
 +		return EINVAL;
 +
 +	NDINIT(&nid, LOOKUP, FOLLOW, UIO_SYSSPACE, str);
  	error = namei(&nid);
  	if (error)
  		return (error);
 @@ -151,10 +154,13 @@ static int
  veriexec_query(prop_dictionary_t dict, prop_dictionary_t rdict, struct lwp *l)
  {
  	struct nameidata nid;
 +	const char *str;
  	int error;

 -	NDINIT(&nid, LOOKUP, FOLLOW, UIO_SYSSPACE,
 -	    prop_string_cstring_nocopy(prop_dictionary_get(dict, "file")));
 +	if (!prop_dictionary_get_cstring_nocopy(dict, "file", &str))
 +		return EINVAL;
 +
 +	NDINIT(&nid, LOOKUP, FOLLOW, UIO_SYSSPACE, str);
  	error = namei(&nid);
  	if (error)
  		return (error);
 @@ -180,6 +186,9 @@ veriexecioctl(dev_t dev, u_long cmd, voi
  	case VERIEXEC_LOAD:
  	case VERIEXEC_DELETE:
  	case VERIEXEC_FLUSH:
 +		if ((flags & FWRITE) == 0)
 +			return EPERM;
 +
  		if (veriexec_strict > VERIEXEC_LEARNING) {
  			log(LOG_WARNING, "Veriexec: Strict mode, modifying "
  			    "tables not permitted.\n");
 @@ -191,6 +200,8 @@ veriexecioctl(dev_t dev, u_long cmd, voi

  	case VERIEXEC_QUERY:
  	case VERIEXEC_DUMP:
 +		if ((flags & FWRITE) == 0)
 +			return EPERM;
  		break;

  	default:

State-Changed-From-To: open->closed
State-Changed-By: elad@NetBSD.org
State-Changed-When: Sun, 14 Dec 2008 23:21:35 +0000
State-Changed-Why:
Fixed, thanks for the report!



From: Elad Efrat <elad@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/39559 CVS commit: src/sys
Date: Sun, 14 Dec 2008 23:20:23 +0000 (UTC)

 Module Name:	src
 Committed By:	elad
 Date:		Sun Dec 14 23:20:23 UTC 2008

 Modified Files:
 	src/sys/dev: verified_exec.c
 	src/sys/kern: kern_verifiedexec.c

 Log Message:
 PR/39559: Juan RP: veriexec(4): too easy to cause a NULL dereference
     through it in kernel

 Patch from PR applied with tiny modifications, thanks!

 Discussed with blymn@ a while ago.


 To generate a diff of this commit:
 cvs rdiff -r1.63 -r1.64 src/sys/dev/verified_exec.c
 cvs rdiff -r1.111 -r1.112 src/sys/kern/kern_verifiedexec.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: jnemeth@victoria.tc.ca (John Nemeth)
To: gnats-bugs@NetBSD.org, kern-bug-people@NetBSD.org, netbsd-bugs@NetBSD.org,
        gnats-admin@NetBSD.org, elad@NetBSD.org, xtraeme@gmail.com
Cc: 
Subject: Re: kern/39559 (veriexec(4): too easy to cause a NULL dereference through it in kernel)
Date: Sun, 14 Dec 2008 15:37:54 -0800

 On Apr 1, 11:29am, elad@NetBSD.org wrote:
 }
 } Synopsis: veriexec(4): too easy to cause a NULL dereference through it in kernel
 } 
 } State-Changed-From-To: open->closed
 } State-Changed-By: elad@NetBSD.org
 } State-Changed-When: Sun, 14 Dec 2008 23:21:35 +0000
 } State-Changed-Why:
 } Fixed, thanks for the report!

      Does this need to be pulled up?

 }-- End of excerpt from elad@NetBSD.org

From: "Elad Efrat" <elad@netbsd.org>
To: "John Nemeth" <jnemeth@victoria.tc.ca>
Cc: gnats-bugs@netbsd.org, xtraeme@gmail.com
Subject: Re: kern/39559 (veriexec(4): too easy to cause a NULL dereference through it in kernel)
Date: Mon, 15 Dec 2008 01:47:33 +0200

 On Mon, Dec 15, 2008 at 1:37 AM, John Nemeth <jnemeth@victoria.tc.ca> wrote:
 > On Apr 1, 11:29am, elad@NetBSD.org wrote:
 > }
 > } Synopsis: veriexec(4): too easy to cause a NULL dereference through it in kernel
 > }
 > } State-Changed-From-To: open->closed
 > } State-Changed-By: elad@NetBSD.org
 > } State-Changed-When: Sun, 14 Dec 2008 23:21:35 +0000
 > } State-Changed-Why:
 > } Fixed, thanks for the report!
 >
 >     Does this need to be pulled up?

 It does not pose a security risk to netbsd-5, because it requires root
 privileges to trigger, but I can submit a pullup request if you (or
 anyone) would like to see this fixed still. :)
 (I figured I should not overload releng with stuff that isn't entirely crucial.)

 Thanks,

 -e.

From: jnemeth@victoria.tc.ca (John Nemeth)
To: elad@netbsd.org
Cc: gnats-bugs@netbsd.org, xtraeme@gmail.com
Subject: Re: kern/39559 (veriexec(4): too easy to cause a NULL dereference through it in kernel)
Date: Sun, 14 Dec 2008 21:40:04 -0800

 On May 6,  8:23pm, "Elad Efrat" wrote:
 } On Mon, Dec 15, 2008 at 1:37 AM, John Nemeth <jnemeth@victoria.tc.ca> wrote:
 } > On Apr 1, 11:29am, elad@NetBSD.org wrote:
 } > }
 } > } Synopsis: veriexec(4): too easy to cause a NULL dereference through it in kernel
 } > }
 } > } State-Changed-From-To: open->closed
 } > } State-Changed-By: elad@NetBSD.org
 } > } State-Changed-When: Sun, 14 Dec 2008 23:21:35 +0000
 } > } State-Changed-Why:
 } > } Fixed, thanks for the report!
 } >
 } >     Does this need to be pulled up?
 } 
 } It does not pose a security risk to netbsd-5, because it requires root
 } privileges to trigger, but I can submit a pullup request if you (or

      That would still be a serious problem.  Part of the reason for
 things like securelevel and veriexec is to protect the system against
 root processes.  It would be nice to see 5.0 go out without any serious
 issues.  Realistically there will be some, but we should get rid of as
 many as is reasonably possible.

 } anyone) would like to see this fixed still. :)
 } (I figured I should not overload releng with stuff that isn't entirely crucial.)

      releng has been doing a good job of keeping on top of things.
 Currently there is only one ticket in the pullup queue which has been
 deliberately stalled.

 }-- End of excerpt from "Elad Efrat"

From: "Elad Efrat" <elad@netbsd.org>
To: "John Nemeth" <jnemeth@victoria.tc.ca>
Cc: gnats-bugs@netbsd.org, xtraeme@gmail.com
Subject: Re: kern/39559 (veriexec(4): too easy to cause a NULL dereference through it in kernel)
Date: Mon, 15 Dec 2008 12:23:29 +0200

 Hi,

 I just submitted a pullup to netbsd-5 for this change.

 Thanks,

 -e.

From: jnemeth@victoria.tc.ca (John Nemeth)
To: elad@netbsd.org
Cc: gnats-bugs@netbsd.org, xtraeme@gmail.com
Subject: Re: kern/39559 (veriexec(4): too easy to cause a NULL dereference through it in kernel)
Date: Mon, 15 Dec 2008 03:08:51 -0800

 On May 7,  6:59am, "Elad Efrat" wrote:
 } 
 } I just submitted a pullup to netbsd-5 for this change.
 } 
 } Thanks,

      Thank you

 }-- End of excerpt from "Elad Efrat"

From: Soren Jacobsen <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/39559 CVS commit: [netbsd-5] src/sys
Date: Thu, 18 Dec 2008 00:56:27 +0000 (UTC)

 Module Name:	src
 Committed By:	snj
 Date:		Thu Dec 18 00:56:27 UTC 2008

 Modified Files:
 	src/sys/dev [netbsd-5]: verified_exec.c
 	src/sys/kern [netbsd-5]: kern_verifiedexec.c

 Log Message:
 Pull up following revision(s) (requested by elad in ticket #189):
 	sys/dev/verified_exec.c: revision 1.64
 	sys/kern/kern_verifiedexec.c: revision 1.112
 PR/39559: Juan RP: veriexec(4): too easy to cause a NULL dereference
     through it in kernel
 Patch from PR applied with tiny modifications, thanks!
 Discussed with blymn@ a while ago.


 To generate a diff of this commit:
 cvs rdiff -r1.63 -r1.63.22.1 src/sys/dev/verified_exec.c
 cvs rdiff -r1.111 -r1.111.4.1 src/sys/kern/kern_verifiedexec.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

>Unformatted:

Home	PR Database Search