When picking winning sec, properly filter out the ones we didn't declare

[phhusson]

Jio has:

07-05 23:00:58.804  2284  2357 D PHH SipHandler: > Security-Server: ipsec-3gpp;q=0.6;alg=hmac-md5-96;ealg=aes-cbc;spi-c=889215030;spi-s=889215031;port-c=32920;port-s=5067, ipsec-3gpp;q=0.4;alg=hmac-md5-96;ealg=des-ede3-cbc;spi-c=889215030;spi-s=889215031;port-c=32920;port-s=5067, ipsec-3gpp;q=0.3;alg=hmac-md5-96;spi-c=889215030;spi-s=889215031;port-c=32920;port-s=5067, ipsec-3gpp;q=0.1;alg=hmac-sha-1-96;ealg=des-ede3-cbc;spi-c=889215030;spi-s=889215031;port-c=32920;port-s=5067, ipsec-3gpp;q=0.2;alg=hmac-sha-1-96;ealg=aes-cbc;spi-c=889215030;spi-s=889215031;port-c=32920;port-s=5067, ipsec-3gpp;q=0.5;alg=hmac-sha-1-96;spi-c=889215030;spi-s=889215031;port-c=32920;port-s=5067
```

We send sha1/aes + sha1/null. Current code sorts by q and takes whichever ealg wins. In this case it's AES, even though that's meant to be AES/MD5. If we take highest q with sha1, then it's sha1/null that wins.

So in this case we're supposed to pick sha1/null, but we wrongly chose sha1/aes

[martinetd]

Eh, why'd they put sha1/aes at a lower q than sha1/null, especially when md5/aes is first :/

I guess that's enough motivation to support AUTH_HMAC_MD5 as well, that ought to be easy enough. I'll send a patch that supports md5 and picks the right pair

[martinetd]

martinetd@383ce05 should work better for this particular case; but thinking back this really should have worked: the Security-Server you gave as an example does list sha1/aes-cbc, with all other values (spi/ports) being identical, so while sha1+aes was't the first choice it should be supported...
I guess ultimately we'll have to leave it up to some configuration option e.g. I sure would want to enable aes if there's a valid option that works with aes even if it's not the first choice (the option could be "disallow null encryption" or something simpler than actually selecting alg/ealg though)

I didn't open a PR because your main branch is quite a bit far behind mine (alarm, some cleanup); but patch should apply cleanly if you want to try, and it should select md5/fix key length as you did so that'll probably work

[phhusson]

***@***.***

<martinetd@383ce05> should work better for this particular case; but thinking back this really should have worked: the Security-Server you gave as an example does list sha1/aes-cbc, with all other values (spi/ports) being identical, so while sha1+aes was't the first choice it should be supported... Nope it shouldn't. The IPSec tunnel that the server prepared is the highest q that is listed in both Security-Server and Security-Client. The server doesn't prepare all intersecting variants (maybe I misunderstood what you said) Thanks for the commit. I wasn't aware I was late, I'll check up on that, thanks. Le ven. 7 juil. 2023 à 15:25, Dominique Martinet ***@***.***> a écrit :

…

***@***.*** <martinetd@383ce05> should work better for this particular case; but thinking back this really should have worked: the Security-Server you gave as an example does list sha1/aes-cbc, with all other values (spi/ports) being identical, so while sha1+aes was't the first choice it should be supported... I guess ultimately we'll have to leave it up to some configuration option e.g. I sure would want to enable aes if there's a valid option that works with aes even if it's not the first choice (the option could be "disallow null encryption" or something simpler than actually selecting alg/ealg though) I didn't open a PR because your main branch is quite a bit far behind mine (alarm, some cleanup); but patch should apply cleanly if you want to try, and it should select md5/fix key length as you did so that'll probably work — Reply to this email directly, view it on GitHub <#13 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAA4OQAOQOU3KHHYXREZNTXPAE4FANCNFSM6AAAAAAZ7IS2LU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[martinetd]

Nope it shouldn't. The IPSec tunnel that the server prepared is the highest
q that is listed in both Security-Server and Security-Client. The server
doesn't prepare all intersecting variants

Ah, I was about to say we don't send hmac-md5 in our security-client, but you've patched that as well in your branch -- I'll fix that to also offer md5.

Ok so that makes sense, we just need to make the option adjust what is sent in Security-Client when we add one.

[martinetd]

@phhusson Sorry should have said I updated the commit, please also take martinetd@c862196 (tip of my securityserver branch) for the Security-Client value