Bug#851060: New dsniff revision
Hi,
On 23/02/17 22:44, Marcos Fouces wrote:
> El 23/02/17 a las 12:57, James Cowgill escribió:
>> Hi,
>>
>> On 22/02/17 23:16, Marcos Fouces wrote:
>>> Hello
>>>
>>> I uploaded to repo a first attempt of libnids1.24 package wich aims to
>>> close its two critical bugs:
>>>
>>> #855602: fixed upstream in 1.24 release.
>> As I wrote in the bugreport, the fix applied by upstream is not enough
>> to fix this. You also need to apply the patch from Fedora otherwise
>> dsniff will FTBFS on i386.
>
> I applied the patch you mentioned and after a rebuild of dsniff and
> libnids, it still seems to work properly at least on amd64
>
> Please, check it:
>
> https://anonscm.debian.org/cgit/pkg-security/libnids.git
I've only looked at the things you've done so far. I think in addition
you should also:
- Rewrite the rules file.
- Add a symbols file since this is a C library.
- Install the library in the multi-arch lib directory.
- Move the stuff in "debian-changes-1.23-2" into a proper patch file.
debian/control:
> | -Build-Depends: libpcap0.8-dev, libnet1-dev (>= 1.1.2.1), debhelper (>= 5),
> autotools-dev, pkg-config, libglib2.0-dev
> | +Build-Depends: libpcap0.8-dev, libnet1-dev (>= 1.1.2.1), debhelper (>=
> 10), autotools-dev, pkg-config, libglib2.0-dev
debhelper (>= 10) implies autotools-dev so you can remove that
dependency. It may be a good idea to sort them as well.
> | -Package: libnids1.21
> | +Package: libnids1.24
This appears to be a case of upstream not knowing how to correctly
version their ABI (there are no ABI changes in 1.24) though as there are
only 2 reverse dependencies you can probably get away with it. This
package will need to go via experimental to handle the transition.
> | Architecture: any
> | Depends: ${shlibs:Depends}, ${misc:Depends}
> | Conflicts: libnids1
> | -Replaces: libnids0, libnids1
> | +Replaces: libnids0, libnids1, libnids1.21
libnids1.24 contains no files which would conflict with any of these
packages, so just remove the Conflicts and Replaces lines here.
> | @@ -22,11 +20,10 @@ Description: IP defragmentation TCP segment reassembly
> library (development)
> | modules.
> | Libnids performs assembly of TCP segments into TCP streams, IP
> | defragmentation, and TCP port scan detection.
> | + This is the development package.
> | .
> |
Usually that line goes below the dot. There's also some trailing
whitespace which you could remove.
Both the debian/*.dirs files are useless. You can remove them.
>> For stretch, the fixes need to be targeted so as to only change as much
>> stuff as necessary to fix the two RC bugs.
>
> I am agree with you, when i fix these bugs i will create a separate git
> branch, cherry-pick only freeze-allowed changes and try to get a package
> ready for stretch.
Ok. Since I can now get dsniff working, I will happily NMU this unless
you want to do it.
> This is the default behaviour of gbp when creating a repo. In
> pkg-security team we don't use it but you can see it in some packages.
> Anyway, i deleted it. :-)
I don't think gbp creates a debian/master branch by default.
Thanks,
James
signature.asc
Description: OpenPGP digital signature
Bug#851060: New dsniff revision
Hi James, El 23/02/17 a las 12:57, James Cowgill escribió: Hi, On 22/02/17 23:16, Marcos Fouces wrote: Hello I uploaded to repo a first attempt of libnids1.24 package wich aims to close its two critical bugs: #855602: fixed upstream in 1.24 release. As I wrote in the bugreport, the fix applied by upstream is not enough to fix this. You also need to apply the patch from Fedora otherwise dsniff will FTBFS on i386. I applied the patch you mentioned and after a rebuild of dsniff and libnids, it still seems to work properly at least on amd64 Please, check it: https://anonscm.debian.org/cgit/pkg-security/libnids.git #851060: i tried using -fno-strict-aliasing flag as reporter indicates. I didn't noticed any change on my amd64, but he talked about armhf arch. I built dsniff (1) against this new version of libnids-dev (2) and here is the result: * Run dsniff * Run the same test as bug reporter: curl -v --basic --user foo:bar http://neverssl.com/ dsniff says this: dsniff: listening on eth0 - 02/22/17 23:56:32 tcp 192.168.1.3.47942 -> s3-website-us-west-2.amazonaws.com.80 (http) GET / HTTP/1.1 Host: neverssl.com Authorization: Basic Zm9vOmJhcg== [foo:bar] So interestingly, after recompiling your new versions of both libnids and dsniff, things start working again. I'm not sure what happened before when I couldn't get anything to work. I really don't know, i couldn' t reproduce this bug at any time. (1) https://anonscm.debian.org/cgit/pkg-security/dsniff.git For stretch, I don't think dsniff needs to be changed. (2) https://anonscm.debian.org/cgit/pkg-security/libnids.git At the moment Debian is frozen and you've made a number of changes here which are not sutible for the freeze. These include: - Packaging a new version of libnids. - Breaking the ABI of libnids. - Bumping debhelper compat For stretch, the fixes need to be targeted so as to only change as much stuff as necessary to fix the two RC bugs. I am agree with you, when i fix these bugs i will create a separate git branch, cherry-pick only freeze-allowed changes and try to get a package ready for stretch. For buster, can I suggest you rewrite the rules file. I expect it could be simplified much more using dh. Sure Looking at the diff from upstream, I can't help but stare in disbelief at this: diff --git a/src/hash.c b/src/hash.c index 7e4c611..3eb28ca 100644 --- a/src/hash.c +++ b/src/hash.c @@ -57,7 +57,8 @@ mkhash (u_int src, u_short sport, u_int dest, u_short dport) u_int res = 0; int i; u_char data[12]; - *(u_int *) (data) = src; + u_int *stupid_strict_aliasing_warnings=(u_int*)data; + *stupid_strict_aliasing_warnings = src; *(u_int *) (data + 4) = dest; *(u_short *) (data + 8) = sport; *(u_short *) (data + 10) = dport; A minor note: as someone not part of the pkg-security team, I find it confusing to have both a "debian/master" and "master" branch in the same repository both referring to Debian packaging. I think you should remove the "master" branch (unless this is a team policy?) Thanks, James This is the default behaviour of gbp when creating a repo. In pkg-security team we don't use it but you can see it in some packages. Anyway, i deleted it. :-) Cheers, Marcos
