On Tue, Jul 05, 2022 at 05:57:25PM -0400, Josh Rickmar wrote:
> On Tue, Jul 05, 2022 at 05:48:22PM -0400, Josh Rickmar wrote:
> > On Tue, Jul 05, 2022 at 11:08:56PM +0200, Stefan Sperling wrote:
> > > On Tue, Jul 05, 2022 at 10:53:34PM +0200, Stefan Sperling wrote:
> > > > On Tue, Jul 05, 2022 at 10:16:13PM +0200, Stefan Sperling wrote:
> > > > > On Tue, Jul 05, 2022 at 03:12:19PM -0400, Josh Rickmar wrote:
> > > > > > -----------------------------------------------
> > > > > > commit 24b0007a1fd9c963f5e1e243919476e449b5c8dc (linux_pipe)
> > > > > > from: Josh Rickmar <jrick@zettaport.com>
> > > > > > date: Tue Jul  5 19:07:51 2022 UTC
> > > > > >  
> > > > > >  use correct pipe ends on linux
> > > > > >  
> > > > > >  Fixes fdopen errors opening the pipe fds to read ssh-keygen stdout.
> > > > > > 
> > > > > >  Reported by abieber@
> > > > > 
> > > > > This does not seem to fix the issue properly for me.
> > > > > 
> > > > > While creation of a signed tag now apparently succeeds, 'got cat' shows
> > > > > no signature on the tag object and verification with got tag -V fails.
> > > > > I am testing on Ubuntu.
> > > > > 
> > > > > 
> > > > 
> > > > The problem seems to be use of fdopen(3) on a pipe.
> > > > 
> > > > With this patch signatures show up in tag objects signed on linux.
> > > > The verification part also uses fdopen and must still be fixed in
> > > > the same way:
> > > 
> > > My previous patch had a bug where sig_len no longer accounted
> > > for the terminating NUL byte of the signature string. Fixed here.
> > > 
> > > Also, error out if the SSH signature has a zero length.
> > 
> > thanks for investigating this and providing this patch.  here is
> > something based on your read loop, but i moved it to buf.c so it can
> > be used for both the signing and verifing path.
> 
> now with less segfaults

and more error checking

-----------------------------------------------
commit f66537004a0a1ad4dbb93590f056d2c7517bc9c1 (linux_pipe)
from: Josh Rickmar <jrick@zettaport.com>
date: Tue Jul  5 22:01:24 2022 UTC
 
 fix pipe usage for linux
 
 This uses the correct read and write ends of the fds returned by
 pipe(2) on linux.  It also moves away from creating FILE* streams with
 fdopen and reading the stream with buf_load (which performs a fstat,
 and breaks due to a zero length file size on linux) by instead reading
 from the fd itself.
 
 Reported by abieber@, with assistance from stsp@
 
diff d89a2828a39d1ae3d6c68b2d325ce6a3d0ca768a f66537004a0a1ad4dbb93590f056d2c7517bc9c1
commit - d89a2828a39d1ae3d6c68b2d325ce6a3d0ca768a
commit + f66537004a0a1ad4dbb93590f056d2c7517bc9c1
blob - 05d16ce90ea867ddc912cd1321d68b0262228307
blob + f086396bf043a23dced98677425945d28189e3f7
--- lib/buf.c
+++ lib/buf.c
@@ -126,6 +126,32 @@ out:
 	return err;
 }
 
+const struct got_error *
+buf_load_fd(BUF **buf, int fd)
+{
+	const struct got_error *err = NULL;
+	unsigned char out[8192];
+	ssize_t r;
+	size_t len;
+
+	err = buf_alloc(buf, 8192);
+	if (err)
+		return err;
+
+	do {
+		r = read(fd, out, sizeof(out));
+		if (r == -1)
+			return got_error_from_errno("read");
+		if (r > 0) {
+			err = buf_append(&len, *buf, out, r);
+			if (err)
+				return err;
+		}
+	} while (r > 0);
+
+	return NULL;
+}
+
 void
 buf_free(BUF *b)
 {
blob - 354d7c4ea8684b77bc84fa0fcad6b6c8e2b2be80
blob + aff1492306e97d3fdd255c836d083eea96b30e19
--- lib/buf.h
+++ lib/buf.h
@@ -51,6 +51,7 @@ struct buf {
 
 const struct got_error *buf_alloc(BUF **, size_t);
 const struct got_error *buf_load(BUF **, FILE *);
+const struct got_error *buf_load_fd(BUF **, int fd);
 void		 buf_free(BUF *);
 void		*buf_release(BUF *);
 u_char		 buf_getc(BUF *, size_t);
blob - 2e773fde3898b103364cc053c27f666b78abea2d
blob + e7a5a2312e750051effc62220e0e5ca92cdd0a24
--- lib/object_create.c
+++ lib/object_create.c
@@ -688,7 +688,6 @@ got_object_tag_create(struct got_object_id **id,
 		msg++;
 
 	if (signer_id) {
-		FILE *out;
 		pid_t pid;
 		size_t len;
 		int in_fd, out_fd;
@@ -743,13 +742,8 @@ got_object_tag_create(struct got_object_id **id,
 			goto done;
 		}
 
-		out = fdopen(out_fd, "r");
-		if (out == NULL) {
-			err = got_error_from_errno("fdopen");
-			goto done;
-		}
 		buf_empty(buf);
-		err = buf_load(&buf, out);
+		err = buf_load_fd(&buf, out_fd);
 		if (err)
 			goto done;
 		sig_len = buf_len(buf) + 1;
blob - ac1b2637f90cec3243da8a059be58205a4621203
blob + 7084c6ce1b92b634c964d90d3ef6e32ce922a786
--- lib/sigs.c
+++ lib/sigs.c
@@ -116,11 +116,11 @@ got_sigs_sign_tag_ssh(pid_t *newpid, int *in_fd, int *
 	} else if (pid == 0) {
 		if (close(in_pfd[1]) == -1)
 			err(1, "close");
-		if (close(out_pfd[1]) == -1)
+		if (close(out_pfd[0]) == -1)
 			err(1, "close");
 		if (dup2(in_pfd[0], 0) == -1)
 			err(1, "dup2");
-		if (dup2(out_pfd[0], 1) == -1)
+		if (dup2(out_pfd[1], 1) == -1)
 			err(1, "dup2");
 		if (execv(GOT_TAG_PATH_SSH_KEYGEN, (char **const)argv) == -1)
 			err(1, "execv");
@@ -128,11 +128,11 @@ got_sigs_sign_tag_ssh(pid_t *newpid, int *in_fd, int *
 	}
 	if (close(in_pfd[0]) == -1)
 		return got_error_from_errno("close");
-	if (close(out_pfd[0]) == -1)
+	if (close(out_pfd[1]) == -1)
 		return got_error_from_errno("close");
 	*newpid = pid;
 	*in_fd = in_pfd[1];
-	*out_fd = out_pfd[1];
+	*out_fd = out_pfd[0];
 	return NULL;
 }
 
@@ -267,7 +267,7 @@ got_sigs_verify_tag_ssh(char **msg, struct got_tag_obj
 	char* parsed_identity = NULL;
 	const char *identity;
 	char* tmppath = NULL;
-	FILE *tmpsig, *out = NULL;
+	FILE *tmpsig = NULL;
 	BUF *buf;
 	int i = 0, j;
 
@@ -342,11 +342,11 @@ got_sigs_verify_tag_ssh(char **msg, struct got_tag_obj
 	} else if (pid == 0) {
 		if (close(in_pfd[1]) == -1)
 			err(1, "close");
-		if (close(out_pfd[1]) == -1)
+		if (close(out_pfd[0]) == -1)
 			err(1, "close");
 		if (dup2(in_pfd[0], 0) == -1)
 			err(1, "dup2");
-		if (dup2(out_pfd[0], 1) == -1)
+		if (dup2(out_pfd[1], 1) == -1)
 			err(1, "dup2");
 		if (execv(GOT_TAG_PATH_SSH_KEYGEN, (char **const)argv) == -1)
 			err(1, "execv");
@@ -356,7 +356,7 @@ got_sigs_verify_tag_ssh(char **msg, struct got_tag_obj
 		error = got_error_from_errno("close");
 		goto done;
 	}
-	if (close(out_pfd[0]) == -1) {
+	if (close(out_pfd[1]) == -1) {
 		error = got_error_from_errno("close");
 		goto done;
 	}
@@ -377,22 +377,16 @@ got_sigs_verify_tag_ssh(char **msg, struct got_tag_obj
 		goto done;
 	}
 
-	out = fdopen(out_pfd[1], "r");
-	if (out == NULL) {
-		error = got_error_from_errno("fdopen");
-		goto done;
-	}
-	error = buf_load(&buf, out);
+	error = buf_load_fd(&buf, out_pfd[0]);
 	if (error)
 		goto done;
 	error = buf_putc(buf, '\0');
 	if (error)
 		goto done;
-	if (close(out_pfd[1]) == -1) {
+	if (close(out_pfd[0]) == -1) {
 		error = got_error_from_errno("close");
 		goto done;
 	}
-	out = NULL;
 	*msg = buf_get(buf);
 	if (WEXITSTATUS(status) != 0)
 		error = got_error(GOT_ERR_BAD_TAG_SIGNATURE);
@@ -400,9 +394,8 @@ got_sigs_verify_tag_ssh(char **msg, struct got_tag_obj
 done:
 	free(parsed_identity);
 	free(tmppath);
+	close(out_pfd[0]);
 	if (tmpsig && fclose(tmpsig) == EOF && error == NULL)
 		error = got_error_from_errno("fclose");
-	if (out && fclose(out) == EOF && error == NULL)
-		error = got_error_from_errno("fclose");
 	return error;
 }