[kamailio/kamailio] ims_registrar_pcscf,ims_qos: added a parameter for trusting the bottom Via header on requests (PR #3891)
<!-- Kamailio Pull Request Template -->
<!-- IMPORTANT: - for detailed contributing guidelines, read: https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md - pull requests must be done to master branch, unless they are backports of fixes from master branch to a stable branch - backports to stable branches must be done with 'git cherry-pick -x ...' - code is contributed under BSD for core and main components (tm, sl, auth, tls) - code is contributed GPLv2 or a compatible license for the other components - GPL code is contributed with OpenSSL licensing exception -->
#### Pre-Submission Checklist <!-- Go over all points below, and after creating the PR, tick all the checkboxes that apply --> <!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines from above--> <!-- If you're unsure about any of these, don't hesitate to ask on sr-dev mailing list --> - [ ] Commit message has the format required by CONTRIBUTING guide - [ ] Commits are split per component (core, individual modules, libs, utils, ...) - Not yet - first let's see if the work is valid, then I'll recompose the whole work to satisfy this. Otherwise... if I need to fix something, it's too hard to work like this... - [ ] Each component has a single commit (if not, squash them into one commit) - ditto - [x] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change - [ ] Small bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [ ] PR should be backported to stable branches - [x] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description <!-- Describe your changes in detail -->
Normally, the IMS P-CSCF should identify the clients (UEs) by the received IP address and ports on Rx. The current code is using a mix of that, plus using Contact and Via headers, with arguable potential security issues.
This patch adds a new parameter to `ims_registrar_pcscf` and `ims_qos` modules, allowing for an optional outsource of the IPsec functionality to another element, which is also in charge of checking/enforcing correct UE Via header. The existing code is allowed to work as before, with the default value of the flag being towards that.
List of functional changes: - `ims_qos` - added `trust_bottom_via` parameter -
List of indirect changes: - default I-CSCF config example contained a questionable line which adds a `+` as a prefix in Request-URI. After way too much time wasted to figure out why the Diameter LIR has bogus SIP or TEL URI values in UserName AVP, I have discovered this. Seems like someone had just tel-URIs in their network, but otherwise the blind addition of this prefix makes no sense to me. - added a `str2ushort()` macro, since code was using some dangerous casting and macros with a larger type -
List of non-functional fixes: - spelling in comments - comments at the end of line moved above the line they refer to; with just 80 columns code-formatting, commenting on the same line provides for some super weird and hard to read code, so IMHO should not be allowed (or ... much harder now... increase to 120 columns) -
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3891
-- Commit Summary --
* squashed work
-- File Changes --
M misc/examples/ims/icscf/kamailio.cfg (4) M src/core/ut.h (36) M src/lib/ims/ims_getters.c (2) M src/modules/ims_icscf/location.c (10) M src/modules/ims_qos/ims_qos_mod.c (27) M src/modules/ims_qos/ims_qos_mod.h (1) M src/modules/ims_qos/rx_aar.h (4) M src/modules/ims_qos/rx_authdata.h (2) M src/modules/ims_qos/rx_avp.c (2) M src/modules/ims_qos/rx_avp.h (1) M src/modules/ims_registrar_pcscf/doc/ims_registrar_pcscf_admin.xml (39) M src/modules/ims_registrar_pcscf/ims_registrar_pcscf_mod.c (18) M src/modules/ims_registrar_pcscf/notify.c (2) M src/modules/ims_registrar_pcscf/save.c (78) M src/modules/ims_registrar_pcscf/service_routes.c (125) M src/modules/ims_registrar_pcscf/subscribe.c (75) M src/modules/ims_registrar_pcscf/subscribe.h (4) M src/modules/ims_usrloc_pcscf/udomain.c (4)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3891.patch https://github.com/kamailio/kamailio/pull/3891.diff
@vingarzan pushed 1 commit.
ca59b6fb3ac4941686a2fbc2c46fa96b86e7b3b9 added documentation of the new parameter also to the ims_qos module
@vingarzan pushed 1 commit.
67f52db5e640ae06e08252fde97c260f0a2eba6e reverting ut.h - not sure why I touched it, also why the checks fail
@herlesupreeth Would appreciate your review here please. We can talk on IM of your choice also if you wish. Thanks!
Hey @vingarzan
Normally, the IMS P-CSCF should identify the clients (UEs) by the received IP address and ports on Rx
Can you please point me to a specification where it states this?
Hey @vingarzan
Normally, the IMS P-CSCF should identify the clients (UEs) by the received IP address and ports on Rx
Can you please point me to a specification where it states this?
I don't have a link... but it seems logical to me. Imagine a scenario where Alice is registered. She then proceeds to send an MESSAGE with Contact: bob, Via: bob. If we identify the UE by the Contact or Via, we've just let an impersonation attack go through.
Sure, there are a lot of things that need to be faked, etc, but from a security stand-point, I'm thinking that the P-CSCF should only identify the UE based on the source IP address and port of the SIP package. The IPsec functionality must also ensure that the UE didn't do IP spoofing (e.g. Alice injected a packet on her SPI, with a source IP from Bob, which is normally prevented by EPC/5GC).
P.S. My PR is not trying to get compliance with this whole point. I'm actually offloading the IPsec work to an external entity, which guarantees that the bottom Via is not spoofed. So I'm adding an optional "trust-the-bottom-Via" flag.
I think this security point should be fixed even outside my use-case and optional flag. But... since I can't see tests around this code, I'm a bit afraid to go in and do changes which would change behaviors for others.
Thanks for the explanation.
I'm actually offloading the IPsec work to an external entity, which guarantees that the bottom Via is not spoofed. So I'm adding an optional "trust-the-bottom-Via" flag.
I didnt understand what you mean by offloading the IPsec work to an external entity. You mean IPSec creation is done by other entity (other than ims_ipsec_pcscf module)?
Also, the naming "trust-the-bottom-Via" doesnt go well in my opinion ("bottom-via" part I mean). While handling SIP REQUEST the first Via needs to be considered and in case SIP REPLY I think last Via needs to be considered
I didnt understand what you mean by offloading the IPsec work to an external entity. You mean IPSec creation is done by other entity (other than ims_ipsec_pcscf module)?
Yup. That whole thing through the kernel is not giving me enough flexibility for the scenario that I'm going after, so I had to do another one.
Also, the naming "trust-the-bottom-Via" doesnt go well in my opinion ("bottom-via" part I mean). While handling SIP REQUEST the first Via needs to be considered and in case SIP REPLY I think last Via needs to be considered
The existing functions use "first" and "last". "Top"/"bottom" seemed better to me, since "last" could mean last added, which would be the "top" one. Or "first" could be interpreted as first added, which would be "top". My version seems to be less controversial maybe?
Naming things it's hard :stuck_out_tongue_closed_eyes: ... let's just pick whatever we'd agree is less confusing for everyone.
I get the argument for first (a.i. top) Via on requests. Yet from the perspective of 3GPP security, there should be a single Via in any requests originating from the UE. Otherwise there is something spying between the UE and the security gateway of the P-CSCF. So I'd argue that the last (a.i. bottom) would be sufficient for all cases, with an extra check, based on local policy, to reject requests from the UE with more than 1 Via. Anyway, this is an ideal case, IRL one could argue that the P-CSCF has multiple parts and could merge Vias internally, etc, so I'd keep it flexible.
tl;dr - if you look from the UE's perspective, a P-CSCF using the bottom Via is protecting you better than one which uses the top one and as such might let someone do a man-in-the-middle attack on you. Makes sense?
Thanks again!!
Here are my further comments. Sorry for nitpicking, but here is my two cents, I would probably issue a PR addressing the `List of non-functional fixes:` mentioned above first and keep the implementation of `trust_bottom_via` in another PR so that its easier to review and rollback if we face issues at a later point.
There are some doubts related to some of the changes in this PR which I will raise shortly
@herlesupreeth commented on this pull request.
&& (ignore_contact_rxport_check
- || (c->received_port == _m->rcv.src_port)
I am not sure about this change.. why was the check for SIP message received source port removed?
Here are my further comments. Sorry for nitpicking, but here is my two cents, I would probably issue a PR addressing the `List of non-functional fixes:` mentioned above first and keep the implementation of `trust_bottom_via` in another PR so that its easier to review and rollback if we face issues at a later point.
Absolutely not a problem! Will do! Fully appreciate your review and time here! I did a draft since I wasn't sure if it all makes sense, but also didn't want to lose whatever smaller fixes I did.
@vingarzan commented on this pull request.
&& (ignore_contact_rxport_check
- || (c->received_port == _m->rcv.src_port)
It's moved to lines 183->184 in the additions.
I wish I hadn't, but since in my scenario the `port-uc` wasn't stored anywhere in Kamailio (`ims_ipsec_pcscf` not loaded), I had to put it behind an if for that module loaded. I'll make further improvement and also skip on `ignore_contact_rxport_check`. But it's still checked, just above.
@vingarzan pushed 1 commit.
abc8f33bc71d8def998f08e514f960d41c95fca7 optimized and fixed the port checking in checkcontact()
@vingarzan pushed 1 commit.
62c091d156f7b20415b0297a7c8bb2f0add03127 ims_registrar_pcscf: added ignore_contact_rxproto_check parameter, to solve the issue of failing from other protocol
@vingarzan commented on this pull request.
&& (ignore_contact_rxport_check
- || (c->received_port == _m->rcv.src_port)
With `ignore_contact_rxport_check=1` the protocol (not port! :upside_down_face:) hit me hard today, when REGISTER happened over TCP and then an MO MESSAGE kept being rejected because ... UDP.
I added a parameter `ignore_contact_rxproto_check` with default `1` (so changing behavior!). My opinion is that in IMS the IPsec SA is negotiated for all transport protocols (so in practice UDP and TCP), hence if a UE managed to correctly encrypt whatever UDP/TCP packet correctly and send it to us on the correct Security-Association flows, we should allow it.
@vingarzan commented on this pull request.
&& (ignore_contact_rxport_check
- || (c->received_proto == _m->rcv.proto))) {
For context - the `ignore_contact_rxproto_check` parameter fixes this logical condition group. I don't know if this was on purpose, or a copy-paste mistake (checking proto for port flag).
@herlesupreeth commented on this pull request.
&& (ignore_contact_rxport_check
- || (c->received_port == _m->rcv.src_port)
My opinion is that in IMS the IPsec SA is negotiated for all transport protocols (so in practice UDP and TCP), hence if a UE managed to correctly encrypt whatever UDP/TCP packet correctly and send it to us on the correct Security-Association flows, we should allow it.
I agree
@vingarzan pushed 3 commits.
5703590ddce0c2f76cc913de211dd3c030d8664b squashed work 88c47f839645b01b1c7f2ed9be3aeffc86d9a6ed optimized and fixed the port checking in checkcontact() 4f2865aee63b685f882d9ff0b521ad9e65a674ec ims_registrar_pcscf: added ignore_contact_rxproto_check parameter, to solve the issue of failing from other protocol
Closed #3891.
Closing as replaced by https://github.com/kamailio/kamailio/pull/3901
-
Dragos Vingarzan -
Supreeth Herle