[PATCH] keyctl: use keyctl_read_alloc() in dump_key_tree_aux()

[PATCH] keyctl: use keyctl_read_alloc() in dump_key_tree_aux()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

dump_key_tree_aux() (part of 'keyctl show') was racy: it allocated a
buffer for the keyring contents, then read the keyring.  But it's
possible that keys are added to the keyring concurrently.  This is
problematic for two reasons.  First, when keyctl_read() is passed a
buffer that is too small, it is unspecified whether it is filled or not.
Second, even if the buffer is filled, some keys (not necessarily even
the newest ones) would be omitted from the listing.

Switch to keyctl_read_alloc() which handles the "buffer too small" case
correctly by retrying the read.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 keyctl.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/keyctl.c b/keyctl.c
index 801a864..65a7397 100644
--- a/keyctl.c
+++ b/keyctl.c
@@ -1808,29 +1808,18 @@ static int dump_key_tree_aux(key_serial_t key, int depth, int more, int hex_key_
 	/* if it's a keyring then we're going to want to recursively
 	 * display it if we can */
 	if (strcmp(type, "keyring") = 0) {
-		/* find out how big the keyring is */
-		ret = keyctl_read(key, NULL, 0);
-		if (ret < 0)
-			error("keyctl_read");
-		if (ret = 0)
-			return 0;
-		ringlen = ret;
 
 		/* read its contents */
-		payload = malloc(ringlen);
-		if (!payload)
-			error("malloc");
-
-		ret = keyctl_read(key, payload, ringlen);
+		ret = keyctl_read_alloc(key, &payload);
 		if (ret < 0)
-			error("keyctl_read");
+			error("keyctl_read_alloc");
 
-		ringlen = ret < ringlen ? ret : ringlen;
+		ringlen = ret;
 		kcount = ringlen / sizeof(key_serial_t);
 
 		/* walk the keyring */
 		pk = payload;
-		do {
+		while (ringlen >= sizeof(key_serial_t)) {
 			key = *pk++;
 
 			/* recurse into nexted keyrings */
@@ -1858,7 +1847,8 @@ static int dump_key_tree_aux(key_serial_t key, int depth, int more, int hex_key_
 							    hex_key_IDs);
 			}
 
-		} while (ringlen -= 4, ringlen >= sizeof(key_serial_t));
+			ringlen -= sizeof(key_serial_t);
+		}
 
 		free(payload);
 	}
-- 
2.15.0.rc2.357.g7e34df9404-goog

Re: [PATCH] keyctl: use keyctl_read_alloc() in dump_key_tree_aux()

[James Morris]

On Thu, 26 Oct 2017, Eric Biggers wrote:

> From: Eric Biggers <ebiggers@google.com>
> 
> dump_key_tree_aux() (part of 'keyctl show') was racy: it allocated a
> buffer for the keyring contents, then read the keyring.  But it's
> possible that keys are added to the keyring concurrently.  This is
> problematic for two reasons.  First, when keyctl_read() is passed a
> buffer that is too small, it is unspecified whether it is filled or not.
> Second, even if the buffer is filled, some keys (not necessarily even
> the newest ones) would be omitted from the listing.
> 
> Switch to keyctl_read_alloc() which handles the "buffer too small" case
> correctly by retrying the read.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>


Reviewed-by: James Morris <james.l.morris@oracle.com>

-- 
James Morris
<james.l.morris@oracle.com>

Re: [PATCH] keyctl: use keyctl_read_alloc() in dump_key_tree_aux()

[Eric Biggers]

On Thu, Oct 26, 2017 at 02:00:08PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> dump_key_tree_aux() (part of 'keyctl show') was racy: it allocated a
> buffer for the keyring contents, then read the keyring.  But it's
> possible that keys are added to the keyring concurrently.  This is
> problematic for two reasons.  First, when keyctl_read() is passed a
> buffer that is too small, it is unspecified whether it is filled or not.
> Second, even if the buffer is filled, some keys (not necessarily even
> the newest ones) would be omitted from the listing.
> 
> Switch to keyctl_read_alloc() which handles the "buffer too small" case
> correctly by retrying the read.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  keyctl.c | 22 ++++++----------------
>  1 file changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/keyctl.c b/keyctl.c
> index 801a864..65a7397 100644
> --- a/keyctl.c
> +++ b/keyctl.c
> @@ -1808,29 +1808,18 @@ static int dump_key_tree_aux(key_serial_t key, int depth, int more, int hex_key_
>  	/* if it's a keyring then we're going to want to recursively
>  	 * display it if we can */
>  	if (strcmp(type, "keyring") = 0) {
> -		/* find out how big the keyring is */
> -		ret = keyctl_read(key, NULL, 0);
> -		if (ret < 0)
> -			error("keyctl_read");
> -		if (ret = 0)
> -			return 0;
> -		ringlen = ret;
>  
>  		/* read its contents */
> -		payload = malloc(ringlen);
> -		if (!payload)
> -			error("malloc");
> -
> -		ret = keyctl_read(key, payload, ringlen);
> +		ret = keyctl_read_alloc(key, &payload);
>  		if (ret < 0)
> -			error("keyctl_read");
> +			error("keyctl_read_alloc");
>  
> -		ringlen = ret < ringlen ? ret : ringlen;
> +		ringlen = ret;
>  		kcount = ringlen / sizeof(key_serial_t);
>  
>  		/* walk the keyring */
>  		pk = payload;
> -		do {
> +		while (ringlen >= sizeof(key_serial_t)) {
>  			key = *pk++;
>  
>  			/* recurse into nexted keyrings */
> @@ -1858,7 +1847,8 @@ static int dump_key_tree_aux(key_serial_t key, int depth, int more, int hex_key_
>  							    hex_key_IDs);
>  			}
>  
> -		} while (ringlen -= 4, ringlen >= sizeof(key_serial_t));
> +			ringlen -= sizeof(key_serial_t);
> +		}
>  
>  		free(payload);
>  	}
> -- 
> 2.15.0.rc2.357.g7e34df9404-goog
> 

Ping.

Re: [PATCH] keyctl: use keyctl_read_alloc() in dump_key_tree_aux()

[Eric Biggers]

On Tue, Feb 20, 2018 at 11:43:49AM -0800, Eric Biggers wrote:
> On Thu, Oct 26, 2017 at 02:00:08PM -0700, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > dump_key_tree_aux() (part of 'keyctl show') was racy: it allocated a
> > buffer for the keyring contents, then read the keyring.  But it's
> > possible that keys are added to the keyring concurrently.  This is
> > problematic for two reasons.  First, when keyctl_read() is passed a
> > buffer that is too small, it is unspecified whether it is filled or not.
> > Second, even if the buffer is filled, some keys (not necessarily even
> > the newest ones) would be omitted from the listing.
> > 
> > Switch to keyctl_read_alloc() which handles the "buffer too small" case
> > correctly by retrying the read.
> > 
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > ---
> >  keyctl.c | 22 ++++++----------------
> >  1 file changed, 6 insertions(+), 16 deletions(-)
> > 
> > diff --git a/keyctl.c b/keyctl.c
> > index 801a864..65a7397 100644
> > --- a/keyctl.c
> > +++ b/keyctl.c
> > @@ -1808,29 +1808,18 @@ static int dump_key_tree_aux(key_serial_t key, int depth, int more, int hex_key_
> >  	/* if it's a keyring then we're going to want to recursively
> >  	 * display it if we can */
> >  	if (strcmp(type, "keyring") = 0) {
> > -		/* find out how big the keyring is */
> > -		ret = keyctl_read(key, NULL, 0);
> > -		if (ret < 0)
> > -			error("keyctl_read");
> > -		if (ret = 0)
> > -			return 0;
> > -		ringlen = ret;
> >  
> >  		/* read its contents */
> > -		payload = malloc(ringlen);
> > -		if (!payload)
> > -			error("malloc");
> > -
> > -		ret = keyctl_read(key, payload, ringlen);
> > +		ret = keyctl_read_alloc(key, &payload);
> >  		if (ret < 0)
> > -			error("keyctl_read");
> > +			error("keyctl_read_alloc");
> >  
> > -		ringlen = ret < ringlen ? ret : ringlen;
> > +		ringlen = ret;
> >  		kcount = ringlen / sizeof(key_serial_t);
> >  
> >  		/* walk the keyring */
> >  		pk = payload;
> > -		do {
> > +		while (ringlen >= sizeof(key_serial_t)) {
> >  			key = *pk++;
> >  
> >  			/* recurse into nexted keyrings */
> > @@ -1858,7 +1847,8 @@ static int dump_key_tree_aux(key_serial_t key, int depth, int more, int hex_key_
> >  							    hex_key_IDs);
> >  			}
> >  
> > -		} while (ringlen -= 4, ringlen >= sizeof(key_serial_t));
> > +			ringlen -= sizeof(key_serial_t);
> > +		}
> >  
> >  		free(payload);
> >  	}
> > -- 
> > 2.15.0.rc2.357.g7e34df9404-goog
> > 
> 
> Ping.

Ping.