[Summary] Thanks a lot Andreas for the detailed and high quality MIR, with relevant researches and background information.
I was first tempted to diff between quagga and frr to do a quick assessement. However, there are too many differences to avoid doing a full package checks. Here are my findings: I can’t give a definitive ack right now until those questions are answered and some few fixes: Notes/required TODOs: * does it need a security review in your opinon? Like, since the first MIR security assessment, a lot of time have passed. Is this the opportunity to benefit from another review or do you think the overall diff from the first MIR in term of security handling is small enough to not justify having a new one? * can you give the exact list of binary packages to promote? You mentioned for instance to keep frr-rpki-rtrlib in universe to avoid pulling librtr to main. I think the definitive will help on the AA side. * It is on the lto-disabled list. Fix, or the work-around should be directly in the package. * There are some compiler warning in the build logs. This is maybe the right time to get them fixed? * finally, once this is promoted, how do we explicitely demote quagga? I don’t find it in the seed. What is going to be uploaded to switch to frr? [Duplication] Fork of quagga, will replace it and the first one will be demoted. [Dependencies] OK: - no -dev/-debug/-doc packages that need exclusion - No dependencies in main that are only superficially tested requiring more tests now. Problems: - new dependencies in main, libyang2: https://launchpad.net/ubuntu/+source/libyang2 #1958293 [Embedded sources and static linking] OK: - no embedded source present - no static linking - does not have odd Built-Using entries [Security] OK: - history of CVEs is expected for this kind of daemon and usage, It does not look concerning - does not use webkit1,2 - does not use lib*v8 directly - does not parse data formats - does not open a port/socket - does not process arbitrary web content - does not use centralized online accounts - does not integrate arbitrary javascript into the desktop - does not deal with system authentication (eg, pam), etc) - does not deal with security attestation (secure boot, tpm, signatures) Problems: - one run a daemon as root, as explained in the description [Common blockers] OK: - does not FTBFS currently - does have a test suite that runs at build time - test suite fails will fail the build upon error from the MIR description. Note that the summary mentions "TOTAL: 0" - does have a non-trivial test suite that runs as autopkgtest - no new python2 dependency [Packaging red flags] OK: Ubuntu does not carry a delta - symbols tracking not applicable for this kind of code. - d/watch is present and looks ok (if needed, e.g. non-native) - Upstream update history is good - Debian/Ubuntu update history is good - the current release is packaged - promoting this does not seem to cause issues for MOTUs that so far maintained the package - no massive Lintian warnings (rather pendantic ones) - d/rules is rather clean Problems: - It is on the lto-disabled list. Fix, or the work-around should be directly in the package, [Upstream red flags] OK: - no incautious use of malloc/sprintf (as far as we can check it) - no use of sudo, gksu, pkexec, or LD_LIBRARY_PATH (usage is OK inside tests) - no use of user nobody - no important open bugs (crashers, etc) in Debian or Ubuntu - no dependency on webkit, qtwebkit, seed or libgoa-* - not part of the UI for extra checks Problems: * Some warnings during build logs: bgpd/bgp_community_alias.c: In function ‘bgp_ca_alias_hash_cmp’: bgpd/bgp_community_alias.c:60:17: warning: ‘strncmp’ specified bound 8228 exceeds source size 8192 [-Wstringop-overread] 60 | return (strncmp(ca1->alias, ca2->alias, sizeof(struct community_alias)) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ bgpd/bgp_community_alias.c: In function ‘bgp_ca_community_hash_cmp’: bgpd/bgp_community_alias.c:43:17: warning: ‘strncmp’ specified bound 8228 exceeds source size 36 [-Wstringop-overread] 43 | return (strncmp(ca1->community, ca2->community, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 44 | sizeof(struct community_alias)) | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from /usr/include/string.h:519, from ./lib/zebra.h:38, from tests/lib/test_nexthop_iter.c:25: In function ‘strncpy’, inlined from ‘str_append’ at tests/lib/test_nexthop_iter.c:37:3, inlined from ‘str_appendf’ at tests/lib/test_nexthop_iter.c:55:2: /usr/include/x86_64-linux-gnu/bits/string_fortified.h:95:10: warning: ‘__builtin_strncpy’ specified bound depends on the length of the source argument [-Wstringop-truncation] 95 | return __builtin___strncpy_chk (__dest, __src, __len, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 96 | __glibc_objsize (__dest)); | ~~~~~~~~~~~~~~~~~~~~~~~~~ tests/lib/test_nexthop_iter.c: In function ‘str_appendf’: tests/lib/test_nexthop_iter.c:37:54: note: length computed here 37 | strncpy((*buf) + strlen(*buf), repr, strlen(repr) + 1); | ^~~~~~~~~~~~ * Use of setuid, linked to the global "needs a new security review" section question. ** Changed in: frr (Ubuntu) Status: New => Incomplete -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/1951834 Title: [MIR]: frr To manage notifications about this bug go to: https://bugs.launchpad.net/ubuntu/+source/frr/+bug/1951834/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
