#350 Inconsistent parsing of From:/To: headers

[killer byte]

Most locations in the code use parse_from() to parse the address out of From: or To: headers. parse_from() supports parsing of addresses wether they are wrapped with <...> or not.

At least one place in the code, sipmsg_breakdown_parse(), does not use parse_from() and only looks for <...> wrapped addresses. The whole code base should be checked to make sure that all places use parse_from().

---

Original bug report below

I am using
Ubuntu 16.04 xenial
Kernel: 4.15.0-43-generic x86_64 (64 bit)
Desktop: MATE 1.12.1
Pidgin 2.12.0 (libpurple 2.12.0)
SIPE 1.24.0 (git commit e2815bb / NTLM / D-Bus / Voice & Video / SRTP / Lync FT / Application Sharing - full)

My Skype for Business account disconnects a lot. When I start Pidgin, it often disconnects straight away and disconnects soon after each time I click Reconnect. invalid-signature.png shows the Pidgin window with the error.

The problem is in siplcs/src/core/sipe-sign.c (repo.or.cz):

:::c
msg->from_url = sipmsg_find_part_of_header(hdr, "<", ">", empty_string);

This is not able to parse an address without angle brackets from the header. Addresses without angle brackets are valid according to RFC 3261.

This log with a from header without angle brackets shows msg->from_url is an empty string after parsing.

:::text
(23:58:04) sipe: MESSAGE <<<<<<<<<< SIP(0x563478a2a620)
(23:58:04) sipe: In file siplcs/src/core/sipe-sign.c
(23:58:04) sipe: In function sipmsg_breakdown_parse()
(23:58:04) sipe: Just processed from header
(23:58:04) sipe: hdr = 'sip:aa.bb@cc.com;tag=8252780899514117_RTCGWFourthServer.1549076448945.0_72580_95153'
(23:58:04) sipe: msg->from_url = ''
(23:58:04) sipe: sip_sec_verify_signature: message is:<TLS-DSK><AEE41192><27><SIP Communications Service><gg.hh.se><6D4CgFC14aAE5AiC9A0mDF3Ct8721bF60Ex91ADx><2><NOTIFY><><8252780899514117_RTCGWFourthServer.1549076448945.0_72580_95153><sip:dd.ee@ff.com><4970262284><><><> signature to verify is:14af5b1253e3d407bbcce9c441a4739198d03bc0
(23:58:04) sipe: sip_transport_input: signature of incoming message is invalid.
(23:58:04) connection: Connection error on 0x5634789a1ad0 (reason: 0 description: Invalid message signature received)

This log with a from header with angle brackets shows msg->from_url is the correct address after parsing.

:::text
(23:58:38) sipe: MESSAGE <<<<<<<<<< SIP(0x563478a2a1e0)
(23:58:38) sipe: In file siplcs/src/core/sipe-sign.c
(23:58:38) sipe: In function sipmsg_breakdown_parse()
(23:58:38) sipe: Just processed from header
(23:58:38) sipe: hdr = '<sip:aa.bb@cc.com>;tag=9521525182348771_RTCGWServer.1549075980088.0_79349_102178'
(23:58:38) sipe: msg->from_url = 'sip:aa.bb@cc.com'
(23:58:38) sipe: sip_sec_verify_signature: message is:<TLS-DSK><9053B051><29><SIP Communications Service><gg.hh.se><D365gA689aEABCiB2D3m88F3t4AE2b1F01xF850x><2><NOTIFY><sip:aa.bb@cc.com><9521525182348771_RTCGWServer.1549075980088.0_79349_102178><sip:dd.ee@ff.com><3615126448><><><> signature to verify is:3759040af5ab98e89d99e274ed2577814c6ab935
(23:58:38) sipe: sip_transport_input: signature of incoming message validated

[killer byte]

Fixed with commit pushed to mob branch
https://repo.or.cz/siplcs.git/commitdiff/726daa5acfc0e7e80a956a4b8ab75a2f8da137fc

Happy to discuss the commit if anyone wants to

[Stefan Becker]

So if you wanted to discuss the change, why didn't send out a review to the SIPE developers?

Your change may be correct, but it doesn't follow the conventions and f.ex. has incorrect test debugging code in it. That mob is world-writable is a left-over from SIPE history, please do not use it as invitation to push changes without review.

I've therefore reset the HEAD of mob to remove your change.

[killer byte]

The Contribute Code page, http://sipe.sourceforge.net/git/, says upload to mob branch, after that, it might be a good idea that the other developers know you just committed a patch. I don't know the process for contributing SIPE code. I was just following the instructions. How do contributors send code to the SIPE developers? Do contributors email just you, chemobejk at gmail.com, do contributors post code in the Help forum, https://sourceforge.net/p/sipe/discussion/688534/ or something else? Maybe the desired process should be explained on the Contribute Code page.

[Stefan Becker]

The web site, sad to say is really out-of-date and unmaintained. I ieven haven't found the time to edit the obsolete PHP code to get rid of the error message. Feel free to contribute.

Your commit message indicates that you didn't follow common Open Source sense: read through the commit log of the project and get a feel how things are done. You could have seen (a) who are the main contributors and (b) how other occasional contributor changes are commited. You didn't even bother to wait a day after opening the bug before pushing your change, i.e. you didn't want to discuss your change beforehand.

[killer byte]

So how do contributors send code to the SIPE developers? How do we discuss code contributions?

[killer byte]

I now realise some of the angle brackets in the bug text were interpreted as formatting and I do not see a way to edit the bug text

[Stefan Becker]

Fixed.

[Stefan Becker]

Your bug report is missing the detailed, unsafe debug log. As per bug instruction on the "create bug" & FAQ page this is usually grounds for closing a bug without comment as INVALID.

Please add the debug log so that we can discuss the merits of your change.

[killer byte]

I created a private bug 351 to attach the unsafe log. I dont see how to privately attach something to this bug.

[Stefan Becker]

If you think posting annoying ping messages every week will "speed up things", you are sadly mistaken. As you have not provided a debug log that you feel confident to attach to this public bug report, you'll have to wait until someone finds some free cycles to obfuscate/strip-down the debug log from your private bug report and attach it here.

[killer byte]

Hello Stefan

This bug report is one thing. Talking to developers about code additions and changes is another.

So if you wanted to discuss the change, why didn't send out a review to the SIPE developers?

This is the first sentence of I think your first message to me. I asked how to do that and I'd still like to know how or where to talk to SIPE developers about code additions and changes.

I get that it would be nice to have a full debug log for this issue but it's 16,000 lines and I don't have time to read and obfuscate a fraction of that. However, whatever about this bug report, I'd like to talk to SIPE developers about code additions and changes. Could you please tell me how or where to talk to the SIPE developers?

Thanks very much

Brendan

[Stefan Becker]

Without a debug log there is nothing to discuss, especially for a change to core functionality. Strangely enough this piece of code has worked for everbody else for 10+ years. The "problem" only seems to affect your OCS installation. Have you ever paused and considered that your setup might be broken?

As for your question: I've already pointed out earlier that in order to contribute to any Open Source project you should learn how to read the version control system change log.

[killer byte]

Reading commit messages or recently committed code does not tell me what the code submission and review process is. Is there an established process for reviewing SIPE code contributions? If not, how do SIPE developers prefer to be contacted? Is there a mailing list, IRC or another way of contacting developers?

I certainly did consider that the setup I'm using might be broken. Then I looked at the standard specifications and found addresses without angle brackets are valid according to RFC 3261 and SIPE can't handle it. parse_from() in sipe-utils.c parses a SIP address from a header and can handle addresses without angle brackets. sipmsg_breakdown_parse() in sipe-sign.c parses an address from a header with a different implementation which can't handle addresses without angle brackets. My code changes sipmsg_breakdown_parse() to use the existing correct implementation in parse_from(). That certainly is something to discuss.

If there is not an established process for reviewing SIPE code contributions, how do SIPE developers prefer to be contacted?

[Stefan Becker]

Then I looked at the standard specifications and found addresses without angle brackets are valid according to RFC 3261 and SIPE can't handle it.

That would unfortunately be the wrong specification for the piece of code you changed. Did you verify your assumption against [MS-SIPAE]?

The 2nd question that immediately pops into mind: if your assumption is correct, then why did you only change one place? I count at least 5 other places where SIPE parses out part of a header using <...>.

[killer byte]

If there is not an established process for reviewing SIPE code contributions, how do SIPE developers prefer to be contacted? I wish you would answer that question.

Microsoft Session Initiation Protocol Authentication Extensions states
"This protocol specifies the authentication extensions to Session Initiation Protocol (SIP). This protocol defines" ... "authentication schemes based on the general authentication framework described in [RFC3261]".
RFC 3261 is a normative reference in MS-SIPAE. I think RFC 3261 is the correct specification for the piece of code I changed. The from and to header fields are defined in RFC 3261, they are not changed by MS SIPAE. I'm not sure what you think I have assumed here.

I changed two places, parsing from addresses and parsing to addresses in sipe-sign.c. The only other addresses that affect the signature causing the problem are related to asserted identity. I wasn't sure what was valid for those addresses, it's specified in another standard and I haven't seen it used in the setup I'm using so I didn't change code for that. I wanted to fix the bug first. If you want to correct it in more places too, let's.

[Stefan Becker]

I have answered your question, but you choose to ignore it.

I tested your change locally and finally realized that the whole bug report and the information you provided is misleading. I have therefore edited the title and updated bug message accordingly. Furthermore your proposed change introduces a memory leak in at least one code path.

I'm stopping this discussion now as I won't invest anymore of my valuable time on it. When I have free cycles I will go through the code base and make the proper changes.

[Stefan Becker]

Fixed by git commit 7648f7bf

CLOSING

[killer byte]

There was nothing misleading about the information I provided and I followed what I thought was the process for contributing. You obviously have very little interest in new people contributing. Didn't expect you to be like that. That's fine, learned my lesson.