Bug#758615: [patch] more error handling remove global state

2014-09-02 Thread Michael Vogt 
On Sun, Aug 31, 2014 at 06:10:36AM +0200, Guillem Jover wrote:
 Hi!
Hello,
 
 On Tue, 2014-08-19 at 11:23:41 +0200, Michael Vogt wrote:
[..]
 Some comments on the points raised in the review, although it's true that
 dpkg itself should only be dealing with “trusted” data, otherwise you are
 going to be happily giving root accesss away, dpkg-deb does not, so it
 must be picky and suspicious when parsing .deb packages. And for most (if
 not all) of the dpkg .deb parsing code I've either rewritten or at least
 extensively reviewed it by now, that obviously does not mean there will
 be no bugs, but besides code staring, unit tests, functional tests [F],
 code checkers like clang, cppcheck and coverity among others do help. So
 I do trust more the dpkg code than the debsig-verify code. Precisely one
 of the reasons for taking it over was to update its .deb format support,
 including LFS. Of course debsig-verify code should be considered more
 sensitive, because it's not just about inspecting, but about deciding
 to end up giving direct root access to possibly untrusted packages.
[..]

thanks for these comments, that is good to know!
 
 Regarding adoption of debsig-verify, I'm planning to work on updating
 the layout of the signatures, and to properly integrate this into dpkg
 proper. Once I start those discussions, I'll try to make sure to keep you
 and Colin Watson on the loop, as you guys seem to be interested in
 this?

Yes, please keep us in the loop.
 
  Attached are two patches that add some additional error checking.
 
 I'll review and merge those in few days, after I finish up some other
 stuff, thanks!

Thanks, that is much appreciated.

  I also started with the removal of the global state
  (attached as well). However it is not very elegant and I wonder if it would
  make more sense to have a 
  
  struct ds_ctx {
 char *deb,
 FILE *deb_fs,
 char *originID
  }
  
  that is passed around as the context instead of my current approach.
 
 Ah, yeah, I thought I had started doing something like that already,
 but I cannot find any branch or stashed change, so either I just
 thought about it or I discarded it at the time. Anyway I'll check
 it out in few days.
[..]

Great, looking forward for your feedback. I guess I need to rework the
coding style a bit (based on the previous review I had) but I guess
its best if I wait for further feedback.

Attached are my remaining patches that add _FILE_OFFSET_BITS 64, add
a README and add a (simple) integration test (with a test origin key).

The test is not using a test framework currently, I'm happy to use
whatever you suggest, shunit2 seems like a good one but I have no
strong preferences either way.

Feedback welcome, and I hope my latest stuff does not contain silly
(debconf jetlag) issues :)

Cheers,
 MichaelFrom 1a4ee2063424f94f4d481f737870892bcf50e8aa Mon Sep 17 00:00:00 2001
From: Michael Vogt m...@ubuntu.com
Date: Thu, 21 Aug 2014 08:30:22 +0200
Subject: [PATCH 7/9] add _FILE_OFFSET_BITS 64

---
 debsig.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/debsig.h b/debsig.h
index ea6edb7..39e78ab 100644
--- a/debsig.h
+++ b/debsig.h
@@ -16,6 +16,7 @@
  * You should have received a copy of the GNU General Public License
  * along with this program.  If not, see https://www.gnu.org/licenses/.
  */
+#define _FILE_OFFSET_BITS 64
 
 #define DEBSIG_POLICIES_DIR_FMT %sDEBSIG_POLICIES_DIR/%s
 #define DEBSIG_KEYRINGS_FMT %sDEBSIG_KEYRINGS_DIR/%s/%s
-- 
2.0.0.rc0

From 79318503b0039b4705019e0308544ceee7f24305 Mon Sep 17 00:00:00 2001
From: Michael Vogt m...@ubuntu.com
Date: Thu, 21 Aug 2014 08:36:18 +0200
Subject: [PATCH 8/9] add README

---
 README | 22 ++
 1 file changed, 22 insertions(+)
 create mode 100644 README

diff --git a/README b/README
new file mode 100644
index 000..150a35f
--- /dev/null
+++ b/README
@@ -0,0 +1,22 @@
+= Debian package signature verification tool =
+
+This tool inspects and verifies binary package digital signatures based
+on predetermined policies, complementing repository signatures or allowing
+to verify the authenticity of a package even after download when detached
+from a repository.
+
+== How to build ==
+
+Ensure the build-dependencies are instaleld by running
+```
+$ dpkg-checkbuilddeps debian/control 
+```
+
+then type:
+```
+$ make
+```
+
+== Testing ==
+
+No automatic testsuite yet, manual testing needs to be performed.
-- 
2.0.0.rc0

From df421bdccf43ae520f676c9d1da0ab5788f1e3a0 Mon Sep 17 00:00:00 2001
From: Michael Vogt m...@ubuntu.com
Date: Tue, 2 Sep 2014 09:52:48 +0200
Subject: [PATCH 9/9] add simple integration test

---
 Makefile  |   3 +
 README|   6 +-
 testing/keyrings/FAD46790DE88C7E2/pubring.gpg | Bin 0 - 1245 bytes
 testing/keyrings/FAD46790DE88C7E2/secring.gpg | Bin 0 - 2547 bytes
 testing/policies/FAD46790DE88C7E2/generic.pol |  22 +++
 testing/test_debsig   |

Bug#758615: [patch] more error handling remove global state

2014-09-02 Thread Guillem Jover 
On Tue, 2014-09-02 at 10:24:55 +0200, Michael Vogt wrote:
 Attached are my remaining patches that add _FILE_OFFSET_BITS 64,

Ah, sorry forgot to mention this because I thought you had already
figured it out and because I mentioned it in passing. The build system
is already supporting LFS by way of «getconf LFS_CFLAGS» and
«getconf LFS_LDFLAGS». So this should end up expanding to the required
flags when needed (it also passes -D_LARGEFILE_SOURCE for example).

 add
 a README and add a (simple) integration test (with a test origin key).

Ah yes, good. I had in mind adding a test suite, but was waiting to
do that after autoconfiscating the build system.

 The test is not using a test framework currently, I'm happy to use
 whatever you suggest, shunit2 seems like a good one but I have no
 strong preferences either way.

So given the above I think I'd like to use autotest, which I'm also
pondering on using for dpkg's functional testsuite itself. But, let
me autoconfiscate it, and then we can see.

 Feedback welcome, and I hope my latest stuff does not contain silly
 (debconf jetlag) issues :)

Nah, just some minor typo(s). ;)

Thanks,
Guillem


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#758615: [patch] more error handling remove global state

2014-08-30 Thread Guillem Jover 
Hi!

On Tue, 2014-08-19 at 11:23:41 +0200, Michael Vogt wrote:
 Package: debsig-verify
 Version: 0.10
 
 because we want to use debsig-verify as part of the click project I
 asked the ubuntu security team for a quick code review [1]. There were
 some issues raised, notably that some error checks are missing and
 that the use of the global state.

Ah, nice! First, thanks for getting this reviewed, that's _always_ much
appreciated. I just took over the package pretty recently, so I've not
have had time yet to do all cleanups I'd like.

Some comments on the points raised in the review, although it's true that
dpkg itself should only be dealing with “trusted” data, otherwise you are
going to be happily giving root accesss away, dpkg-deb does not, so it
must be picky and suspicious when parsing .deb packages. And for most (if
not all) of the dpkg .deb parsing code I've either rewritten or at least
extensively reviewed it by now, that obviously does not mean there will
be no bugs, but besides code staring, unit tests, functional tests [F],
code checkers like clang, cppcheck and coverity among others do help. So
I do trust more the dpkg code than the debsig-verify code. Precisely one
of the reasons for taking it over was to update its .deb format support,
including LFS. Of course debsig-verify code should be considered more
sensitive, because it's not just about inspecting, but about deciding
to end up giving direct root access to possibly untrusted packages.

Also because libdpkg is supposed to be a public library at some point,
and because I maintain both packages, I take into account the needs of
them when doing changes on the dpkg side.

  [F] https://anonscm.debian.org/cgit/dpkg/pkg-tests.git, see the
  t-deb-format ones for example.

Regarding adoption of debsig-verify, I'm planning to work on updating
the layout of the signatures, and to properly integrate this into dpkg
proper. Once I start those discussions, I'll try to make sure to keep you
and Colin Watson on the loop, as you guys seem to be interested in this?

 Attached are two patches that add some additional error checking.

I'll review and merge those in few days, after I finish up some other
stuff, thanks!

 I also started with the removal of the global state
 (attached as well). However it is not very elegant and I wonder if it would
 make more sense to have a 
 
 struct ds_ctx {
char *deb,
FILE *deb_fs,
char *originID
 }
 
 that is passed around as the context instead of my current approach.

Ah, yeah, I thought I had started doing something like that already,
but I cannot find any branch or stashed change, so either I just
thought about it or I discarded it at the time. Anyway I'll check
it out in few days.

 And please let me know if you prefer a different workflow for (many)
 patches like this, I can also publish my git branch somewhere if that
 is easier for you.

Nah, I prefer patches to pulls, as they are easier to review. Feel
free to send either bug reports or just patch series directly to the
mailing list, as you prefer, I really don't mind.

Thanks,
Guillem


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#758615: [patch] more error handling remove global state

2014-08-19 Thread Michael Vogt 
Package: debsig-verify
Version: 0.10

Hello,

because we want to use debsig-verify as part of the click project I
asked the ubuntu security team for a quick code review [1]. There were
some issues raised, notably that some error checks are missing and
that the use of the global state.

Attached are two patches that add some additional error checking.

I also started with the removal of the global state
(attached as well). However it is not very elegant and I wonder if it would
make more sense to have a 

struct ds_ctx {
   char *deb,
   FILE *deb_fs,
   char *originID
}

that is passed around as the context instead of my current approach.

And please let me know if you prefer a different workflow for (many)
patches like this, I can also publish my git branch somewhere if that
is easier for you.

Feedback/review welcome!

Thanks,
 Michael


[1] 
https://bugs.launchpad.net/ubuntu/+source/debsig-verify/+bug/1358272/comments/2
From 8b89723dc6618d2718b4fa83d01c5df03ac83fca Mon Sep 17 00:00:00 2001
From: Michael Vogt m...@ubuntu.com
Date: Tue, 19 Aug 2014 10:09:24 +0200
Subject: [PATCH 1/5] add error checking on fork()

---
 gpg-parse.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/gpg-parse.c b/gpg-parse.c
index ab450af..14a9175 100644
--- a/gpg-parse.c
+++ b/gpg-parse.c
@@ -20,7 +20,7 @@
 /*
  * routines to parse gpg output
  */
-
+#include errno.h
 #include stdio.h
 #include string.h
 #include sys/types.h
@@ -120,7 +120,10 @@ char *getSigKeyID (const char *deb, const char *type) {
 	 (ds_write = fdopen(pwrite[1], w)) == NULL)
 	ds_fail_printf(DS_FAIL_INTERNAL, error opening file stream for gpg);
 
-if (!(pid = fork())) {
+pid = fork();
+if(pid  0)
+   ds_fail_printf(DS_FAIL_INTERNAL, failed to fork (errno %s), strerror(errno));
+if (pid == 0) {
 	/* Here we go */
 	dup2(pread[1],1); close(pread[0]); close(pread[1]);
 	dup2(pwrite[0],0); close(pwrite[0]); close(pwrite[1]);
@@ -186,7 +189,10 @@ int gpgVerify(const char *data, struct match *mtc, const char *sig) {
 	return 0;
 }
 
-if (!(pid = fork())) {
+pid = fork();
+if(pid  0)
+   ds_fail_printf(DS_FAIL_INTERNAL, failed to fork (%s), strerror(errno));
+if (pid == 0) {
 	if (DS_LEV_DEBUG  ds_debug_level) {
 	close(0); close(1); close(2);
 	}
-- 
2.0.0.rc0

From 8bc395f20d958cde6bf079d130a3de7118a922d5 Mon Sep 17 00:00:00 2001
From: Michael Vogt m...@ubuntu.com
Date: Tue, 19 Aug 2014 10:30:20 +0200
Subject: [PATCH 2/5] add error/eof checking into getSigKeyID()

---
 gpg-parse.c | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/gpg-parse.c b/gpg-parse.c
index 14a9175..e051519 100644
--- a/gpg-parse.c
+++ b/gpg-parse.c
@@ -98,13 +98,13 @@ char *getKeyID (const struct match *mtc) {
 }
 
 char *getSigKeyID (const char *deb, const char *type) {
-static char buf[2048];
+char buf[2048];
 int pread[2], pwrite[2], t;
-off_t len = checkSigExist(type);
 pid_t pid;
 FILE *ds_read, *ds_write;
 char *c, *ret = NULL;
 
+off_t len = checkSigExist(type);
 if (!len)
 	return NULL;
 
@@ -134,15 +134,19 @@ char *getSigKeyID (const char *deb, const char *type) {
 
 /* First, let's feed gpg our signature. Don't forget, our call to
  * checkSigExist() above positioned the deb_fs file pointer already.  */
-t = fread(buf, 1, sizeof(buf), deb_fs);
-while(len  0) {
+do {
+   t = fread(buf, 1, sizeof(buf), deb_fs);
+   if (ferror(deb_fs))
+  ds_fail_printf(DS_FAIL_INTERNAL, getSigKeyID: error reading signature (%s),
+ strerror(errno));
+
 	if (t  len)
 	fwrite(buf, 1, len, ds_write);
 	else
 	fwrite(buf, 1, t, ds_write);
 	len -= t;
-	t = fread(buf, 1, sizeof(buf), deb_fs);
-}
+} while(len  0 || !feof(deb_fs));
+
 if (ferror(ds_write))
 	ds_fail_printf(DS_FAIL_INTERNAL, error writing to gpg);
 fclose(ds_write);
-- 
2.0.0.rc0

From 3096b1471e3248b09d4d5eaec618a1002e6acf8c Mon Sep 17 00:00:00 2001
From: Michael Vogt m...@ubuntu.com
Date: Tue, 19 Aug 2014 10:44:58 +0200
Subject: [PATCH 3/5] remove global *deb var

---
 ar-parse.c  |  2 +-
 debsig-verify.c | 23 +++
 debsig.h|  4 ++--
 gpg-parse.c |  2 +-
 misc.c  |  4 ++--
 5 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/ar-parse.c b/ar-parse.c
index 477bf15..5146491 100644
--- a/ar-parse.c
+++ b/ar-parse.c
@@ -39,7 +39,7 @@
  * nothing important is going to be zero length anyway, so we treat it as
  * non-existant.  */
 off_t
-findMember(const char *name)
+findMember(const char *deb, const char *name)
 {
 char magic[SARMAG+1];
 struct ar_hdr arh;
diff --git a/debsig-verify.c b/debsig-verify.c
index 6adf7a9..33aaa1a 100644
--- a/debsig-verify.c
+++ b/debsig-verify.c
@@ -36,7 +36,6 @@
 char originID[2048];
 char *rootdir = ;
 
-char *deb = NULL;
 FILE *deb_fs = NULL;
 
 #define CTAR(x) control.tar # x
@@ -68,7 +67,7 @@ static