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
