[Home]

Summary:ASTERISK-03600: [patch] TXTCIDName() lookup mangles response
Reporter:Jared Smith (jsmith)Labels:
Date Opened:2005-02-27 21:54:43.000-0600Date Closed:2008-01-15 15:27:34.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) ENUM-lookup.ethereal
( 1) TXTCIDName.patch
( 2) TXTCIDName.stable.patch
( 3) TXTCIDName.try2-HEAD.patch.txt
( 4) TXTCIDName.try2-STABLE.patch.txt
( 5) TXTCIDName.try3-STABLE.patch.txt
Description:When doing a TXTCIDName() lookup, the application seems to mangle the response it gets back from DNS.  I've run a packet sniffer and verified that it does in fact get the correct response from DNS.

I'm looking into a fix, but in the meantime I thought I'd report it.

****** STEPS TO REPRODUCE ******

exten=>123,1,TXTCIDName(18662331454)
exten=>123,2,NoOP(XXX${TXTCIDNAME}XXX)
exten=>123,3,Playback(vm-goodbye)

Notice the value of ${TXTCIDNAME}.  Instead of gettign set to "Jared Smith", it gets set to "^KJa".

****** ADDITIONAL INFORMATION ******

I found that this same bug was reported to the mailing list back in August.

http://lists.digium.com/pipermail/asterisk-users/2004-August/058151.html
Comments:By: Kevin P. Fleming (kpfleming) 2005-02-28 00:04:56.000-0600

Please the bug posting guidelines; this is not a major bug.

Also, we need a sample input and output showing the failure. Just telling us it "mangles the response" doesn't really provide much to go on :-) If you can provide the DNS records involved (assuming they are not public) and the result you get after calling the application, that would be very helpful.
By: Jared Smith (jsmith) 2005-02-28 00:37:08.000-0600

The DNS entry is public -- simply set your enum.conf file to search e164.org (in addition to or instead of e164.arpa).  Or lookup the DNS record manually:

[root@old asterisk]# host -a 4.5.4.1.3.3.2.6.6.8.1.e164.org
Trying "4.5.4.1.3.3.2.6.6.8.1.e164.org"
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 55786
;; flags: qr rd ra; QUERY: 1, ANSWER: 2, AUTHORITY: 7, ADDITIONAL: 5

;; QUESTION SECTION:
;4.5.4.1.3.3.2.6.6.8.1.e164.org.        IN      ANY

;; ANSWER SECTION:
4.5.4.1.3.3.2.6.6.8.1.e164.org. 600 IN  NAPTR   100 10 "u" "E2U+IAX2" "!^\\+18662331454$!iax2:guest@voip.jaredsmith.net!" .
4.5.4.1.3.3.2.6.6.8.1.e164.org. 600 IN  TXT     "Jared Smith"

;; AUTHORITY SECTION:
e164.org.               600     IN      NS      ns2.e164.org.
e164.org.               600     IN      NS      ns3.e164.org.
e164.org.               600     IN      NS      ns3.bcwireless.net.
e164.org.               600     IN      NS      apollo.bcwireless.net.
e164.org.               600     IN      NS      mutual.bcwireless.net.
e164.org.               600     IN      NS      alberta.bcwireless.net.
e164.org.               600     IN      NS      alberta-2.bcwireless.net.

;; ADDITIONAL SECTION:
ns3.bcwireless.net.     157335  IN      A       204.50.24.6
apollo.bcwireless.net.  157335  IN      A       204.50.80.5
mutual.bcwireless.net.  3599    IN      A       204.50.80.11
alberta.bcwireless.net. 3599    IN      A       209.115.243.234
alberta-2.bcwireless.net. 3599  IN      A       64.141.95.61

Received 385 bytes from 10.0.0.200ASTERISK-49 in 329 ms


As you can tell from the attached ethereal packet trace, the DNS lookup worked correctly.  (The packet trace contains a few extra packets, but it should be blatently obvious which one to look at.)  For digging through the C code, the problem appears to be in dns_parse_answer() in dns.c.
By: Kevin P. Fleming (kpfleming) 2005-02-28 00:55:53.000-0600

OK, so that's very helpful, thanks.

What are you seeing in the result variable after running the TXTCIDName() application?
By: Jared Smith (jsmith) 2005-02-28 21:46:48.000-0600

I put the result in "Steps to Reproduce" above. Instead of having ${TXTCIDNAME} set to "Jared Smith" as I expected, it was set to "^KJa", where ^K is a vertical tab.
By: Kevin P. Fleming (kpfleming) 2005-03-01 01:02:45.000-0600

OK, sorry, I missed that. A quick look over the code doesn't show anything obvious, so I'm not sure where to go from here. Hopefully someone who understands the Asterisk ENUM/DNS code better than I will jump in...
By: Mark Spencer (markster) 2005-03-02 00:11:31.000-0600

Well, just looking at it from the 50,000 foot view, it would appear that no attempt is made to actually parse the txt information elements...  It assumes the field is simply text, literally, which I don't beleive is actually the case.  Someone is going to have to look at nslookup source code or similar and figure out what the structure of it is.
By: Olle Johansson (oej) 2005-03-02 00:36:26.000-0600

I can confirm this bug and gave the same advice to jsmith yesterday, someone needs to read the RFC here.
By: Jared Smith (jsmith) 2005-03-02 00:46:57.000-0600

I was wrong, the error is in the txt_callback function in enum.c.  The string copy was going off of sizeof(c->txt), but c->txt was empty.  Instead, it should have been working off of the length of answer.  (It also wasn't null-terminating the string properly.)

I've attached a patch which appears to completely fix the problem.  Please look over it and make sure I haven't done anything stupid.

FWIW, I have a disclaimer on file.
By: Jared Smith (jsmith) 2005-03-02 00:59:30.000-0600

Patch added for stable branch of Asterisk.
By: Olle Johansson (oej) 2005-03-02 01:51:12.000-0600

You need to make sure that you don't copy beyond the boundaries of c->txt, that's the reason for the original sizeof(c->txt) in strncpy.
By: Kevin P. Fleming (kpfleming) 2005-03-02 10:03:58.000-0600

Yes, as oej has noted (and I noticed in another bug) this code has some serious flaws.

When you do the strncpy, you need to limit it to c->txtlen, which holds the size of the buffer. c->txtlen should _not_ be updated to the length of the answer that is being returned, because that information doesn't go anywhere anyway, and if there are multiple calls to the txt_callback function it will have the wrong buffer size (although smaller, which would be safe).
By: Jared Smith (jsmith) 2005-03-06 15:33:18.000-0600

Ok, I'm still trying to fix this bug, but (as evidenced by my earlier attempts at a patch) my C coding skills are very rusty.  Anything I say below may be wrong -- but I'm doing my best to explain what I've found so far.

The reason this application fails is that c->txt has no memory allocated to it -- it's just a pointer.  So while I'll admit that my patch attempts have buffer overflows, I'd gently point out that the current code does too :-)

So in the current code, it does the following:

(enum.c, txt_callback function, around line 271)
strncpy(c->txt, answer, sizeof(c->txt) - 1);

What would be the best way to allocate memory for c->txt so that we can copy in answer?  I tried the following, but it didn't work:

c->txt = strndup(answer, len);

In short, I expeced that to work, but I think app_txtcidname.c is keeping a pointer to the previous c->txt, where we're stuffing the answer into the newly memory allocation.  Does this make any sense?  If I do use strndup(), how do I get the address of the new memory allocation back to app_txtcidname.c?
By: Jared Smith (jsmith) 2005-03-06 22:12:05.000-0600

Ok, I've finally wrapped my head around the problem and uploaded a new patch, which is much cleaner.  It is named TXTCIDName.try2-HEAD.patch.txt.  (I'll add a patch for -STABLE in a few minutes.)

Basically, what was happening in the old code is that we were calling sizeof(c->txt) instead of c->txtlen.  Because c->txt is just a pointer, sizeof(c->txt) always returned four, so it ended up copying the first three characters and then setting the fourth character to a null.  Instead, my patch correctly uses c->txtlen to know the size of the string pointed to by c->txt, so that we can copy the appropriate number of characters.

Additionally, we needed to skip over the first byte of answer, as it was a vertical tab character.

Please look over this patch and let me know if there's still anything wrong with it.
By: Jared Smith (jsmith) 2005-03-06 22:21:00.000-0600

Uploaded a patch for stable named TXTCIDName.try2-STABLE.patch.txt
By: Mark Spencer (markster) 2005-03-12 01:00:17.000-0600

Added to CVS, thanks!
By: Kevin P. Fleming (kpfleming) 2005-03-12 01:13:13.000-0600

These patches contain serious errors; they are casting "\0" to a char (or u_char), which is completely wrong.

Please use '\0' as is proper C syntax for assigning a zero into a char variable.
By: Mark Spencer (markster) 2005-03-12 11:02:13.000-0600

Fixed in CVS head again, sorry.
By: damin (damin) 2005-03-16 16:22:09.000-0600

Adding a corrected patch for Stable based on kpflemmings suggestion, and requesting that this also be committed to stable before 1.0.7 is released.
By: damin (damin) 2005-03-16 16:24:36.000-0600

TXTCIDName.try3-STABLE.patch.txt is nothing more than a find/replace of "\0" to '\0'. Applies cleanly to 1.0.7 RC1 and Stable CVS. Requesting that this be considered for inclusion in 1.0.7 stable.
By: Mark Spencer (markster) 2005-03-16 18:08:36.000-0600

Marking as resolved for Russell to look at for 1.0.7
By: Russell Bryant (russell) 2005-03-16 18:25:02.000-0600

I have fixed this in 1.0 and it will be in 1.0.7.

I changed the patch just a touch because there is no need to cast the '\0'.
By: Digium Subversion (svnbot) 2008-01-15 15:27:26.000-0600

Repository: asterisk
Revision: 5175

U   trunk/enum.c

------------------------------------------------------------------------
r5175 | markster | 2008-01-15 15:27:26 -0600 (Tue, 15 Jan 2008) | 2 lines

Fix TXTCIDName app (bug ASTERISK-3600)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=5175
By: Digium Subversion (svnbot) 2008-01-15 15:27:27.000-0600

Repository: asterisk
Revision: 5176

U   trunk/enum.c

------------------------------------------------------------------------
r5176 | markster | 2008-01-15 15:27:26 -0600 (Tue, 15 Jan 2008) | 2 lines

Fix casting error (bug ASTERISK-3600, take 2)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=5176
By: Digium Subversion (svnbot) 2008-01-15 15:27:34.000-0600

Repository: asterisk
Revision: 5184

U   branches/v1-0/enum.c

------------------------------------------------------------------------
r5184 | russell | 2008-01-15 15:27:33 -0600 (Tue, 15 Jan 2008) | 2 lines

fix TXTCIDName (bug ASTERISK-3600)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=5184