Closed phhusson closed 1 year ago
martinetd
commented
1 year ago 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
commented
1 year ago https://github.com/martinetd/android_ims/commit/383ce056b6a90fa042d8e6346d0fe2f679afc782 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
commented
1 year ago @.*** https://github.com/martinetd/android_ims/commit/383ce056b6a90fa042d8e6346d0fe2f679afc782 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 :
@.*** https://github.com/martinetd/android_ims/commit/383ce056b6a90fa042d8e6346d0fe2f679afc782 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 https://github.com/phhusson/ims/issues/13#issuecomment-1625413357, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAA4OQAOQOU3KHHYXREZNTXPAE4FANCNFSM6AAAAAAZ7IS2LU . You are receiving this because you authored the thread.Message ID: @.***>
martinetd
commented
1 year ago 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
commented
1 year ago @phhusson Sorry should have said I updated the commit, please also take https://github.com/martinetd/android_ims/commit/c862196a581363eb52eead6a746337a9d1b83722 (tip of my securityserver branch) for the Security-Client value
Jio has:
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