<!-- 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 --> - [x] Commit message has the format required by CONTRIBUTING guide - [x] Commits are split per component (core, individual modules, libs, utils, ...) - [x] Each component has a single commit (if not, squash them into one commit) - [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 - [x] Small bug fix (non-breaking change which fixes an issue) - [ ] 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 --> The module has been broken in d00ceda2c04. The code to replace strcpy() did not terminate the target sting with \0.
Discovered by our voiptests rig:
https://github.com/sippy/voiptests/actions/workflows/kamailio.yml
Results in a broken SDP (note garbage around o= attribute:
``` 1681 00:00:08.271/GLOBAL/alice_ua: RECEIVED message from [::1]:5060: 1682 SIP/2.0 183 Session Progress 1683 Via: SIP/2.0/UDP [::1]:5061;received=::1;rport=5061;branch=z9hG4bK20a1169c6781e4bfe26554e84d3b4925 1684 Record-Route: <sip:[::1];ftag=l5v4RNY4iYqP.*!0LQUA.MLVq8RX'm!q;lr> 1685 From: "Alice Smith" <sip:alice_2_ipv6@[::1]>;tag=l5v4RNY4iYqP.*!0LQUA.MLVq8RX'm!q 1686 To: <sip:bob_2@[::1]>;tag=c77c4b2c8430cf6c18566bc3d9212ed3 1687 Call-ID: O%CciL!Yf0-SQn]G46v4J(SPddRr<MtH@)<0.ev}(]q.%X[> 1688 CSeq: 200 INVITE 1689 Content-Type: application/sdp 1690 Content-Length: 296 1691 1692 v=0 1693 o=- 577651195072 577651195072 IN IP6 ::1\x86zU 1694 s=BOBSDP Session 1695 c=IN IP6 ::1\x86zU 1696 t=0 0 1697 m=image 13424 udptl t38 1698 a=T38FaxVersion:0 1699 a=T38MaxBitRate:14400 1700 a=T38FaxRateManagement:transferredTCF 1701 a=T38FaxMaxBuffer:262 1702 a=T38FaxMaxDatagram:176 1703 a=T38FaxUdpEC:t38UDPRedundancy 1704 a=nortpproxy:yes
1740 00:00:08.322/GLOBAL/alice_ua: RECEIVED message from 127.0.0.1:5060: 1741 SIP/2.0 183 Session Progress 1742 v: SIP/2.0/UDP 127.0.0.1:5061;received=127.0.0.1;rport=5061;branch=z9hG4bK8e357573055f35a768af0c9be78c836d 1743 Record-Route: <sip:127.0.0.1;ftag=q3~s'YH'`.Krs%LQKdFWrWnWgwPHJHnf;lr> 1744 f: "Alice Smith" <sip:alice_7_ipv4@127.0.0.1>;tag=q3~s'YH'`.Krs%LQKdFWrWnWgwPHJHnf 1745 t: <sip:bob_7@127.0.0.1>;tag=8ee80e5205ef358c3cb868fb5e5614f8 1746 i: m6!(QAFQlv}vTlycDH[rpMjW}mVV1phG@z/c{KI:L*n'[`%15 1747 CSeq: 200 INVITE 1748 c: application/sdp 1749 l: 328 1750 1751 v=0 1752 o=- 577651195078 577651195078 IN IP4 127.0.0.1z\xcfpzG\xd1\xcb`\xef=\x86zU 1753 s=BOBSDP Session 1754 c=IN IP4 127.0.0.1z\xcfpzG\xd1\xcb`\xef=\x86zU 1755 t=0 0 1756 m=image 13696 udptl t38 1757 a=T38FaxVersion:0 1758 a=T38MaxBitRate:14400 1759 a=T38FaxRateManagement:transferredTCF 1760 a=T38FaxMaxBuffer:262 1761 a=T38FaxMaxDatagram:176 1762 a=T38FaxUdpEC:t38UDPRedundancy 1763 a=nortpproxy:yes ```
Looks like there were multiple changes of the same nature. All of them probably needs to be re-evaluated to make sure this pattern is not propagated. You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3524
-- Commit Summary --
* rtpproxy: make sure we null terminate the copy of the string.
-- File Changes --
M src/modules/rtpproxy/rtpproxy.c (3)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3524.patch https://github.com/kamailio/kamailio/pull/3524.diff
Merged #3524 into master.
My bad, added fixes for my commits. thanks @sobomax !
``` commit 192b681ce7fcd868a4a47a7c26863c733321b8d4 (HEAD -> master, origin/master, origin/HEAD) Author: Victor Seva linuxmaniac@torreviejawireless.org Date: Wed Jul 26 15:00:38 2023 +0200
sipcapture: make sure we null terminate the copy of the string
bug introduced at cbd7810fff3d5145c1ce34c0e362b5590bb92a12
commit 3c1700fb7693c05025be1058e856fe610f4be031 Author: Victor Seva linuxmaniac@torreviejawireless.org Date: Wed Jul 26 14:53:26 2023 +0200
rtpproxy: make sure we null terminate the copy of the string.
bug introduced at d00ceda2c04 ```
No problem, Victor!
Our Kamailio CI job is happy again! :)
https://github.com/sippy/voiptests/actions/runs/5663713633
-Max P.S. On the subject of that commit, I really doubt it increased the quality of the code. Replacing the library function with a hand-rolled equivalent just to silence the static analyzer is not only pointless exercise (if the buffer overflows, it would still overflow) but also prone to introducing new bugs as this encounter has proven, I hope.
On Wed, Jul 26, 2023 at 6:05 AM Victor Seva ***@***.***> wrote:
My bad, added fixes for my commits. thanks @sobomax https://github.com/sobomax !
commit 192b681ce7fcd868a4a47a7c26863c733321b8d4 (HEAD -> master, origin/master, origin/HEAD) Author: Victor Seva ***@***.***> Date: Wed Jul 26 15:00:38 2023 +0200
sipcapture: make sure we null terminate the copy of the string bug introduced at cbd7810fff3d5145c1ce34c0e362b5590bb92a12
commit 3c1700fb7693c05025be1058e856fe610f4be031 Author: Victor Seva ***@***.***> Date: Wed Jul 26 14:53:26 2023 +0200
rtpproxy: make sure we null terminate the copy of the string. bug introduced at d00ceda2c04
— Reply to this email directly, view it on GitHub https://github.com/kamailio/kamailio/pull/3524#issuecomment-1651767068, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABVMJWFD5SLCAMK3VU4ONLXSEIYXANCNFSM6AAAAAA2XRKA3Y . You are receiving this because you were mentioned.Message ID: ***@***.***>