[PATCH] cifskey: better use snprintf()

[PATCH] cifskey: better use snprintf()

[Sebastian Krahmer]

Prefer snprintf() over sprintf() in cifskey.c
Projects that fork the code (pam_cifscreds) can't rely on
the max-size parameters.

Signed-off-by: Sebastian Krahmer <krahmer-l3A5Bk7waGM@public.gmane.org>
---


--- cifskey.c.orig	2014-04-08 13:10:41.653435040 +0200
+++ cifskey.c	2014-04-14 11:19:07.000118206 +0200
@@ -29,7 +29,8 @@
 {
 	char desc[INET6_ADDRSTRLEN + sizeof(KEY_PREFIX) + 4];
 
-	sprintf(desc, "%s:%c:%s", KEY_PREFIX, keytype, addr);
+	if (snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr) >= (int)sizeof(desc))
+		return -1;
 
 	return keyctl_search(DEST_KEYRING, CIFS_KEY_TYPE, desc, 0);
 }
@@ -38,15 +39,18 @@
 key_serial_t
 key_add(const char *addr, const char *user, const char *pass, char keytype)
 {
-	int len;
+	int len = 0;
 	char desc[INET6_ADDRSTRLEN + sizeof(KEY_PREFIX) + 4];
 	char val[MOUNT_PASSWD_SIZE +  MAX_USERNAME_SIZE + 2];
 
 	/* set key description */
-	sprintf(desc, "%s:%c:%s", KEY_PREFIX, keytype, addr);
+	if (snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr) >= (int)sizeof(desc))
+		return -1;
 
 	/* set payload contents */
-	len = sprintf(val, "%s:%s", user, pass);
+	len = snprintf(val, sizeof(val), "%s:%s", user, pass);
+	if (len >= (int)sizeof(val))
+		return -1;
 
 	return add_key(CIFS_KEY_TYPE, desc, val, len + 1, DEST_KEYRING);
 }


-- 

~ perl self.pl
~ $_='print"\$_=\47$_\47;eval"';eval
~ krahmer-l3A5Bk7waGM@public.gmane.org - SuSE Security Team

Re: [PATCH] cifskey: better use snprintf()

[Jeff Layton]

On Mon, 14 Apr 2014 11:39:41 +0200
Sebastian Krahmer <krahmer-l3A5Bk7waGM@public.gmane.org> wrote:

> 
> Prefer snprintf() over sprintf() in cifskey.c
> Projects that fork the code (pam_cifscreds) can't rely on
> the max-size parameters.
> 
> Signed-off-by: Sebastian Krahmer <krahmer-l3A5Bk7waGM@public.gmane.org>
> ---
> 
> 
> --- cifskey.c.orig	2014-04-08 13:10:41.653435040 +0200
> +++ cifskey.c	2014-04-14 11:19:07.000118206 +0200
> @@ -29,7 +29,8 @@
>  {
>  	char desc[INET6_ADDRSTRLEN + sizeof(KEY_PREFIX) + 4];
>  
> -	sprintf(desc, "%s:%c:%s", KEY_PREFIX, keytype, addr);
> +	if (snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr) >= (int)sizeof(desc))
> +		return -1;
>  
>  	return keyctl_search(DEST_KEYRING, CIFS_KEY_TYPE, desc, 0);
>  }
> @@ -38,15 +39,18 @@
>  key_serial_t
>  key_add(const char *addr, const char *user, const char *pass, char keytype)
>  {
> -	int len;
> +	int len = 0;

This initialization doesn't appear to be needed.

>  	char desc[INET6_ADDRSTRLEN + sizeof(KEY_PREFIX) + 4];
>  	char val[MOUNT_PASSWD_SIZE +  MAX_USERNAME_SIZE + 2];
>  
>  	/* set key description */
> -	sprintf(desc, "%s:%c:%s", KEY_PREFIX, keytype, addr);
> +	if (snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr) >= (int)sizeof(desc))
> +		return -1;
>  
>  	/* set payload contents */
> -	len = sprintf(val, "%s:%s", user, pass);
> +	len = snprintf(val, sizeof(val), "%s:%s", user, pass);
> +	if (len >= (int)sizeof(val))
> +		return -1;
>  
>  	return add_key(CIFS_KEY_TYPE, desc, val, len + 1, DEST_KEYRING);
>  }
> 
> 

I think we probably need to clean up the error handling in the callers
of these functions, but that's a separate problem. I'll go ahead and
merge this, and we can do that in a separate patch.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] cifskey: better use snprintf()

[Jeff Layton]

On Mon, 14 Apr 2014 11:39:41 +0200
Sebastian Krahmer <krahmer-l3A5Bk7waGM@public.gmane.org> wrote:

> 
> Prefer snprintf() over sprintf() in cifskey.c
> Projects that fork the code (pam_cifscreds) can't rely on
> the max-size parameters.
> 
> Signed-off-by: Sebastian Krahmer <krahmer-l3A5Bk7waGM@public.gmane.org>
> ---
> 
> 
> --- cifskey.c.orig	2014-04-08 13:10:41.653435040 +0200
> +++ cifskey.c	2014-04-14 11:19:07.000118206 +0200
> @@ -29,7 +29,8 @@
>  {
>  	char desc[INET6_ADDRSTRLEN + sizeof(KEY_PREFIX) + 4];
>  
> -	sprintf(desc, "%s:%c:%s", KEY_PREFIX, keytype, addr);
> +	if (snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr) >= (int)sizeof(desc))
> +		return -1;
>  
>  	return keyctl_search(DEST_KEYRING, CIFS_KEY_TYPE, desc, 0);
>  }
> @@ -38,15 +39,18 @@
>  key_serial_t
>  key_add(const char *addr, const char *user, const char *pass, char keytype)
>  {
> -	int len;
> +	int len = 0;
>  	char desc[INET6_ADDRSTRLEN + sizeof(KEY_PREFIX) + 4];
>  	char val[MOUNT_PASSWD_SIZE +  MAX_USERNAME_SIZE + 2];
>  
>  	/* set key description */
> -	sprintf(desc, "%s:%c:%s", KEY_PREFIX, keytype, addr);
> +	if (snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr) >= (int)sizeof(desc))
> +		return -1;
>  
>  	/* set payload contents */
> -	len = sprintf(val, "%s:%s", user, pass);
> +	len = snprintf(val, sizeof(val), "%s:%s", user, pass);
> +	if (len >= (int)sizeof(val))
> +		return -1;
>  
>  	return add_key(CIFS_KEY_TYPE, desc, val, len + 1, DEST_KEYRING);
>  }
> 
> 

Slightly modified version merged. Thanks for the patch!
-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH] cifskey: better use snprintf()

[Sebastian Krahmer]

On Tue, Apr 08, 2014 at 01:23:26PM -0400, Jeff Layton wrote:

> > > If you do that then you don't need to use strlen() either.
> > > 
> > > -- 
> > > Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> > 
> 
> Ok, I think you're correct about snprintf. I got it confused with
> sprintf, which doesn't always NULL-terminate.
> 
> If it does indeed always null-terminate then there is indeed no harm in
> using strlen, it's just not as efficient. Why not instead simply take
> the return value of snprintf and use that to determine whether the
> output got truncated? I think we'd rather return an error if it is,
> than pass in a possibly bogus string to add_key().

Could be done indeed.

Sebastian

-- 

~ perl self.pl
~ $_='print"\$_=\47$_\47;eval"';eval
~ krahmer-l3A5Bk7waGM@public.gmane.org - SuSE Security Team

Re: [PATCH] cifskey: better use snprintf()

[Jeff Layton]

On Tue, 8 Apr 2014 16:41:29 +0200
Sebastian Krahmer <krahmer-l3A5Bk7waGM@public.gmane.org> wrote:

> Hi
> 
> C standard says snprintf() is writing
> the terminating 0-byte (thats indeed the real beauty of snprintf).
> Nevertheless snprintf() return value may be
> larger than buffer size (# bytes that would have been written).
> So strlen() should be safe and the buffer is 0-terminated in any case.
> 
> Sebastian
> 
> On Tue, Apr 08, 2014 at 10:32:12AM -0400, Jeff Layton wrote:
> > On Tue, 8 Apr 2014 14:44:44 +0200
> > Sebastian Krahmer <krahmer-l3A5Bk7waGM@public.gmane.org> wrote:
> > 
> > > 
> > > Prefer snprintf() over sprintf() in cifskey.c
> > > Projects that fork the code (pam_cifscreds) can't rely on
> > > the max-size parameters. Also use strlen() for determining
> > > buffer size, as snprintf() may return values larger than buffer size.
> > > 
> > > 
> > > Signed-off-by: Sebastian Krahmer <krahmer-l3A5Bk7waGM@public.gmane.org>
> > > ---
> > > 
> > > 
> > > --- cifskey.c.orig	2014-04-08 13:10:41.653435040 +0200
> > > +++ cifskey.c	2014-04-08 14:28:54.457766913 +0200
> > > @@ -20,6 +20,7 @@
> > >  #include <sys/types.h>
> > >  #include <keyutils.h>
> > >  #include <stdio.h>
> > > +#include <string.h>
> > >  #include "cifskey.h"
> > >  #include "resolve_host.h"
> > >  
> > > @@ -29,7 +30,7 @@
> > >  {
> > >  	char desc[INET6_ADDRSTRLEN + sizeof(KEY_PREFIX) + 4];
> > >  
> > > -	sprintf(desc, "%s:%c:%s", KEY_PREFIX, keytype, addr);
> > > +	snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr);
> > 
> > If we're concerned about buffer overruns here, then shouldn't you be
> > checking the return value of snprintf() to ensure that the string above
> > is NULL terminated?
> > 
> > >  
> > >  	return keyctl_search(DEST_KEYRING, CIFS_KEY_TYPE, desc, 0);
> > >  }
> > > @@ -38,15 +39,14 @@
> > >  key_serial_t
> > >  key_add(const char *addr, const char *user, const char *pass, char keytype)
> > >  {
> > > -	int len;
> > >  	char desc[INET6_ADDRSTRLEN + sizeof(KEY_PREFIX) + 4];
> > >  	char val[MOUNT_PASSWD_SIZE +  MAX_USERNAME_SIZE + 2];
> > >  
> > >  	/* set key description */
> > > -	sprintf(desc, "%s:%c:%s", KEY_PREFIX, keytype, addr);
> > > +	snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr);
> > >  
> > >  	/* set payload contents */
> > > -	len = sprintf(val, "%s:%s", user, pass);
> > > +	snprintf(val, sizeof(val), "%s:%s", user, pass);
> > >  
> > > -	return add_key(CIFS_KEY_TYPE, desc, val, len + 1, DEST_KEYRING);
> > > +	return add_key(CIFS_KEY_TYPE, desc, val, strlen(val) + 1, DEST_KEYRING);
> > >  }
> > > 
> > > 
> > 
> > Ditto with the above checks. Just because you're using snprintf doesn't
> > mean that the resulting string will be NULL terminated. You need to
> > check the return value of snprintf and ensure that it fits within the
> > buffer and that it ended up being NULL terminated.
> > 
> > If you do that then you don't need to use strlen() either.
> > 
> > -- 
> > Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> 

Ok, I think you're correct about snprintf. I got it confused with
sprintf, which doesn't always NULL-terminate.

If it does indeed always null-terminate then there is indeed no harm in
using strlen, it's just not as efficient. Why not instead simply take
the return value of snprintf and use that to determine whether the
output got truncated? I think we'd rather return an error if it is,
than pass in a possibly bogus string to add_key().

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH] cifskey: better use snprintf()

[Sebastian Krahmer]

Hi

C standard says snprintf() is writing
the terminating 0-byte (thats indeed the real beauty of snprintf).
Nevertheless snprintf() return value may be
larger than buffer size (# bytes that would have been written).
So strlen() should be safe and the buffer is 0-terminated in any case.

Sebastian

On Tue, Apr 08, 2014 at 10:32:12AM -0400, Jeff Layton wrote:
> On Tue, 8 Apr 2014 14:44:44 +0200
> Sebastian Krahmer <krahmer-l3A5Bk7waGM@public.gmane.org> wrote:
> 
> > 
> > Prefer snprintf() over sprintf() in cifskey.c
> > Projects that fork the code (pam_cifscreds) can't rely on
> > the max-size parameters. Also use strlen() for determining
> > buffer size, as snprintf() may return values larger than buffer size.
> > 
> > 
> > Signed-off-by: Sebastian Krahmer <krahmer-l3A5Bk7waGM@public.gmane.org>
> > ---
> > 
> > 
> > --- cifskey.c.orig	2014-04-08 13:10:41.653435040 +0200
> > +++ cifskey.c	2014-04-08 14:28:54.457766913 +0200
> > @@ -20,6 +20,7 @@
> >  #include <sys/types.h>
> >  #include <keyutils.h>
> >  #include <stdio.h>
> > +#include <string.h>
> >  #include "cifskey.h"
> >  #include "resolve_host.h"
> >  
> > @@ -29,7 +30,7 @@
> >  {
> >  	char desc[INET6_ADDRSTRLEN + sizeof(KEY_PREFIX) + 4];
> >  
> > -	sprintf(desc, "%s:%c:%s", KEY_PREFIX, keytype, addr);
> > +	snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr);
> 
> If we're concerned about buffer overruns here, then shouldn't you be
> checking the return value of snprintf() to ensure that the string above
> is NULL terminated?
> 
> >  
> >  	return keyctl_search(DEST_KEYRING, CIFS_KEY_TYPE, desc, 0);
> >  }
> > @@ -38,15 +39,14 @@
> >  key_serial_t
> >  key_add(const char *addr, const char *user, const char *pass, char keytype)
> >  {
> > -	int len;
> >  	char desc[INET6_ADDRSTRLEN + sizeof(KEY_PREFIX) + 4];
> >  	char val[MOUNT_PASSWD_SIZE +  MAX_USERNAME_SIZE + 2];
> >  
> >  	/* set key description */
> > -	sprintf(desc, "%s:%c:%s", KEY_PREFIX, keytype, addr);
> > +	snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr);
> >  
> >  	/* set payload contents */
> > -	len = sprintf(val, "%s:%s", user, pass);
> > +	snprintf(val, sizeof(val), "%s:%s", user, pass);
> >  
> > -	return add_key(CIFS_KEY_TYPE, desc, val, len + 1, DEST_KEYRING);
> > +	return add_key(CIFS_KEY_TYPE, desc, val, strlen(val) + 1, DEST_KEYRING);
> >  }
> > 
> > 
> 
> Ditto with the above checks. Just because you're using snprintf doesn't
> mean that the resulting string will be NULL terminated. You need to
> check the return value of snprintf and ensure that it fits within the
> buffer and that it ended up being NULL terminated.
> 
> If you do that then you don't need to use strlen() either.
> 
> -- 
> Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

-- 

~ perl self.pl
~ $_='print"\$_=\47$_\47;eval"';eval
~ krahmer-l3A5Bk7waGM@public.gmane.org - SuSE Security Team

Re: [PATCH] cifskey: better use snprintf()

[Jeff Layton]

On Tue, 8 Apr 2014 14:44:44 +0200
Sebastian Krahmer <krahmer-l3A5Bk7waGM@public.gmane.org> wrote:

> 
> Prefer snprintf() over sprintf() in cifskey.c
> Projects that fork the code (pam_cifscreds) can't rely on
> the max-size parameters. Also use strlen() for determining
> buffer size, as snprintf() may return values larger than buffer size.
> 
> 
> Signed-off-by: Sebastian Krahmer <krahmer-l3A5Bk7waGM@public.gmane.org>
> ---
> 
> 
> --- cifskey.c.orig	2014-04-08 13:10:41.653435040 +0200
> +++ cifskey.c	2014-04-08 14:28:54.457766913 +0200
> @@ -20,6 +20,7 @@
>  #include <sys/types.h>
>  #include <keyutils.h>
>  #include <stdio.h>
> +#include <string.h>
>  #include "cifskey.h"
>  #include "resolve_host.h"
>  
> @@ -29,7 +30,7 @@
>  {
>  	char desc[INET6_ADDRSTRLEN + sizeof(KEY_PREFIX) + 4];
>  
> -	sprintf(desc, "%s:%c:%s", KEY_PREFIX, keytype, addr);
> +	snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr);

If we're concerned about buffer overruns here, then shouldn't you be
checking the return value of snprintf() to ensure that the string above
is NULL terminated?

>  
>  	return keyctl_search(DEST_KEYRING, CIFS_KEY_TYPE, desc, 0);
>  }
> @@ -38,15 +39,14 @@
>  key_serial_t
>  key_add(const char *addr, const char *user, const char *pass, char keytype)
>  {
> -	int len;
>  	char desc[INET6_ADDRSTRLEN + sizeof(KEY_PREFIX) + 4];
>  	char val[MOUNT_PASSWD_SIZE +  MAX_USERNAME_SIZE + 2];
>  
>  	/* set key description */
> -	sprintf(desc, "%s:%c:%s", KEY_PREFIX, keytype, addr);
> +	snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr);
>  
>  	/* set payload contents */
> -	len = sprintf(val, "%s:%s", user, pass);
> +	snprintf(val, sizeof(val), "%s:%s", user, pass);
>  
> -	return add_key(CIFS_KEY_TYPE, desc, val, len + 1, DEST_KEYRING);
> +	return add_key(CIFS_KEY_TYPE, desc, val, strlen(val) + 1, DEST_KEYRING);
>  }
> 
> 

Ditto with the above checks. Just because you're using snprintf doesn't
mean that the resulting string will be NULL terminated. You need to
check the return value of snprintf and ensure that it fits within the
buffer and that it ended up being NULL terminated.

If you do that then you don't need to use strlen() either.

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

[PATCH] cifskey: better use snprintf()

[Sebastian Krahmer]

[-- Attachment #1: Type: text/plain, Size: 1635 bytes --]


Prefer snprintf() over sprintf() in cifskey.c
Projects that fork the code (pam_cifscreds) can't rely on
the max-size parameters. Also use strlen() for determining
buffer size, as snprintf() may return values larger than buffer size.


Signed-off-by: Sebastian Krahmer <krahmer-l3A5Bk7waGM@public.gmane.org>
---


--- cifskey.c.orig	2014-04-08 13:10:41.653435040 +0200
+++ cifskey.c	2014-04-08 14:28:54.457766913 +0200
@@ -20,6 +20,7 @@
 #include <sys/types.h>
 #include <keyutils.h>
 #include <stdio.h>
+#include <string.h>
 #include "cifskey.h"
 #include "resolve_host.h"
 
@@ -29,7 +30,7 @@
 {
 	char desc[INET6_ADDRSTRLEN + sizeof(KEY_PREFIX) + 4];
 
-	sprintf(desc, "%s:%c:%s", KEY_PREFIX, keytype, addr);
+	snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr);
 
 	return keyctl_search(DEST_KEYRING, CIFS_KEY_TYPE, desc, 0);
 }
@@ -38,15 +39,14 @@
 key_serial_t
 key_add(const char *addr, const char *user, const char *pass, char keytype)
 {
-	int len;
 	char desc[INET6_ADDRSTRLEN + sizeof(KEY_PREFIX) + 4];
 	char val[MOUNT_PASSWD_SIZE +  MAX_USERNAME_SIZE + 2];
 
 	/* set key description */
-	sprintf(desc, "%s:%c:%s", KEY_PREFIX, keytype, addr);
+	snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr);
 
 	/* set payload contents */
-	len = sprintf(val, "%s:%s", user, pass);
+	snprintf(val, sizeof(val), "%s:%s", user, pass);
 
-	return add_key(CIFS_KEY_TYPE, desc, val, len + 1, DEST_KEYRING);
+	return add_key(CIFS_KEY_TYPE, desc, val, strlen(val) + 1, DEST_KEYRING);
 }


-- 

~ perl self.pl
~ $_='print"\$_=\47$_\47;eval"';eval
~ krahmer-l3A5Bk7waGM@public.gmane.org - SuSE Security Team


[-- Attachment #2: cifskey-overflow.patch --]
[-- Type: text/x-patch, Size: 1201 bytes --]

--- cifskey.c.orig	2014-04-08 13:10:41.653435040 +0200
+++ cifskey.c	2014-04-08 14:28:54.457766913 +0200
@@ -20,6 +20,7 @@
 #include <sys/types.h>
 #include <keyutils.h>
 #include <stdio.h>
+#include <string.h>
 #include "cifskey.h"
 #include "resolve_host.h"
 
@@ -29,7 +30,7 @@
 {
 	char desc[INET6_ADDRSTRLEN + sizeof(KEY_PREFIX) + 4];
 
-	sprintf(desc, "%s:%c:%s", KEY_PREFIX, keytype, addr);
+	snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr);
 
 	return keyctl_search(DEST_KEYRING, CIFS_KEY_TYPE, desc, 0);
 }
@@ -38,15 +39,14 @@
 key_serial_t
 key_add(const char *addr, const char *user, const char *pass, char keytype)
 {
-	int len;
 	char desc[INET6_ADDRSTRLEN + sizeof(KEY_PREFIX) + 4];
 	char val[MOUNT_PASSWD_SIZE +  MAX_USERNAME_SIZE + 2];
 
 	/* set key description */
-	sprintf(desc, "%s:%c:%s", KEY_PREFIX, keytype, addr);
+	snprintf(desc, sizeof(desc), "%s:%c:%s", KEY_PREFIX, keytype, addr);
 
 	/* set payload contents */
-	len = sprintf(val, "%s:%s", user, pass);
+	snprintf(val, sizeof(val), "%s:%s", user, pass);
 
-	return add_key(CIFS_KEY_TYPE, desc, val, len + 1, DEST_KEYRING);
+	return add_key(CIFS_KEY_TYPE, desc, val, strlen(val) + 1, DEST_KEYRING);
 }