Summary:	ASTERISK-25490: [patch]SDP crypto tag is validated incorrectly
Reporter:	Joerg Sonnenberger (joerg)	Labels:
Date Opened:	2015-10-22 18:42:41	Date Closed:	2017-03-29 17:51:08
Priority:	Major	Regression?
Status:	Closed/Complete	Components:	Channels/chan_sip/SRTP
Versions:	13.13.1 14.2.1	Frequency of Occurrence	Constant
Related Issues:
Environment:	Interoperability with Snom D725	Attachments:	( 0) patch-channels_sip_sdp__crypto.c
Description:	When trying to forward a call from a D725 with encrypted RTP, the crypto handshake fails as the phone tries to use a zero crypto tag. A potential fix can be found in https://www.netbsd.org/~joerg/patch-channels_sip_sdp__crypto.c The same issue should apply to newer releases as well, but I can't test that easily.
Comments:	By: Asterisk Team (asteriskteam) 2015-10-22 18:42:43.466-0500 Thanks for creating a report! The issue has entered the triage process. That means the issue will wait in this status until a Bug Marshal has an opportunity to review the issue. Once the issue has been reviewed you will receive comments regarding the next steps towards resolution. A good first step is for you to review the [Asterisk Issue Guidelines|https://wiki.asterisk.org/wiki/display/AST/Asterisk+Issue+Guidelines] if you haven't already. The guidelines detail what is expected from an Asterisk issue report. Then, if you are submitting a patch, please review the [Patch Contribution Process|https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process]. By: Richard Mudgett (rmudgett) 2015-10-23 11:08:02.535-0500 Thanks for the contribution! If you'd like your contribution to be included faster, you should submit your patch for code review by the Asterisk Developer Community. To do so, please follow the Code Review [1] instructions on the wiki. Be sure to: * Verify that your patch conforms to the Coding Guidelines [2] * Review the Code Review Checklist [3] for common items reviewers will look for * If necessary, provide tests for the Asterisk Test Suite that verify the correctness of your patch [4] When ready, submit your patch and any tests to Review Board [5] for code review. Thanks! [1] https://wiki.asterisk.org/wiki/display/AST/Code+Review [2] https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines [3] https://wiki.asterisk.org/wiki/display/AST/Code+Review+Checklist [4] https://wiki.asterisk.org/wiki/display/AST/Asterisk+Test+Suite+Documentation [5] https://wiki.asterisk.org/wiki/display/AST/Review+Board+Usage Also patches *must* be submitted with a signed license agreement in place no matter how trivial the patch.  A link to an external location is not acceptable as the patch cannot be confirmed as licensed. By: Joerg Sonnenberger (joerg) 2015-10-23 17:33:26.371-0500 Sorry, couldn't figure out how to do the patch attachment during the initial submission. I've signed the CLA in the past, just don't have time to fight with gerrit right now. By: Joerg Sonnenberger (joerg) 2015-10-24 14:39:06.650-0500 There is a typo in the patch, should be sizeof(buf). By: Alexander Traud (traud) 2016-12-27 05:46:09.149-0600 In the world of Asterisk, a contributor has to go through all steps himself (reporting, debugging, patching, code review). Only in a few cases, the Asterisk Team takes over an issue. Only in rar cases, another contributor takes over the issue. In this case, I would like to take over and submit your change for code review in Gerrit, because I possess a Snom D725 myself. Furthermore, I did several changes to sRTP in Asterisk already. Finally, your patch is straight forward and nothing else must be developed. However: Yet, I was not able to reproduce your issue. # What do you mean by ‘forward’ exactly – how to you forward a call? I tried Forwarding via the phone and the web interface which gave me SIP status 3xx. I tried Transfer and Xfer which did not give me any new SDP and therefore no crypto line. Apple once had a concise [step-by-step guide|http://web.archive.org/web/20160324163652/https://developer.apple.com/bug-reporting/using-bug-reporter/problem-detail/] how to report issues. That would help to reproduce your issue, so I can test with the latest Asterisk 13 and 14. # Do you still face this issue with the latest firmware? a) [8.7.5.35|http://downloads.snom.com/fw/snom725-8.7.5.35-SIP-r.bin] is the current release version (Nov. 2015). b) [8.7.5.44|http://downloads.snom.com/fw/mru-preview/snom725-8.7.5.44-SIP-r.bin] is the current beta version (Mar. 2015). c) [8.9.3.56|http://downloads.snom.net/interop/firmware/8.9.3.56/snom725-8.9.3.56-SIP-r.bin] is the latest version of a new branch (Dec. 2016). I tested these three versions. You posted your issue before 8.7.5.35 was released. # Technically, this is an issue in the Snom firmware because the crypto tag should not use a zero value but start with 1. Did you report this issue to Snom as well? Nevertheless, for compatibility, I do not see any reasons why your patch should not be included in Asterisk. # Looking at your patch, please, explain why you copy the value over into a new buffer. By: Joerg Sonnenberger (joerg) 2017-01-15 15:58:51.746-0600 This is not a bug in the Snom firmware. To quote RFC 4568, section 4.1:  The tag is a decimal number used as an identifier for a particular   crypto attribute (see Section 9.1 for details); leading zeroes MUST   NOT be used.  The tag MUST be unique among all crypto attributes for   a given media line. As such, nothing prohibits it from being a plain zero. As for reproducing it, I am somewhat reluctant to back out the change from my production machine and I will not be able to set up a test machine for a while. There are two possible situations from memory: (1) Redirection from the Snom phone via the phone API. (2) Picking up a call from a second Snom phone, i.e. by BLF indicator. In either case, it is important that secure_bridge_signaling and secure_bridge_media are set. By: Alexander Traud (traud) 2017-01-16 02:53:05.237-0600 This *is* an issue in the Snom software because I am not aware of anyone else using a tag of 0. Although it is not a software bug per se, it is an interoperability issue for sure. Yes, the BNF in the RFC allows a 0 as tag value. However, actually, your quoted sentence does not allow a 0 as tag value because that could be understood as ‘leading’ zero. From that quote and that interpretation, the tag should be empty, which is not allowed by the BNF. Consequently, a 0 wold not be allowed for the tag. Furthermore, all examples in the RFC start with 1. Anyway, such a discussion is fruitless, because these RFC are not bullet proof (the BNF should match the examples; why that leading zero text is there is questionable as well). The situation is as it is. I see no reason for Asterisk not to allow 0. I see no reason for Snom not to go for 1 as they do in all other scenarios. Therefore, please, report this issue to Snom as well. It is *their* job to judge on this matter. I am quite sure, for sake of interoperability, they change. Same here, I would love to drive this through Code Review within Asterisk and allow a 0. But I have to test the resulting code. If you change to a non-PFS based TLS Cipher Suite like {{tlscipher=AES128-SHA}} in the configuration file {{sip.conf}}, you should be able to track that issue via Wireshark even in a production deployment. Alternatively, you go for {{sip set debug on}} in the Asterisk CLI. I tried a ‘redirection’ from the physical user interface of my Snom D725 and got a starting tag value of 1. Which button(s) do you press exactly, when you redirect? By: Joerg Sonnenberger (joerg) 2017-01-19 16:27:52.400-0600 Seriously, no. The requirement to rule out leading zero is a simple way to avoid ambiguity and issues like parsing numbers as octal. Interpreting "0" as leading zero is not a sensible interpreation. By: Alexander Traud (traud) 2017-01-20 02:37:12.443-0600 I interpret it that way and I have a degree in that stuff. If Octal is the reason, the RFC should state it because that is a reason of another domain (not SDP but programming language literals). Please, report this issue to Snom as well, because I cannot—I am not able to create tickets with them. Or are you not allowed either? Let them decide! Furthermore, please, give me the chance to reproduce your issue, so I can test your patch, so I can submit it to Code Review. Or stated differently: You are discussing with the wrong one. I sacrifice my time and try to get your change into Asterisk. By: Ross Beer (rossbeer) 2017-01-20 03:02:35.401-0600 What Snom D725 firmware are you using? The latest for the D725 can be downloaded from here: http://downloads.snom.com/interop/firmware/8.9.3.57/snom725-8.9.3.57-SIP-r.bin By: Alexander Traud (traud) 2017-03-29 08:42:00.777-0500 I was not able to reproduce this with my Snom D725. However, the RTX 9430 (Single-cell DECT-IP base) faces the same issue. I reported this to RTX via Snom, because the Snom M300 (part of the Snom M325 package) is actually a RTX 9430. There are more manufactures which re-labeled that product, like Agfeo DECT IP-Basis XS, Alcatel-Lucent 8318, Konftel IP DECT 10, and Mitel RFP 12. As stated before, I do not see a reason, why Asterisk should not support zero as tag value as well. Furthermore, it might take years until all RTX 9430 in the field are updated. However, I was not able to test the code contribution here to go for code review. Yes, I could have used SIPp instead. Thanks to my Snom M325, I was able to test the code and submitted a change for review (see the Gerrit tab). [~joerg], I just changed the accepted value range in Asterisk. I have not understood why you create a new buffer. Is this because of possible white-space? By: Joerg Sonnenberger (joerg) 2017-03-29 15:48:22.990-0500 The extra buffer is only for the verification that the input matches the parsed result, i.e. no additional junk is present like trailing garbage, plus signs, whitespace etc. That's the only reason for it. By: Friendly Automation (friendly-automation) 2017-03-29 17:51:08.758-0500 Change 5354 merged by Joshua Colp: srtp: Allow zero as tag value for a sRTP Crypto Suite. [https://gerrit.asterisk.org/5354|https://gerrit.asterisk.org/5354] By: Friendly Automation (friendly-automation) 2017-03-29 17:51:24.674-0500 Change 5355 merged by Joshua Colp: srtp: Allow zero as tag value for a sRTP Crypto Suite. [https://gerrit.asterisk.org/5355|https://gerrit.asterisk.org/5355] By: Friendly Automation (friendly-automation) 2017-03-29 19:02:00.711-0500 Change 5356 merged by Joshua Colp: srtp: Allow zero as tag value for a sRTP Crypto Suite. [https://gerrit.asterisk.org/5356|https://gerrit.asterisk.org/5356] By: Friendly Automation (friendly-automation) 2017-03-30 08:30:40.453-0500 Change 5364 merged by zuul: srtp: Remove test for zero tag. [https://gerrit.asterisk.org/5364|https://gerrit.asterisk.org/5364] By: Alexander Traud (traud) 2017-04-03 02:09:02.529-0500 bq. i.e. no additional junk is present like trailing garbage, plus signs, whitespace etc. That's the only reason for it. It is always good to give concrete examples and mention those in a comment within the code itself. Not everyone is able to reverse engineer the idea behind a patch. Furthermore, it is always good to limit a change to one issue (start at index 0, do not allow +/- signs, remove whitespace, find the first digit automatically). That ‘start at index 0’ issue has gone through code review and was included by now and closed this issue automatically. If one of that other changes should go into Asterisk as well, please, say so.